9

While I was recently working at a big company, I noticed that the programmers there followed this coding style:

Suppose I have a function that returns 12 if the input is A, 21 if the input is B, and 45 if the input is C.

So I can write the function signature as:

int foo(String s){
    if(s.equals("A"))      return 12;
    else if(s.equals("B")) return 21;
    else if(s.equals("C")) return 45;
    else throw new RuntimeException("Invalid input to function foo");
}

But on code review I was asked to change the function to the following:

int foo(String s){
    HashMap<String, Integer> map = new HashMap<String, Integer>();
    map.put("A", 12);
    map.put("B", 21);
    map.put("C", 45);
    return map.get(s);
}

I can't convince myself on why the second code is better than the first. The second code would definitely take more time to run.

The only reason to use the second code can be that it offers better readability. But if the function is getting called many times then wouldn't the second function slow down the running time of the utility calling it?

What do you think about this?

  • 4
    For three values, a Map seems like overkill (switch seems more appropriate than if-else). But at some point, it becomes problematic. The main advantage of using a Map is that you can load it from a file or a table etc. If you are hard-coding the input to the map, I'm not seeing a lot of value over a switch. – JimmyJames Jan 19 '17 at 16:44
16

The point is to move the creation of the hashmap outside the function and do it once (or just less times than otherwise).

private static final Map<String, Integer> map;
static{
    Map<String, Integer> temp = new HashMap<String, Integer>();
    temp.put("A", 12);
    temp.put("B", 21);
    temp.put("C", 45);
    map = Collections.unmodifiableMap(temp);//make immutable
}

int foo(String s){
    if(!map.containsKey(s))
        throw new RuntimeException("Invalid input to function foo");

    return map.get(s);
}

However java has since java7 been able to have (final) strings in switches:

