Я унаследовал некоторый ужасный код, который я включил короткий пример ниже.
- Есть ли название для этого конкретного анти-паттерна?
Какие рекомендации по рефакторингу это?
// 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) { }
Ответы:
Код плох не только потому, что магические числа , но и потому, что он объединяет несколько значений в коде возврата, скрывая в своем значении ошибку, предупреждение, разрешение на создание сеанса или комбинацию из трех, что делает его плохой вклад для принятия решений.
Я бы предложил следующий рефакторинг: возвращение перечисления с возможными результатами (как предложено в других ответах), но добавление в перечисление атрибута, указывающего, является ли это отказом, отказом (я позволю вам пройти этот последний раз) или если все в порядке (PASS):
==> LoginResult.java <==
==> Severity.java <==
==> Test.java <==
Выходные данные для Test.java, показывающие серьезность для каждого LoginResult:
На основании значения перечисления и его серьезности вы можете решить, будет ли создание сеанса продолжаться или нет.
РЕДАКТИРОВАТЬ:
В ответ на комментарий @ T.Sar я изменил возможные значения серьезности на PASS, WAIVER и DENIAL вместо (OK, WARNING и ERROR). Таким образом, становится ясно, что DENIAL (ранее ERROR) сама по себе не является ошибкой и не обязательно должна приводить к выбрасыванию исключения. Вызывающий объект проверяет объект и решает, выдавать или нет исключение, но DENIAL - это действительный статус результата, полученный в результате вызова
processLogin(...)
.источник
Severity
Это пример Primitive Obsession - использование примитивных типов для «простых» задач, которые в итоге становятся не такими простыми.
Возможно, он начинался как код, который возвращал a
bool
для обозначения успеха или неудачи, а затем превращался в состояние,int
когда существовало третье состояние, и в конечном итоге становился целым списком почти недокументированных состояний ошибок.Типичный рефакторинг для этой проблемы - создать новый класс / struct / enum / object / что угодно, что может лучше представлять рассматриваемое значение. В этом случае вы можете подумать о переключении на объект
enum
, содержащий условия результата, или даже на класс, который может содержать в случаеbool
успеха или неудачи сообщение об ошибке, дополнительную информацию и т. Д.Дополнительные шаблоны рефакторинга, которые могут быть полезны, см . В Таблице индустриальных логик « Запахи к рефакторингу» .
источник
Я бы назвал это случаем «магических чисел» - чисел, которые являются особыми и не имеют очевидного значения сами по себе.
Рефакторинг, который я применил бы здесь, заключается в реструктуризации возвращаемого типа в перечисление, поскольку оно инкапсулирует проблему домена в типе. Работа с ошибками компиляции, возникающими из-за этого, должна быть возможной по частям, поскольку перечисления java могут быть упорядочены и пронумерованы. Даже если это не так, им не должно быть трудно иметь дело с ними напрямую, вместо того, чтобы отступать к целям.
источник
if (processLogin(..) == 3)
Это особенно неприятный фрагмент кода. Антипаттерн известен как «магические коды возврата». Вы можете найти обсуждение здесь .
Многие из возвращаемых значений указывают на ошибки. Есть обоснованные дебаты о том, использовать ли обработку ошибок для управления потоком, но в вашем случае, я думаю, есть 3 случая: успех (код 4), успех, но нужно сменить пароль (код 5) и «не разрешено». Итак, если вам не нужно использовать исключения для управления потоком, вы можете использовать исключения для обозначения этих состояний.
Другим подходом было бы реорганизовать дизайн, чтобы вы возвращали объект «пользователь» с атрибутом «профиль» и «сеанс» для успешного входа в систему, атрибут «must_change_password», если необходимо, и набор атрибутов, чтобы указать, почему журнал -в сбой, если это был поток.
источник