Переработка функции, возвращающей целочисленный код, который представляет множество различных состояний

10

Я унаследовал некоторый ужасный код, который я включил короткий пример ниже.

  • Есть ли название для этого конкретного анти-паттерна?
  • Какие рекомендации по рефакторингу это?

    // 0=Need to log in / present username and password
    // 2=Already logged in
    // 3=Inactive User found
    // 4=Valid User found-establish their session
    // 5=Valid User found with password change needed-establish their session
    // 6=Invalid User based on app login
    // 7=Invalid User based on network login
    // 8=User is from an non-approved remote address
    // 9=User account is locked
    // 10=Next failed login, the user account will be locked
    
    public int processLogin(HttpServletRequest request, HttpServletResponse response, 
                            int pwChangeDays, ServletContext ServContext) { 
    }
    
a_b
источник
2
Что такое "найдено-установлено" и "необходимо-установлено" ?
Тулаинс Кордова
4
Это должно быть тире , читаемое как "найден действительный пользователь: установите его сеанс".
Би Джей Майерс
2
@A_B Какие из этих возвращаемых значений являются успешными входами в систему, которые являются неудачными входами в систему. Не все очевидны.
Тулаинс Кордова
@A_B Означает ли «установить сессию» «сессия установлена» или «требуется сессия установить»?
Тулин Кордова
@ TulainsCórdova: «Установить» означает столько же, сколько и «создать» (по крайней мере, в этом контексте), поэтому «установить свою сессию» примерно равно «создать свою сессию»
hoffmale

Ответы:

22

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

Я бы предложил следующий рефакторинг: возвращение перечисления с возможными результатами (как предложено в других ответах), но добавление в перечисление атрибута, указывающего, является ли это отказом, отказом (я позволю вам пройти этот последний раз) или если все в порядке (PASS):

public LoginResult processLogin(HttpServletRequest request, HttpServletResponse response, 
                            int pwChangeDays, ServletContext ServContext) { 
    }

==> LoginResult.java <==

public enum LoginResult {
    NOT_LOGGED_IN(Severity.DENIAL),
    ALREADY_LOGGED_IN(Severity.PASS),
    INACTIVE_USER(Severity.DENIAL),
    VALID_USER(Severity.PASS),
    NEEDS_PASSWORD_CHANGE(Severity.WAIVER),
    INVALID_APP_USER(Severity.DENIAL),
    INVALID_NETWORK_USER(Severity.DENIAL),
    NON_APPROVED_ADDRESS(Severity.DENIAL),
    ACCOUNT_LOCKED(Severity.DENIAL),
    ACCOUNT_WILL_BE_LOCKED(Severity.WAIVER);

    private Severity severity;

    private LoginResult(Severity severity) {
        this.severity = severity;
    }

    public Severity getSeverity() {
        return this.severity;
    }
}

==> Severity.java <==

public enum Severity {
    PASS,
    WAIVER,
    DENIAL;
}

==> Test.java <==

public class Test {

    public static void main(String[] args) {
        for (LoginResult r: LoginResult.values()){
            System.out.println(r + " " +r.getSeverity());           
        }
    }
}

Выходные данные для Test.java, показывающие серьезность для каждого LoginResult:

NOT_LOGGED_IN : DENIAL
ALREADY_LOGGED_IN : PASS
INACTIVE_USER : DENIAL
VALID_USER : PASS
NEEDS_PASSWORD_CHANGE : WAIVER
INVALID_APP_USER : DENIAL
INVALID_NETWORK_USER : DENIAL
NON_APPROVED_ADDRESS : DENIAL
ACCOUNT_LOCKED : DENIAL
ACCOUNT_WILL_BE_LOCKED : WAIVER

На основании значения перечисления и его серьезности вы можете решить, будет ли создание сеанса продолжаться или нет.

РЕДАКТИРОВАТЬ:

