Is it good practice to define private constant strings that have the same name as their values? Take the following code for example.
public class Example {
private static final String FIRST_KEY = "firstKey";
private static final String SECOND_KEY = "secondKey";
private static final String THIRD_KEY = "thirdKey";
private static final String SOME_KEY = "someKey";
private static final String SOME_OTHER_KEY = "someOtherKey";
private static final String BLAH = "blah";
public Map<String, Object> createMap() {
Map<String, Object> map = new HashMap<>();
map.put(FIRST_KEY, getFromDB());
map.put(SECOND_KEY, computeValue2());
map.put(THIRD_KEY, computeValue3());
map.put(SOME_KEY, "any value");
map.put(SOME_OTHER_KEY, new Object());
map.put(BLAH, new Object());
return map;
}
...
}
I believe this does not add any value when they are used only once (or maybe even twice). They are definitely harder to read when I’m checking some JSON generated content from the map. I need to jump to the constant definition to make sure that JSON key and map key are actually the same. This convention was used in many projects I worked on, and I have yet to understand why. Simpler refactoring should not be the reason as long as keys are not used too many times. It’s overuse of string constants in my opinion. Private string constants are helpful, when their names really explain content but in this example they do not (and are basically just copies).
4
String constants are redundant, if you only ever use them once. But as soon as you have two usages (even in the same class), it’s better to have the string constant.
If the string constant is only used once, but you need to identify the string in your code for possible future uses, then it’s better to have the string constant.
If those strings are only ever going to be used to define the map, I’d say “no.” Just put them in the map, directly. You might be mapping to a table in a database, for instance, and may never need to refer to the map directly in code.
If the strings are going to be used elsewhere in the program to look up items from the map, then yes, you need string constants. But then they need to be publicly defined, so that the rest of your code can access them.
6
I’d say a const string is better than a string literal, but neither are particularly good in most cases.
If you have a discrete set of related names that are logically interchangeable, use the type system. If a full new type is overkill, at least use an enumeration. Image sizes, for example, would be better modeled as an enumeration imo.
Another perspective is scope. Sure, it’s not very nice if a class starts with 20 rows of const declarations (unless it’s a class that’s actually meant for storing some constant values), but this could well just be a scoping problem. Unless the values are used globally, pull them down to private. Unless they’re used in the whole class, pull them down into local scope, or encapsulate them in their own type that can be referenced/passed around where it’s needed.
I definitely think your example code looks horrible, but this is not because of constants. It’s because it has abyssmal cohesion. There’s no focus and it’s not clear what the class is meant for, so it’s hard to say how it “should” look.
2
If you accidentally mistype a string literal, your code will compile happily and it won’t be until run-time when it starts to behave erratically. If instead you had used named constants, you would have gotten a compile-time error that points you exactly to the typo. Of course, you can still mistype the constant definition but at least, it will then be wrong consistently.
As Robert Harvey mentions, if you only need the constant once, there is nothing gained from using the named constant. But be aware of future additions to the code. I would add that in a dynamic language (one that is not statically syntax checked) the benefits are reduced, but it’s still better to get a run-time error that says a variable is used before definition than having the code silently behave wrongly.
1