Говнокод, часть 1
Опубликовано: 28 августа 2015 Обновлено: 10 ноября 2016

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

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

На работе дали задание добавить в один проект небольшой функционал. Проект разрабатывался в году 2007 (± 1 год). Бекенд проекта выполнен на java, jsp, фронтенд – html, css, js.

Выкачиваю из репозитория исходники, открываю проект, пытаюсь разобраться. Ищу как идет маршрутизация по страницам.. ага, обработка параметра action из строки запроса.. ох, ты:

if ("".equals(action)) { redirectPage = "blank.jsp"; ... } else if ("new".equals(action)) { redirectPage = "tasks/new_task.jsp"; ... } else if ("user_page".equals(action)) { redirectPage = "tasks/user_page.jsp"; ... } else if ("user_event_list".equals(action)) { redirectPage = "tasks/user_event_list.jsp"; ... } ...

И так почти 100 условий! 100 (!) условий, Карл!

  • Нереальное дублирование кода. Почти 100 однотипных условий для определения целевой страницы.
  • Низкая читаемость кода. Указанный выше код я немного облагородил, реальный код куда менее читаемый.
  • Маршрутизацию страниц стоит сделать через маппинг action – целевая страница.
  • В простом случае таблицу маппинга можно зашить в static блок.
  • В лучшем случае таблицу маппинга стоит вынести в конфигурационный файл.

Иду дальше по коду, нахожу обработку ajax-запросов:

if (method.equals("changeEndText")) { ... out.print("1"); } else if (method.equals("changeStartText")) { ... out.print("1"); } else if (method.equals("unsubscribe")) { ... out.print("1"); } ...

И так для каждого ajax-запроса! Даже интересно стало, а как же это говнище на фронтенде обрабатывается?

var cbFuncUnsubscribe = function (data) { if (data != null) { data = $.trim(data); if (data == "1") { ... } else alert("Неудача"); } else alert("Неудача"); };

*facepalm* На каждый запрос свой разбор ответа..

  • Огромное количество ajax-обработчиков в одном (!) файле. Читаемость и поддерживаемость такого кода стремится к нулю.
  • Свой формат передачи данных, который отличается у разных обработчиков.
  • Для каждого запроса на фронтенде свой разбор ответа и своя обработка ошибок.
  • Точку входа для ajax-запросов стоит вынести в отдельный сервлет. В сервлете сделать общий механизм обработки ошибок и формирования ответа.
  • Каждый конкретный обработчик ajax-запроса вынести в отдельный класс.
  • Опять же, маршрутизацию запросов стоит сделать через маппинг.
  • В простом случае таблицу маппинга можно зашить в static блок.
  • В лучшем случае таблицу маппинга стоит формировать по аннотациям к классам обработчиков.
  • Формат передачи данных не надо придумывать свой, стоит использовать уже устоявшийся и проверенный временем формат, например, json, в крайнем случае – xml.
  • На фронтенде сделать единый разбор ответа и единую обработку ошибок ajax-запросов.

А вот пример еще одного ajax-обработчика, в котором формируем json-ответ руками:

<%@ page contentType="application/json;charset=windows-1251" %> ... result.append("{"); ... String text = rs.getString("ms_text"); if (text == null) { text = ""; } else { text = text.replaceAll("[\n]", ""); text = text.replaceAll("[\r]", ""); text = text.replaceAll("[\t]", " "); text = text.replaceAll("[']", "\""); text = text.replaceAll("[\\\\]", "/"); } result.append("'text': '").append(text).append("'"); ... result.append("}"); ... %><%=result.toString()%>