В ответ на комментарий @ T.Sar я изменил возможные значения серьезности на PASS, WAIVER и DENIAL вместо (OK, WARNING и ERROR). Таким образом, становится ясно, что DENIAL (ранее ERROR) сама по себе не является ошибкой и не обязательно должна приводить к выбрасыванию исключения. Вызывающий объект проверяет объект и решает, выдавать или нет исключение, но DENIAL - это действительный статус результата, полученный в результате вызова processLogin(...).

  • ПРОЙДИТЕ: создайте сеанс, если он еще не существует
  • ОТКАЗ ОТ ОТВЕТА: на этот раз, но в следующий раз пользователь может не пройти
  • ОТКАЗ: извините, пользователь не может пройти, не создавать сеанс
Тулаинс Кордова
источник
Вы также можете создать «сложное» перечисление (перечисление с атрибутами), чтобы встроить уровень ошибки в перечисление. Однако будьте осторожны, потому что, если вы используете инструменты сериализации Somme, это может пройти не очень хорошо.
Вальфрат
Сброс исключений в случаях ошибок и сохранение только перечислений для успеха также вариант.
Т. Сар
@ T.Sar Ну, как я понял, это не ошибки как таковые, а отказ по какой-либо причине создать сеанс. Я отредактирую ответ.
Тулаинс Кордова
@ T.Sar Я изменил значения на PASS, WAIVER и DENIAL, чтобы было ясно, что то, что я ранее называл ERROR, является действительным статусом. Может быть, теперь я должен придумать лучшее имя дляSeverity
Тулаинс Кордова
Я думал о другом с моим предложением, но мне очень понравилось ваше предложение! Я подбрасываю +1, точно!
Т. Сар
15

Это пример Primitive Obsession - использование примитивных типов для «простых» задач, которые в итоге становятся не такими простыми.

Возможно, он начинался как код, который возвращал a boolдля обозначения успеха или неудачи, а затем превращался в состояние, intкогда существовало третье состояние, и в конечном итоге становился целым списком почти недокументированных состояний ошибок.

Типичный рефакторинг для этой проблемы - создать новый класс / struct / enum / object / что угодно, что может лучше представлять рассматриваемое значение. В этом случае вы можете подумать о переключении на объект enum, содержащий условия результата, или даже на класс, который может содержать в случае boolуспеха или неудачи сообщение об ошибке, дополнительную информацию и т. Д.

Дополнительные шаблоны рефакторинга, которые могут быть полезны, см . В Таблице индустриальных логик « Запахи к рефакторингу» .

Би Джей Майерс
источник
7

Я бы назвал это случаем «магических чисел» - чисел, которые являются особыми и не имеют очевидного значения сами по себе.

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

Daenyth
источник
Это не то, что обычно подразумевается под «магическими числами».
D Drmmr
2
Он будет отображаться как магические номера на сайтах вызовов, как вif (processLogin(..) == 3)
Дейнит,
@DDrmmr - это именно то, что подразумевается под запахом кода «магические числа». Эта сигнатура функции почти наверняка подразумевает, что processLogin () содержит строки типа «return 8;» в своей реализации, и это в значительной степени заставляет код, использующий processLogin (), выглядеть примерно так "if (resultFromProcessLogin == 7) {".
Стивен С. Сталь
3
@Stephen Фактическое значение чисел здесь не имеет значения. Они просто удостоверения личности. Термин магические числа обычно используется для значений в коде, которые имеют значение, но значение которого недокументировано (например, в имени переменной). Замена значений здесь именованными целочисленными переменными не решит проблему.
D Drmmr
2

Это особенно неприятный фрагмент кода. Антипаттерн известен как «магические коды возврата». Вы можете найти обсуждение здесь .

Многие из возвращаемых значений указывают на ошибки. Есть обоснованные дебаты о том, использовать ли обработку ошибок для управления потоком, но в вашем случае, я думаю, есть 3 случая: успех (код 4), успех, но нужно сменить пароль (код 5) и «не разрешено». Итак, если вам не нужно использовать исключения для управления потоком, вы можете использовать исключения для обозначения этих состояний.

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

Невилл Кайт
источник