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

Question

There are different ways to set a member variable from a constructor. I am actually discussing how to properly set the final member variable, in particular the map that is loaded by the helper class elements.

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

In the above example, the helper class is as follows:

 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 it is better to use a method to set such a variable in the constructor. So, the base class will 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 can not save the final modifier and get a compiler error (the final variable cannot be set)

Another way to do this:

 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 Helper class method 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 I can create a new map using new HashMap<String, Command>(); ? . My main question is: if there is a recommended way to do this, given that some functions come from this helper static method, how is the way to load the actual map using records?

Create a new map in a base class or Helper class? In both cases, Helper will actually load, and the base link to a map containing specific commands will be closed and final.

Are there even more elegant ways to do this other than the options I'm considering?

Answer 1

• If you want this to be impossible, you do not need to use third-party APIs, you can use: java.util.Collections.unmodifiableMap(Map m)

• The most common way to do this:

 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 to me quite reasonable for the helper class to create the map according to your first code snippet. You set a variable in the constructor - I don't see a problem.

As the throat says, making the map unchanged will be a nice touch here, but apart from that, I would just use the code from the first fragment.

(I assume that in real life it really should be an instance variable, and not static, by the way?)

Answer 3

If you want these cards to be unchanged, check out the Google Collection API . To quote related 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

Do you consider using the Template Builder as in Effective Java 2nd ed. ?

You could capture all the logic of building a map in one place (thus, you would not have two separate classes for support). The base will 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); } } } 

Base clients will now work as follows:

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

Try so that the client class does not need to create a map, and you could build it all with 1 line. The disadvantage is that the base cannot be expanded because the constructor is now closed (maybe you want it, maybe not).

EDIT: if goof in build (), where I passed commands instead, as I originally planned 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 would personally 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() { ... } } 

to ensure that the download occurs only once. There is no getCommand, because you must also synchronize, and each search will be more expensive. I assume that the card is read-only, otherwise you will need a synchronized card anyway in a multi-threaded environment.

Answer 7

You can also use dual-binding initialization, although whether you think it is cleaner is probably a matter of taste, but at least it has the advantage 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() ); } } ); } }