Constructive Criticism Requested

[Danickar]

So I just finished a simple mod that creates and plays a game of Tic Tac Toe in the world.  The board, made of 9 blocks containing their tile entities is created by using a ttt item.  This is my first attempt at something more complicated than a block or item and with code that requires multiple parts talking to each other.  I am sure there are better ways to do a lot of what I have done, so I am posting all of the code here for anyone to review and critique.

 

Main file:

 

  Reveal hidden contents

package powell.games;

import net.minecraft.block.Block;
import net.minecraft.creativetab.CreativeTabs;
import net.minecraft.item.ItemStack;
import powell.games.item.Items;
import powell.games.network.CommonProxy;
import powell.games.block.Blocks;
import cpw.mods.fml.common.Mod;
import cpw.mods.fml.common.Mod.EventHandler;
import cpw.mods.fml.common.Mod.Instance;
import cpw.mods.fml.common.SidedProxy;
import cpw.mods.fml.common.event.FMLInitializationEvent;
import cpw.mods.fml.common.event.FMLPostInitializationEvent;
import cpw.mods.fml.common.event.FMLPreInitializationEvent;
import cpw.mods.fml.common.network.NetworkMod;

@Mod(modid=ModInfo.ID, name=ModInfo.NAME, version=ModInfo.VERSION)
@NetworkMod(channels = {"gamesChannel"}, clientSideRequired=true, serverSideRequired = true)


public class Games
{
// The instance of your mod that Forge uses.
@Instance(value = ModInfo.ID)
public static Games instance;

public static CreativeTabs tabGames = new CreativeTabs("tabGames") 
{
	public ItemStack getIconItemStack() 
	{
		return new ItemStack(Block.glowStone, 1, 0);
	}
};

public static final int DIMENSIONID = 7;

// Says where the client and server 'proxy' code is loaded.
@SidedProxy(clientSide="powell.games.network.ClientProxy", serverSide="powell.games.network.CommonProxy")
public static CommonProxy proxy;

@EventHandler // used in 1.6.2
//@PreInit    // used in 1.5.2
public void preInit(FMLPreInitializationEvent event) 
{
	Items.init();
	Blocks.init();
}


@EventHandler // used in 1.6.2
//@Init       // used in 1.5.2
public void load(FMLInitializationEvent event) 
{
	Items.load();
	Blocks.load();
	Blocks.registerTileEntities();
}

@EventHandler // used in 1.6.2
//@PostInit   // used in 1.5.2
public void postInit(FMLPostInitializationEvent event) 
{
	// Stub Method

}
}

 

 

Block files:

 

  Reveal hidden contents

package powell.games.block;

import net.minecraft.block.Block;
import powell.games.entity.tileentity.TTTBlockEntity;
import cpw.mods.fml.common.registry.GameRegistry;
import cpw.mods.fml.common.registry.LanguageRegistry;

public class Blocks
{
public static Block tttBlock;

public static void init()
{
	tttBlock = new TTTBlock(1442);
}

public static void load()
{
	GameRegistry.registerBlock(tttBlock, "tttBlock");
	LanguageRegistry.addName(tttBlock, "Tic Tac Toe Block");
}

public static void registerTileEntities() 
{
	GameRegistry.registerTileEntity(TTTBlockEntity.class, "tttBlockEntity");
}
}

 

 

 

  Reveal hidden contents

package powell.games.block;

import net.minecraft.block.Block;
import net.minecraft.block.BlockContainer;
import net.minecraft.block.material.Material;
import net.minecraft.client.renderer.texture.IconRegister;
import net.minecraft.entity.player.EntityPlayer;
import net.minecraft.tileentity.TileEntity;
import net.minecraft.util.Icon;
import net.minecraft.world.IBlockAccess;
import net.minecraft.world.World;
import powell.games.Games;
import powell.games.entity.tileentity.TTTBlockEntity;
import cpw.mods.fml.relauncher.Side;
import cpw.mods.fml.relauncher.SideOnly;

