Creating enchantment objects based on configuration

Question

I'm searching a way to literally shorten my code. For the moment my code is this;

 if(getDashConfig().getBoolean("enchantments.multifirearrow.enabled")){
 MultiFireArrow fire = new MultiFireArrow();
 DashEnchant.getEnchants().put(1, fire);
 DashEnchant.getStringEnchants().put("multifirearrow", fire);
 if(debug()){
 getLogger().info("MultiFireArrow enchant registred");
 }
 if(getDashConfig().getBoolean("enchantments.bolt.enabled")){ 
 Bolt bolt = new Bolt();
 DashEnchant.getEnchants().put(2, bolt);
 DashEnchant.getStringEnchants().put("bolt", bolt);
 if(debug()){ getLogger().info("Bolt enchant registred"); }
 }
 if(getDashConfig().getBoolean("enchantments.randomspeed.enabled")){
 SpeedRandom speed = new SpeedRandom();
 DashEnchant.getEnchants().put(3, speed);
 DashEnchant.getStringEnchants().put("randomspeed", speed);
 if(debug()){ getLogger().info("RandomSpeed enchant registred"); }

Not that presentable.. uh? How i would literally shorten it? I don't want 350 lines like that.

• Dave Newton: You're not asking how to make creating instances more concise-you're asking how to refactor a process that happens to include creating instances. Ultimately this is asking for an OOD (Object Oriented Design) tutorial. I'd take a step back and research exactly that, focusing on what OOP is all about, how Java implements it, and how to refactor code to various patterns.
• 200_success: Where do 1, 2, and 3 come from, and what do they mean? So that we met advise you properly, please provide more context for this code. We can't necessarily improve it in isolation. See How to Ask.
• Sapus Boh: I did, i did, don't worry

Answer 1

Assuming you already have a class called Enchant (should be a superclass of your enchantments), you could use an enum to organize all of your enchantments:

public enum Enchantments {
 MULTI_FIRE(1, "multifirearrow") {
 Enchant getEnchant() { return new MultiFireArrow(); }
 }, BOLT(2, "bolt") {
 Enchant getEnchant() { return new Bolt(); }
 }, RANDOM_SPEED(3, "randomspeed") {
 Enchant getEnchant() { return new SpeedRandom(); }
 };
 //Add more enchantments here!
 abstract Enchant getEnchant();
 public void enchant() {
 Enchant enchant = getEnchant();
 DashEnchant.getEnchants().put(this.index, enchant);
 DashEnchant.getStringEnchants().put(this.name, enchant);
 if(debug()) getLogger().info(this.toString() + " enchant registered");
 }
 public boolean isEnabled() {
 return getDashConfig().getBoolean("enchantments." + this.name + ".enabled"); 
 }
 private String name; 
 private int index; 
 private Enchantment(int index, String name) {
 this.index = index;
 this.name = name;
 }
}

and then do something like:

for(Enchantment e : Enchantment.values())
 if(e.isEnabled()) e.enchant();

You could make the code even shorter using reflection and enum ordinals but there is a point where shortening the code doesn't really help anymore (plus the added speed penalty.) Nonetheless: (replace MY_PACKAGE_NAME with your package)

public enum Enchantment {
 MULTI_FIRE, BOLT, SPEED_RANDOM;
 //Add more enchantments here!
 public void enchant() {
 Enchant enchant = Class.forName(MY_PACKAGE_NAME +
 toUpperCamel(this.toString(), "_")).newInstance();
 DashEnchant.getEnchants().put(this.ordinal() + 1, enchant); //ordinal starts at 0
 DashEnchant.getStringEnchants().put(this.replaceAll("_", "").toLowerCase(), enchant);
 if(debug()) getLogger().info(this.toString() + " enchant registered");
 }
 public boolean isEnabled() {
 return getDashConfig().getBoolean("enchantments." +
 this.toString().replaceAll("_", "").toLowerCase() + ".enabled"); 
 }
 private static String toUpperCamel(String a, String spl) {
 StringBuilder b = new StringBuilder(a.length());
 for(String s : a.split(spl)) {
 b.append(Character.toUpperCase(s.charAt(0)));
 b.append(s.substring(1).toLowerCase());
 } return b.toString();
 }
}

Answer 2

You could put similar code in a method:

void addEnchant(int no, String name, Class<?> clazz)
 throws InstantiationException, IllegalAccessException {
 if(getDashConfig().getBoolean("enchantments." + name + ".enabled")){
 Object enchant = clazz.newInstance();
 DashEnchant.getEnchants().put(no, enchant);
 DashEnchant.getStringEnchants().put(name, enchant);
 if(debug()){
 getLogger().info(clazz.getName() + " enchant registred");
 }
 }
}

Then use it like this:

addEnchant(1, "multifirearrow", MultiFireArrow.class);
addEnchant(2, "bolt", Bolt.class);
addEnchant(3, "randomspeed", RandomSpeed.class);

• Dave Newton: I'd pull out the strings, though; too easy to fat-finger it. In your solution it'd be just as easy to embed them in the classes you pass to addEnchant, but ultimately, it's be better to have a list of all the whatever-these-are and have them all implement an interface, or be a subclass or enum etc.

Answer 3

First step : use a private method and exploit much more the dashconfig object to store and retrieve all required information related to the enchantment.

The idea is filling more properties information in dashconfig and creating a method with a single parameter which is the name of enchantement you try to initialize.
From this name, the helper method will retrieve all associated value :

• the boolean enabled
• the full name of the Enchantment class to instantiate (so instantiation by reflection)
• the number associated to the enchantment.

The initAndConfig() method could be like that :

public void initAndConfigureEnabledEnchantments(String enchantmentName)
 String enchantmentProperty = enchantmentName + ".enchantments";
 // fail fast
 if(!getDashConfig().getBoolean(enchantmentProperty + ".enabled")){
 return;
 }
 String classFullQualified = 
 getDashConfig().getString(enchantmentProperty + ".class");
 int number = getDashConfig().getInteger(enchantmentProperty + 
 ".number");
 Class<? extends Enchantment> newInstanceClass = (Class<? extends Enchantment>) Class.forName(classFullQualified );
 Enchantment newInstance = newInstanceClass.newInstance(); 
 DashEnchant.getEnchants().put(number, newInstance );
 DashEnchant.getStringEnchants().put(enchantmentName, newInstance);
 if(debug()){
 getLogger().info("enchantment " + enchantmentName + " registred");
 }
 }

And you could call it like that :

initAndConfigureEnabledEnchantments("multifirearrow"); initAndConfigureEnabledEnchantments("bolt"); initAndConfigureEnabledEnchantments("randomspeed");

• Sapus Boh: I like that way. But since ''dashConfig'' is a file where users will configure their settings it could look a bit messy. Ill still take it in consideration tho
• davidxxx: Thank you, I like too :) In this case, you could have two configuration files : a user configuration file for user with only information required for them and another configuration file for the sake of the application maintainability that will contain classes and numbers properties. Users would not need to touch it.
• Sapus Boh: Not a bad idea :)

