Proper way to declare and set a private final member variable from the constructor in Java?

Question

There are different ways to set a member variable from the constructor. I am actually debating how to properly set a final member variable, specifically a map which is loaded with entries by a helper class.

public class Base {
    private final Map<String, Command> availableCommands;
    public Base() {
        availableCommands = Helper.loadCommands();  
    }
}

In the above example the helper class looks like this:

public class Helper {
    public static Map<String, Command> loadCommands() {
        Map<String, Command> commands = new HashMap<String, Command>();
        commands.put("A", new CommandA());
        commands.put("B", new CommandB());
        commands.put("C", new CommandC());

        return commands;
    }
}

My thought is, that is better practice to use a method to set such a variable in the constructor. So Base class would look something like this:

public class Base {
    private final Map<String, Command> availableCommands;
    public Base() {
        this.setCommands();  
    }
    private void setCommands() {
        this.availableCommands = Helper.loadCommands();
    }
}

But now I cannot maintain the final modifier and get a compiler error (Final variable cannot be set)

Another way to do this would be:

public class Base {
    private final Map<String, Command> availableCommands = new HashMap<String, Command>();
    public Base() {
        this.setCommands();
    }
    private void setCommands() {
        Helper.loadCommands(availableCommands);
    }
}

But in this case the method in the Helper class would change to:

public static void loadCommands(Map<String, Command> commands) {
    commands.put("A", new CommandA());
    commands.put("B", new CommandB());
    commands.put("C", new CommandC());
}

So the difference is where do I create a new map with new HashMap<String, Command>();? My main question is if there is a recommended way to do this, given that part of the functionality comes from this Helper's static method, as a way to load the actual map with entries?

Do I create the new map in my Base class or the Helper class? In both cases Helper will do the actual loading and Base's reference to the map holding the concrete commands will be private and final.

Are there perhaps other more elegant ways to do this besides the options I am considering?

Answer 1

1. If you want it inmutable you do not need to use 3rd party APIs, you can use: java.util.Collections.unmodifiableMap(Map m)

2. The most common way to do this would be:

public class Base {
private final Map availableCommands;
public Base(){
  availableCommands=new HashMap(); // or any other kind of map that you wish to load
  availableCommands = Helper.loadCommands(availableCommands);  
 }
}

Answer 2

It seems entirely reasonable to me for the helper class to create the map, as per your first code snippet. You are setting the variable in the constructor - I can't see the problem.

As yawn says, making the map immutable would be a nice touch here, but other than that I'd just use the code from the first snippet.

(I assume in real life this really needs to be an instance variable, rather than a static one, by the way?)

Answer 3

If you want such maps to be immutable have a look at the Google Collection API. To quote the linked documentation:

static final ImmutableMap<String, Integer> WORD_TO_INT =
       new ImmutableMap.Builder<String, Integer>()
           .put("one", 1)
           .put("two", 2)
           .put("three", 3)
           .build();

Answer 4

Have you considered using a Builder pattern like the one in Effective Java 2nd ed.?

You could capture all the map construction logic in one place (thus you wouldn't have 2 separate classes to maintain). Base would look like this:

public class Base {

    private final Map<String, Command> commands;

    private Base(Builder b) {
        commands = b.commands;
    }

    public static class Builder() {

        private final Map<String, Command> commands;

        public Builder() {
            commands = new HashMap<String, Command>();
        }

        public Builder addCommand(String name, Command c) {
            commands.put(name, c);
            return this;
        }

        public Base build() {
            return new Base(this);
        }
    }
}

Clients of Base would now work like this:

Base b = new Base.Builder().addCommand("c1", c1).addCommand("c2", c2).build();

Upshot is that the client class doesn't need to know that they need to build a Map and you could essentially build it all with 1 line. Downside is that Base cannot be extended because the constructor is private now (maybe you want that, maybe you don't).

EDIT: Had a goof in build() where I passed commands instead of this as I originally intended EDIT2: Mistakenly called add instead of put in Base.Builder.addCommand

Answer 5

Why don't you just do

private final Map<String, Command> availableCommands = Helper.loadCommands();  

?

Answer 6

I personally would rename the Helper class to something like CommandHolder:

public class CommandHolder {
    private static Map<String, Command> availableCommands;
    private static CommandHolder instance;

    private CommandHolder{}
    public static synchronized Map<String, Command> getCommandMap() {
        if (instance == null) {
            instance = new CommandHolder();
            instance.load();
        }
        return availableCommands
    }
    private void load() {
        ...
    }
}

synchronized to make sure the loading takes place only once. No getCommand because that one must then also be synchronized, and each lookup would be more expensive. I assume the map is read-only, otherwise you'd need a synchronizedMap anyway in a multi-threaded environment.

Answer 7

You can also use double-brace initialization, although whether or not you think it's cleaner is probably a matter of taste, but at least has the benefit of having all the initialization code in one place:

public class Base {
    public final Map< String, Command > availableCommands;

    public Base() {
        availableCommands = Collections.unmodifiableMap( new HashMap() {
            {
                put( "A", new CommandA() );
                put( "B", new CommandB() );
            }
        } );
    }
}