Насколько плох этот код? - PullRequest
Новая
       26

Насколько плох этот код?

1 голос
/ 26 февраля 2009

Я унаследовал приложение для онлайн-викторин, написанное на C #, с этими строками кода повсюду.

Так насколько же плох этот код?

С какими потенциальными проблемами я могу столкнуться?

Как я могу улучшить это?

Код: 

strTestPasses += "<tr valign=\"top\"><td><b>Subject</b></td><td>" + ((Hashtable)((ArrayList)((Hashtable)MultipleTestPasses[i])["HasMultipleDataSet"])[j])["subject"] + "</td></tr>";

Ответы [ 12 ]

14 голосов
/ 26 февраля 2009

Я также ненавижу эти «строки кода». Когда люди спрашивают меня: «Какой самый большой проект, над которым вы работали», я отвечаю «1 строка кода». Когда другие бросают мне вызов: «Сколько строк кода вы можете написать в день?», Я отвечаю им: «Только один мой брат. Но это единственная верная линия '

8 голосов
/ 26 февраля 2009

Чтобы начать рефакторинг, могу ли я предложить: 

TestPassesBuilder.AppendFormat(
    "<tr valign='top'><td><b>Subject</b></td><td>{0}</td></tr>", 
    MultipleTestPasses[i]["HasMultipleDataSet"][j]["subject"]
  );

или 

TestPassesBuilder.AppendFormat(
    "<tr valign='top'><td><b>{0}</b></td><td>{1}</td></tr>",
    "Subject", 
    MultipleTestPasses[i]["HasMultipleDataSet"][j]["subject"]
  );

Где TestPassesBuilder - это, конечно, StringBuilder, а MultipleTestPasses был преобразован для использования соответствующих универсальных типов коллекций, а не мерзости ArrayList / HashTable. Второй вариант также позволит в какой-то момент заголовок для каждой строки быть разложен в переменную.

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

7 голосов
/ 26 февраля 2009

Прошел ли он свой юнит-тест?

2 голосов
/ 26 февраля 2009

Вы знаете, как они говорят "... только мать могла любить ..."

Ну, в этом случае, только компилятор может любить.

2 голосов
/ 26 февраля 2009

Опять же, как субъективный ответ на субъективный вопрос, это не красиво. Опять же, если это небольшой сайт, возможно, было бы легче сделать это «быстро и грязно», чем что-то более правильное, например, C # / ASP -> XML -> XSLT -> HTML (или что-то подобное в любом случае - я немного цепляюсь за соломинку)

2 голосов
/ 26 февраля 2009

Это не здорово, но я видел хуже.

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

1 голос
/ 27 февраля 2009

Это очень плохо, поскольку содержит следующие недостатки:

  • конкатенация строк, тогда как шаблон StringBuilder или View может быть намного лучше
  • Просмотр информации (например, HTML) на уровне, не связанном с пользовательским интерфейсом (вероятно)
  • бизнес-логика Ambiguos ("HasMultipleDataSets")
  • Нетипизированные наборы данных (значение индекса столбца - строка)
  • Слишком много в одной строке кода
1 голос
/ 26 февраля 2009 
((Hashtable)((ArrayList)((Hashtable)MultipleTestPasses[i])["HasMultipleDataSet"])[j])["subject"]

Ох ... мои глаза ...

1 голос
/ 26 февраля 2009

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

  1. HTML-макет не отделен от содержимого.
  2. Тема должна быть возвращена из описательно названной функции.

Я бы предпочел увидеть что-то вроде: 

strTestPassesStringBuilder.Append(newTableRow("Subject", getSubject());

, где опции и valign контролируются с помощью CSS.

1 голос
/ 26 февраля 2009

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

Также при рефакторинге используйте строковые литералы с @ для html.

Мне лично все равно, использует ли он какие-либо шаблоны для html, если это маленький проект. Шаблоны часто для небольших проектов излишни.

Страница:
...