Loading modifications at runtime

Question

I'm working on a game engine among other things in Java, and would like to know how to optimise my code, as it is ridiculously ugly and bloated. I'm terrible with operating the java.util.File and need my code to be reviewed by others.

In essence, I have a folder hierarchy setup in the following manner:

• Custom folder which the user decides on. Variable is folder.
  • More folders. Each one corresponding to a mod.
    • mod.cfg file. I do checks to see if it contains information too.
    • Java class referred from the mod.cfg file, needs to exist AND be of a specific object, namely Modification.

If all of this works, then the mod is loaded into memory.

public static void loadModifications(String folder) { // TODO: Optimise modification loading.
 File enclosingFolder = new File(folder);
 if (enclosingFolder.exists()) {
 for (File modFolder : enclosingFolder.listFiles()) {
 if (modFolder.isDirectory()) {
 File modConfig = new File(modFolder.getPath() + "/mod.cfg");
 if (modConfig.exists()) {
 String modName = (String) ConfigHandler.getPropertyValue(modConfig.getPath(), "mod_name");
 String modMain = (String) ConfigHandler.getPropertyValue(modConfig.getPath(), "mod_main");
 try {
 if (modMain != null) {
 try {
 String className = modFolder.getPath().replaceFirst(enclosingFolder.getPath(), "");
 className = className.substring(1, className.length());
 className += "." + modMain;
 className = className.replaceAll("/", ".");
 Object modClass = Class.forName(className).newInstance();
 if (modName != null) {
 if (modClass instanceof Modification) {
 Logger.log(LogLevel.MESSAGE, "Loading modification: '" + modClass.getClass().getName() + "' as '" + modName + "'");
 ((Modification) modClass).setConfigFile(modConfig.getPath());
 ((Modification) modClass).init();
 Logger.log(LogLevel.MESSAGE, ((Modification) modClass).getInfo());
 mods.put(modName, (Modification) modClass);
 } else {
 Logger.log(LogLevel.WARNING, "Modification: '" + modName + "' does not derive from abstract class: 'Modification', ignoring.");
 }
 } else {
 Logger.log(LogLevel.WARNING, "'mod_name' undefined for: '" + modFolder.getPath() + "'");
 }
 } catch (InstantiationException | IllegalAccessException e) {
 Logger.log(LogLevel.WARNING, "Error loading main class for modification '" + modName + "', ignoring.");
 }
 } else {
 Logger.log(LogLevel.WARNING, "'mod_main' undefined for: '" + modFolder.getPath() + "', ignoring.");
 }
 } catch (ClassNotFoundException e) {
 e.printStackTrace();
 }
 } else {
 Logger.log(LogLevel.WARNING, "Configuration file does not exist for modification: '" + modFolder.getPath() + "', ignoring.");
 }
 }
 }
 } else {
 Logger.log(LogLevel.ERROR, "Failed to locate modification folder: '" + folder + "'");
 }
 }

Footnote:

• Logger and LogLevel refers to a customised logging method I implemented.

• ConfigHandler refers to a property loader from files. Albeit named mod.cfg, the file has the structure of a Java Properties file.

Answer 1

1) Few tweaks on condition checking to improve performance a bit.

if (modMain != null) {
 try {
 if (modName != null) {
 String className = modFolder.getPath().replaceFirst(enclosingFolder.getPath(), "");
 className = className.substring(1, className.length());
 className += "." + modMain;
 className = className.replaceAll("/", ".");
 Object modClass = Class.forName(className).newInstance();
 if (modClass instanceof Modification) {
 Logger.log(LogLevel.MESSAGE, "Loading modification: '" + modClass.getClass().getName() + "' as '" + modName + "'");
 ((Modification) modClass).setConfigFile(modConfig.getPath());
 ((Modification) modClass).init();
 Logger.log(LogLevel.MESSAGE, ((Modification) modClass).getInfo());
 mods.put(modName, (Modification) modClass);
 } else {
 Logger.log(LogLevel.WARNING, "Modification: '" + modName + "' does not derive from abstract class: 'Modification', ignoring.");
 }
 } else {
 Logger.log(LogLevel.WARNING, "'mod_name' undefined for: '" + modFolder.getPath() + "'");
 }
 } catch (InstantiationException | IllegalAccessException e) {
 Logger.log(LogLevel.WARNING, "Error loading main class for modification '" + modName + "', ignoring.");
 }
} else {
 Logger.log(LogLevel.WARNING, "'mod_main' undefined for: '" + modFolder.getPath() + "', ignoring.");
}

