Обеспечение качества кода в масштабных проектах

Автор оригинала: John Terenzio
  • Перевод


Когда осенью 2012 года я пришёл в Airbnb, то здесь мягко выражаясь, был некоторый разброд и шатание. Некоторое время назад компания начала расти и развиваться огромными темпами. В первую очередь это выражалось в объёмах трафика и транзакций. Чтобы справляться со всем этим, очень быстро увеличили и штат разработчиков. За год до моего прихода в группе было 16 человек, со мной было около 40, а сейчас уже свыше 130. И одной из главных проблем, вызванной всеми этими процессами, стало сохранение качества кода в стремительно увеличивающемся и усложняющемся проекте.

Оглядываясь назад, мне кажется, что это был переломный момент в истории нашей компании. Взрывное развитие стало причиной возникновения ряда технических и культурных вызовов для Airbnb, которые в большинстве своём пересекались друг с другом. Нам и раньше приходилось решать многочисленные трудные задачи, но в целом они были связаны с:

• масштабированием цельных приложений на Ruby on Rails, которые создавались безо всякого расчёта на последующую масштабируемость,
• и увеличением группы разработчиков для решения предыдущей задачи.

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

Сейчас я работаю в группе, занимающейся разработкой модуля для обработки платежей. Поэтому для нас качество кода, его устойчивость и удобство сопровождения имеют критически важное значение. Учитывая объём транзакций, недопустимы даже маленькие ошибки. За последний год мы проделали большую работу по выработке правил и методик, позволяющих писать код быстро и в то же время качественно. В первую очередь, речь идёт о об установлении и соблюдении при кодинге специфических стилей и условий, регулярной проверке кода коллегами (peer review) и тестировании.

Согласованность кода


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

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

У себя в Airbnb мы улучшали согласованность кода в течение двух лет. Сначала мы разработали руководство по стилю (было несколько версий этого руководства). Самые детальные руководства по JavaScript и Ruby, также есть по RSpec, дизайну API, проектированию услуг и т.д. Несмотря на то, что эти руководства основаны на наших специфических требованиях и условиях, многие из них теперь используются как другими коллективами, так и одиночным разработчиками. Конечно, потенциальная польза этих руководств зависит от типа приложения, над которым вы работаете, но если вы разрабатываете на JavaScript или Ruby, то рекомендую в любом случае ознакомиться с ними. Хотя бы для вдохновения.

Некоторые правила приняты нами исключительно произвольно. Например, по отступам (табуляция и пробел), или на какой строке располагать открывающую фигурную скобку. Эти и прочие моменты — дело вкуса. Тут важно, чтобы правила соблюдались всеми причастными лицами. С другой стороны, в ряде случаев возможны варианты.

Возьмём длину строки. Мы предпочитаем делать строки короткими и наглядными. Короткие строки не только более читабельны, но и позволяют задавать операторы в более простой форме (особенно, когда они используются с описательными именами переменных). Методы, состоящие из серий коротких простых операторов, легче для восприятия и модифицирования. Даже диффы в результате точнее показывают, что поменялось между коммитами. А поскольку мы пишем тестопригодный код, содержащий небольшие, лаконичные методы, то это подталкивает к созданию очень «чистой», модульной и лёгкой для понимания кодовой базы.

Рассмотрим пример:

usernames = Users.where(:status => 1, :deleted => false).map{ |u| u.first_name.downcase }.reject{ |n| n == ‘john’ }.map{ |n| n.titleize }


Допустим, нам нужно поменять регистр. Дифф покажет нечто подобное:

-  usernames = Users.where(:status => 1, :deleted => false).map{ |u| u.first_name.downcase }.reject{ |n| n == ‘john’ }.map{ |n| n.titleize }

+ usernames = Users.where(:status => 1,  :deleted => false).map{ |u| u.first_name.upcase }.reject{ |n| n == ‘JOHN’ }.map{ |n| n.titleize }


Трудно сказать навскидку, что именно поменялось, не пропарсив строку у себя в голове. А если бы код был изначально написан так:

users = Users.where(:status => 1, :deleted => false)
usernames = users.
      map{ |user| user.first_name.downcase }.
      reject{ |name| name == ‘john’ }.
      map{ |name| name.titleize }


То разница в диффе была бы куда нагляднее:

users = Users.where(:status => 1, :deleted => false)
usernames = users.
      - map{ |user| user.first_name.downcase }.
      - reject{ |name| name == ‘john’ }.
      + map{ |user| user.first_name.upcase }.
      + reject{ |name| name == ‘JOHN’ }.
      map{ |name| name.titleize }


