Является ли создание исключения анти-паттерном здесь?

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

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

В обсуждаемом решении использовался следующий шаблон (ПРИМЕЧАНИЕ: это не настоящий код, очевидно, он упрощен для наглядности):

public class Preferences {
    // null values are legal
    private Map<String, String> valuesFromDatabase;

    private static Map<String, String> defaultValues;

    class KeyNotFoundException extends Exception {
    }

    public String getByKey(String key) {
        try {
            return getValueByKey(key);
        } catch (KeyNotFoundException e) {
            String defaultValue = defaultValues.get(key);
            valuesFromDatabase.put(key, defaultvalue);
            return defaultValue;
        }
    }

    private String getValueByKey(String key) throws KeyNotFoundException {
        if (valuesFromDatabase.containsKey(key)) {
            return valuesFromDatabase.get(key);
        } else {
            throw new KeyNotFoundException();
        }
    }
}

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

По сути, это два метода, которые можно использовать, просто чтобы что-то сообщить друг другу.

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

Контраргумент состоял в том, что getValueByKey- частный метод - как определено прямо сейчас, не имеет естественного способа информировать публичный метод как о значении, так и о наличии ключа. (Если это не так, его нужно добавить, чтобы значение могло быть обновлено).

Возвращение nullбыло бы неоднозначным, так nullкак это совершенно законное значение, поэтому неясно, означало ли это, что ключа не было, или был null.

