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

Question

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

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

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

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

Код:

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

Answer 1

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

Answer 2

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

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 должен быть преобразован в реальный объект. Поскольку похоже, что он использует жестко закодированные ключи, каждый "ключ" действительно соответствует свойству класса.

Answer 3

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

Answer 4

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

Answer 5

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

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

Answer 6

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

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

Answer 7

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

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

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

Answer 8

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

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

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

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

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

Answer 9

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

• конкатенация строк, тогда как StringBuilder или шаблон View могут быть намного лучше
• Просмотр информации (например, HTML) на уровне, не связанном с пользовательским интерфейсом (вероятно)
• Неоднозначная бизнес-логика ("HasMultipleDataSets")
• Нетипизированные наборы данных (значение индекса столбца - строка)
• Слишком много в одной строке кода

Answer 10

Ну, субъективно, я бы сказал, что это ужасно, как грех.

Answer 11

((Hashtable)((ArrayList)((Hashtable)MultipleTestPasses[i])["HasMultipleDataSet"])[j])["subject"]

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

Answer 12

HTML завернутый.