9 января 2012 в 00:20

Читайте код, с остальным справится компилятор

Введение


Уже не в первый раз мне задают связанные вопросы:
«Зачем ты делаешь так много функций?»;
«Зачем ты выносишь, однократно используемый, код в функции?»;
«Остальные не знакомы с твоими правилами именования функций. Как они будут с этим работать?». Поэтому опишу свое видение проблемы. Ну а сообщество подскажет, к чему же стоит стремиться.

Ситуации


В процессе работы встречается разный код. Его надо сопровождать, исправлять в нем ошибки, добавлять новый функционал. Также пишется новый код, с которым предстоит работать в дальнейшем другим. Опишу несколько ситуаций:

Ситуация №1 Код делает слишком много

В классе есть доминирующий метод размером чуть меньше самого класса, при этом он содержит в себе всю логику и инфраструктурный код;

Ситуация №2 Логика кода понятна только после его изучения

Код хоть и невелик, но что он делает понять достаточно сложно;

Ситуация №3 Добавляется новый функционал

Имеется достаточно сложный код, в который необходимо добавить новые функции;

Ситуация №4 Пишется новый код

Код должен иметь низкую связность, хорошее покрытие тестами, сопровождаемость и пр;

Причины


В каждой из этих ситуаций, я стараюсь сделать минимальный рефакторинг. Делаю код более понятным и простым.

Чаще всего выношу код в метод. Если позволяет API, то делаю более сложные рефакторинги. Условные выражения, если они сходу не понятны, стараюсь вынести в метод с понятным названием.

Всегда руководствуюсь правилом: «код необходимо читать, а не компилировать и выполнять», что вполне соответствует идеологии, описанной Фаулером.
Каждая функция должна иметь хорошее название, если она делает не то, что описано в названии, ей должно быть подобрано новое имя. Комментарии, являющиеся переводом кода — излишни (Exception — Ошибка).

Всегда стремлюсь оставить код менее пахнущим, чем он был до этого. О запахах кода можно прочесть у многих, в частности у Фаулера.

Последствия


Почти после каждой более-менее существенной правки, мне задают вопросы, описанные выше. Говорят, что код стал менее понятным, становится трудно найти нужную строку и пр.

Причины


На самом деле проблема кроется как в коде, так и в разработчике, который привык к коду.

Предлагаю провести эксперимент.

Возьмите неизвестный участок кода, который написан хорошо. Он должен иметь методы со сложностью меньше 5 (имеется в виду упрощенная трактовка цикломатическая оценка). Если вам будет тяжело читать этот код, то обратите внимание, что вы пытаетесь транслировать код. Проверяете, как будет выполняться программа, поэтому неудобства доставляет постоянное перескакивание с метода на метод. Вы компилируете и выполняете код в голове.

Если код читать легко, то, скорее всего, вы больше внимания обращаете на сигнатуры методов, на их названия. Вы читаете код.

Для кода, содержащего несколько функций со сложностью более 30 верно обратное (к примеру, более 100 строк). Его читать невозможно, зато выполнение может принести свои плоды. Если вы не собьетесь в пятнадцатом вложении условия на двадцатой итерации цикла… Удачи!

Заключение


Можно сказать, что чем проще код, тем лучше. Только если вам кажется, что ваш код прост, то его можно упростить еще на несколько пунктов. Ничего страшного, что функция сейчас используется в одном месте, ничего, что ее название более 20 символов. Главное чтобы код читался, можно сказать, как художественная книга. Именно код, а не комментарии.

Хорошие примеры можно найти в трудах Гласса, Фаулера, Макконнелла, Мартина.

Еще хорошим контролем качества кода являются тесты. Не в том смысле, что в нем не ошибок — ошибки есть везде. Если, при написании теста, вы задумались на пару минут о том, как тестировать код, то он, на самом деле, сложноват, его можно упростить. Порой, приходят настоящие озарения.

Пишите и читайте свой код, а все остальное сделает компилятор!

Полезные ссылки:
1. Совершенный код. С. Макконнелл
2. Рефакторинг. Улучшение существующего кода. М. Фаулер
3. Чистый код. Р. Мартин
4. Greg Wilson — What We Actually Know About Software Development, and Why We Believe It's True
5. SOLID Principles with Uncle Bob — Robert C. Martin
6. Неплохой англоязычный ресурс sourcemaking.com
Денис Сапоненко @VaiMR
карма
32,5
рейтинг 0,0
Похожие публикации
Самое читаемое Разработка

