Multilingual command handler using inheritance

By Posted on

Problem

My problem is that you have a lot of ifconditions to be checked and inside that if condition you have multiple lines of code. I want to eliminate it and use inheritance instead. For every if statement I will be using a separate class to handle it. This is more like a derivative of the command pattern but not command pattern itself.

I have an interface which contains only one method which defines the task I want it to be implemented. I have an abstract class implementing that interface and contains a static map and static methods to add values and retrieves the task. The classes I specified will be extending this Handler class.

This is my scenario:

I am sending a string as parameter to get a specific translator. Let’s say Sinhala or Tamil or English. On each implementation of those translators, I will be adding the necessary values to the map in the Handler class in a static block. Basically adding them in the class load time.

Command Interface

public interface Command {
    LanguageTranslator getLangTranslator();
}

Handler Abstract class

public abstract class Handler implements Command {

    private static Map<String, Command> map = new HashMap<>();


    public static void AddToMap(String key, Command command){

        map.put(key,command);
    }
    public static LanguageTranslator getTranslator(String value) throws Exception {
        if (!(map.containsKey(value))){
            throw new LanguageNotFoundException("Translator not found");
        }else {
            return map.get(value).getLangTranslator();
        }
    }


 }

Sinhala Translator 

public class SiCommand extends Handler {
    static {
        Handler.AddToMap("1",new SiCommand());
    }

    @Override
    public LanguageTranslator getLangTranslator() {
        return new SiTranslator();
    }
}

Tamil Translator

public class TiCommand extends Handler {
    static {
        Handler.AddToMap("2",new TiCommand());
    }

    @Override
    public LanguageTranslator getLangTranslator() {
        return new TiTranslator();
    }
}

English Translator 

public class EnCommand extends Handler {
    static {
        Handler.AddToMap("3",new EnCommand());
    }

@Override
    public LanguageTranslator getLangTranslator() {
        return new EnTranslator();
    }
}

Demo 

 RowTranslator rowTranslator = Handler.getTranslator("2");

Questions

Since all values are assigned to the map during the class loading time these are my questions:

  1. Is using static blocks like this a good practice?

  2. I want to eliminate the use of if conditions as I mentioned before. By using this method, whenever someone wants to add a new if condition, you just only have to create a new class extending handler and implement it. Therefore the outside user doesn’t need to know the internal handling of the Command‘s subclasses. The output is just gonna be a translator. Is this a good way of doing it? What are the better alternatives? What should be adjusted?

Note: I will also be focusing strictly on Open/Close principle as well.

Solution

Static registration should be avoided. Not only because it is global code, burdening start-up and the class must be loaded (!), but also for unit-testing and diversification (proposing Tamil+English and Sinhala+Tamil as two configurable choices).

This is almost something for declarative extensions, say in XML with class loading with Class.forName.

You also might opt for loading by convention or configuration: instead of 1, 2, 3 use “si”, “ti”, “en” checked by a config properties “translators=si,ti,en” or “si=mypackage.SiTranslator”.

Or a discovery system: java SPI (Service Provider Interface): in META-INF/services/ a text file mypackage.Command containing the class name(s). You can have several jars.

A map held in a singleton object is not bad, with a small, not very dynamic domain, like languages.

Map<String, Command> commandsMap = new HashMap<>();

But you might consider the overhead of 3 or more languages, when only 2 are used on average. Say if they load a dictionary. In that case: register first
producers of those classes, which you then place in the map with a java 8 Supplier<Command>:

register("si", SiTranslator::new);
register("ta", TaTranslator::new);
register("en", EnTransleightour::new);

A special factory class as your Handlers does not seem needed, or can in java 8 be defined as interface annotated with @FunctionalInterface and written as anonymous lambda.

Open/Close would imply using the java SPI, where you plug-in a new language, possibly in a separate jar. You still better use “si” (best the Locale key) instead of a number “1”. One interface method would be String getKey().

However I would not say that a declarative or map registry at one place would be a sin. A specified orchestrated system behavior (at one point) is just as worthwile as independent extendibility. Only if you want to plug-in new languages, say separate jars, then it matters.