Answer 4

I'd do something like this:

If MultiFireArrow , Bolt and SpeedRandom has a superclass or a common interface you can use it otherwise add an interface or superclass (based on your needs)

In my example I used InterfaceObj an interface implemented in all three classes.

then

setParam(getDashConfig().getBoolean("enchantments.multifirearrow.enabled"), new MultiFireArrow(), 1, "multifirearrow", "MultiFireArrow");
setParam(getDashConfig().getBoolean("enchantments.bolt.enabled"), new Bolt(), 2, "bolt", "Bolt");
setParam(getDashConfig().getBoolean("enchantments.randomspeed.enabled"), new SpeedRandom(), 3, "randomspeed", "RandomSpeed");
public void setParam(boolean checks, InterfaceObj iob, int index, String text, String log)
{
 if(checks)
 {
 DashEnchant.getEnchants().put(index, iob);
 DashEnchant.getStringEnchants().put(text, iob);
 if(debug()){
 getLogger().info(log + " enchant registred");
 }
 }
}

if your getBoolean is always in the format: enchantments.<ACTION>.enabled you can move the getter into the setParam method.

setParam("multifirearrow", new MultiFireArrow(), 1, "multifirearrow", "MultiFireArrow");
setParam("bolt", new Bolt(), 2, "bolt", "Bolt");
setParam("randomspeed", new SpeedRandom(), 3, "randomspeed", "RandomSpeed");
public void setParam(String checks, InterfaceObj iob, int index, String text, String log)
{
 if(getDashConfig().getBoolean("enchantments."+checks+".enabled"))
 {
 DashEnchant.getEnchants().put(index, iob);
 DashEnchant.getStringEnchants().put(text, iob);
 if(debug()){
 getLogger().info(log + " enchant registred");
 }
 }
}

• Sapus Boh: Actually, this interface must have some methods, no? And thank you, it looks pretty neat
• Marcx: It is not mandatory to have methods declared in interfaces...
• Marcx: if the getboolean is always in the format: "enchantments.<ACTION>.enabled" you can move it into the setParam method and pass only the action ifself... I will update my answer adding this modification
• Sapus Boh: I'm just having a problem, my hashmaps require a <Int, DashEnchant> or String, DashEnchant. I cannot put the interface InterfaceObj. (All enchantments extends DashEnchant)
• Sapus Boh: Nevermind, i've rereaded carefully