Просто феерически глупый код. За такое надо просто увольнять.

  • Формирование json-ответа вручную и совсем не по спецификации. По спеке ключи, как и строки, выделяются двойными кавычками.
  • Вся суть вызовов replaceAll состоит в том, чтобы заменить или удалить из строки спецсимволы.
  • Метод строк replaceAll довольно затратен, поскольку реализован через регулярные выражения.
  • Дублирование кода. Указанный фрагмент не раз встречался в коде проекта.
  • Низкая читаемость и поддерживаемость кода.
  • Формирование ответа в нужном формате должен выполнять сериализатор. Лучше использовать готовые библиотеки. Но даже если реализуете велосипед, делайте его по спецификации (json).
  • Никакой модификации данных не должно быть! Данные должны передаваться по своему значению, а не подгоняться, чтобы не сломался парсер.
  • В низкоуровневом коде стоит избегать использования затратных методов (например, регулярных выражений).

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

var isNS4 = (document.layers); var isIE4 = (document.all && !document.getElementById); var isIE5 = (document.all && document.getElementById); var isNS6 = (!document.all && document.getElementById); function switchDiv(objElement, bolVisible) { if (isNS4 || isIE4) { if (!bolVisible) { objElement.visibility = "hidden"; } else { objElement.visibility = "visible"; } } else if (isIE5 || isNS6) { if (!bolVisible) { objElement.style.display = "none"; } else { objElement.style.display = ""; } } return 1; }

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

  • Нет условия для браузеров, кроме IE и NS. Видимо, предполагалось, что никто не будет заходить с иных браузеров. оО
  • Скрытие элементов совсем не однотипное: если display: none; удаляет элемент из потока отрисовки, то visibility: hidden; просто делает элемент прозрачным.
  • Невероятно глупый подход, учитывая, что код датируется 2007 годом.
  • В проекте используется библиотека jQuery, потому для смены видимости элемента достаточно вызвать метод toggle.

Идем дальше по проекту, что же это:

html+= "<a href='javascript:void(0)' onclick=\"elm = $('#detail_user_info<%=currUser%>'); if (elm.css('display') != 'block') { elm.css('display', 'block'); } else { elm.css('display', 'none'); }\">";

Охохохо.. даже не знаю, с чего начать. Вроде всего одна строчка, а такой кошмар.

  • Указание обработчика события в html-коде – это жесткая связь представление – поведение и засорение глобального scope'а.
  • Указание javascript-кода в качестве обработчика. Одно дело указать функцию в качестве обработчика, но кусок javascript-кода – это просто клиника.
  • Глупый алгоритм смены видимости элемента – как писал уже выше, в jQuery есть метод toggle.
  • Магическая ссылка на элемент, используя переменную с jsp-слоя.

Еще не пойму, откуда пошла мода писать javascript:void(0), тогда как достаточно javascript:;.

В конечном счете, говнокод выше можно переписать так:

html+= '<a href="javascript:;" class="toggle-user" data-user="<%=currUser%>">'; ... $(document).on('click', '.toggle-user', function(e) { var user = $(e.currentTarget).data('user'); $('#detail_user_info' + user).toggle(); });

В html-коде используем только html-код! Данные с jsp-слоя привязываем через data- атрибуты. Используем общий обработчик для всех ссылок.

function checkDigits (el) { var valid_chars = "1234567890"; var cur_value = el.value; var new_value = ""; for(var i = 0; i < cur_value.length; i++) { if (valid_chars.indexOf(cur_value.charAt(i)) != -1) { new_value += cur_value.charAt(i); } else { alert("Введите количество минут (положительное число).") } } el.value = new_value; }

Ммммм.. какой тонкий троллинг пользователя, когда он указал нецифровые символы – заспамим его алертами.

  • Последовательная проверка всех символов строки. Выбрасывание ошибки на каждом (!) невалидном символе.
  • Привязка только к input-полям из-за использования свойства value.
  • Подмена значения целевого поля.
  • Отображение ошибок через alert.
  • Не стоит подменять значения полей. Если пользователь указал некорректное значение, надо уведомить его об этом – пусть он сам решает, что делать.
  • Если в указанном значении есть ошибки, надо сообщить об этом ровно один раз.
  • Отображение ошибок ввода должно вписываться в общую концепцию обработки ошибок и обработки форм.
  • Валидация полей должна определяться декларативно, например, в html-коде формы.