public class TTTBlock extends BlockContainer
{
private Icon full;
private Icon x;
private Icon o;
private boolean showX;
private boolean showO;

public TTTBlock(int par1)
{
	super(par1, Material.rock);
	setStepSound(Block.soundStoneFootstep);
	setCreativeTab(Games.tabGames);
	setHardness(500000F);
	setResistance(500000F);
	setUnlocalizedName("tttBlock");
	showX = showO = false;
}

@Override
public boolean isOpaqueCube()
{
	return false;
}

@SideOnly(Side.CLIENT)
public void registerIcons(IconRegister iconRegister)
{

	//block texture
	blockIcon = iconRegister.registerIcon("games:tictactoe");
	x = iconRegister.registerIcon("games:x");
	o = iconRegister.registerIcon("games:o");

}


@Override
public boolean onBlockActivated(World par1World, int par2, int par3, int par4, EntityPlayer par5EntityPlayer, int par6, float par7, float par8, float par9)
{
	if(!showX && !showO)
	{
		System.out.println("was right clicked");
		//showX = true;
		TTTBlockEntity be = (TTTBlockEntity)par1World.getBlockTileEntity(par2, par3, par4);
		be.makePlay(par1World);

	}


	return true;
}

@Override
public TileEntity createNewTileEntity(World world)
{
	return new TTTBlockEntity();
}

@Override
public Icon getIcon(int side, int meta) 
{
	//System.out.println("inside getIcon with meta " + meta);
	return meta == 0 ? blockIcon : meta == 1 ? x  : o;
}



}

 

 

Entity:

 

  Reveal hidden contents

package powell.games.entity;


import powell.games.Games;
import cpw.mods.fml.common.registry.EntityRegistry;

public class Entities
{
private static int modEntityID = 0;

public static void init()
{
	EntityRegistry.registerModEntity(TTTEntity.class, "Tic Tac Toe Game", ++modEntityID, Games.instance, 64, 10, true);
}
}

 

 

 

  Reveal hidden contents

package powell.games.entity;

import net.minecraft.entity.Entity;
import net.minecraft.nbt.NBTTagCompound;
import net.minecraft.world.World;
import powell.games.block.Blocks;
import powell.games.entity.tileentity.*;

public class TTTEntity extends Entity
{
private int turn;


public TTTEntity(World par1World)
{
	super(par1World);
	turn = 0;
}

public TTTEntity(World world, int x, int y, int z)
{
	super(world);
	turn = 0;

	for(int xx = 0; xx < 3; xx++)
	{
		for(int zz = 0; zz < 3; zz++)
		{
			System.out.println("X " + (x+xx) + " Y " + y + " Z " + (z+zz));
			world.setBlock(x, y, z + zz, Blocks.tttBlock.blockID);
			TTTBlockEntity bleh = (TTTBlockEntity)world.getBlockTileEntity(x, y, z +zz);
			bleh.registerBoard(this);
		}
		y++;
	}
}

@Override
protected void entityInit()
{
	// TODO Auto-generated method stub

}

@Override
protected void readEntityFromNBT(NBTTagCompound nbttagcompound)
{
	// TODO Auto-generated method stub

}

@Override
protected void writeEntityToNBT(NBTTagCompound nbttagcompound)
{
	// TODO Auto-generated method stub

}

public boolean getPlay()
{
	return (turn++ % 2) == 0;
}

}

 

 

Tile Entity:

 

  Reveal hidden contents

package powell.games.entity.tileentity;

import powell.games.entity.TTTEntity;
import net.minecraft.nbt.NBTTagCompound;
import net.minecraft.tileentity.TileEntity;
import net.minecraft.world.World;

public class TTTBlockEntity extends TileEntity
{
private boolean turn;
private boolean played;
private TTTEntity board;

public TTTBlockEntity()
{
	turn = true;
	played = false;
}

public void makePlay(World world)
{
	if(!played)
	{
		System.out.println("in the make play");
		turn = board.getPlay();
		world.setBlockMetadataWithNotify(xCoord, yCoord, zCoord, turn ? 1 : 2, 3);
		played = !played;
	}


}

@Override
public void writeToNBT(NBTTagCompound compound) 
{
	super.writeToNBT(compound);

}

@Override
public void readFromNBT(NBTTagCompound compound) 
{
	super.readFromNBT(compound);

}

public void registerBoard(TTTEntity b)
{
	board = b;
}
}

 

 