Забавно, но изначально те из нас, кто продвигать такой подход, поначалу столкнулись с сопротивлением коллег. Пришлось проявить настойчивость, и в результате это вошло у всех нас в привычку — делать короткие строки. Совсем экстремальные примеры на эту тему можно увидеть в руководствах Joint Strike Fighter C++ и JPL C Coding. Очевидно, что эти стандарты избыточны для большинства потребительских веб-приложений, поэтому всегда соотносите уровень проекта и цели, которых хотите достичь соблюдением тех или иных правил. Важно соблюдать баланс.

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

Проверка кода коллегами


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

Мы стараемся вовлекать в аудит всех, кто так или иначе причастен к данной части кода или функционала. В результате по наиболее нетривиальным pull request возникает как минимум одно-два независимых обсуждения. Иногда процесс разбора заходит так далеко, что некоторым приходится изучать ранее неизвестные для себя вещи (например, процессинг кредитных карт, внутреннюю организацию баз данных, криптографию и т.д.). Если изменение в коде затрагивает какой-то большой важный вопрос, то мы создаём соответствующие рекомендации и документацию. Хочу подчеркнуть, что при рецензировании у нас не допускается апеллирование к своему статусу или должности. Ценится вклад любого члена команды, и в бой выкатываются наилучшие решения. Всё это является и хорошей школой для новичков, к слову говоря.

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

Вот пример ситуации, когда главную роль играет не стиль кодинга, а мудрый подход:

rus_users = User.active.select{ |u| ‘RUS’ == u.country }
puts “There are #{rus_users.length} RUS users today”


и

rus_users = User.active.where(:country => ‘RUS’)
puts “There are #{rus_users.count} RUS users today”


Первый вариант более ресурсоёмок, и при больших объёмах данных может привести к катастрофическому падению производительности. Добавление select к объекту User.active означает, что Rails обратится к MySQL, вызовет и инстанцирует всех активных пользователей, положит их в массив, итерирует его и выберет пользователей, чья страна соответствует RUS. И всё это лишь для того, чтобы посчитать их.

Во втором примере мы тоже начинаем с объекта User.active, но затем применяем фильтр where. Первая строка не инициирует какие-либо запросы к базе данных. Когда во второй строке, когда мы запрашиваем счётчик, Rails лишь делает запрос SELECT COUNT(*) и не заморачивается вызовом строк или инстанцированием моделей.

Вот ещё пример:

плохо:

amount = Payout.
where(:successful => true).
where(‘DATE(timestamp) = ?’, Date.today).
inject(0) { |sum, p| sum += p.amount }


хорошо:

amount = Payout.
where(:successful => true).
where(‘DATE(timestamp) = ?’, Date.today).
sum(:amount)


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

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

Тестирование


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

Заключение


Подведу итоги всему сказанному.

Все члены профессиональной команды разработчиков, какого бы размера она не была, должны писать код в одном, заранее утверждённом стиле. Это помогает широко использовать удачные подходы и методики, облегчает сопровождение и обслуживание кода другими людьми. Лучшим началом будет создание руководств по стилю кодинга. Даже обманчиво простые и очевидные вещи могут сильно влиять на качество кода, его устойчивость и простоту рефакторинга.

Необходимо активно и конструктивно рецензировать код друг друга. Как минимум один человек, помимо автора, должен просмотреть все диффы. Доброжелательное внесение предложений и их проверка кода является залогом сплочения коллектива и роста профессионального уровня. Это помогает новичкам быстрее и полнее учиться, обретать полезные навыки и привычки. В конце концов, это может уберечь ваше приложение от серьёзного падения.

Важно развивать сильную культуру тестирования, не идти в этом на компромиссы или «срезать дорогу». Чем больше мы тестируем, тем больше это превращается в привычку. В результате можно даже полюбить этот процесс.
Airbnb
62,86
Компания
Поделиться публикацией