int foo(String s){
    switch(s){
    case "A":
        return 12;
    case "B": 
        return 21;
    case "C": 
        return 45;
    default: throw new RuntimeException("Invalid input to function foo");
}
  • 1
    I don't see how this answers OPs question, so -1 there. But +1 for suggesting switch. – user949300 Jan 19 '17 at 16:55
  • It shows how to properly implement the coding style to actually make sense and possibly improve performance. It still doesn't make sense for 3 choices, but the original code was probably much longer. – Florian F Jan 19 '17 at 20:50
12

In your second example, the Map should be a private static member to avoid redundant initialization overhead.

For large amounts of values, the map will perform better. Using a hashtable, one can look up the answer in constant time. The multiple-if construct has to compare the input to each of the possibilities until it finds the right answer.

In other words, the map lookup is O(1) while the ifs are O(n) where n is the number of possible inputs.

Map creation is O(n), but is only done once if it is static constant state. For a lookup performed frequently the map will outperform the if statements in the long run, at the cost of slightly more time when the program is starting up (or the class is loaded, depending on language).

That being said, the map is not always the right tool for this job. It is nice when there are a lot of values, or the values must be configurable through a text file, user input, or database (in which case the map acts as a cache).

  • Yes, for large amounts of values, the map will perform better. But the amount of values is fixed, and it's three. – RemcoGerlich Jan 19 '17 at 10:59
  • the creation of the map is O(N) only the searching in it is O(1). – Pieter B Jan 19 '17 at 11:46
  • Good points, I clarified my answer. – user22815 Jan 19 '17 at 14:43
  • The Map also requires auto- unboxing which affects performance very slightly. – user949300 Jan 19 '17 at 16:42
  • @user949300 in Java, yes, and the code in the question appears to be Java. However, it is not tagged with any language, and the approach works across multiple languages (including C# and C++, neither of which requires boxing). – user22815 Jan 19 '17 at 16:49
3

There are two speeds in software: the time it takes to write/read/debug the code; and the time it takes to execute the code.

If you can convince me (and your code reviewers) that the hashmap function is indeed slower than the if/then/else (after refactoring to make a static hashmap) AND you can convince me/reviewers that its called enough times to make an actual difference, then go ahead and replace the hashmap with the if/else.

Otherwise, the hashmap code is eminently readable; and (probably) bugfree; you can determine that quickly just by looking at it. You can't really say the same thing about the if/else without really studying it; the difference is even more exaggerated when there are hundreds of options.

  • 3
    Well, that objection falls apart when one compares with a switch instead... – Deduplicator Jul 14 '15 at 21:45
  • Also falls apart if you write the if-statements in a single line. – gnasher729 Jul 15 '15 at 6:37
  • 2
    By putting the hashmap creation somewhere else, you're just making it harder to figure out what actually happens. You need to see those keys and values to know what the actual effect of the function is. – RemcoGerlich Jan 19 '17 at 10:58
2

I greatly prefer the HashMap style answer.

There's a metric for this

There is a code quality metric called Cyclomatic Complexity. This metric basically counts the number of different paths through the code (how to compute Cyclomatic Complexity).

For every possible execution path a method becomes harder and harder to both understand AND fully test for correctness.

It boils down to the fact that "controlling keywords" like: ifs, elses, whiles, etc... leverage boolean tests that can be wrong. Repeated use of "controlling keywords" produces fragile code.

Additional Benefits

Also, the "map-based approach" encourages developers to think of the input-output pairs as a dataset that can be extracted, reused, manipulated at runtime, tested, and verified. For example, below I rewrote "foo" so that we are not permanently locked into "A->12, B->21, C->45":

int foo(String s){
    HashMap<String, Integer> map = getCurrentMapping();
    return map.get(s);
}

rachet_freak mentions this type of refactor in his answer, he argues for speed and reuse, I am arguing for runtime flexibility (although using a immutable collection can have enormous benefits depending on the situation)

  • 1
    Adding that runtime flexibility is either a great forward looking idea, or needless over-enginnering that makes it much harder to figure out what is happening. :-). – user949300 Jan 19 '17 at 16:50
  • @JimmyJames The link works for me: It is: leepoint.net/principles_and_practices/complexity/… – Ivan Jan 19 '17 at 16:55
  • 1
    @user949300 The point is that the key/value data backing the "foo" method is a separate concept that could merit some form of clarity. The amount of code you want to write to isolate the Map will depend greatly on how many items the Map contains and how often those items might need to change. – Ivan Jan 19 '17 at 17:04
  • 1
    @user949300 Part of the reason I suggest pulling out the if-else chain is that I have seen if-else chains exist in groups. Kinda of like roaches, if there is one method based on an if-else chain there is likely another method elsewhere in the code-base with a similar/identical if-else chain. Extracting the Map pays dividends if we assume there might be other methods that use a similar look-up / switch-like logic structure. – Ivan Jan 19 '17 at 17:11
  • I too have seen duplicate, almost identical if/else blocks scattered all over the place. Well put – Jon Chesterfield Jan 19 '17 at 17:18
1

Data is better than code. Not least because it's too tempting to add yet another branch to code, yet adding a row to a table is tough to get wrong. This question is a small instance of this. You're writing a lookup table. Either write an implementation, complete with conditional logic and documentation, or write the table out then lookup in it.

A table of data is always a better representation of some data than code is, modulo optimisation passes. How difficult the table is to express may be language dependent - I don't know Java, but would hope it can implement a look up table more simply than the example in the OP.

This is a lookup table in python. If this is viewed as inviting conflict, please consider that the question is not tagged java, the refactoring is language agnostic and that most people don't know java.

def foo(s):
    return {
               "A" : 12,
               "B" : 21,
               "C" : 45,
           }[s]

The idea of restructuring the code to reduce runtime has merit, but I'd much rather use a compiler that hoists the common setup than do so myself.

  • -1 not an answer to the question. – Pieter B Jan 19 '17 at 11:41
  • In what sense? I would have asked the author to replace the if else chain with a map on the basis that data and code should be separate. This is a clear reason why the second code is better than the first, though all the map.put calls are unfortunate. – Jon Chesterfield Jan 19 '17 at 14:15
  • 2
    @JonChesterfield This answer is basically "use a better language", which is rarely helpful. – walpen Jan 19 '17 at 14:56
  • @walpen fair point. Ivan has made roughly the same observation via Java so I didn't clearly need to drop to python. I'll see if I can clean it up a little – Jon Chesterfield Jan 19 '17 at 17:05
  • In some older java code, I had need of a few maps for something very similar, so wrote a little utility to convert N-dimensional arrays of 2-D arrays into a map. A bit hacky but worked well. This answer points out the power of Python / JS in supporting JSONy notation so simply and easily. – user949300 Jan 19 '17 at 21:04

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.