[ACCEPTED]-Are hard-coded STRINGS ever acceptable?-hard-coding

If I use a string once in the code, I don't 5 generally worry about making it a constant 4 somewhere.

If I use a string twice in the 3 code, I'll consider making it a constant.

If I use 2 a string three times in the code, I'll almost 1 certainly make it a constant.

if (config_options.isTrue('FOO_ENABLED')) {...
}

Restrict your hard coded Y check to one 8 place, even if it means writing a wrapper 7 class for your Map.

if (config_options.isFooEnabled()) {...
}

Might seem okay until 6 you have 100 configuration options and 100 5 methods (so here you can make a judgement 4 about future application growth and needs 3 before deciding on your implementation). Otherwise 2 it is better to have a class of static strings 1 for parameter names.

if (config_options.isTrue(ConfigKeys.FOO_ENABLED)) {...
}

I really should use constants and no hard 4 coded literals.

You can say they won't be 3 changed, but you may never know. And it 2 is best to make it a habit. To use symbolic 1 constants.

In my experience, this kind of issue is 43 masking a deeper problem: failure to do 42 actual OOP and to follow the DRY principle.

In 41 a nutshell, capture the decision at startup 40 time by an appropriate definition for each 39 action inside the if statements, and then throw 38 away both the config_options and the run-time tests.

Details 37 below.

The sample usage was:

if (config_options.value('FOO_ENABLED') == 'Y') ...

which raises 36 the obvious question, "What's going on in 35 the ellipsis?", especially given the following 34 statement:

(Of course, this same option may 33 need to be checked in many places in the 32 system code.)

Let's assume that each of these 31 config_option values really does correspond to a single 30 problem domain (or implementation strategy) concept.

Instead 29 of doing this (repeatedly, in various places 28 throughout the code):

1. Take a string (tag),
2. Find its corresponding other string (value),
3. Test that value as a boolean-equivalent,
4. Based on that test, decide whether to perform some action.

I suggest encapsulating 27 the concept of a "configurable action".

Let's 26 take as an example (obviously just as hypthetical 25 as FOO_ENABLED ... ;-) that your code has to work in 24 either English units or metric units. If 23 METRIC_ENABLED is "true", convert user-entered data from 22 metric to English for internal computation, and 21 convert back prior to displaying results.

Define 20 an interface:

public interface MetricConverter {
    double toInches(double length);
    double toCentimeters(double length);
    double toPounds(double weight);
    double toKilograms(double weight);
}

which identifies in one place 19 all the behavior associated with the concept 18 of METRIC_ENABLED.

Then write concrete implementations 17 of all the ways those behaviors are to be 16 carried out:

public class NullConv implements MetricConverter {
    double toInches(double length) {return length;}
    double toCentimeters(double length) {return length;}
    double toPounds(double weight)  {return weight;}
    double toKilograms(double weight)  {return weight;}
}

and

// lame implementation, just for illustration!!!!
public class MetricConv implements MetricConverter {
    public static final double LBS_PER_KG = 2.2D;
    public static final double CM_PER_IN = 2.54D
    double toInches(double length) {return length * CM_PER_IN;}
    double toCentimeters(double length) {return length / CM_PER_IN;}
    double toPounds(double weight)  {return weight * LBS_PER_KG;}
    double toKilograms(double weight)  {return weight / LBS_PER_KG;}
}

At startup time, instead 15 of loading a bunch of config_options values, initialize 14 a set of configurable actions, as in:

MetricConverter converter = (metricOption()) ? new MetricConv() : new NullConv();

(where 13 the expression metricOption() above is a stand-in for 12 whatever one-time-only check you need to 11 make, including looking at the value of 10 METRIC_ENABLED ;-)

Then, wherever the code 9 would have said:

double length = getLengthFromGui();
if (config_options.value('METRIC_ENABLED') == 'Y') {
    length = length / 2.54D;
}
// do some computation to produce result
// ...
if (config_options.value('METRIC_ENABLED') == 'Y') {
    result = result * 2.54D;
}
displayResultingLengthOnGui(result);

rewrite it as:

double length = converter.toInches(getLengthFromGui());
// do some computation to produce result
// ...
displayResultingLengthOnGui(converter.toCentimeters(result));