Комментарии 18

    +7
    Рассмотрение коллегами

    image
    Я бы назвал пункт «Проверка кода коллегами» или просто Code Review.
      0
      если вы можете по стилю определить, кто из ваших коллег написал код — то это очень плохой знак

      Я, иногда, себя чувствую экстрасенсом. Не только знаю, кто написал, но и в каком настроении…
        +9
        Дифф покажет нечто подобное:

        Ни в коем случае не спорю с вашими доводами, но правильный дифф покажет всё-таки нечто подобное:
          +7
          Ну опять же — смотря где дифф смотреть. А вот конфликты резольвить определенно удобнее будет с описанным в статье подходом.
            0
            Предлагаете найти программу, которая позволяет подобным образом делать merge?

            Описанный в статье подход плох тем, что он unenforceable. Нельзя сделать формальную метрику «длина строки не более 40 символов», засунуть её в git hook и надеяться на то, что ни у кого не появится необходимости действительно сделать такую строку. Без формальных метрик этот подход особенно плох, потому что новоприбывшие коллеги всегда будут ошибаться.

            Есть только одно хорошее решение. Нужно использовать хороший софт, и не подстраиваться под плохой.

            Вот авторы использовали лямбда-функцию со своим ORM, и получили серьёзные проблемы с производительностью. Тем не менее, если бы ORM через рефлексию подобные лямбды переводил в SQL, или если бы он делал custom SQL functions, или хотя бы выдавал предупреждение «осторожно, таблица будет загружена целиком», таких проблем бы вообще не возникало.

            Но авторы пошли путём Роскомнадзора, (почти) решили проблему административно, продолжив использовать низкокачественное ПО.
              0
              > потому что новоприбывшие коллеги всегда будут ошибаться.

              Такой ерунде новоприбывшие коллеги обучаются максимум со второго тыканья носом.

              > Предлагаете найти программу, которая позволяет подобным образом делать merge?

              Ну только «подобным образом _удобно_ делать». Заодно можно найти тех, кому удобно читать текст длинными строками, а не колонками (у человеков с этим проблемы, им почему то удобен spritz).
              Потом обучить новоприбывших коллег этой прекрасной, но никому не известной, программе.

              Ну а потом посчитать убитые на это человекочасы и сформулировать чем же тут административное решение проигрывает
                0
                Такой ерунде новоприбывшие коллеги обучаются максимум со второго тыканья носом.

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

                Заодно можно найти тех, кому удобно читать текст длинными строками, а не колонками (у человеков с этим проблемы, им почему то удобен spritz).

                А потом найти хотя бы одну книгу по матану поверх spritz. К сожалению, код это вам не худлит, он всегда читается с использованием мозгов, длина строк со сложностью прочтения коррелирует не так уж и сильно (если не говорить про вырожденные случаи вроде однострочных программ, которые никто в продакшен и так не напишет).

                Впрочем, длинные строки говорят не о том, что «кто-то тут не умеет оформлять», а о том, что тут ORM с использованием method chaining вместо хорошего набора комбинаторов. ПО плохое, как всегда.
                  0
                  Так вы просто смотрели как текст носом месяцами и вручали про ущербные технологии, или Вы нашли и внедрили совершенный софт?

                  У вас недовольство технологиями не так уж сильно коррелирует с желанием ими поделиться. От административных решений толку больше
                    0
                    * Так вы просто смотрели как тыкают носом месяцами и бурчали про ущербные технологии
              0
              Везде можно так смотреть git diff --word-diff или отдельные утилиты типа dwdiff всё покажут и в консоли.
              Я за короткие строки, но дифф тут явно не аругмент
            +7
            Всё это прекрасно, кроме одного: главная страница вашего сайта своей тяжестью просто убивает обычные компьютеры.
            А уж эта новомодная фича с живым видео на фоне, которое ещё и стартует само без спроса…
              +1
              Какие-то детские примеры с использованием where и sum. У них реально был такой код? Реально для кого-то неочевидно, что лучше фильтровать суммировать на уровне базы данных, а не приложения?
                –2
                Я бы сказал по другому — «какой то самопиар»
                  0
                  Приэтом делать map поверх массива AR сущностей вместо того, чтобы сделать #pluck в первом примере.
                    0
                    Ага, их код:
                    usernames = Users.where(:status => 1, :deleted => false).map{ |u| u.first_name.downcase }.reject{ |n| n == ‘john’ }.map{ |n| n.titleize }
                    


                    Мой код:
                    usernames = Users.where(status: 1, deleted: false).where("lower(first_name)!='john'").pluck('lower(first_name)').map(&:titleize)
                    

                    В прочем тут тоже видны косяки:
                    Что за «status: 1»? Это же классическое волшебное число, которого нужно избегать (как минимум scope-ом заменить)
                  +4
                  Да, глубина рассмотрения вопроса захватывает дух: обеспечение качества кода в масштабных проектах достигается 1) кодинг-стайл 2) код-ревью 3) тестирование.

                  Видно человеку сказали «надо попиариться, напиши как мы работаем», думал автор не долго. А ведь компания-то — передовая. Для социума то, что они делают, это — прорыв, повышают организованность человеческого общества програмными средствами в рамках капиталистического хозяйствования в масштабах Земли. Бросают вызов одной из крупнейших, консервативнейших и most established индустрий планеты — гостиничной. В данном случае автору лучше было бы просто нащёлкать фоток из офиса, тем более из «130 человек», наверняка есть и девушки красивые, и модные хипстеры, есть что показать наверняка.
                    0
                    Автоматизацию проверки и коррекции стиля js кода какую-то используете в IDE? Если да, то можно еще несколько предложений именно об этом?
                      0
                      Господи, когда уже прекратят выпускать зеркала-мониторы? Смотреть на такой сплошная боль

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

                      Самое читаемое