Комментарии (112)

  • +13
    Смотрите только не получите в результате равиоли-код.
    • +5
      Конечно, надо знать меру. «Равиоли-код» часто появляется вследствие плохих названий методов. Когда они просто описывают содержимое, а не назначение и возможности.
      • +1
        а плохие названия методов зачастую являются следствием дырявой абстракции. Все от ситуации зависит, конечно…
    • 0
      А чем плох равиоли-код? Насколько я понимаю, это в принципе то, к чему мы стремимся при ОО-разработке: маленькие идеально инкапсулированные кусочки кода. Гугль говорит, что это антоним спегетти-кода, а Вики несколько туманно намекает на то же самое.
      Что же за конфликт терминологий такой?
      • +5
        Крайние случаи редко бывают оптимальными в реальном мире. В погоне за инкапсуляцией, слабой связанностью и прочими «паттернами» легко превратить код приложения в месиво классов и методов в одну строку, которые инвертят друг другу контроль, что-то слушают, о чем-то сообщают, чего-то химичат фабричат, но вот что конкретно происходит, когда нажимаешь кнопку «Сделай мне...»ой, просто кнопку — не понять.
        • 0
          Благодарю. Что-то подобное я подозревал (и даже немножко нагуглил), просто не был уверен насчет верного названия сего безобразия. Действительно, все начинает сходиться на названиях методов/функций.
      • 0
        Да, есть ещё и такой антипаттерн…
  • +16
    «Зачем ты выносишь, однократно используемый, код в функции?»
    Раньше я тоже не любил выносить однократно используемый код в функции. Теперь стараюсь разделить логику работы на маленькие кусочки, каждый из которых оформляю отдельной функцией, так действительно удобнее читать! Если нужны подробности реализации, всегда можно провалиться в одну из функций и посмотреть детали.
    • +9
      Так ещё и тестировать удобнее.

      Вообще разделение задачи на N подзадач — наше всё.
      • +8
        Я бы даже сказал, что «разделение задачи на N подзадач» — это и есть наша работа.
      • +4
        принцип «Разделяй и властвуй»
      • 0
        Во всем нужно знать меру. Можно так «доразбиваться», что код станет читаемым, как книга, но править его будет одно мучение. ;-)
    • +8
      На самом деле раздражает когда простой функционал который в одном методе занял бы 50 строк побит на 10 функций просто потому что разработчик решил что поиск позиции разделителя это должен быть не string.IndexOf(SEPARATOR) а отдельная фукнция размером в одну строку, якобы это отдельный блок логики.

      Много маленьких методов с именами по 30 символов довольно трудно поддерживать. Если что либо изменилось — будет куча работы по рефакторингу. История как с коментариями.

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

      Лично я не выношу методы до 5 строк, либо до второго идентичного использования, либо до 3-4 неидентичного использования. Неидентичное использование подразумевает одинаковый код, но с чуть отличающейся логической функцией, что ознаачет что имплементация может поменяться и совпадает она временно\случайно.
      • +5
        Если сейчас код не будет вынесен, то совсем не факт, что его вынесут через пять лет, проще будет написать дубль. Я с такой ситуацией сталкиваюсь повсеместно. С разбиением надо знать меру. Код имеет тенденцию разрастаться, усложняться. Рефакторинг делать гораздо проще, если уже есть разбиение на методы, связность то ниже, нет необходимости вникать в хитросплетения логики внутри большого метода.
      • НЛО прилетело и опубликовало эту надпись здесь
        • 0
          Как приятно что вы тоже читали эту замечательную книгу :)
      • +1
        красота, как уже заметили, у каждого своя.

        Но есть вполне себе формальные критерии. Если SEPARATOR универсальный, и много где его ищут по разным случаям, действительно нет особого смысла выносить это в FindSeparator. Но если подумать чуть дальше, что делают найдя? Бьют по нему строку на две? На список по всем вхождениям? Вот такого рода методы — почему бы не сделать отдельно, и не назвать более внятно?

        Количество строк и сколько раз используется, на мой вкус, не суть важно. Если это парсинг бинарного протокола, и только в одном месте деление потока на кадры по одному разделителю, я всё равно предпочту иметь отдельный метод который получает кадр данных, с названием типа getFrame или как-то так. И не важно, что реализация этого не будет меняться вообще, и разделители тоже. Важно, что читая тот код, где этот кадр данных получают, я прочту «получить кадр», и точно буду знать — мне тут вернут кадр данных.
    • +1
      Тут есть как обратная сторона такого рода: Вот разнесли вы один метод из 15 строк на три, которые вызывают друг друга по очереди, ОК все работает, все понятно. Затем, под какую-то другую задачу, другой разработчик ищет место, куда бы ему воткнуться в вашей логике, места подходящего не находит и модифицирует один или даже все три ваших новых метода, добавляя ветвление. Потом это повторяется еще несколько раз. В результате мы имеем пачку методов, выполняющих ряд похожих операций и связанных множеством перекрестных связей. И с каждой новой модификацией все менее понятно, откуда же этот клуб начинать разворачивать. В особенно запутанном случае в этом клубке возникают рекурсии и в конце концов он идет на помойку почти целиком.

      Еще один минус — найти место для начала отладки. Согласитесь, проще воткнуть «бряк» в начале одного увесистого метода, чем во всех трёх маленьких методах, не зная наперёд где нам повезет.

      Я конечно не призываю делать god-object'ы и god-методы, но во всяком рефакторинге нужна мера.
      • 0
        Дублирование кода — это очень плохая практика. Не надо ухудшать код правками. Используйте ревью. Практика, которую вы советуете приводит к методам в 2-4 тысячи строк, а то и больше (10 тыс. и это не предел). На любой вопрос, а как это работает, у более опытных разработчиков всегда готов ответ: «debug в руки и вперед!». Неоднократно с этим сталкивался. Прежде, чем вносить правку стоит подумать, а не писать кое-как.
        • +1
          Вы какой-то предельный случай сейчас приводите. В моей практике методов больше 2-3 экранов не было. Но выносить код в отдельный метод с заранее неясной целью — плохая практика. Знаете, лучше класс из 10 методов по 100 строк чем класс из 200 методов по 5 строк, хотя и то и другое плохо.
          Но дробление на классы без веских на то оснований умножает сущности, что еще хуже.
          • 0
            Случай не предельный, а вполне обычный. Вам очень повезло, если не приходилось сидеть в дебаге по два дня в методе больше 2к строк. А уж если еще и рекурсию туда приплели, то можно вешаться.
            • 0
              Ну я надеюсь вы жестоко покарали написавших такое?
              • +1
                Нет возможности =) Давят авторитетом.
        • +1
          На счет дублирования люто с вами согласен. Код не должен повторяться в программе более 1-2 раз. Но выделять одноразово используемый код в метод только ради уменьшения методов — не есть хорошая практика.
          • 0
            Уменьшение методов — естественных эффект рефакторинга и тестирования. Код, которому больше 5-10 лет имеет тенденцию приобретать запахи. Это в самом начале кажется, что незачем разбивать метод из 10 строк на два, но через некоторое время он может серьезно разрастись, тогда его уже будет не понять.

            Уменьшение методов ради уменьшения — бессмысленно, надо стараться улучшить читаемость кода, сделать его понятным и простым. Но в этом, я думаю, мы солидарны.
            • 0
              Согласен. Хороших код, тот который за эти 5 лет ни разу не дописывался но активно используется извне.
              • 0
                Но не по причине, потому что все боятся туда лезть. Не по принципу «работает — не трогай». :)
  • 0
    Что такое «сложность кода более 5?» Что за попугаи?
    • 0
      Строки?
    • +2
      На самом деле метрик много: cmcons.com/articles/CC_CQ/dev_metrics/mertics_part_1/. Я пользуюсь более простой оценкой, предложенной Макконелом, если я не ошибаюсь. Она заключается в подсчете количества существенных операций в участке кода. То есть если в методе два оператора присваивания, одно ветвление и цикл, то сложность будет порядка 4. Данная метрика отлично подходит для определения сложности понимания кода на отдельных его участках. Есть и более формальные способы оценки сложности, которые подходят для оценки ПО в целом или отдельных алгоритмов.
      • +1
        Цикломатическая сложность — очень подходит для оценки сложности чтения кода. Наверное этот топологический метод и имелся в виду.
      • 0
        Сложность может быть хоть 125, но одна функция — это код, который можно описать, грубо, как транзакция.
        • 0
          Да, столкнулся я с такой ошибкой с транзакциями в старом коде. Где то их добавили, а где то забыли. Куда проще создать функцию вида:
          void transactFunction() {
            try {
              transaction.start();
            
              someCodeFunction();
          
              transaction.commit();
            } catch(Exception ex) {
              transaction.rollback();
            }
          }
          


          Такой важный функционал достоин отдельной функции. А далее наследование, полиморфизм.

          А функции-транзакции, если не имеют определенной синхронизации, не так уж тразнакционны.
          • 0
            Если разбивать сложный алгоритм на функции, то для транзакций главное — открывать и закрывать их в одной и той же функции. Что верно и для других выделений ресурсов, как то открытие/закрытие файлов, создание/разрушение объектов, установка/снятие локов и т.д. Исключением может быть, разве что, не алгоритмическая, а полностью событийная модель приложения.
  • +3
    и тут мне вспоминается Objective-C
    • –1
      Да-да, Obj-C классно читается, почти литературный текст.
      • +7
        А мне наоборот кажется, что Objective C ужасен. Вырвиглазные скобочки, плюсики-минусики в объявлениях методов, единственное что хорошо — возможность использовать именованные параметры.
        • –1
          вы видимо совсем не работали с этим языком ;)
          • 0
            Работал около полугода нонстоп (правда давно уже было это, около 2 лет назад, сейчас даже синтаксис подзабыл, но ощущения остались). И там, где можно было, я использовал С++ вместо Objective-C — писал нормальные классы, реализующие логику. А к Objective-C обращался только при необходимости вызвать NS-API или что-то сделать в UI-коде.
      • –1
        Согласен. На мой взгляд Objective-C один из самых удобочитаемых языков.
        • 0
          Ещё есть AppleScript!
          Вот где программа выглядит, как разговорная речь:
          set theTextString to «Apple Computer»
          set computerStringStart to offset of «Computer» in theTextString
          display dialog computerStringStart -->
          покажет 7 — это позиция начала слова 'Computer' в строке «Apple Computer»
          • 0
            Хех, напомнило мой первый язык программирования. Скриптовый в программе Toolbook. Там тоже было что то вроде:
            to handle button down
            show button knopka
            go to page 3
            
          • НЛО прилетело и опубликовало эту надпись здесь
          • 0
            А еще есть Chef
            Hello, world с Википедии:

             Hello World Souffle.
             
             Ingredients.
             72 g haricot beans
             101 eggs
             108 g lard
             111 cups oil
             32 zucchinis
             119 ml water
             114 g red salmon
             100 g dijon mustard
             33 potatoes
             
             Method.
             Put potatoes into the mixing bowl.
             Put dijon mustard into the mixing bowl.
             Put lard into the mixing bowl.
             Put red salmon into the mixing bowl.
             Put oil into the mixing bowl.
             Put water into the mixing bowl.
             Put zucchinis into the mixing bowl.
             Put oil into the mixing bowl.
             Put lard into the mixing bowl.
             Put lard into the mixing bowl.
             Put eggs into the mixing bowl.
             Put haricot beans into the mixing bowl.
             Liquefy contents of the mixing bowl.
             Pour contents of the mixing bowl into the baking dish.
             
             Serves 1.
            
  • +17
    Странно, что имя Мартина Фаулера упоминается три раза, а Стива Макконнелла только один, причём написано с двумя ошибками. Имел честь видеть Фаулера лично, он год назад читал шестичасовой доклад по Continuous Delivery на мюнхенской конференции OOP 2011. Излагает очень талантливо и знает о чём говорит, но боготворить его всё же не стоит. Основополагающим трудом по написанию кода считается именно Code Complete Стива Макконнелла. Причём это не только моё мнение, книга заняла первое место в голосовании по теме What is the single most influential book every programmer should read? на StackOverflow. Фаулер там на почётном седьмом месте и он в целом пишет не столько про код, сколько про процессы — начал с Agile Manifesto, потом перешёл к Continuous Integration, сейчас двигает Continuous Delivery. Для него читаемость кода лишь один из факторов улучшающих качество проекта, важный, но далеко не единственный. Вот у Стив действительно целая книжка про то как писать код, там страниц двадцать лишь про то как оформлять комментарии.

    Ещё могу порекомендовать Clean Code Роберта Мартина, у него не только про читаемость, но и про то как писать код, на который не будет страшно взглянуть после нескольких лет непрерывных правок и багфиксов.
    • 0
      Мартина как раз упомянул, отличная книга. Опечатку подправил, спасибо!
  • +5
    Вместо такого ответа лучше бы просто сослались на «Совершенный код» Макконнелла. Полезнее было бы.
    • +2
      Всегда ссылаюсь, часто не срабатывает. В наших краях «непуганых программистов», не так уж и много авторитетов.
      • +1
        Это верно, но ссылаться всё же не помешает. Это я к тому что статья безусловно полезна, но думается в такого рода публикациях нужен в конце раздел с гиперссылками на «must read» книги, другие статьи и т.д.

        Например, если «непуганому программисту» лень читать томик Макконнелла, то может он всё же решится по пути на работу прослушать подкаст "SOLID Principles with Uncle Bob" c Мартином в главной роли.
        • 0
          Да, думал, кидайте ссылки, добавим!
          • +1
            Я делал серию статей по поводу S.O.L.I.D. blog.byndyu.ru/2009/10/solid.html, написано по-русски с примерами из моих проектов.
  • +1
    С именованием проблемы часто бывают даже когда по-русски пытаешься сформулировать, не говоря о последующем переводе на английский — или теряется важная часть описания UserRepository::getForCondition7() или $this->isAllowed(), или становится совершенно нечитаемый типа UserRepository::getActiveConfirmedWithFilledCountryOrPhoneNumber() или $this->isGreaterOrEqualThenMinimalAgeOrLessThenMinimalAgeAndHaveParentPermission(self::minimalAge). Или это нормальные имена? :)
    • +7
      Имена, по крайней мере public методов, должны иметь семантическое значение, а не рассказывать об особенностях реализации. Ведь реализация в любой момент может измениться и тогда придётся или переименовывать метод и перекомпилировать все использующие его модули, или же имя метода начинает лгать о его содержании.

      Например getActiveConfirmedWithFilledCountryOrPhoneNumber может означать getActiveConfirmedAndValid, при том что понятие Valid сегодня действительно гласит «с заполненным кодом страны или телефонным номером», но завтра эта реализация имеет право поменяться.
      • +1
        Так это семантика (для случая с репозиторием). Реализация может быть на SQL, NoSQL, на файлах, получаться с удаленного сервера по HTTP или ещё как. Собственно для того и введен класс UserRepository (вообще он не статический, а получает storage как параметр конструктора), чтобы не зависеть от реализации хранилища. А вот само условие прямиком из ТЗ, только с русского переведено и там таких ещё парочка без объяснения их назначения, не называть же их getSet_15_1, getSet_15_2 и т. п. по номера пунктов ТЗ.

        С возрастом, да, неудачный пример, можно, что-то более семантическое придумать, типа isCanUsePaydFeatures, по крайней мере назначение ясно вроде бы.
        • +2
          isCanUsePaydFeatures
          Просто canUsePaydFeatures.
          • 0
            Сколько не пытался запомнить английскую грамматику (и школа, и два института, и курсы, и сам по самоучителям) — не могу. Какие-то «мнемонические» правила выучил типа «булева проверка начинается с is» и всё. :(
            • +2
              Занятно. А я в школе «занимался» французским, а в университете 4 курса английским (интенсивно только 1 курс, т.к догонял). Короче, тут модальный глагол, поэтому для вопросительного предложения не надо глагола связки (be).

              Не совсем круто использовать только правила вида «булева проверка начинается с is». Бывают случаи когда, например, довольно намного лучше читается код с глаголами типа has, can. Больше даже что-то на ум не приходит, ну кроме to be. Может это и имеет смысл. Типа проверка состояния — is/was, возможность осуществления чего-либо — can, наличие свойства/объекта/особенности — has. Больше особо ничего и не надо :-)
              • +1
                Может сравнение двух схожих (вроде как) языков помогло лучше запоминать. Не первый случай встречаю и именно с французским. Какие-то сопоставления общего и различий.

                Со чтением проблем особых нет. И, в принципе, вспоминаю сейчас, что can/has встречал всегда без is, но вот как пишу про это забываю. В коде может глаз бы и резануло сразу, а в комментах как-то не заметно…
    • +5
      Стоит пересмотреть код. OR — означает, что функция делает как минимум две задачи, скорее всего одна из них решается не очень хорошо. Надо рассмотреть вариант создания двух функций, выполняющих одно действие, но на отлично. Сам убеждаюсь, что такие имена возникают из за ошибок декомпозиции. Для первой версии кода это не так страшно, но рефакторинг тут необходим.
      • +2
        В случае с репозиторием всё название метода, по сути, SQL-условие (WHERE u.active AND u.confirmed AND (u.country IS NOT NULL OR u.phone_number IS NOT NULL) перенесенное практически прямо из ТЗ — нужен заказчику зачем-то (а зачем не говорит) список активных подтвержденных записей с заполненными страной или номером телефона. Можно было бы применить цепочку фильтров к полному набору записей, но зная особенности реализации (обертка для MySQL) как-то не тру получится тащить всю таблицу (очень большую, допустим) с сервера БД и фильтровать его локально.
        • 0
          В этом случае однозначно надо искать компромисс.
        • 0
          Если заказчику нужен, тогда так и называйте (может стоит чуть сократить getActiveConfirmedWithCountryOrPhone) — по существу этот метод является частью публичного API, т.к. требования к нему предъявляются из вне.

          Я бы попробовал добиться от заказчика назначения этого метода и переименовал бы его в соответствии с этим назначением.
          Плюс у меня в голове автоматически всплывает warning: Не придется ли мне писать тысячу подобных методов на каждую прихоть заказчика? Может стоит сделать API для произвольного (или ограниченого) конструирования фильтров?

          Еще вариант: например если заказчику действительно нужно несколько подобных методов, но, допустим, все записи должны быть активны и подтверждены, можно создать отдельный репозиторий, по типу ActiveUserRepository

          • 0
            Или развести на два репозитория, а заказчику дать фасад к обоим только с теми методами, которые в ТЗ.
        • +2
          +1 к тому, что при проектировании API нужно исходить из целей.

          Ведь это самое «страна или номер телефона» что-то для заказчика значит, более прикладное. Вот этим самым и нужно называть. Они к таким критериям пришли, очевидно не просто так, но всё равно изначальная задача «проверить nnnnn» осталась, и наверняка она называется как-то ощутимо короче и понятнее.
          А если вернуться к изначальной задаче, возможно вылезут другие критерии, может быть даже другое понимание как эту самую проверку осуществлять более правильно.

          Попробуйте разговорить заказчика, в худшем случае — потратите немного времени на общение, в лучшем — API будет более простой, задача будет более понятна и решаться будет проще/полнее/итд.
          • 0
            Как это описывал Макконнелл: используйте терминологию проблемной области вместо терминологии реализации.
    • +3
      Это не имена, это реализация функций внутри имени)
      • 0
        :) Так описывает, что метод делает, а не как он это делает (второй пример не удачный, согласен).
        • +4
          Нет. Он описывает, как это делает:
          getActiveConfirmedWithFilledCountryOrPhoneNumber: function () {
            $this
              ->getActive()
              ->where( 'confirmed', 1 )
              ->where( 'country', Db::NOT_NULL )
              ->where( 'phoneNumber', Db::NOT_NULL )
          }
          


          Вот. Название метода полностью повторяет его содержимое. И смысл?
          Если это настраиваемые пользователем фильтры, то стоило их декомпозировать до такого состояния:

          function getFilled (array $filled) {
             $this
                ->getActive()
                ->confirmed()
                ->whereFilled( $this->filterFilled($filled) );
          }
          


          В контроллере будет какой-то такой код:

          function action_get_field (Request $request) {
             $this->render(
                $this->model->getFilled(
                   $request->post('filled')
                )
             );
          }
          


          Практика показывает, что, в большинстве случаев, программист не может внятно назвать метод когда не понимает, что он делает (суть и дух приложения).
          • 0
            Код немного не такой, фильтры не настраиваемый, все условия «захардкожены» в ТЗ и код контроллера вообще две строки (первую очень похожую на вашу первый код как раз вынес в метод репозитория, чтоб не завязываться на все эти цепочки where, характерные для SQL ORM в контроллере), но не сказал бы, что стало читабельней.

            Действительно не понимаю суть и дух этих пунктов ТЗ (а так обычное приложение знакомств для фб/вк), нужно несколько списков с разными комбинаций условий и имя getFilled не подходит, поскольку все условия содержат «заполнены поля такие-то», и пренебречь Active и Confirmed тоже нельзя, не во всех условиях они есть (а где-то есть «Неактивные»). Ну не называть же мне их getList1, getList2 и т. п. из-за того, что в ТЗ на UI админки они фигурируют под «Список 1» (см. п. 15.1), «Список 2» (см. п. 15.2) и т. д.?
            • 0
              а бывает ли getActiveConfirmed без всяких там?
              если да — чем для заказчика отличаются ActiveConfirmed от ActiveConfirmedWith*?

              Насколько я понял, есть несколько вариантов заполнения, и методы такого рода позволяют отобрать те или иные варианты. В одних случаях нужны активные и подтвержденные, в других нужно что бы была заполнена страна итд.

              Но зачем всё это нужно? Сколько вообще таких случаев? В каких случаях какие сочетания проверяются?
              • 0
                Вот такого не бывает.

                Не раскрывает зачем. Соответствующая страница просто должна вывести список таких-то полей для учёток из админки под говорящими названиями «Список 1». Штук 5 таких страниц (и соответственно методов) с неясным назначением. Единственное, что роднит поля выбора и вывода, что всё относится к ПДн (ФИО, ДР, адреса, мыло, телефоны и т. п.), но посетители сами их указывают и дают согласие на обработку.
                • 0
                  если не раскрывает и повлиять на это нет возможности, значит это внешние требования, которые просто нужно выполнить.

                  В целом-же разговор идет о тех вещах, в которых у разработчика право голоса, как минимум.

                  Но на уровне менеджера проекта, таки есть смысл акцентировать на этих требованиях, что это увеличение рисков в плане поддержки и развития. Если у разработчика нет ясного понимания что и как должно происходить с точки зрения прикладной области, даже если не он её реализует — возрастает риск того, что вылезет нечто неожиданное.
          • +2
            Согласен. На основной из задач разработчика, является правильная, логичная и простая декомпозиция требований. А уже затем идет перенос компилируемую среду. Для кода, который пишется в спешке, его требования не понятны и пр, существует рефакторинг. Надо стараться его использовать.
  • +1
    Насчет «а остальное сделает компилятор» — это так-то верно, но вы поосторожнее с этим. Абстракции текут, знаете ли. Когда пишете код, и хотите чтобы он был производительным — полезно знать, как работает внутри компилятор. А то вот, как тут недавно писали, руби интерпретатор, например, оптимизирует строки, которые короче 23 байт ;)
    • –1
      С этим согласен, в java тоже есть такие оптимизации и в c++ их иногда приходится использовать, и в других языках они точно есть. Тут делается упор на читаемость кода. Нет необходимости компилировать код в уме или сидеть день с отладчиком, чтобы понять, что же он делает
  • +3
    Осторожно замечу, что парадигма меняется.
    Я про async/await с методами в C#. Если в sequential коде вызов метода практически бесплатная операция, то аналогичное с async-методом, да который еще и что-то возвращает — очень и очень даже не бесплатная (только добавив async уже в 13 раз медленнее тупо зайти в пустой метод, что эквивалентно 40 итерациям в пустом цикле синхронного метода): обработка и делегирование исключений, чтобы await-вызов метода можно было просто обернуть в обычный try-catch, а не парсить aggregate exceptions с тасками, как раньше; continuations и много чего еще типа аллокаций в кучи и т. д. — достаточно глянуть на количество генерируемого IL-кода.

    Так что, ребята, без фанатизма. Иногда ничего страшного не дробить метод, а сделать внутри него пару region, которые в свернутом виде будут на глаз эквивалентны двум сигнатурам.

    Поэтому каждый раз когда в асинхронном коде (а под windows 8 его будут уже писать все, если кто-то еще не пользуется async_ctp) вы создаете кучу маленьких асинзронных методов, представьте, что вы, как минимум, добавляете по 40 итераций пустого цикла в каждый из них (не говоря уже о кучи других ранее упомянутых вещей).

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

    Для тех, кто вообще не в теме, рекомендую: channel9.msdn.com/Events/BUILD/BUILD2011/TOOL-829T
    • +1
      вы создаете кучу маленьких асинзронных методов

      Ну так асинхронные методы же явно помечать надо как async.
      Я не думаю, что люди, которые в своём развитии дошли до осознания необходимости маленьких методов, будут делать каждый такой метод асинхронным :)
      • +4
        Парадигму маленьких методов можно и вдолбить. В школе сказали, что квадрат гипотинузы равен сумме квадратов катетов и 99,9 % просто пользуются, не доходя (в этом случае в смысле «доказать») до этого самостоятельно. Так и тут, прочитает novice урывками статью и начнет слепо и безрассудно лепить везде. А потом узнает про модификатор async (опять же, невникая в детали) и точно также просто добавит везде в существующий код (в котором, как вы можете догадаться, все донельзя раздробленно). Вот и получится то, о чем пол-лекции разглагольствует парень на channel9.
        Такое сплошь и рядом. Прекарсный пример куча тормозных приложений на сервелате, где недалекие разработчики применяют шейдер к бордеру, который оборачивают вокруг всего дерева контролов (и уводя тем самым процессор под 100 % только потому, что в текстовом поле будет мигать курсор).
    • 0
      Преждевременная оптимизация… Кто сказал, что в коде именно это его замедляет? Отчет профайлера по всему приложению?

      Синхронизированные методы и пр, это отдельная история. Но единственное место, где приходилось укрупнять методы, разворачивать циклы и делать дополнительные оптимизации — это java ME. В других приложениях всегда есть более медленные участки (проблемы вида: все методы синхронизированы, все методы static и пр. — это прежде всего проблемы).

      Вызов метода — наносекунды, кидание исключения — миллисекунды, обращение к бд и работа по сети — секунды.
      • 0
        Кого «его» замедляет? По какому приложению? Мы с вами об одних вещах говорим? Видео смотрели? Task parallel library + async CTP знаете?

        Я говорю о том, что вызов вот этого:
        public static async Task SimpleMethod()
        {
        Console.WriteLine(«Hello»);
        }
        …в ≈13–14 раз медленнее, чем вызов вот этого:
        public static void SimpleMethod()
        {
        Console.WriteLine(«Hello»);
        }
        …ибо в первом случае под катом и try/catch, и вызов методов специфических методов фреймворка, и т. д. И все становится еще сложнее, если мы возвращаем и принимаем параметры (см. видео).

        Понятно, что в большинстве случаев эта разница будет незаметна, но предупрежден — значит вооружен. И к обсуждаемому типу фатального рефакторинга методов в будущем (т. е. уже сейчас) нужно подходить с осторожностью и без фанатизма.
        • –1
          Различные способы синхронизации должны использоваться разумно и в меру. Только там, где это действительно необходимо. Синхронизировать абсолютно все методы нет смысла. К тому же есть отличный инструмент декомпозиции — ООП, который позволяет группировать критичные участки кода и оптимизировать их отдельно. Видео посмотрел. Всегда делаю выбор в пользу понятности и простоты кода.
          • 0
            Вы не считаете async CTP упрощением? Код ведь теперь выглядит как обычный sequential. Не нужны никакие колбеки, event-based pattern (а-ля BackgroundWorker и т. д.) или begin/end. В большинстве случаев теперь не нужно думать о потоках, контекстах выполнения (а то ведь раньше не дай бог что-то ui-ное не из ui-потока обновишь), делегированию и обработке исключений и т. д.

            Это не просто способ синхронизации — это координальное упрощение всей мультипоточности до той абстракции, что о потоках и их синхронизации просто теперь можешь не думать, а вызывать методы так, как будто они выполняются синхронно.
            • –1
              Согласен, куда же без упрощения. Использовать удобные языковые средства надо. В статье же говорится об общем улучшении читаемости кода.
        • 0
          Я говорю о том, что вызов вот этого:
          public static async Task SimpleMethod()
          {
          Console.WriteLine(«Hello»);
          }
          …в ≈13–14 раз медленнее, чем вызов вот этого:
          public static void SimpleMethod()
          {
          Console.WriteLine(«Hello»);
          }


          а во сколько раз отличаются эти варианты?

          public static async Task SimpleMethod() {
              veryHardQueryToDatabase();
          }
          // vs
          public static void SimpleMethod() {
              veryHardQueryToDatabase();
          }
          • 0
            Вы сейчас о киллограмах, а мы о метрах. Речь о том, что вызов кучки асинхронных методов вместо одного не то же самое, что вызов кучки синхронных вместо одного синхронного. В последнем случае на сегодняшний день один метод или несколько друг за другом — это практически одно и то же по затратам и времени (оптимизации, инлайн, предугадывание flow и т. д.),
    • +1
      continuations
      А причём тут они вообще? Это же вроде как Mono-specific фича, реализующая дешёвые програмно-управляемые потоки (кстати, офигенная штука, надо будет как-нибудь статейку на эту тему написать).
      • +1
        Тогда держите набор топиков для вас на ближайшую пару неделю: Task parallel library, async ctp.

        Однако вкратце отвечу на ваш вопрос. Если раньше вы бы в UI-потоке написали такую строчку:
        byte[] bigData = LongDownload(string address);
        …то ваше приложение бы зависло на энное количество секунд, пока качается файл.

        Однако теперь одно небольшое изменение + небольшие изменения в сам метод LongDownload и вот это:
        byte[] bigData = await LongDownload(string address);
        …не приведет к зависанию. Как спросите вы? Метод сразу сделает return и продолжит с той же строчки уже после завершения. Под капотом за вас пишется спагетти и Task.ContinueWith + куча обработок и делегирования ошибок (то, что до async_ctp приходилось писать руками).
        • 0
          async, если не ошибаюсь, есть обычный синтаксический сахар, да ещё и с не вполне очевидным поведением. Вы почитайте текст по ссылке, там тоже много интересного.
    • 0
      Вызов функций всегда затратен, если это не inline. В наследство к нашему проекту досталась кучка макросов, сделанный специально для замены вызовов функций (как бы inline). В наши времена это уже не важно, но по традиции все пользуются макросами.
      • 0
        Ну тут уже серьезнее дела в виде перемещения со стека в кучу, аллокации на куче, напряги сборщика мусора и т. д. Отсюда и overhead по сравнению с обычными методами.
  • +3
    Я воспринимаю такие вопросы как признание в плохом знание языка
    Код становится проще семантически, а технически — сложнее.
  • +1
    Стараюсь соблюдать эти правила? особенно в логически-сложных участках кода.
    Удобство в первую очередь — это чтение кода, когда строк не много и описано методами (функциями)
    К такому коду возвращаться через время легко. Ну и не забывать все же документировать методы.
    Ну и новым сотрудникам вникать проще, читая тонны кода сразу же — это стресс.
    Второе удобство — это при юнит-тестировании.

  • +2
    > «Зачем ты выносишь, однократно используемый, код в функции?»

    Глаза завернулись внутрь. Откуда в рунете такая любовь к запятым?

    По существу статьи:

    Если писать в функциональном стиле, то код более-менее приводится к нормальному виду: разбивается на логические блоки, небольшие, выполняющие только что-то одно функции и т.п.

    Но не сразу, конечно :)
  • –1
    Тут я думаю стоит учитывать зачем Вы открыли код.
    Если Вы его «читаете», то Вы скорее всего новичок и Ваша задача быстро влиться в команду и быстро понять, что вообще тут происходит.
    Если Вы его «исполняете», то Вы скорее всего ищете ошибки, потому что «читая» код Вы их не найдёте (читатель всегда доверяет писателю не так ли?).
    Отсюда на мой взгляд простой вывод — надо соблюдать меру и учитывать, что код будет и «исполняться» и «читаться».
    ЗЫ: а насчёт выделения разового кода в отдельную функцию, да просто потому что это отдельная логическая единица, если компилятор посчитает выделение неуместным заинлайнит не маленький в конце концов.
    • +1
      > потому что «читая» код Вы их не найдёте (читатель всегда доверяет писателю не так ли?).

      Неверно. Code Review — это главный способ нахождения ошибок (ссылку, увы, сейчас не найду, но, по-моему, об этом говорится тут: vimeo.com/9270320)
      • +1
        По-моему мы по разному понимаем «читать»/«исполнять», для успешного Code Review имхо простого «чтения» не всегда достаточно.
        На мой взгляд чтение не подразумевает полного изучения кода, ограничиваясь логическими блоками, то есть, грубо говоря, есть функция getCircleArea(double radius), тогда «читая» код Вы верите, что эта функция действительно возвращает площадь круга, а «исполняя» код мы должны удостовериться, что использованная формула нахождения площади правильная. Хотя я всё же погорячился пожалуй, логические ошибки проще искать на уровне «чтения», а ошибки уровня «исполнения» надо бы покрывать тестами.
        • 0
          Думаю, под «читать» имеется повседневная работа с кодом, под «исполнением» — поиск ошибки в конкретном участке (после чтения, конечно).
          Ревью кода — это отдельная работа, со своей спецификой.
  • 0
    > потому что «читая» код Вы их не найдёте (читатель всегда доверяет писателю не так ли?).

    Неверно. Code Review — это главный способ нахождения ошибок (ссылку, увы, сейчас не найду, но, по-моему, об этом говорится тут: vimeo.com/9270320)
  • 0
    Ревью очень важно. Всегда стоит доверять не писателю, а коду. Считаю лишним заглядывать в тело функции, только потому что хочу ее использовать. А вот если она делает не то что я ожидал, то начинаю исследование. При не совсем ясном имени, сигнатуре, стараюсь делать рефакторинг.

    Иногда такие неожиданности попадаются: две функции readFileAsString и readFileAsStream. По логике должны делать одно и то же, но возвращать результат в разных форматах. Логически ожидаю, что одна вызывает другую. Пишу код, а он не работает, заглядываю в реализацию. А там первая функция читает файл из BLOB БД, а вторая из ФС и, в некоторых случаях, делает запрос к удаленному серверу. Странно, но факт.
  • +2
    Где-то на середине статьи стало понятно, что в конце будет упомянут Макконнелл.
    Так и вышло.
    • 0
      Может еще кого-нибудь посоветуете почитать? Помимо упомянутых в статье?
      • 0
        Нет, едва ли. У меня довольно скептическое отношение к литературе про «хороший стиль». Куда полезнее просто пытаться экспериментировать с архитектурно сложными проектами.
        Наверное, из всего что видел, понравились только «Эффективное использование C++» Скотта Майерса, ввиду своей конкретики, прагматичности и отсутствия воды.
        • 0
          Очень стоящая книга. В статье старался избегать упоминания книг для конкретных языков. Год назад перешел с c++ на java. Все больше убеждаюсь, что хорошие практики можно примерять вне зависимости от языка.
      • 0
        Мне очень понравилась «Эффективная работа с унаследованным кодом» Майкла Физерса — перекликается и с «Совершенным кодом», и с «Рефакторингом...», но с упором не на то, как сделать свой текущий код читаемым для других, а на то, как сделать чужой нечитаемый код читаемым для себя и других, не теряя при этом его функций, как документированных, так и нет.
  • +7
    Я поставил минус, потому что «вот опять».
    Очередной разработчик, прочитав хорошие книжки, решает состряпать статью на полтора абзаца о том, что только что прочитал. Ну зачем? Везде, где вы спросите об искусстве программирования, вас ткнут в эти книги. И только читая их целиком, можно понять, что упомянутые тезисы действительно работают. Потому что авторы посвятили этому целые книги, потратили гораздо больше, чем пару часов на написание своего труда и имеют гораздо больший, чем несколько лет, опыт в разработке. Так зачем их в очередной раз пытаться пересказать на одной страничке?

    Поделитесь лучше собственным опытом разработки чего-нибудь, укажите какие подводные камни посчастливилось повстречать, какие проблемы удалось решить. Принесет гораздо больше пользы.
    • +5
      Обязательно напишу что-нибудь более фундаментальное!
      Многие программисты считают себя слишком крутыми, чтобы читать какие-нибудь книги. «Некогда думать — надо писать код.» Спорить с ними бессмысленно, а вот дать ссылку на краткое описание проблемы и мнение большего количества людей очень полезно. Может, это мотивирует прочесть полезную литературу.

      Поставил плюс за понимание проблемы и альтернативное мнение.
    • 0
      Правда многие разработчики никогда не спрашивают об искусстве программирования и вообще ничего не знают что оно существует. Автор дает им шанс узнать что такое искусство существует и вкратце говорит в чем его суть, дает ссылки где про это прочитать.
  • +2
    а мне понравился этот момент:

    Почти после каждой [моей] более-менее существенной правки, <...> Говорят, что код стал менее понятным, становится трудно найти нужную строку и пр.

    На самом деле проблема кроется как в коде, так и в разработчике, который привык к коду.


    Действительно. Где-же еще?
  • НЛО прилетело и опубликовало эту надпись здесь
    • НЛО прилетело и опубликовало эту надпись здесь
  • +1
    Я бы порекомендовал еще вот этот ресурс по теме: sourcemaking.com/refactoring
    • 0
      Да, спасибо. Неплохой ресурс.

Только зарегистрированные пользователи могут оставлять комментарии. Войдите, пожалуйста.