Using if-condition or HashMap?

Question

I was asked this question in an interview to improve the code that was provided. The provided code used lot of if statements and therefore I decided to use HashMap as retrieval would be faster. Unfortunately, I was not selected for the position. I am wondering if someone knows a better way than what I did to improve the code?

/* The following Java code is responsible for creating an HTML "SELECT" list of
   U.S. states, allowing a user to specify his or her state. This might be used,
   for instance, on a credit card transaction screen. 

   Please rewrite this code to be "better". Submit your replacement code, and 
   please also submit a few brief comments explaining why you think your code 
   is better than the sample.  (For brevity, this sample works for only 5 
   states. The real version would need to work for all 50 states. But it is 
   fine if your rewrite shows only the 5 states here.)
 */

/* Generates an HTML select list that can be used to select a specific U.S. 
   state.
 */

public class StateUtils {

  public static String createStateSelectList() {

    return
      "<select name=\"state\">\n"
    + "<option value=\"Alabama\">Alabama</option>\n"
    + "<option value=\"Alaska\">Alaska</option>\n"
    + "<option value=\"Arizona\">Arizona</option>\n"
    + "<option value=\"Arkansas\">Arkansas</option>\n"
    + "<option value=\"California\">California</option>\n"
    // more states here
    + "</select>\n"
    ;
  }


  /* Parses the state from an HTML form submission, converting it to the 
     two-letter abbreviation. We need to store the two-letter abbreviation 
     in our database.
   */

  public static String parseSelectedState(String s) {
    if (s.equals("Alabama"))     { return "AL"; }
    if (s.equals("Alaska"))      { return "AK"; }
    if (s.equals("Arizona"))     { return "AZ"; }
    if (s.equals("Arkansas"))    { return "AR"; }
    if (s.equals("California"))  { return "CA"; }
    // more states here
  }

  /* Displays the full name of the state specified by the two-letter code. */

  public static String displayStateFullName(String abbr) {
  {
    if (abbr.equals("AL")) { return "Alabama";    }
    if (abbr.equals("AK")) { return "Alaska";     }
    if (abbr.equals("AZ")) { return "Arizona";    }
    if (abbr.equals("AR")) { return "Arkansas";   }
    if (abbr.equals("CA")) { return "California"; }
    // more states here
  }
}

My solution

/* Replacing the various "if" conditions with Hashmap<key, value> combination 
   will make the look-up in a constant time while using the if condition 
   look-up time will depend on the number of if conditions.
 */

import java.util.HashMap;

public class StateUtils {

  /* Generates an HTML select list that can be used to select a specific U.S. 
     state.
   */
  public static String createStateSelectList() {
    return "<select name=\"state\">\n"
    + "<option value=\"Alabama\">Alabama</option>\n"
    + "<option value=\"Alaska\">Alaska</option>\n"
    + "<option value=\"Arizona\">Arizona</option>\n"
    + "<option value=\"Arkansas\">Arkansas</option>\n"
    + "<option value=\"California\">California</option>\n"
    // more states here
    + "</select>\n";
  }

  /* Parses the state from an HTML form submission, converting it to the 
     two-letter abbreviation. We need to store the two-letter abbreviation 
     in our database.
   */

  public static String parseSelectedState(String s) {
    HashMap<String, String> map = new HashMap<String, String>();
    map.put("Alabama", "AL");
    map.put("Alaska", "AK");
    map.put("Arizona", "AZ");
    map.put("Arkansas", "AR");
    map.put("California", "CA");

    // more states here

    String abbr = map.get(s);
    return abbr;
  }

  /* Displays the full name of the state specified by the two-letter code. */

  public static String displayStateFullName(String abbr) {
    {

      HashMap<String, String> map2 = new HashMap<String, String>();
      map2.put("AL", "Alabama");
      map2.put("AK", "Alaska");
      map2.put("AZ", "Arizona");
      map2.put("AR", "Arkansas");
      map2.put("CA", "California");

      // more state abbreviations here here

      String full_name = map2.get(abbr);
      return full_name;
    }
  }
}

Answer 1

I think there are many things wrong with your code, not least the recreation of the Map for each method call.

I would start at the very beginning, with interfaces. We need two things; a State and a StateResolver. The interfaces would look like this:

public interface State {

    String fullName();

    String shortName();
}

public interface StateResolver {

    State fromFullName(final String fullName);

    State fromShortName(final String shortName);

    Set<? extends State> getAllStates();
}

This allows the implementation to be swapped out for something more sensible at a later stage, like a database. But lets stick with the hardcoded states from the example.

I would implement the State as an enum like so:

public enum StateData implements State {

    ALABAMA("Alabama", "AL"),
    ALASKA("Alaska", "AK"),
    ARIZONA("Arizona", "AZ"),
    ARKANSAS("Arkansas", "AR"),
    CALIFORNIA("Californiaa", "CA");

    private final String shortName;
    private final String fullName;

    private StateData(final String shortName, final String fullName) {
        this.shortName = shortName;
        this.fullName = fullName;
    }

    @Override
    public String fullName() {
        return fullName;
    }

    @Override
    public String shortName() {
        return shortName;
    }
}

But, as mentioned above, this can be replaced with a bean loaded from a database. The implementation should be self-explanatory.

Next onto the resolver, lets write one against our enum:

public final class EnumStateResolver implements StateResolver {

