4

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).

7

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.

| improve this answer | |
  • I would prefer to make constants even from "single use" cases - I have seen too much cases where a literal was used, and then the literal was typed in a bazillion places - better start with a constant to encourage usage of that one. Additionally, constants are better to document than string literals - for example "Remember if the table definition changes, change this key too!" – Christian Sauer Mar 25 '16 at 15:31
  • 4
    Of course you do. But look at the guy's code; it's twice as long with the constants, and if the reason for having constants doesn't exist, it's basically filler. I was pretty clear in my answer that, if there is ever any chance that the string will be used twice, it needs to be in a constant. – Robert Harvey Mar 25 '16 at 15:33
  • Just to be clear: classes are longer, about 300 lines on the average. Sometimes there are 20 map keys at the beginning of the class body. I can't say there is no chance key won't be used twice. It happens but very rarely. I try to follow YAGNI principle, so I would extract constant when I use key more times. – Karol Lewandowski Mar 25 '16 at 18:25
  • If you're making the map public so you can get values from it from your whole codebase, I'd probably argue that you should expose values via properties/methods rather than letting people access stuff via non-typesafe keys. this is arguable depending on your use cases though, but it's something to consider. – sara Mar 26 '16 at 14:18
  • hmm well the keys would probably be typesafe I guess, I meant that there's no guarantee no one won't do a lookup on "lol this key doesn't exist :pp", so it'd be better to define the possible values to lookup via the api – sara Mar 26 '16 at 14:23
2

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.

| improve this answer | |
  • In my real project such classes create and render JSON objects. Values are evaluated basing on complex algorithms or retrieved from database. I will correct key names to make them more abstract. I agree that they can be confusing now. – Karol Lewandowski Mar 25 '16 at 13:56
  • 1
    Well, assuming that your design in general is sound, I'd still say it's a lot better to represent a key by a constant symbol rather than by a literal. Ignoring the advantages like mitigating the risk of spelling errors, letting you change the value of the symbol rather than searching up each individual literal and so on, it's just good practice. If you feel like you get few benefits today, this lays a good foundation for tomorrow. If you put a key into a map, you'll get it out at some point, so you're almost guaranteed 2 usages. DRY kicks in right away. To me a const beats a literal any day. – sara Mar 25 '16 at 14:15
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.

| improve this answer | |
  • even in dynamic languages, stuff like this can be caught sometimes. running JS in strict mode for example. but it's true that some advantages are lost! – sara Mar 26 '16 at 14:17

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.