If modName is NULL, there is no point in loading the class details and creating its instance. This saves some time. Handle exceptions accordingly.

2) If you have access to Modification class and can create its instance using NEW operator, it is preferred over reflection as reflection takes more time.

This is my suggestion. Please try and let me know if you found any difference in performance.

• user106033: Haha, atleast the "arrow" isn't as visible anymore. Thankyou for the optimisation! I will leave the question available for longer to see if anyone can cut it down further!
• user106033: Code tested, works lovely. I currently receive a 4ms boost in performance through a quick benchmark. Will make a substantial difference upon loading 10's of mods.
• Sunil Dabburi: Glad to know it helped! :)

Answer 2

I hope that you are allowed to use Java 8 in the project (anyway, nothing is said in the original question about the version of Java).

The same goal can be achieved with almost no ugly if branches. The idea is the following: 1) use stream operations in order to obtain the references to all existing mod.cfg files; 2) extract the entire routine of class loading and instantiation of Modification objects into a dedicated method; 3) fill the mods Map with these instances.

In the example below I skip logging instructions. They can be easily added where necessary inside the lambdas.

public static void loadModifications(String folder) throws IOException { // 1
 Objects.requireNonNull(folder);
 Files.find(Paths.get(folder), 1, (path, attrs) ->
 Files.isDirectory(path) && Files.isReadable(path) //2
 ).map(dir -> dir.resolve("mod.cfg")) //3
 .filter(modConfig -> Files.exists(modConfig)) //4
 .flatMap(ModLoader::loadModification) //5
 .forEach(modification -> {
 String modName = "{init it here}";
 mods.put(modName, modification); //6
 });
}
private static Stream<Modification> loadModification(Path modConfigPath) {
 // put the algo of Modification instances creation here
 // if it is initialized properly, return Stream.of(mod);
 // otherwise return Stream.empty();
}

Comments for the numbered lines:

1. Method signature change may be avoided if you think it is possible to handle the IOE inside the method.

2. Keeps only readable folders inside the enclosing folder.

3. Creates a stream of expected mod.cfg files.

4. Keeps only mod.cfg that really exist.

5. Creates a stream of optional Modification objects for each existing mod.cfg.

6. Fills the mods map with created instances of Modifications. I'd also underline that this original way of filling the map is not very clean from the point of view of concurrency, but the context is not enough to judge about it.

• RubberDuck: Functional programming is great, but it has a way of obfuscating just how much the code is looping. Just something for OP to keep in mind. (That the loops are still there, they just look different.)
• user106033: Thankyou for the optimisation, looks great! However, may I point out that yes, I am programming this in Java 8, however was looking for backward compatibility... I'll think about weather or not this will be a defining point on going 8+ or 6+.
• Antot: @RubberDuck: of course, this is just another way to loop on same things, but at least it makes it more readable and avoids many of those nested blocks. @frayment: This is an optimization of readability, but not necessarily of performance that still needs to be tested. BTW, I've just made another fix that completely eliminates ifs, by returning Stream<Modification> from the extracted method.

Answer 3

My main issue is that this code follow the "arrow anti-pattern", which makes it hard to read. Once you start nesting ifs, your code quickly becomes hard to follow, especially when there are try...catch blocks involved.

I know how to solve this in C#; however I'm no Java expert so I can't advice you on how to solve this. In C# I'd possibly convert this method into a class and each if would perhaps become a method, but I don't know if that's the right way in Java.

• user106033: I'm sorry about readability, truly. That's why I absolutely hate my implementation.
• user106033: As for C#, give me an example and I'll try porting it for you.
• BCdotWEB: @frayment lostechies.com/derekgreer/2009/10/05/the-arrow-anti-pattern or lostechies.com/chrismissal/2009/05/27/… or blog.codinghorror.com/flattening-arrow-code
• user106033: I seem to be getting DNS errors connection to lostechies.com, however CodingHorror has very good points. I'll see if I can edit my code.