Item:

 

  Reveal hidden contents

package powell.games.item;

import net.minecraft.item.Item;
import cpw.mods.fml.common.registry.GameRegistry;
import cpw.mods.fml.common.registry.LanguageRegistry;

public class Items
{
public static Item ttt;

public static void init()
{
	ttt = new TTTItem(9500);
}

public static void load()
{
	GameRegistry.registerItem(ttt,"tttItem");
	LanguageRegistry.addName(ttt, "Tic Tac Toe");

}
}

 

 

 

  Reveal hidden contents

package powell.games.item;

import net.minecraft.client.renderer.texture.IconRegister;
import net.minecraft.entity.player.EntityPlayer;
import net.minecraft.item.Item;
import net.minecraft.item.ItemStack;
import net.minecraft.world.World;
import powell.games.Games;
import powell.games.entity.TTTEntity;
import cpw.mods.fml.relauncher.Side;
import cpw.mods.fml.relauncher.SideOnly;

public class TTTItem extends Item
{
public TTTItem(int par1)
{
	super(par1);
	this.maxStackSize = 1;
	this.setCreativeTab(Games.tabGames);
	this.setUnlocalizedName("tttItem");
}

@SideOnly(Side.CLIENT)
@Override
public void registerIcons(IconRegister iconRegister)//updateIcons
{
	this.itemIcon = iconRegister.registerIcon("games:tictactoe");
}


public boolean onItemUse(ItemStack par1ItemStack, EntityPlayer par2EntityPlayer, World par3World, int par4, int par5, int par6, int par7, float par8, float par9, float par10)
    {
        int i1 = par3World.getBlockId(par4, par5, par6);
        
        System.out.println("Block id is " + i1);
        par3World.spawnEntityInWorld(new TTTEntity(par3World, par4, par5 + 1, par6));
        
        return true;


    }
}

 

[GotoLink]

I don't understand why you used an Item and an Entity.

You can place the blocks themselves, and they already contain the TileEntity.

 

My advice:

Merge the entity-placing-blocks code into the "item use" method, then move that content into the "block placed" method, finally delete everything about the item and the entity. The TileEntity should store everything related to the play.

Not optimal, but better.

Then you could improve the thing by using only one TileEntity, for example in the middle block.

With two block states based on metadata, only one having the TileEntity. (do not use BlockContainer, prefer hasTileEntity(metadata)...)

The "block placed" method would check around for other similar blocks, and set the middle block to the state with a TileEntity, when the board is complete.

The "block activated" method would check around for a block with the tileentity, and write to it.

[Danickar]

My thought behind the Entity itself was a central place to keep track of the game.  For TTT, a rather simple game for sure, it is keeping track of whose turn it is, so that clicking on any of the blocks creates the appropriate X or O.  I wasn't sure of the best way to get a blocks to find a TileEntity to figure out whose turn it was.

[GotoLink]

You are not keeping track of the game at all.

You are not saving anything to the NBT methods.

[Danickar]

Yes, I know it isn't saving it, I wasn't concerned with this particular version with that part.  I meant keeping track so that when one cube is activated it is X and the next cube is O, etc. 

[GotoLink]

You could do something like this in your TileEntity:

private boolean mark = true;
private String[] players = new String[2];

public boolean getNextMark(){//returns a boolean opposite each time, equivalent to 0 or X
    mark = !mark;
    return mark;
}

public boolean isNextPlayer(EntityPlayer player){//check that a player can play this game, on this turn
    return player.username.equals(players[mark?0:1]);
}

public boolean setPlayer(EntityPlayer player){//sets a player, return true if it was added to the game
    if(players[0]==null){
        players[0] = player.username;
        return true;
    }else if(players[1]==null){
        players[1] = player.username;
        return true;
    }
    return false;
}