<tr> <td <%=counter % 2 == 0 ? "style=\"background-color: #F0F0F0;\"" : ""%>> ... </td> <td <%=counter % 2 == 0 ? "style=\"background-color: #F0F0F0;\"" : ""%>> ... </td> </tr>

Какое незамысловатое оформление таблички.

  • Использование inline-стилей в html-коде – это жёсткая связь разметки и оформления.
  • Дублирование кода. Вместо смены стиля всей строки, меняется стиль каждой ячейки.
  • Низкая читаемость кода.
  • В идеале в разметке не должно быть inline-стилей, равно как и javascript'а.
  • В идеале все оформление должно находиться только в css-файлах.
  • Управление оформлением должно быть реализовано посредством css-селекторов (например, классов).

В 2007 году код выше можно было бы переписать как:

<tr class="row-<%=counter % 2 == 0 ? "even" : "odd"%>"> <td>...</td> <td>...</td> </tr> .row-even td { ... } .row-odd td { ... }

Меняем оформление, используя только css-классы, при этом управляем всей строкой в целом.

С использованием CSS3 код можно еще больше упростить:

<tr> <td>...</td> <td>...</td> </tr> tr:nth-child(even) td { ... } tr:nth-child(odd) td { ... }

Меняем оформление, используя только css-селекторы.

$.cookie('<%=CoreBean.DRTSESSIONID%>', '<%=sessionId%>', { domain: '<%=CoreBean.domainForCookies%>', expires: 3 });

А вот таким костылем коллеги устанавливают сессионную куку. Видимо, столкнулись с тем, что каждый <jsp:include page="..." /> порождает для вложенной страницы новые HttpServletRequest и HttpServletResponse.

И они не придумали ничего лучше, кроме как прокинуть данные на javascript-слой (!) и выставить куку через плагин jQuery.cookie. О какой безопасности тут речь? Обычные программисты выставляют кукам флаг httpOnly, чтобы исключить доступ с javascript-слоя, а тут данные уже в готовом виде.

  • Ненужное делегирование поведения на другой слой приложения.
  • Намеренная брешь в безопасности.
  • Самое верное решение в такой ситуации – не выставлять куки внутри jsp-страниц.
  • Но если архитектура не позволяет отказаться от такого подхода, то стоит выставлять куки на корневом HttpServletResponse, ссылку на который хранить в атрибутах HttpServletRequest.
    Кроме этого, надо не забыть про <jsp:include page="..." flush="false" />, иначе вся затея потеряет смысл.

<script type="text/javascript"> function doSubmit() { var f = document.theForm; f.act.value = "parse"; f.submit(); } </script> ... <form action="" name="theForm" method="POST"> <input name="act" type="hidden" value="" /> <input type="button" onclick="doSubmit();" value="Отправить" /> </form>

Пфффф.. зачем делать кнопку, поведение которой – это установка поля и отправка формы.
Почему просто не указать значение поля и не использовать <input type="submit" />?

Если идея заключалась в разделении варианта обработки формы в зависимости от значения act, то это минимум глупо. Такое разделение можно получить через отдельные кнопки с указанием имени:

<input type="submit" name="doOne" value="Делай раз" /> <input type="submit" name="doTwo" value="Делай два" />

При отправке такой формы в дополнение к полям формы будет добавлен параметр doOne или doTwo в зависимости от того на какую кнопку нажали.

Кроме указанных примеров, в проекте было еще много мест, где, как сказал бы Мартин Фаулер, код дурно пахнет. Но они не так интересны и в целом даже банальны.

В следующих частях выпуска статей о говнокоде постараюсь откопать еще более интересные вещи.

Добавить комментарий к статье:
― показать еще комментарии ―