    private final Set<? extends State> states;
    private final Map<String, State> shortNameSearch;
    private final Map<String, State> longNameSearch;

    {
        states = Collections.unmodifiableSet(EnumSet.allOf(StateData.class));
        shortNameSearch = new HashMap<>();
        longNameSearch = new HashMap<>();
        for (final State state : StateData.values()) {
            shortNameSearch.put(state.shortName(), state);
            longNameSearch.put(state.fullName(), state);
        }
    }

    @Override
    public State fromFullName(final String fullName) {
        final State s = longNameSearch.get(fullName);
        if (s == null) {
            throw new IllegalArgumentException("Invalid state full name " + fullName);
        }
        return s;
    }

    @Override
    public State fromShortName(final String shortName) {
        final State s = shortNameSearch.get(shortName);
        if (s == null) {
            throw new IllegalArgumentException("Invalid state short name " + shortName);
        }
        return s;
    }

    @Override
    public Set<? extends State> getAllStates() {
        return states;
    }

}

Again this is self explanatory. Variables are at the instance level. The only dependency on the StateData class is in the initialiser block. This would obviously need to be rewritten for another State implementation but that should be not big deal. Notice this class throws an IllegalArgumentException if the state is invalid - this would need to be handled somewhere, somehow. It is unclear where this would happen but something that needs to be considered.

Finally we implement the required methods in the class

public final class StateUtils {

    private static final StateResolver STATE_RESOLVER = new EnumStateResolver();
    private static final String OPTION_FORMAT = "<option value=\"%1$s\">%1$s</option>\n";

    public static String createStateSelectList() {
        final StringBuilder sb = new StringBuilder();
        sb.append("<select name=\"state\">\n");
        for (final State s : STATE_RESOLVER.getAllStates()) {
            sb.append(String.format(OPTION_FORMAT, s.fullName()));
        }
        sb.append("</select>\n");
        return sb.toString();
    }

    public static String parseSelectedState(final String s) {
        return STATE_RESOLVER.fromFullName(s).shortName();
    }

    public static String displayStateFullName(final String abbr) {
        return STATE_RESOLVER.fromShortName(abbr).fullName();
    }
}

Notice we only reference the implementation at the top of the utility class, this makes swapping out the implementation quick and painless. We use a static final reference to that the StateResolver is created once and only once. I have also replaced the hardcoded creation of the select with a dynamic loop based one. I have also used a formatter to build the select.

It should be noted that it is never a good idea to build HTML in Java and anyone that does so should have unspeakable things done to them.

Needless to say you should have thorough unit tests against each and every line of the above code.

In short your answer doesn't really come close to a proper, extensible, enterprise solution to the problem at hand. My solution might seem overkill, and you may be right. But I think it's the correct approach because abstraction is key to reusable code.

Answer 2

The only mistake you made was to rebuild the map every time around. If you had built the Map just once - perhaps in a constructor I suspect you would have done fine.

public class StateUtils {

    class State {

        final String name;
        final String abbreviation;

        public State(String name, String abbreviation) {
            this.name = name;
            this.abbreviation = abbreviation;
        }
    }
    final List<State> states = new ArrayList<State>();

    {
        states.add(new State("Alabama", "AL"));
        states.add(new State("Alaska", "AK"));
        states.add(new State("Arizona", "AZ"));
        states.add(new State("Arkansas", "AR"));
        states.add(new State("California", "CA"));
    }
    final Map<String, String> nameToAbbreviation = new HashMap<String, String>();

    {
        for (State s : states) {
            nameToAbbreviation.put(s.name, s.abbreviation);
        }
    }
    final Map<String, String> abbreviationToName = new HashMap<String, String>();

    {
        for (State s : states) {
            nameToAbbreviation.put(s.abbreviation, s.name);
        }
    }

    public String getStateAbbreviation(String s) {
        return nameToAbbreviation.get(s);
    }

    public String getStateName(String abbr) {
        return abbreviationToName.get(abbr);
    }
}

Answer 3

To avoid manually maintaining 2 maps and keeping them in sync I would just create the second one as the first one inverted. See here on how to do it.

Also as pointed out by others you need to create your maps only once outside of method call.

** Just for fun a way to do it in Scala **

val m = Map("AL" -> "Alabama", "AK" -> "Alaska")
m map { case (k, v) => (v, k) }
// gives: Map(Alabama -> AL, Alaska -> AK)

Answer 4

Everyone seems focused on the parse, but the create can be improved, too. Get all of the state names, sort them alphabetically, and iterate over that collection to create each option. That way, the states used for parsing are always in sync with the states used for cresting. If you add a new state, you only need to add it to the "master" Enum (or whatever), and both methods will reflect the change.

Answer 5

One thing I don't like about your code is that you create a hashmap each time the method is called. The map should be created just once, at class init time, and referenced from the method.

Answer 6

What you did wrong is what guys are saying - you are creating a new HashMap every time the method is invoked - a static field could rather congaing the data, and populating it only once the class is being loaded my the JVM.

I'd rather use simple switch on strings - the search is not worse than that of HashMap (at least asymptotically) but you don't use extra memory. Though you need two long switches - more code.

But than again HashMap solution the the later one would be the same for me.