The Open/Close Principle is important for multitudes, maybe fine grained business rules. Open domains of a large magnitude.

  1. Using static blocks to do wiring like this is generally not a good idea, for multiple reasons:

    • It makes unit testing harder
    • It gives the class another responsibility, or reason to change
    • It essentially turns the map in Handler into a global variable, and that is bad for a number of reasons: http://c2.com/cgi/wiki?GlobalVariablesAreBad

  2. The general idea of your design is OK, but it can be simplified, and the names can be improved upon.

    • I would call your Handler a TranslatorRegistry, since its responsibility is to hold and provide Translators.
    • The individual language Commands should be called “factories”, as per the Factory pattern. (http://www.oodesign.com/factory-pattern.html).
      However, these factories don’t seem to serve a purpose. The Handler could simply hold references to the LanguageTranslators directly.
    • I’m not sure why your individual language Commands inherit from Handler, but in the code you’ve posted it has no purpose. It can be removed without affecting the behavior of the program.

Using these suggestions, your code would look something like this:

class TranslatorRegistry {
    private Map<String, LanguageTranslator> translators = new HashMap<>();

    public void register(String key, LanguageTranslator translator) {
        translators.put(key, translator);
    }

    public LanguageTranslator get(String key) {
        if (!translators.containsKey(value)) {
            throw new LanguageNotFoundException("Translator not found");
        } else {
            return translators.get(key);
        }
    }
}

and then:

// Somewhere in your program:
void registerTranslators() {
    translatorRegistry.register("1", new SiTranslator());
    translatorRegistry.register("2", new TiTranslator());
    translatorRegistry.register("3", new EnTranslator());
}

The method that you have followed is one of the recommended ways of refactoring i.e Replace Conditional statements with Polymorphism. In one my recent projects, I had applied this sort of refactoring. One of the changes that I had done was to create a Map <Enum,Command> . I suppose that the code can be more readable by applying Handler.AddToMap(Languages.ENGLISH, new EnCommand());

Secondly, one of the problems can be that when a new class is implemented by somebody, if the static block is not added, then the translator may return null. My advice would be to follow a few norms.

  1. You need to specify either a DefaultHandler that will execute if the command Object is not found. So, setting a default Language Handler option is crucial.
  2. Perhaps you may want to exit or throw an appropriate exception if the Language is not found.
  3. What I am about to suggest may not be the best practice but I am still gonna go for it. You may also want to initialize the hashMap in one place on startup and have a proper javadoc indicating for the next generation of developers that an entry has to be added to the map in order to enable that language handler. Although this does break some design principles, I find that when I look at my initialize method, I can quickly see all the language handlers that are supported. I find this readability to be more convenient. I am sure this may not resonate well in all scenarios, but this approach has worked for me.

Currently you have 

public class TiCommand extends Handler {
    static {
        Handler.AddToMap("2",new TiCommand());
    }

which calls 

    public static void AddToMap(String key, Command command){

        map.put(key,command);
    }

Note that this would usually be called addToMap in Java.

Now let’s add FrCommand 

public class FrCommand extends Handler {
    static {
        Handler.AddToMap("2", new FrCommand());
    }

You of course point out that I can’t add FrCommand as 2. You already used 2. But here’s the thing. What’s stopping me from using 2? Absolutely nothing except me realizing that I shouldn’t. And if I do reuse an existing number, what happens? In this case, either FrCommand would overwrite TiCommand or vice versa.

There are several potential fixes here.

  1. You could have addToMap check that there are no conflicts before adding.
  2. You could change the Map to a List instead and use numeric keys rather than strings.
  3. You could switch the string keys to be more unique or obvious. For example, instead of numbers, you could have “en”, “ti”, and “si”.
  4. Make the key the class name, e.g. TiCommand.

I’d lean towards doing both #1 and #3. #3 seems a clearer system, and #1 will catch it if someone makes a duplicate.

Leave a Reply

Your email address will not be published. Required fields are marked *