How to improve my codding style for registry

[Enderlook]

Asking for help in this forum, some people have told me that I'm using a common wrong coding style that I should avoid:

 

So, I want to ask a few questions:

 

Currently, I have a common, a client and a server (which I don't use) proxy. That topic said everything from common proxy should be moved to the main file. So my question is if I should do something like this:

 

Basically:

• Move everything from CommonProxy to main file (ExtraCraft).
• Delete CommonProxy.
• Change inheritance of ClientProxy from CommonProxy to ExtraCraft.
• Change inheritance of ServerProxy from CommonProxy to ExtraCraft.

 

CommonProxy: (Delete it)

//@Mod.EventBusSubscriber(modid = ExtraCraft.MOD_ID)
public class CommonProxy {
	
	// Config instance
    /*public static Configuration config;
    
	public void preInitializationEvent(FMLPreInitializationEvent event) {
		File directory = event.getModConfigurationDirectory();
        config = new Configuration(new File(directory.getPath(), "modtut.cfg"));
        Config.readConfig();
	}
	
	public void initializationEvent(FMLInitializationEvent event) {
	}
	
	public void postInitializationEvent(FMLPostInitializationEvent event) {
        if (config.hasChanged()) {
            config.save();
        }
	}
	
	@SubscribeEvent
	public static void registerItems(final RegistryEvent.Register<Item> event) {		
		ModItems.registerItems(event);
		ModBlocks.registerItemBlocks(event);
	}
	
	@SubscribeEvent
	public static void registerBlocks(final RegistryEvent.Register<Block> event) {
		ModBlocks.registerBlocks(event);
	}
	
	public void registerRenderers() {
		
	}*/
}

ClientProxy: (Change inheritance)

@Mod.EventBusSubscriber(modid = ExtraCraft.MOD_ID, value = Side.CLIENT)
public class ClientProxy extends ExtraCraft /*CommonProxy*/{
	
	@Override
	public void preInitializationEvent(FMLPreInitializationEvent event) {
		super.preInitializationEvent(event);
	}
	
	@Override
	public void initializationEvent(FMLInitializationEvent event) {
		super.initializationEvent(event);
	}
	
	@Override
	public void postInitializationEvent(FMLPostInitializationEvent event) {
		super.postInitializationEvent(event);		
	}
	
	@SubscribeEvent
	public static void registerModels(ModelRegistryEvent event) {		
		ModItems.initializeItemsModels();
		ModBlocks.initializeBlockModels();
	}

	@Override
	public void registerRenderers() {
		
	}
}

ServerProxy: (Change inheritance)

@Mod.EventBusSubscriber(modid = ExtraCraft.MOD_ID, value = Side.SERVER)
public class ServerProxy extends ExtraCraft /*CommonProxy*/ {
	
	@Override
	public void preInitializationEvent(FMLPreInitializationEvent event) {
		super.preInitializationEvent(event);
	}
	
	@Override
	public void initializationEvent(FMLInitializationEvent event) {
		super.initializationEvent(event);
	}
	
	@Override
	public void postInitializationEvent(FMLPostInitializationEvent event) {
		super.postInitializationEvent(event);
	}
}

ExtraCraft: (Get all the code from CommonProxy)

@Mod(modid = ExtraCraft.MOD_ID, name = ExtraCraft.NAME, version = ExtraCraft.VERSION)
/*ADD*/ @Mod.EventBusSubscriber(modid = ExtraCraft.MOD_ID)
public class ExtraCraft {
	
	public static final String MOD_ID = "extracraft";
	public static final String NAME = "ExtraCraft Mod";
	public static final String VERSION = "0.1.0.0";
	
	/*ADD*/ public static Configuration config;

	static Logger logger; // private
	
	@SidedProxy(clientSide = "net.enderlook.extracraft.proxy.ClientProxy", serverSide = "net.enderlook.extracraft.proxy.ServerProxy")
	//private static CommonProxy proxy;
	
	@Mod.Instance
	private static ExtraCraft instance;
	
	@Mod.EventHandler
	public void preInitializationEvent(FMLPreInitializationEvent event) {
		logger = event.getModLog();
		//proxy.preInitializationEvent(event);		
		/*ADD*/ File directory = event.getModConfigurationDirectory();
		/*ADD*/ config = new Configuration(new File(directory.getPath(), "modtut.cfg"));
		/*ADD*/ Config.readConfig();
		ModBlocks.init();;
	}
	
	@Mod.EventHandler
	public void initializationEvent(FMLInitializationEvent event) {		
		//proxy.initializationEvent(event);
		ModRecipes.init();
	}
	
	@Mod.EventHandler
	public void postInitializationEvent(FMLPostInitializationEvent event) {
		//proxy.postInitializationEvent(event);
		/*ADD*/ if (config.hasChanged()) {
		/*ADD*/     config.save();
		/*ADD*/ }
	}
	
	public static String appendModID(String value) {
		return MOD_ID + ":" + value;
	}
	
	/*ADD*/ @SubscribeEvent
	/*ADD*/ public static void registerItems(final RegistryEvent.Register<Item> event) {		
	/*ADD*/ 	ModItems.registerItems(event);
	/*ADD*/ 	ModBlocks.registerItemBlocks(event);
	/*ADD*/ }
	
	/*ADD*/ @SubscribeEvent
	/*ADD*/ public static void registerBlocks(final RegistryEvent.Register<Block> event) {
	/*ADD*/ 	ModBlocks.registerBlocks(event);
	/*ADD*/ }
	
	/*ADD*/ public void registerRenderers() {
	/*ADD*/ 	
	/*ADD*/ }
	
}

The code in comments will be deleted, code with "/*ADD*/" will be added.

 

Furthermore this critical change, should I do another critical change.

Is fine my "registerItems" and "registerBlocks" event or I should try also to move them?. So, should I delete that last 3 methods and just add "@SubscribeEvent" on each one in their classes's methods ("ModItems", "ModBlocks")

[V0idWa1k3r]

Don't make your proxies extend your mod class, that makes no sense. Make an interface for them(IProxy for example) where you would define the methods your proxies should provide like I do here.

14 minutes ago, Enderlook said:

Is fine my "registerItems" and "registerBlocks" event or I should try also to move them?. So, should I delete that last 3 methods and just add "@SubscribeEvent" on each one in their classes's methods ("ModItems", "ModBlocks")

There is no harm in the way it is. It is my personal opinion that having a method the only function of which is to call another static method is redundant and adds unnecessary code. If you are registering your blocks in method X make that method the event handler - there is no need to have a separate method that only calls this one and does nothing else.

[Draco18s]

The only other thing I suggest you do is get rid of the IHasModel interface, if you have one per Code Style #3. Your ModelRegistryEvent handler can do all of the work just fine without having to cast and request.

[Enderlook]

4 hours ago, V0idWa1k3r said:

Don't make your proxies extend your mod class, that makes no sense. Make an interface for them(IProxy for example) where you would define the methods your proxies should provide like I do here.

There is no harm in the way it is. It is my personal opinion that having a method the only function of which is to call another static method is redundant and adds unnecessary code. If you are registering your blocks in method X make that method the event handler - there is no need to have a separate method that only calls this one and does nothing else.

Ok, thanks, I'll perform the update.

2 hours ago, Draco18s said:

The only other thing I suggest you do is get rid of the IHasModel interface, if you have one per Code Style #3. Your ModelRegistryEvent handler can do all of the work just fine without having to cast and request.

Ok, I was about to ask that also. Thanks, I'll perform the modifications.

[Enderlook]

4 hours ago, V0idWa1k3r said:

Don't make your proxies extend your mod class, that makes no sense. Make an interface for them(IProxy for example) where you would define the methods your proxies should provide like I do here.

There is no harm in the way it is. It is my personal opinion that having a method the only function of which is to call another static method is redundant and adds unnecessary code. If you are registering your blocks in method X make that method the event handler - there is no need to have a separate method that only calls this one and does nothing else.

I've tried to remove the inheritance from ExtraCraft making an interface but it produces a crash:

Description: There was a severe problem during mod loading that has caused the game to fail

net.minecraftforge.fml.common.LoaderException: net.minecraftforge.fml.common.LoaderException: Attempted to load a proxy type net.enderlook.extracraft.proxy.ClientProxy into net.enderlook.extracraft.ExtraCraft.instance, but the types don't match
	at net.minecraftforge.fml.common.ProxyInjector.inject(ProxyInjector.java:102)
	at net.minecraftforge.fml.common.FMLModContainer.constructMod(FMLModContainer.java:603)
	at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at sun.reflect.NativeMethodAccessorImpl.invoke(Unknown Source)
	at sun.reflect.DelegatingMethodAccessorImpl.invoke(Unknown Source)
	at java.lang.reflect.Method.invoke(Unknown Source)
	at com.google.common.eventbus.Subscriber.invokeSubscriberMethod(Subscriber.java:91)
	at com.google.common.eventbus.Subscriber$SynchronizedSubscriber.invokeSubscriberMethod(Subscriber.java:150)
	at com.google.common.eventbus.Subscriber$1.run(Subscriber.java:76)
	at com.google.common.util.concurrent.MoreExecutors$DirectExecutor.execute(MoreExecutors.java:399)
	at com.google.common.eventbus.Subscriber.dispatchEvent(Subscriber.java:71)
	at com.google.common.eventbus.Dispatcher$PerThreadQueuedDispatcher.dispatch(Dispatcher.java:116)
	at com.google.common.eventbus.EventBus.post(EventBus.java:217)
	at net.minecraftforge.fml.common.LoadController.sendEventToModContainer(LoadController.java:218)
	at net.minecraftforge.fml.common.LoadController.propogateStateMessage(LoadController.java:196)
	at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at sun.reflect.NativeMethodAccessorImpl.invoke(Unknown Source)
	at sun.reflect.DelegatingMethodAccessorImpl.invoke(Unknown Source)
	at java.lang.reflect.Method.invoke(Unknown Source)
	at com.google.common.eventbus.Subscriber.invokeSubscriberMethod(Subscriber.java:91)
	at com.google.common.eventbus.Subscriber$SynchronizedSubscriber.invokeSubscriberMethod(Subscriber.java:150)
	at com.google.common.eventbus.Subscriber$1.run(Subscriber.java:76)
	at com.google.common.util.concurrent.MoreExecutors$DirectExecutor.execute(MoreExecutors.java:399)
	at com.google.common.eventbus.Subscriber.dispatchEvent(Subscriber.java:71)
	at com.google.common.eventbus.Dispatcher$PerThreadQueuedDispatcher.dispatch(Dispatcher.java:116)
	at com.google.common.eventbus.EventBus.post(EventBus.java:217)
	at net.minecraftforge.fml.common.LoadController.distributeStateMessage(LoadController.java:135)
	at net.minecraftforge.fml.common.Loader.loadMods(Loader.java:593)
	at net.minecraftforge.fml.client.FMLClientHandler.beginMinecraftLoading(FMLClientHandler.java:225)
	at net.minecraft.client.Minecraft.init(Minecraft.java:513)
	at net.minecraft.client.Minecraft.run(Minecraft.java:421)
	at net.minecraft.client.main.Main.main(Main.java:118)
	at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at sun.reflect.NativeMethodAccessorImpl.invoke(Unknown Source)
	at sun.reflect.DelegatingMethodAccessorImpl.invoke(Unknown Source)
	at java.lang.reflect.Method.invoke(Unknown Source)
	at net.minecraft.launchwrapper.Launch.launch(Launch.java:135)
	at net.minecraft.launchwrapper.Launch.main(Launch.java:28)
	at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at sun.reflect.NativeMethodAccessorImpl.invoke(Unknown Source)
	at sun.reflect.DelegatingMethodAccessorImpl.invoke(Unknown Source)
	at java.lang.reflect.Method.invoke(Unknown Source)
	at net.minecraftforge.gradle.GradleStartCommon.launch(GradleStartCommon.java:97)
	at GradleStart.main(GradleStart.java:25)
Caused by: net.minecraftforge.fml.common.LoaderException: Attempted to load a proxy type net.enderlook.extracraft.proxy.ClientProxy into net.enderlook.extracraft.ExtraCraft.instance, but the types don't match
	at net.minecraftforge.fml.common.ProxyInjector.inject(ProxyInjector.java:95)
	... 43 more

Not sure why. I've done this interface:

public interface IProxy {
	public void preInitializationEvent(FMLPreInitializationEvent event);
	public void initializationEvent(FMLInitializationEvent event);
	public void postInitializationEvent(FMLPostInitializationEvent event);	
}

@Mod(modid = ExtraCraft.MOD_ID, name = ExtraCraft.NAME, version = ExtraCraft.VERSION)
@Mod.EventBusSubscriber(modid = ExtraCraft.MOD_ID)
public class ExtraCraft implements IProxy {
	
	public static final String MOD_ID = "extracraft";
	public static final String NAME = "ExtraCraft Mod";
	public static final String VERSION = "0.1.0.0";	
	public static Configuration config;
	static Logger logger;
	
	@SidedProxy(clientSide = "net.enderlook.extracraft.proxy.ClientProxy", serverSide = "net.enderlook.extracraft.proxy.ServerProxy")
	
	@Mod.Instance
	private static ExtraCraft instance;
	
	@Mod.EventHandler
	public void preInitializationEvent(FMLPreInitializationEvent event) {
		logger = event.getModLog();
		File directory = event.getModConfigurationDirectory();
		config = new Configuration(new File(directory.getPath(), "modtut.cfg"));
		Config.readConfig();
		ModBlocks.init();;
	}
	
	@Mod.EventHandler
	public void initializationEvent(FMLInitializationEvent event) {
		ModRecipes.init();
	}
	
	@Mod.EventHandler
	public void postInitializationEvent(FMLPostInitializationEvent event) {
		if (config.hasChanged()) {
			config.save();
		}
	}
	
	public static String appendModID(String value) {
		return MOD_ID + ":" + value;
	}
	
	/*public void registerRenderers() {}*/
}

@Mod.EventBusSubscriber(modid = ExtraCraft.MOD_ID, value = Side.CLIENT)
public class ClientProxy implements IProxy {
	
	@Override
	public void preInitializationEvent(FMLPreInitializationEvent event) {}
	
	@Override
	public void initializationEvent(FMLInitializationEvent event) {}
	
	@Override
	public void postInitializationEvent(FMLPostInitializationEvent event) {}
	
	@SubscribeEvent
	public static void registerModels(ModelRegistryEvent event) {		
		ModItems.initializeItemsModels();
		ModBlocks.initializeBlockModels();
	}

	/*public void registerRenderers() {}*/
}

@Mod.EventBusSubscriber(modid = ExtraCraft.MOD_ID, value = Side.SERVER)
public class ServerProxy implements IProxy {
	
	@Override
	public void preInitializationEvent(FMLPreInitializationEvent event) {}
	
	@Override
	public void initializationEvent(FMLInitializationEvent event) {}
	
	@Override
	public void postInitializationEvent(FMLPostInitializationEvent event) {}
}

 

Edited August 5, 2018 by Enderlook

[Draco18s]

6 minutes ago, Enderlook said:

	@SidedProxy(clientSide = "net.enderlook.extracraft.proxy.ClientProxy", serverSide = "net.enderlook.extracraft.proxy.ServerProxy")
	
	@Mod.Instance
	private static ExtraCraft instance;

 

You're missing the proxy field.

[Enderlook]

1 minute ago, Draco18s said:

You're missing the proxy field.

Oh, before the "update" I had:

private static CommonProxy proxy;

But now I don't use a CommonProxy, so, what should I write? ClientProxy, ServerProxy or make both proxies?

[Enderlook]

9 minutes ago, diesieben07 said:

 

A variable with a value of the interface itself? It works... but I don't understand its usage...thanks

Edited August 5, 2018 by Enderlook

[Enderlook]

4 minutes ago, diesieben07 said:

If you do not understand the purpose of interfaces I suggest you read up on basic OOP.

Ok, I'm quite new in Java and its the first programming language I see with interfaces (I also now just a bit of Python, JS and VBA).

Edited August 5, 2018 by Enderlook

[Enderlook]

Also, I've another question. In my class when I register all the items or all the blocks, should I declare each variable (item or block) as null and then in a method (init) give them values?

I mean, instead of (a piece of my class):

@EventBusSubscriber(modid = ExtraCraft.MOD_ID)
public class ModItems {
	
	public static final ToolMaterial EMERALD_MATERIAL_TOOL = EnumHelper.addToolMaterial("Emerald", 3, 1200, 9f, 3f, 22).setRepairItem(new ItemStack(Items.EMERALD));
			
	@ObjectHolder(ExtraCraft.MOD_ID + ":candy")
	public static final ItemCandy ITEM_CANDY = new ItemCandy();	
	@ObjectHolder(ExtraCraft.MOD_ID + ":diamond_apple")
	public static final ItemDiamondApple ITEM_DIAMOND_APPLE = new ItemDiamondApple();  
	@ObjectHolder(ExtraCraft.MOD_ID + ":emerald_axe")
	public static final ItemEmeraldAxe ITEM_EMERALD_AXE = new ItemEmeraldAxe();
  
	public static final CItemFood[] ITEMS_FOOD = new CItemFood[] {
		ITEM_CANDY, ITEM_DIAMOND_APPLE
	};
	public static final CItemToolAxe[] ITEMS_TOOL_AXE = new CItemToolAxe[] {
		ITEM_EMERALD_AXE
	};
	[...]
}

Or use an init method:

@EventBusSubscriber(modid = ExtraCraft.MOD_ID)
public class ModItems {
	
	public static ToolMaterial EMERALD_MATERIAL_TOOL = null;
			
	@ObjectHolder(ExtraCraft.MOD_ID + ":candy")
	public static ItemCandy ITEM_CANDY = null;
	@ObjectHolder(ExtraCraft.MOD_ID + ":diamond_apple")
	public static ItemDiamondApple ITEM_DIAMOND_APPLE = null;  
	@ObjectHolder(ExtraCraft.MOD_ID + ":emerald_axe")
	public static ItemEmeraldAxe ITEM_EMERALD_AXE = null;
  
	public static CItemFood[] ITEMS_FOOD;
	public static CItemToolAxe[] ITEMS_TOOL_AXE;

	public static void init() {
		EMERALD_MATERIAL_TOOL = EnumHelper.addToolMaterial("Emerald", 3, 1200, 9f, 3f, 22).setRepairItem(new ItemStack(Items.EMERALD));
		ITEMS_FOOD = new CItemFood[] {
			new ItemCandy(), new ItemDiamondApple()
		};
		ITEMS_TOOL_AXE;= new CItemToolAxe[] {
			new ItemEmeraldAxe()
		};
	}

	[...]
}

Or even maybe I should instead of use "new ItemCandy(), new ItemDiamondApple()" etc use "ITEM_CANDY, ITEM_DIAMON_APPLE" etc in the array? But in order to do that I won't be able to "ModItems.init()" on "preInitializationEvent" in ExtraCraft because items will be null...

 

[Draco18s]

22 minutes ago, Enderlook said:

	public static final CItemFood[] ITEMS_FOOD = new CItemFood[] {
		ITEM_CANDY, ITEM_DIAMOND_APPLE
	};
	public static final CItemToolAxe[] ITEMS_TOOL_AXE = new CItemToolAxe[] {
		ITEM_EMERALD_AXE
	};

 

Congrats you've created an array of nulls!

[Enderlook]

1 minute ago, Draco18s said:

Congrats you've created an array of nulls!

Are you talking about the first example or the second one? Because I'm using right now the first for items and it works perfectly. Also, I'm using something similar to the second one for blocks and also work, that is why I'm asking about it.

[Draco18s]

Ah, yes. I missed seeing the break/incorrect visual parsing.

 

In any case, yes, you want to use an Init method.

[Enderlook]

1 minute ago, Draco18s said:

Ah, yes. I missed seeing the break/incorrect visual parsing.

 

In any case, yes, you want to use an Init method.

Ok, and it's fine using ´new´ for the items? Or I should do something especial?

		ITEMS_FOOD = new CItemFood[] {
			new ItemCandy(), new ItemDiamondApple()
		};

 

[Draco18s]

If your items set their own registry name, yes.

[Enderlook]

1 minute ago, Draco18s said:

If your items set their own registry name, yes.

...mmm... each item class has its own ´setRegistryName()´ with a value, do you mean that?

[Draco18s]

Yes.

[Enderlook]

Ok, thanks!