Because all 8 of the implementation details related to 7 that one concept are now packaged cleanly, all 6 future maintenance related to METRIC_ENABLED can be done 5 in one place. In addition, the run-time 4 trade-off is a win; the "overhead" of invoking 3 a method is trivial compared with the overhead 2 of fetching a String value from a Map and 1 performing String#equals.

I realise the question is old, but it came 44 up on my margin.

AFAIC, the issue here has 43 not been identified accurately, either in 42 the question, or the answers. Forget about 41 'harcoding strings" or not, for a moment.

1. The 40 database has a Reference table, containing 39 config_options. The PK is a string.

2. There are two types 38 of PKs:

   • Meaningful Identifiers, that the 37 users (and developers) see and use. These 36 PKs are supposed to be stable, they can 35 be relied upon.

   • Meaningless Id columns which 34 the users should never see, that the developers 33 have to be aware of, and code around. These 32 cannot be relied upon.

3. It is ordinary, normal, to 31 write code using the absolute value of a 30 meaningful PK IF CustomerCode = "IBM" ... or IF CountryCode = "AUS" etc.

   • referencing the absolute value of a meaningless PK is not acceptable (due to auto-increment; gaps being changed; values being replaced wholesale).
     .

4. Your reference table 29 uses meaningful PKs. Referencing those literal 28 strings in code is unavoidable. Hiding 27 the value will make maintenance more difficult; the 26 code is no longer literal; your colleagues 25 are right. Plus there is the additional 24 redundant function that chews cycles. If 23 there is a typo in the literal, you will 22 soon find that out during Dev testing, long 21 before UAT.

   • hundreds of functions for hundreds 20 of literals is absurd. If you do implement 19 a function, then Normalise your code, and 18 provide a single function that can be used 17 for any of the hundreds of literals. In 16 which case, we are back to a naked literal, and 15 the function can be dispensed with.

   • the point 14 is, the attempt to hide the literal has 13 no value.
     .

5. It cannot be construed as "hardcoding", that 12 is something quite different. I think that 11 is where your issue is, identifying these 10 constructs as "hardcoded". It is just referencing 9 a Meaningfull PK literally.

6. Now from the 8 perspective of any code segment only, if 7 you use the same value a few times, you 6 can improve the code by capturing the literal 5 string in a variable, and then using the 4 variable in the rest of the code block. Certainly 3 not a function. But that is an efficiency 2 and good practice issue. Even that does 1 not change the effect IF CountryCode = @cc_aus

I believe that the two reasons you have 9 mentioned, Possible misspelling in string, that 8 cannot be detected until run time and the 7 possibility (although slim) of a name change 6 would justify your idea.

On top of that you 5 can get typed functions, now it seems you 4 only store booleans, what if you need to 3 store an int, a string etc. I would rather 2 use get_foo() with a type, than get_string("FOO") or 1 get_int("FOO").

I think there are two different issues here:

• In the current project, the convention of using hard-coded strings is already well established, so all the developers working on the project are familiar with it. It might be a sub-optimal convention for all the reasons that have been listed, but everybody familiar with the code can look at it and instinctively knows what the code is supposed to do. Changing the code so that in certain parts, it uses the "new" functionality will make the code slightly harder to read (because people will have to think and remember what the new convention does) and thus a little harder to maintain. But I would guess that changing over the whole project to the new convention would potentially be prohibitively expensive unless you can quickly script the conversion.
• On a new project, symbolic constants are the way IMO, for all the reasons listed. Especially because anything that makes the compiler catch errors at compile time that would otherwise be caught by a human at run time is a very useful convention to establish.

0

Another thing to consider is intent. If 10 you are on a project that requires localization 9 hard coded strings can be ambiguous. Consider 8 the following:

const string HELLO_WORLD = "Hello world!";
print(HELLO_WORLD);

The programmer's intent is 7 clear. Using a constant implies that this 6 string does not need to be localized. Now 5 look at this example:

print("Hello world!");

Here we aren't so sure. Did 4 the programmer really not want this string 3 to be localized or did the programmer forget 2 about localization while he was writing 1 this code?

I too prefer a strongly-typed configuration 11 class if it is used through-out the code. With 10 properly named methods you don't lose any 9 readability. If you need to do conversions 8 from strings to another data type (decimal/float/int), you 7 don't need to repeat the code that does 6 the conversion in multiple places and can 5 cache the result so the conversion only 4 takes place once. You've already got the 3 basis of this in place already so I don't 2 think it would take much to get used to 1 the new way of doing things.