Аннотация:
В статье рассматриваются некоторые ошибки, которые часто встречаются в коде программ. Даны рекомендации, как не стоит писать код, на какие этапы разработки кода нужно обращать внимание в первую очередь.
ОЕБ>Авторы: ОЕБ> Burd ОЕБ> Zheka.O
ОЕБ>Аннотация: ОЕБ>В статье рассматриваются некоторые ошибки, которые часто встречаются в коде программ. Даны рекомендации, как не стоит писать код, на какие этапы разработки кода нужно обращать внимание в первую очередь.
эдакий краткий пересказ Мартина "Чистый код". зачем, не ясно
Очень много мест, где личные вкусовые предпочтения авторов подаются как
нечто "очевидное для всех". Со словами "сделаем код проще" предлагаются
простыни вдвое-втрое длиннее исходной, плодится туча мелких функций
используемых 1 раз, нарушается правило единственной точки возврата,
рекомендуется не оставлять комментариев чего этим кодом хотели достичь
(наверно это должно постигаться ясновидением из названия функций).
ОЕБ>Авторы: ОЕБ> Burd ОЕБ> Zheka.O
ОЕБ>Аннотация: ОЕБ>В статье рассматриваются некоторые ошибки, которые часто встречаются в коде программ. Даны рекомендации, как не стоит писать код, на какие этапы разработки кода нужно обращать внимание в первую очередь.
Спасибо за статью.
у меня вопрос по обработкам ошибок.
известны ли авторам типовые решения при обработке ошибок при записи в лог.
предположим есть приложение — робот работающий на отдельном сервере. свои ошибки он записывает в лог (файл, почта, бд...)
как обрабатывать ошибки записи в лог, если их не скрывать?
закрывать приложение? а как же отказоустойчивость?
повторная запись в лог?
кэширование сообщения для лога с последующей записью в лог?
O>простыни вдвое-втрое длиннее исходной, плодится туча мелких функций
таким образом код разбивается на логические куски с возможностью повторного использования.
O>используемых 1 раз, нарушается правило единственной точки возврата, O>рекомендуется не оставлять комментариев чего этим кодом хотели достичь O>(наверно это должно постигаться ясновидением из названия функций).
необходимо называть функции так, что б из ее имени было ясно что она делает.
Здравствуйте, Enomay, Вы писали:
E>люди, которые ленятся читать книги, статьи так же читать не станут. оно им не интересно.
Не всегда дело в лени. У меня на некоторые книги просто нет времени. Приходтся выбирать что читать
Если бы по "отложенным" был бы такой "краткий пересказ", то его бы прочел.
E>необходимо называть функции так, что б из ее имени было ясно что она делает.
"Что" делает, и "для какой цели в данном случае" — вообще говоря не одно и то же.
E>>необходимо называть функции так, что б из ее имени было ясно что она делает. O>"Что" делает, и "для какой цели в данном случае" — вообще говоря не одно и то же.
а что, по коду не ясно для какой цели эта функция используется в другой функции, из названия которой так же очевидно её предназначение?
Здравствуйте, Аноним, Вы писали:
А>как обрабатывать ошибки записи в лог, если их не скрывать?
Места для записи логов бывают разные. Но, в конечном счете — падать.
Если не получилось записать в файл, о какой дальнейшей работе можно вести речь?
Здравствуйте, HowardLovekraft, Вы писали:
А>>как обрабатывать ошибки записи в лог, если их не скрывать? HL>Если не получилось записать в файл, о какой дальнейшей работе можно вести речь?
Позволю себе не согласиться. Допустим, закончилось место на диске, куда пишет лог, но приложение же может работать и дальше. Ну будут сидеть без лога, порой отказоустойчивость важнее логирования.
в общем какого-нибудь универсального решения нет судя по всему.
если закончилось место можно отправить почту или чего-нибудь еще придумать все зависит от приложения и фантазии
главное если почта не отправилась при этом опять не писать а файл
хотелось именно поянять как кто поступает в таких случаях.
Здравствуйте, mrjeka, Вы писали:
M>Позволю себе не согласиться. Допустим, закончилось место на диске, куда пишет лог, но приложение же может работать и дальше. Ну будут сидеть без лога, порой отказоустойчивость важнее логирования.
Исхожу из своих практическийх наблюдений и на истину в последней инстанции не претендую.
Запись в лог — это, обычно, очень примитивная операция, по сравнению с основным функционалом приложения.
Если примитивная операция не выполняется, то с очень высокой вероятностью, остальное тоже работать не будет.
Т. е. скорее всего, вы получите картину, когда вроде и процесс работает, но толку от него нет, полезная работа не выполняется, а диагностическая информация отсутствует.
Да, сходу можно придумать ситуацию, когда лог пишется в базу, и в один прекрасный момент база становится недоступна. Вроде бы ничего страшного. Но, обычно получается так, что это же приложение использует этот же сервер баз данных для работы с основной базой (или вообще лог живет в той же базе, что и рабочие данные). И какой смысл тогда пытаться работать дальше?
Или в вашем примере с местом на диске — вряд ли приложение использует этот диск только для логов. Попытается записать что-то в файл в процессе работы, и что в результате?
А если исключение, возникшее при записи в лог, не столь очевидно и связано с багом в логгере?
Все-таки, обеспечение отказоустойчивости — это комплекс мер. IMHO, не стоит все навешивать исключительно на софт.
Здравствуйте, Osaka, Вы писали:
O>Очень много мест, где личные вкусовые предпочтения авторов подаются как O>нечто "очевидное для всех". Со словами "сделаем код проще" предлагаются O>простыни вдвое-втрое длиннее исходной, плодится туча мелких функций O>используемых 1 раз, нарушается правило единственной точки возврата, O>рекомендуется не оставлять комментариев чего этим кодом хотели достичь O>(наверно это должно постигаться ясновидением из названия функций).
Присоединяюсь. Данный "индусский код" до "модернизации" смотрится понятнее, чем после. Потому как "как слышится, так и пишется", тьфу, "как читается, так и работает", а после изменений — просто то же самое "в голове" смотрящего додумывается. С лишней точкой возврата — точно вредный совет. При переделках кода (например, рефакторинге) влететь с ней — проще простого. С маскировкой исключений — не факт. Есть сценарии, где пустая строка лучше, например, при вычисляемом заполнении ячейки grid. И вообще — может адрес необязателен или, например, потом валидация всей введенной простыни идёт ?
Здравствуйте, Osaka, Вы писали:
O>Очень много мест, где личные вкусовые предпочтения авторов подаются как O>нечто "очевидное для всех". Со словами "сделаем код проще" предлагаются O>простыни вдвое-втрое длиннее исходной, плодится туча мелких функций O>используемых 1 раз, нарушается правило единственной точки возврата, O>рекомендуется не оставлять комментариев чего этим кодом хотели достичь O>(наверно это должно постигаться ясновидением из названия функций).
Какие-то абстрактные наезды. Читал статью, и не припомню таких мест. Хотелось бы более развренутой критики. Типа вот вам цитата, а вот мои мысли по ней.
Есть логика намерений и логика обстоятельств, последняя всегда сильнее.
Здравствуйте, HowardLovekraft, Вы писали:
А>>как обрабатывать ошибки записи в лог, если их не скрывать? HL>Места для записи логов бывают разные. Но, в конечном счете — падать.
Зависит от требований.
HL>Если не получилось записать в файл, о какой дальнейшей работе можно вести речь?
Из реальной жизни. РСДН падал просто потому, что на диске куда записывались логи Веб-сервера кончилось место. При этом функциональность сайта не нарушалась. Просто терялись не очень-то нужные логи. Я бы предпочел чтобы сайт работал и слал на мыло админу сообщения, что кончилось место под логи.
Есть логика намерений и логика обстоятельств, последняя всегда сильнее.
Здравствуйте, Аноним, Вы писали:
А>в общем какого-нибудь универсального решения нет судя по всему.
А>если закончилось место можно отправить почту или чего-нибудь еще придумать все зависит от приложения и фантазии А>главное если почта не отправилась при этом опять не писать а файл
А>хотелось именно поянять как кто поступает в таких случаях.
Ты сам все правильно написал. Универсальных решений не бывает. Твой подход кажется вполне разумным. Пробуй, а там будет видно. Это единственное универсальное решение.
Есть логика намерений и логика обстоятельств, последняя всегда сильнее.
Здравствуйте, VladD2, Вы писали:
VD>Через какой домен входишь?
Как всегда — httр://rsdn.ru.
Клик по ссылке на статью ведет на адрес httр://rsdn.ru/article/?1111, а затем
выполняется редирект на httр://rsdn.ru/article/net/BadCode.xml, где пустой экран.
Здравствуйте, okman, Вы писали:
O>Как всегда — httр://rsdn.ru.
Странно.
O>Клик по ссылке на статью ведет на адрес httр://rsdn.ru/article/?1111, а затем O>выполняется редирект на httр://rsdn.ru/article/net/BadCode.xml, где пустой экран.
Кликаю. И все ОК.
Есть логика намерений и логика обстоятельств, последняя всегда сильнее.
Дело личного вкуса. Я для себя практически взял за правило сохранять результат функции в локальную переменную перед ретурном. Сложности чтения не добавляет, но дает возможность ставить бряку на ретурн и смотреть результат. Сильно помогает при отладке. Стопроцентное покрытие автотестами бывает только в сказках.
2) В примере из той же главы в самом "кошерном" варианте есть такой код:
{
if (CaсhedWebsites != null)
return CaсhedWebsites;
}
...
private List<Website> CaсhedWebsites
{
get
{
return RequestState.GetValue<List<Website>>("Backoffice", "AllWebsites");
}
}
Избавились от промежуточного стостояния, но какой ценой. На ровном месте. Имхо — не приемлимо.
Здравствуйте, VladD2, Вы писали:
VD>Какие-то абстрактные наезды. Читал статью, и не припомню таких мест. Хотелось бы более развренутой критики. Типа вот вам цитата, а вот мои мысли по ней.
Вообще статья хорошая, а цели у нее и вовсе отличные. Я за больше таких статей, но иногда конкретика в этой статье подкачивает:
Очень неожиданные сигнатуры методов, как параметр OrderPaid я ожидаю что-нибудь типа Order, а никак не IPaymentContext context.
Вообще внутренние "микрометоды" — это очень часто зло(кстати вроде у того же Макконела есть такая идея). Я бы сказал, что эти методы надо или поднимать до уровня Extension к Order с претензией на Reusable, или опускать до уровня переменной в использующем их методе, не усложняя внутренней структуры класса.
public void UpdateBasket(IEnumerable<IBasketLineData> linesData)
{
IBasket customerBasket = GetCurrentBasket();
if (customerBasket == null)
return;
foreach (IBasketLineData lineData in linesData)
{
BasketLine line = GetLine(customerBasket.BasketLines, lineData);
if (line != null) //<--Bad
{
ProcessLine(customerBasket, lineData.Stock, line);
}
}
SaveBasket(customerBasket);
}
private void ProcessLine(IBasket customerBasket, int quantity, BasketLine line)
{
//...
}
private BasketLine GetLine(BasketLines basketLines, IBasketLineData lineData)
{
if (!IgnoreLine(lineData.BasketLineId))
return null; //<--Badreturn basketLines.GetById(lineData.BasketLineId);
}
Тут по моим представлениям в итоге получилась плохая практика, не знаю, как ее назвать правильно. Короче когда вроде и порефакторили, а в результате методы начали общаться на языке допущений "я возвращаю null, если линия предназначена к игнорированию". Это вообще плохой свойство кода такие допущения, сложно отлаживать и передавать эту информацию, кроме как явно, комментарием — типа "я возвращаю null если нет в базе". Такое уместно, только если basketLines.GetById тоже может вернуть null, и такое допущение уже есть. Да и название GetLine слишком простое, чтобы ожидать какую-то проверку на игнор. Вынос Ignore спорен, я бы вернул обратно наверх, например в виде if (Ignore) continue; или даже .Where в foreach.
Здравствуйте, AK85, Вы писали:
AK>Дело личного вкуса. Я для себя практически взял за правило сохранять результат функции в локальную переменную перед ретурном. Сложности чтения не добавляет, но дает возможность ставить бряку на ретурн и смотреть результат. Сильно помогает при отладке. Стопроцентное покрытие автотестами бывает только в сказках.
Дело вкуса.
Но в этом вопрос автор по-моему прав:
"зачем переменной result присваивать значение, которое позже может измениться"
Заполнение result честнее заполнять в else.
AK>2) В примере из той же главы в самом "кошерном" варианте есть такой код: AK>
AK>Избавились от промежуточного стостояния, но какой ценой. На ровном месте. Имхо — не приемлимо.
По моим представлениям, тут вопрос проходит ли CaсhedWebsites некие "Критерии свойства", т.е.
1) определенность (не зависит от времени, состояния побочных объектов, рандома)
2) достаточно быстро работает
Иначе его честнее назвать GetCaсhedWebsites() и общаться с ним в другом формате, типа AK>
AK>{
AK> var cachedWebsites = GetCaсhedWebsites();
AK> if (cachedWebsites != null)
AK> return cachedWebsites ;
AK>}
AK>...
AK>
Конкретно в этом случае мб и удовлетворяет — RequestState врядли измениться до какого-нибудь рефреша, да и операция GetValue наврядли такая уж тормозная — скорее всего все исполняется в памяти.
Homo Guglens
Re: Неправильная обработка исключений. Пример "Quantity"
AK>>Избавились от промежуточного стостояния, но какой ценой. На ровном месте. Имхо — не приемлимо.
N>По моим представлениям, тут вопрос проходит ли CaсhedWebsites некие "Критерии свойства", т.е. N>1) определенность (не зависит от времени, состояния побочных объектов, рандома) N>2) достаточно быстро работает
Другими словами, оверхед по ресурсам и вообще работоспособность метода зависит от реализации этого самого свойства.
В замен мы получаем чуть более понятный код (по мнению автора).
N>Иначе его честнее назвать GetCaсhedWebsites() и общаться с ним в другом формате, типа AK>>
AK>>{
AK>> var cachedWebsites = GetCaсhedWebsites();
AK>> if (cachedWebsites != null)
AK>> return cachedWebsites ;
AK>>}
AK>>...
AK>>
С подобного кода и начиналась история в статье, но автор говорит что это лишнее стостояние.
Re: Генерируйте исключения в момент обнаружения ошибки
Идея верна: "Не маскируйте ошибочные ситуации". Я считаю, что это очень важно, из-за этого у нас часто возникают ошибки (остался код в наследство). Но Вы плохо развили идею. Не всегда нужен такое жесткое поведение как выкидывание исключения. Лично я использую три варианта:
1) при поведении как выше имя метода должно заканчиваться на OrDefault:
Здравствуйте, Nikolay_P_I, Вы писали:
N_P>С лишней точкой возврата — точно вредный совет. При переделках кода (например, рефакторинге) влететь с ней — проще простого.
А какие именно проблемы могут возникнуть с этим? Приходит в голову разве что освобождение ресурсов, но если использовать using то все должно быть нормально.
Здравствуйте, Nuseraro, Вы писали:
N>Очень неожиданные сигнатуры методов, как параметр OrderPaid я ожидаю что-нибудь типа Order, а никак не IPaymentContext context.
В целом согласен, улучшенный вариант можно ещё улучшать. Но в данном примере показана конкретная проблема (сложные и трудные для понимания условные выражения с пояснениями в виде комментариев), и при рефакторинге мы концентрировались только на решении этой проблемы, чтобы акцентировать на ней внимание.
N>Вообще внутренние "микрометоды" — это очень часто зло(кстати вроде у того же Макконела есть такая идея). Я бы сказал, что эти методы надо или поднимать до уровня Extension к Order с претензией на Reusable, или опускать до уровня переменной в использующем их методе, не усложняя внутренней структуры класса.
В виде Extension к Order методы смотрелись бы лучше, согласен. Но по поводу переменной скорее не соглашусь. В варианте с методами данный код легче дальше рефакторить, например перенести блок кода в другое место. При использовании переменных, их тоже бы пришлось перемещать вслед за основным блоком. Да и изменять маленький метод, по моему, проще и безопаснее, нужно концентрироваться только на данном маленьком куске кода, не вдаваясь в детали где и как он используется.
N>Тут по моим представлениям в итоге получилась плохая практика, не знаю, как ее назвать правильно. Короче когда вроде и порефакторили, а в результате методы начали общаться на языке допущений "я возвращаю null, если линия предназначена к игнорированию". Это вообще плохой свойство кода такие допущения, сложно отлаживать и передавать эту информацию, кроме как явно, комментарием — типа "я возвращаю null если нет в базе". Такое уместно, только если basketLines.GetById тоже может вернуть null, и такое допущение уже есть. Да и название GetLine слишком простое, чтобы ожидать какую-то проверку на игнор. Вынос Ignore спорен, я бы вернул обратно наверх, например в виде if (Ignore) continue; или даже .Where в foreach.
Полностью согласен, все таки лучше было бы не выносить Ignore.
Здравствуйте, VladD2, Вы писали:
VD>Из реальной жизни. РСДН падал просто потому, что на диске куда записывались логи Веб-сервера кончилось место.
Это security-фича, т.к. место на диске вполне могло закончиться из-за осознанных действий атакующего, направленных именно на то, чтобы нейтрализовать логирование и осуществить некие действия, не оставив за собой следов. С этой точки зрения, безусловное вываливание IIS в 500-ую ошибку при невозможности записи логов понятно. Непонятны две вещи:
1. Почему разработчики IIS сочли, что за ними должно быть последнее слово в принятии решения о подобном поведении сервера? IMHO, это нужно было сделать опцией, включенной по умолчанию.
2. Падать при первой же ошибке записи логов в место их хранения, с учетом того, что рядом простаивает полупустой террабайтный диск, а в оперативной памяти хватит места для хранения логов за несколько дней (в сжатом без потерь виде) — по меньшей мере странно. Разработчикам IIS следовало бы продумать альтернативные конфигурируемые варианты временного сохранения логов, в случае невозможности их записи в основное хранилище.
Т.е. сама идея отказа сервиса из-за невозможности журналирования правильная и здравая, но вот конкретно в IIS реализована через традиционное место
Здравствуйте, Pro100Oleh, Вы писали:
PO>1) при поведении как выше имя метода должно заканчиваться на OrDefault:
Понравился подход, возьму на вооружение.
Здравствуйте, Аноним, Вы писали:
N_P>>С лишней точкой возврата — точно вредный совет. При переделках кода (например, рефакторинге) влететь с ней — проще простого.
А>А какие именно проблемы могут возникнуть с этим? Приходит в голову разве что освобождение ресурсов, но если использовать using то все должно быть нормально.
1) При модификации кода сложнее 10 строк очень просто мазнуть взглядом и не заметить точки возврата из середины функции. Например, раньше возвращали lenght, теперь position. В конце сделали lenght+startpos, в середине пропустили.
2) При смене возвращаемого типа очень муторно и не прозрачно. Например, раньше возвращали lenght, теперь класс {Lenght, Position, Token}. А у вас первая точка возврата ДО вычисления токена. Придется курочить код, да еще вспоминать и разбираться — что он делал.
Здравствуйте, Огинский Евгений, Бурда Роман, Вы писали:
ОЕБ>В статье рассматриваются некоторые ошибки, которые часто встречаются в коде программ. Даны рекомендации, как не стоит писать код, на какие этапы разработки кода нужно обращать внимание в первую очередь.
К одному известному композитору пришел начинающий и предложил прослушать его симфонию.
После окончания симфонии маэстро сказал : "В Вашей симфонии действительно содержится много нового и интересного."
-Вы думаете ? — расцвел дебютант.
Маэстро продолжил : Жаль только, что интересное в ней не ново, а новое — не интересно.
With best regards
Pavel Dvorkin
Re[2]: Неправильная обработка исключений. Пример "Quantity"
Здравствуйте, Pro100Oleh, Вы писали:
PO>Это плохой подход, так как Вы заменили одну "атомарную" операцию на несколько. Что будет, если свойство Text поменяется между вызовами?[/c#]
Атомарность тут даже вторична. Главная проблема, что код оказался замусорен дублирующимися длинными вызовами, соответственно кроме технических характеристик (время работы, атомарность) резко ухудшилась и читабельность.
Re[3]: Неправильная обработка исключений. Пример "Quantity"
Здравствуйте, Undying, Вы писали:
U>Атомарность тут даже вторична. Главная проблема, что код оказался замусорен дублирующимися длинными вызовами, соответственно кроме технических характеристик (время работы, атомарность) резко ухудшилась и читабельность.
AK>Дело личного вкуса. Я для себя практически взял за правило сохранять результат функции в локальную переменную перед ретурном. Сложности чтения не добавляет, но дает возможность ставить бряку на ретурн и смотреть результат. Сильно помогает при отладке. Стопроцентное покрытие автотестами бывает только в сказках.
Не имею ничего против сохранения результата в переменную для облегчения отладки. Тут вопрос в том, какое значение содержит переменная, например в середине функции. Я за то чтобы переменная с результатом не содержала промежуточных или неправильных значений.
Что если у вас такая функция:
protected string GetClass(object url)
{
string result = string.Empty;
if (SiteMap.CurrentNode != null && SiteMap.CurrentNode.Url == url.ToString())
result = "class=\"active\"";
if (...)
result = "class=\"disabled\"";
return result;
}
Как можно описать состояние переменной после первого if-а? Как недоинициализированное что ли А использование переменной с таким состоянием ещё больше усложняет код.
Здравствуйте, Nikolay_P_I, Вы писали: N_P>1) При модификации кода сложнее 10 строк очень просто мазнуть взглядом и не заметить точки возврата из середины функции. Например, раньше возвращали lenght, теперь position. В конце сделали lenght+startpos, в середине пропустили.
Это если мы возвращаем разный length. В примере речь идёт о побочных ветках, когда должен возвращаться null, 0, или -1. Безо всякой дальнейшей математики.
N_P>2) При смене возвращаемого типа очень муторно и не прозрачно. Например, раньше возвращали lenght, теперь класс {Lenght, Position, Token}. А у вас первая точка возврата ДО вычисления токена. Придется курочить код, да еще вспоминать и разбираться — что он делал.
Ну, отловить проблемы нам поможет компилятор. Починить проблемы — следование другим рекомендациям из статьи. В частности, разбиение метода на более мелкие подметоды, так, чтобы основная логика стала понятной.
Уйдемте отсюда, Румата! У вас слишком богатые погреба.
Здравствуйте, HowardLovekraft, Вы писали:
HL>Здравствуйте, Аноним, Вы писали:
А>>как обрабатывать ошибки записи в лог, если их не скрывать? HL>Места для записи логов бывают разные. Но, в конечном счете — падать. HL>Если не получилось записать в файл, о какой дальнейшей работе можно вести речь?
Здравствуйте, Sinclair, Вы писали:
N_P>>1) При модификации кода сложнее 10 строк очень просто мазнуть взглядом и не заметить точки возврата из середины функции. Например, раньше возвращали lenght, теперь position. В конце сделали lenght+startpos, в середине пропустили.
S>Это если мы возвращаем разный length. В примере речь идёт о побочных ветках, когда должен возвращаться null, 0, или -1. Безо всякой дальнейшей математики.
И пример в статье и мой пример — всего лишь пример. Хотите ближе к ? Пожалуйста — была GetElement(id) с return null при "не найдено", стала GetElementOrDefault(id) с return new Element(). В конце поправили, в середине — забыли. В коде сложнее "пример в статье" фразу "return null" просмотреть — опять же легче легкого.
N_P>>2) При смене возвращаемого типа очень муторно и не прозрачно. Например, раньше возвращали lenght, теперь класс {Lenght, Position, Token}. А у вас первая точка возврата ДО вычисления токена. Придется курочить код, да еще вспоминать и разбираться — что он делал. S>Ну, отловить проблемы нам поможет компилятор. Починить проблемы — следование другим рекомендациям из статьи. В частности, разбиение метода на более мелкие подметоды, так, чтобы основная логика стала понятной.
Именно в данном случае вместо рефакторинга 1 функции вам, скорее всего, придется рефакторить еще и ее под.функции потому как туда и оттуда новые типы принимать\передавать надо. Как бы в итоге еще и не больше провозиться придется.
P.S. Вычленение малых функций тоже не всегда хорошо. Особенно, если код линейный и не переиспользуемый. Потому как каждая функция — это какие-то условности и негласные договоренности (что входит, что выходит, как ошибается и что делает). Конечно, не стоит экономить на названиях функций, но их информативность тоже не всеобъемлюща. В качестве примера — как говоряще назвать такое:
foreach
{
Element ElementB;
if (InputStore.TryGet(ElementA.ID, out ElementB))
{
ElementA.Value = ElementB.Value;
}
else
{
ElementA.Value = ElementFormat.DefaultValue;
}
ElementA.ModificationTime = DateTime.UTCNow
}
Разбить на 3 функции с простыми названиями и foreach в каждой ?
Здравствуйте, Огинский Евгений, Бурда Роман, Вы писали:
Оффтоп.
Что за странный распил оценок. Все удваивается. Потому что авторов целых два. Тогда наверно оценки надо делить, а не удваивать. А то можно же собраться человек двадцать, написать статью, и такой распил устроить, никакая другая статья не сравнится.
Здравствуйте, Огинский Евгений, Бурда Роман, Вы писали:
ОЕБ>В статье рассматриваются некоторые ошибки, которые часто встречаются в коде программ. Даны рекомендации, как не стоит писать код, на какие этапы разработки кода нужно обращать внимание в первую очередь.
Кстати, вы не видите противоречий между этими двумя приемами упрощения?
Первый прием. Код:
// Не отправлено ли уведомление ранее и наступило ли время отправки уведомления.if (!context.Order.Fields.ContainsKey("OrderMailSend")
&& (context.Order.PaymentStatus == PaymentStatus.Paid
|| (context.Order.PaymentStatus == PaymentStatus.InProgress
&& GetPaymentMethod(context).StartsWith("banktransfer")
)
)
)
{
IEnumerable<string> email = GetEmailAddress(context);
MailNotification.SendOrderConfirmation(email, context.Order);
// Отмечаем что уведомление о заказе отправлено
context.Order.Fields.Add(
new EntityField { Name = "OrderMailSend", Value = true });
}
Здравствуйте, Nikolay_P_I, Вы писали:
N_P>И пример в статье и мой пример — всего лишь пример. Хотите ближе к ? Пожалуйста — была GetElement(id) с return null при "не найдено", стала GetElementOrDefault(id) с return new Element(). В конце поправили, в середине — забыли. В коде сложнее "пример в статье" фразу "return null" просмотреть — опять же легче легкого.
Ок, давайте обсудим новый пример.
Я правильно понимаю, что вы имеете в виду код типа:
public Element GetElement(int id)
{
if (id < 0)
return null;
var index = this.Elements.IndexOf(id);
if (index < 0)
return null;
return this.Elements[id];
}
И риск исправить его вот так:
public Element GetElementOrDefault(int id)
{
if (id < 0)
return new Element(); // Ok;var index = this.Elements.IndexOf(id);
if (index < 0)
return null; // ай, шайтан! проглядел.return this.Elements[id];
}
Да?
Тогда я соглашусь с вашей точкой зрения сразу же, как только вы объясните мне, чем ваш подход с "result variable" поможет в таком коде:
public Element GetElementOrDefault(int id)
{
var result = default(Element);
if (id >= 0)
{
var index = this.Elements.IndexOf(id);
if (index >= 0)
result = this.Elements[id];
else
result = new Element(); // Ok;
}
else
result = null; // опять забыл!return result;
}
Если вы имеете в виду вот такое решение:
public Element GetElementOrDefault(int id)
{
var result = new Element();
if (id >= 0)
{
var index = this.Elements.IndexOf(id);
if (index >= 0)
result = this.Elements[id];
}
return result;
}
То оно тоже не шибко хорошо, т.к. провоцирует трудноотлаживаемые ошибки в логике, которая завязывается на определённое текущее значение этого result. Из-за большого количества присваиваний в result в теле длинной функции никаких выводов о реальном значении в месте возврата сделать нельзя. Точнее, можно, но сложность этого в точности равна сложности анализа множественных точек возврата. Скорее наоборот — в первоначальном варианте мы могли легко увидеть все места, где возвращается null (или new Element()). В новом варианте понять, в каких случаях возвращается new Element(), достаточно тяжко.
Недавно отлаживал код, переведённый с Delphi. Там вообще нет return statement, поэтому всё реализовано именно так, как вам нравится. Понять логику — чудовищно сложная задача. Result присваивается по полсотни раз на метод, да ещё и грешит штуками типа if (многоэтажка) Result = Result.Parent else Result = какая-то-ещё-локальнаяпеременная.Sibling
N_P>Именно в данном случае вместо рефакторинга 1 функции вам, скорее всего, придется рефакторить еще и ее под.функции потому как туда и оттуда новые типы принимать\передавать надо. Как бы в итоге еще и не больше провозиться придется.
Смотря в каком стиле написаны эти подфункции.
N_P>
N_P>Разбить на 3 функции с простыми названиями и foreach в каждой ?
Ну, если вы не будете заниматься ерундой для искусственного
наращивания LOC, то получится простой и понятный код:
void ApplyValuesFromInputStore(IEnumerable elements, Store inputStore)
{
Element source;
foreach(target in elements)
{
target.Value
= inputStore.TryGet(target.ID, out source)
? source.Value
: ElementFormat.DefaultValue;
target.ModificationTime = DateTime.UTCNow; // кстати, вот это должно быть частью сеттера Value;
}
}
Этот код уже можно понять без поллитры. В нём нет лишнего состояния.
Можно сделать наоборот — реализовать чинилку для отдельного элемента:
Здравствуйте, Pavel Dvorkin, Вы писали:
PD>К одному известному композитору пришел начинающий и предложил прослушать его симфонию. PD>После окончания симфонии маэстро сказал : "В Вашей симфонии действительно содержится много нового и интересного." PD>-Вы думаете ? — расцвел дебютант. PD>Маэстро продолжил : Жаль только, что интересное в ней не ново, а новое — не интересно.
Чесно говоря, мы не ставили перед собой цель написать о чем то новом и интересном в статье. На работе, часто встречаемся с одними и теми же проблемами (довольно таки неинтересными). Просто решили описать типичные недостатки на реальных примерах из наших проектов, и показать возможные пути устранения этих недостатков своим коллегам. И оформили все в виде статьи, может ещё кому то будет интересно.
U>Что здесь по сути сделано? Сложные вызовы заменены на короткие псевдонимы.
Не совсем. Здесь сложные вызовы разбиты на логически завершенные операции, которые выполняют одно действие и имеют понятные имена. Если бы мы просто хотели заменить сложные вызовы на короткие псевдонимы, то написали бы так:
if (OrderMailNotSentAndMailNotificationRequired(context))
{
SendOrderConfirmationAndMarkOrderAsSent(context);
}
U>Второй прием. Код:
U>
U>public int Quantity
U>{
U> get
U> {
U> int quantity;
U> if(int.TryParse(TxtQuantity.Text, out quantity))
U> return quantity;
U> return 0;
U> }
U>}
U>
U>Заменен на:
U>
U>public int Quantity
U> {
U> get
U> {
U> if (!int.Valid(TxtQuantity.Text))
U> return 0;
U> return int.Parse(TxtQuantity.Text);
U> }
U> }
U>
U>Что здесь по сути сделано? Множественное использование короткого псевдонима заменено на множественное использование сложных вызовов.
Здесь тоже попытались разбить операцию на две части, чтобы каждая часть выполняла свое логически завершенное действие и имела понятное имя.
Здравствуйте, Nuseraro, Вы писали:
N>Я бы сказал, что эти методы надо или поднимать до уровня Extension к Order с претензией на Reusable, или опускать до уровня переменной в использующем их методе, не усложняя внутренней структуры класса.
Или взять более выразительный язык, в котором микрометоды можно делать локальными функциями.
Таким образом, они не будут отсвечивать в классе.
N>Тут по моим представлениям в итоге получилась плохая практика, не знаю, как ее назвать правильно. Короче когда вроде и порефакторили, а в результате методы начали общаться на языке допущений "я возвращаю null, если линия предназначена к игнорированию".
Согласен. Но на C# лучше делать трудно.
Тут нужно что-то чуть более выразительное.
Например, так:
Кстати названия методов тоже плохие.
//отделяем логику обновления текущей корзины от собственно обновления корзиныpublic UpdateCurrentBasket(linesData : IEnumerable[IBasketLineData]) : void
{
when (GetCurrentBasket() is Some(customerBasket))
{
UpdateBasket(customerBasket, linesData)
SaveCurrentBasket(customerBasket);
}
}
//Это возможно нужно сделать методом расширениемprivate UpdateBasket(customerBasket : IBasket, linesData : IEnumerable[IBasketLineData]) : void
{
foreach (lineData in linesData)
when (!IgnoreLine(lineData.BasketLineId))
when (customerBasket.BasketLines.GetById(lineData.BasketLineId) is Some(line))
if (lineData.Stock != 0)
line.Quantity = lineData.Stock;
else
customerBasket.BasketLines.Remove(line);
}
Если несколько when идут подряд, то отступ после каждого делать не имеет смысла.
В данном случае все функции, которые могут не вернуть значение, возвращают этот тип:
public variant option[T]
{
| None
| Some { val : T; }
}
В случае если функции нечего возвращать она возвращает None.
Если есть чего то Some в который завернуто возвращаемое значение.
Это позволяет четко показать, какая функция всегда возвращает значение, а какая может и не вернуть.
При этом компилятор заставит проверить, что функция вернула то, что надо.
Конструкция вида
when (GetCurrentBasket() is Some(customerBasket))
проверяет что из функции вернули Some и достает из него customerBasket.
После is может идти любой паттерн.
... << RSDN@Home 1.2.0 alpha 4 rev. 1472>>
Пусть это будет просто:
просто, как только можно,
но не проще.
(C) А. Эйнштейн
Re[2]: Генерируйте исключения в момент обнаружения ошибки
Здравствуйте, Sinclair, Вы писали:
S>Ну, если вы не будете заниматься ерундой для искусственного S>наращивания LOC, то получится простой и понятный код: S>
S>void ApplyValuesFromInputStore(IEnumerable elements, Store inputStore)
S>{
S> Element source;
S> foreach(target in elements)
S> {
S> target.Value
S> = inputStore.TryGet(target.ID, out source)
S> ? source.Value
S> : ElementFormat.DefaultValue;
S> target.ModificationTime = DateTime.UTCNow; // кстати, вот это должно быть частью сеттера Value;
S> }
S>}
S>
S>Этот код уже можно понять без поллитры. В нём нет лишнего состояния.
Сорри, но вот за такой код хочется что-нибудь нехорошее сделать
Такой иф разбивать на несколько строчек совершенно не допустимо. он должен быть в одну строчку, не помещается — пишим нормальный иф!
Я вообше не сразу въехал что target.ModificationTime = DateTime.UTCNow; не часть ифа.
Здравствуйте, gwg-605, Вы писали: G6>Сорри, но вот за такой код хочется что-нибудь нехорошее сделать
Делайте G6>Такой иф разбивать на несколько строчек совершенно не допустимо. он должен быть в одну строчку, не помещается — пишим нормальный иф!
Это зависит от принятого стандарта кодирования.
Лично мне наоборот — тяжело искать глазом в строке двоеточие. Если у вас в проекте такой стандарт, то через день вы будете читать такую конструкцию с налёту.
G6>Я вообше не сразу въехал что target.ModificationTime = DateTime.UTCNow; не часть ифа.
Как я уже писал, этой строки вообще быть не должно, т.к. свойства даны нам вместо полей ровно для таких сценариев.
Уйдемте отсюда, Румата! У вас слишком богатые погреба.