Использование if-condition или HashMap?

Question

Мне задали этот вопрос в интервью, чтобы улучшить код, который был предоставлен. Предоставленный код использовал много операторов if, и поэтому я решил использовать HashMap, поскольку поиск будет быстрее. К сожалению, я не был выбран для этой должности. Мне интересно, знает ли кто-нибудь лучший способ, чем то, что я сделал для улучшения кода?

/* 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 
    } 
} 

Мое решение

/* 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

Я думаю, что с вашим кодом есть много чего не так, не в последнюю очередь для отдыха Map для каждого вызова метода.

Я бы начал с самого начала с интерфейсов. Нам нужны две вещи; a State и a StateResolver. Интерфейсы будут выглядеть следующим образом:

public interface State { 

    String fullName(); 

    String shortName(); 
} 

public interface StateResolver { 

    State fromFullName(final String fullName); 

    State fromShortName(final String shortName); 

    Set<? extends State> getAllStates(); 
} 

Это позволяет реализация выгружена на что-то более разумное на более позднем этапе, как базы данных. Но давайте придерживаться жестко закодированных состояний из примера.

Я бы реализовать State как enum так:

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; 
    } 
} 

Но, как уже упоминалось выше, это может быть заменен боба загруженным из базы данных. Реализация должна быть понятной.

Далее на распознаватель, позволяет написать один против наших 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; 
    } 

} 

Опять же, это говорит само за себя. Переменные находятся на уровне экземпляра. Единственная зависимость от класса StateData находится в блоке инициализации. Это, очевидно, необходимо будет переписать для другой реализации State, но это не должно быть большой проблемой. Обратите внимание, что этот класс бросает IllegalArgumentException, если состояние недействительно - это должно быть каким-то образом обработано. Неясно, где это произойдет, но что-то, что нужно учитывать.

Наконец мы реализуем необходимые методы в классе

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(); 
    } 
} 

Уведомления мы только ссылаться на реализации в верхней части класса утилиты, это делает выгрузив осуществление быстрым и безболезненным. Мы используем ссылку static final, что StateResolver создается один раз и только один раз. Я также заменил hardcoded создание select с использованием динамического цикла. Я также использовал форматировщик для создания select.

Следует отметить, что никогда не было хорошей идеей для создания HTML-кода на Java, и любой, кто делает это, должен иметь неописуемые вещи.

Излишне говорить, что вы должны иметь тщательные единичные тесты против каждой строки вышеуказанного кода.

Короче говоря, ваш ответ на самом деле не приближается к правильному, расширяемому, корпоративному решению проблемы. Мое решение может показаться излишним, и вы можете быть правы. Но я думаю, что это правильный подход, потому что абстракция является ключом к повторному использованию кода.

Answer 2

Единственная ошибка, которую вы сделали было восстановить карту на каждый раз, когда вокруг. Если бы вы построили карту только один раз - возможно, в конструкторе, я подозреваю, что вы бы все сделали хорошо.

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

Одна вещь, которая мне не нравится в вашем коде, заключается в том, что вы создаете хэш-карту каждый раз, когда вызывается метод. Карта должна быть создана только один раз, во время запуска класса и ссылаться на метод.

Answer 4

Чтобы избежать ручного сохранения двух карт и сохранения их в синхронизации, я бы просто создал второй, как первый инвертированный. См. here о том, как это сделать.

Также, как указано другими, вам необходимо создать свои карты только один раз за пределами вызова метода.

** Просто для удовольствия способа сделать это в Scala **

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

Answer 5

То, что вы сделали неправильно, что ребята говорят - вы создаете новый HashMap каждый раз, когда вызывается метод - статическое поле может скорее обходить данные и заполнять их только после того, как класс загружается моей JVM.

Я бы предпочел использовать простой переключатель на строках - поиск не хуже, чем у HashMap (по крайней мере, асимптотически), но вы не используете дополнительную память. Хотя вам нужны два длинных переключателя - больше кода.

Но, чем снова решение HashMap, то более поздний вариант будет таким же для меня.

Answer 6

Все, кажется, сосредоточены на синтаксическом разборе, но создание также может быть улучшено. Получите все имена состояний, отсортируйте их по алфавиту и повторите по этой коллекции, чтобы создать каждый option. Таким образом, состояния, используемые для синтаксического анализа, всегда синхронизируются с состояниями, используемыми для гребня. Если вы добавляете новое состояние, вам нужно только добавить его в «Master» Enum (или что-то еще), и оба метода отражают это изменение.