getValueByKeyпришлось бы вернуть что-то вроде a Tuple<Boolean, String>, bool установлен в true, если ключ уже существует, чтобы мы могли различать (true, null)и (false, null). ( outПараметр может быть использован в C #, но это Java).

Это хорошая альтернатива? Да, вам нужно определить какой-то одноразовый класс для эффекта Tuple<Boolean, String>, но затем мы избавляемся от него KeyNotFoundException, так что это уравновешивает. Мы также избегаем накладных расходов на обработку исключения, хотя это не имеет большого значения с практической точки зрения - здесь нет никаких соображений по поводу производительности, это клиентское приложение, и пользовательские настройки не будут извлекаться миллионы раз в секунду.

Вариант этого подхода может использовать Guava Optional<String>(Guava уже используется в проекте) вместо некоторого пользовательского Tuple<Boolean, String>, и тогда мы можем различить Optional.<String>absent()и «надлежащий» null. Это все еще кажется хакерским, хотя, по причинам, которые легко увидеть - введение двух уровней «нули», кажется, злоупотребляет концепцией, стоящей за созданием Optionals в первую очередь.

Другой вариант - явно проверить, существует ли ключ (добавить boolean containsKey(String key)метод и вызывать getValueByKeyтолько в том случае, если мы уже заявили, что он существует).

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

Я, может быть, и тут раскалываюсь, но мне любопытно, на что бы вы поставили, чтобы быть ближе к наилучшей практике в этом случае. Я не нашел ответа в руководствах по стилю Oracle или Google.

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

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

Это намного чище, чем ваша версия ( getValueByKey()метод удален):

public String getByKey(String key) {
    if (valuesFromDatabase.containsKey(key)) {
        return valuesFromDatabase.get(key);
    } else {
        String defaultValue = defaultValues.get(key);
        valuesFromDatabase.put(key, defaultvalue);
        return defaultValue;
    }
}

Исключение следует выдавать только в том случае, если вы не знаете, как устранить ошибку локально.

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

Лучшее решение (при условии, что вы все еще на Java 7) будет использовать опциональный Guava; Я не согласен, что его использование в этом случае было бы хакерским. Мне кажется, основываясь на расширенном объяснении Optional в Guava , что это прекрасный пример того, когда его использовать. Вы различаете «значение не найдено» и «значение найдено нулем».

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

Подход с кортежем немного хакерский, потому что второе поле не имеет смысла, когда логическое значение ложно. Проверять, существует ли ключ заранее, глупо, потому что карта ищет ключ дважды. (Ну, вы уже делаете это, так что в этом случае трижды.) OptionalРешение идеально подходит для этой проблемы. Может показаться немного ироничным хранить a nullв Optional, но если это то, что пользователь хочет сделать, вы не можете сказать нет.

Как отметил Майк в комментариях, есть проблема с этим; ни Guava, ни Java 8 не Optionalпозволяют хранить nulls. Таким образом, вам нужно будет свернуть свой собственный, который - хотя и простой - включает в себя немалое количество шаблонов, так что может быть излишним для того, что будет использоваться только один раз внутри страны. Вы также можете изменить тип карты на Map<String, Optional<String>>, но обработка Optional<Optional<String>>s становится неудобной.

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

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

Проблема именно в этом. Но вы уже опубликовали решение самостоятельно:

Вариант этого подхода может использовать необязательный Guava (Guava уже используется в проекте) вместо некоторого настраиваемого кортежа, и тогда мы можем различить Optional.absent () и «правильный» ноль . Это все еще кажется хакерским, хотя, по причинам, которые легко увидеть - введение двух уровней «нулевой», кажется, злоупотребляет концепцией, которая стояла за созданием Optionals, во-первых.

Однако не используйте null или Optional . Вы можете и должны использовать Optionalтолько. Для вашего «хакерского» ощущения, просто используйте его вложенным, чтобы вы в конечном итоге дали понять, что Optional<Optional<String>>в базе данных может быть ключ (первый уровень параметров) и что он может содержать предопределенное значение (второй уровень параметров).

Этот подход лучше, чем использование исключений, и его довольно легко понять, если Optionalон для вас не нов.

Также обратите внимание, что он Optionalимеет некоторые функции комфорта, так что вам не придется делать всю работу самостоятельно. Они включают:

• static static <T> Optional<T> fromNullable(T nullableReference)преобразовать входные данные вашей базы данных в Optionalтипы
• abstract T or(T defaultValue) чтобы получить значение ключа внутреннего слоя параметров или (если его нет) получить значение ключа по умолчанию

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

Посмотрим, как реализована реализация Properties.getProperty(String)(из Java 7):

Object oval = super.get(key);
String sval = (oval instanceof String) ? (String)oval : null;
return ((sval == null) && (defaults != null)) ? defaults.getProperty(key) : sval;

Как вы цитировали, на самом деле нет необходимости «злоупотреблять исключениями для контроля потока».

Аналогичный, но несколько более краткий подход к ответу @ BЈовић также может быть следующим:

public String getByKey(String key) {
    if (!valuesFromDatabase.containsKey(key)) {
        valuesFromDatabase.put(key, defaultValues.get(key));
    }
    return valuesFromDatabase.get(key);
}

Хотя я думаю, что ответ @ BЈовић в порядке, если getValueByKeyон больше нигде не нужен, я не думаю, что ваше решение плохое, если ваша программа содержит оба варианта использования:

• поиск по ключу с автоматическим созданием в случае, если ключ не существует заранее, и

• поиск без этого автоматизма, без каких-либо изменений в базе данных, хранилище или карте ключей (подумайте о том, чтобы быть getValueByKeyпубличным, а не частным)

Если это ваша ситуация и пока приемлемый результат производительности, я думаю, что ваше предлагаемое решение полностью в порядке. У него есть преимущество, заключающееся в том, что он позволяет избежать дублирования поискового кода, он не опирается на сторонние фреймворки и довольно прост (по крайней мере, на мой взгляд).

Фактически, в такой ситуации зависит от контекста, является ли отсутствующий ключ «исключительной» ситуацией или нет. Для контекста, где это, getValueByKeyнужно что-то подобное . Для контекста, где ожидается автоматическое создание ключа, имеет смысл предоставить метод, который повторно использует уже доступную функцию, проглатывает исключение и обеспечивает другое поведение при сбое. Это можно интерпретировать как расширение или «декоратор» getValueByKey, а не как функцию, где «исключение используется для потока управления».

Конечно, существует третий вариант: создайте третий закрытый метод, возвращающий Tuple<Boolean, String>, как вы предложили, и повторно используйте этот метод как в, так getValueByKeyи в getByKey. Для более сложного случая, который мог бы быть действительно лучшей альтернативой, но для такого простого случая, как показано здесь, у этого ИМХО есть запах чрезмерной инженерии, и я сомневаюсь, что код становится действительно более понятным таким образом. Я здесь с самым лучшим ответом здесь Карла Билефельдта :

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

Optionalэто правильное решение. Тем не менее, если вы предпочитаете, есть альтернатива, которая имеет меньше ощущения "двух нулей", рассмотрим дозорного.

Определите личную статическую строку keyNotFoundSentinelсо значением new String(""). *

Теперь приватный метод может, return keyNotFoundSentinelа не throw new KeyNotFoundException(). Открытый метод может проверить это значение

String rval = getValueByKey(key);
if (rval == keyNotFoundSentinel) { // reference equality, not string.equals
    ... // generate default value
} else {
    return rval;
}

На самом деле, nullэто особый случай стража. Это дозорное значение, определяемое языком, с несколькими особыми поведениями (т. Е. Четко определенным поведением, если вы вызываете метод null). Просто бывает так полезно иметь такого стража, что его использует почти любой язык.

* Мы намеренно используем, new String("")а не просто ""предотвращаем «интернирование» строки в Java, что дает ей ту же ссылку, что и в любой другой пустой строке. Поскольку мы выполнили этот дополнительный шаг, мы гарантируем, что указанная строка keyNotFoundSentinelявляется уникальным экземпляром, который нам нужен, чтобы гарантировать, что он никогда не появится на самой карте.

Учитесь на основе, которая извлекла уроки из всех болевых точек Java:

.NET предоставляет два гораздо более элегантных решения этой проблемы, примером которых являются:

• Dictionary<TKey, TValue>.TryGetValue(TKey, out TValue)
• Nullable<T>.GetValueOrDefault(T default)

Последний очень легко написать на Java, первый просто требует вспомогательный класс «сильная ссылка».