Важность проверки кода при разработке программного обеспечения нельзя недооценивать. Правильно проведенный анализ кода не только улучшит качество кода и выявит потенциальные проблемы на очень ранней стадии, но и поможет сформировать навыки разработчика. Однако в большинстве случаев проверка кода означает просто беглый взгляд на изменения и предоставление одного или двух комментариев. Не многие делают подробные и сквозные обзоры. Если вы ищете, как лучше выполнить проверку кода, вот несколько рекомендаций, которые могут вам помочь. Прежде чем перейти к техническим вопросам, давайте обсудим несколько нетехнических вопросов:
Первый момент, который следует недооценить, заключается в том, что обзоры кода потенциально могут стать точкой столкновения между Рецензентом и разработчиком. Разработчики (особенно в годы становления карьеры) склонны проявлять собственническое отношение к коду, который они пишут. Каким бы дерьмовым ни был их код, они думают, что это лучший код, когда-либо написанный. В подобных случаях Рецензент должен добавить немного сахара в комментарии. Кроме того, сидеть рядом с разработчиком во время проверки кода помогает больше, чем выполнять удаленную/отключенную проверку кода.
Далее, проверка кода – это НЕ проверка форматирования или какого-либо статического анализа вручную. У каждого человека будут личные предпочтения в отношении таких элементов, как табуляция/пробелы или скобки в той же строке/следующей строке и т.д. Лучше всего уладить это каким-то образом (диктатура старшего парня в основном работает, но да, кулачный бой тоже является решением), и все остальные должны следовать ему. Лучше всего интегрировать это в настройки форматирования кода в вашей любимой IDE, чтобы он форматировался по мере ввода/сохранения. Аналогичным образом вы можете выполнять весь статический анализ кода (например, потенциальные исключения нулевого указателя или использование устаревших методов) с помощью различных инструментов и интегрировать то же самое в свою среду разработки и процесс сборки.
Наконец, не предоставляйте субъективные комментарии к обзору. Это приводит только к столкновениям. Когда вы предоставляете какой-либо комментарий к обзору, если комментарий/запрошенное изменение неочевидны, необходимо предоставить веские аргументы в пользу изменения. Это очень помогает в отношениях между Рецензентом и Разработчиком, а также помогает Разработчику понять то, о чем он не знает.
Теперь давайте посмотрим на цель проверок кода.
- Убедитесь, что изменение соответствует функциональным требованиям
- Если возможно, определите любые потенциальные проблемы (отдельные ошибки, потенциальная проблема с производительностью и т.д.)
- Удобочитаем и удобен в обслуживании
При проверке кода следует соблюдать и проверять следующие рекомендации:
Присвоение имен:
“Роза с любым другим названием пахла бы так же сладко”. Что ж, давайте отдадим должное Шекспиру, это правда. Та же логика не может быть применена к именованию классов/методов/переменных в программировании. Скажем, метод, который удаляет объект из базы данных, лучше называть Entity.удалить(). Любое другое имя, такое как Entity.hide() или Entity.remove(), не является таким же правильным и/или эффективным, как delete().
Один пример из реальной жизни. Много лет назад я наткнулся на метод с именем Tree.search Для узла (имя). Это было реализовано как обход узлов в дереве, и если найден узел с именем, верните то же самое, если не найдено, затем создайте узел с этим именем; обновите дерево, чтобы иметь узел; вставьте узел в базу данных и верните узел
Представьте себе замешательство вызывающих этот метод. Никто бы не ожидал, что вставка в базу данных будет результатом метода “поиска”. Это не тот случай, если метод был назван как createIfNotExists().
Таким образом, имена, раскрывающие намерения, очень важны для понимания и поддержания кодовой базы.
Соответствует архитектуре:
Архитектура программного обеспечения – это выбор дизайна. Какую структуру использовать; каковы отдельные модули; как они взаимодействуют сами с собой; какие библиотеки использовать; каковы сквозные проблемы и как их реализовать.
Единая Ответственность:
Каждый метод и каждый класс должны делать только одно, и делать это хорошо. Во время проверки мы должны убедиться, что это действительно происходит. В практических случаях, когда единая ответственность невозможна, мы должны убедиться, что метод, по крайней мере, разбит на более мелкие методы, каждый из которых несет одну ответственность. В приведенном выше примере createIfNotExists() выполняет две функции (1) Поиск узла, (2) Создание узла. Было бы лучше, если бы мы могли разбить функциональность на два разных метода Tree.search() и Tree.create(), а Tree.createIfNotExists() – это просто оболочка, которая вызывает оба метода.
Абстракция:
Каждый уровень/класс/метод должен иметь надлежащую абстракцию.
В многоуровневой архитектуре каждый уровень должен эффективно скрывать сложности нижележащего уровня и обеспечивать абстракцию для вышележащих слоев. Например, Уровень доступа к данным эффективно справится со сложностями Хранилища данных (внешние ключи, транзакции и т.д.) И предоставит логическую модель для работы. Уровень обслуживания будет использовать логическую модель и раскрывать функциональные возможности. Уровень пользовательского интерфейса/API будет использовать эти сервисы и предоставлять пользовательский интерфейс или API. Теперь у каждого слоя есть абстракция. Бизнес-уровень не должен работать с внешними ключами или обрабатывать HTTP-запрос/ответ.
Аналогично, если класс с именем Binary Tree пытается проанализировать конфигурационный файл для чтения некоторой конфигурации, это также является непрочной абстракцией. Синтаксический анализ и настройка переменных конфигурации и среды лучше всего выполняются другим классом, в то время как абстракция двоичного дерева должна обрабатывать только операции реализации двоичного дерева.
Обеспечить правильный уровень абстракции:
В объектно-ориентированном проектировании абстракция предоставляет вам более простой способ взаимодействия, скрывая все ее внутренние компоненты. Не переоценивайте больше того, что должно быть известно клиентам. Примеры:
- Должен ли этот метод/переменная быть действительно общедоступным? Или, может быть, защита подойдет?
- В некоторых случаях свойство должно быть изменяемым только внутри класса, в то время как клиенты должны иметь доступ к свойству только для чтения. В этом случае требуется наличие общедоступного метода получения, но зачем нужен общедоступный метод установки?
- Возвращает ли метод getChildren() список? Что делать, если клиент изменит список? Автоматически ли адаптируется класс дерева? Может быть, мы должны возвращать перечисляемый объект, чтобы клиент мог выполнять итерации и получать доступ к дочерним элементам, но не мог изменять коллекцию?
Обеспечьте правильный уровень абстракций, чтобы не было никаких неожиданных поведений.
Способ контракта:
Каждый метод будет иметь контракт. Контракт состоит из предварительного условия, Исполнения/Поведения и Постусловия. Например, давайте рассмотрим метод insert(TreeNode), который вставляет новую запись в базу данных. Потенциальными предварительными условиями являются (1) TreeNode не должен быть null и (2) поле name не должно быть null. Выполнение/поведение заключается в вставке строки в базу данных, а Постусловие заключается в том, что запись в базе данных должна существовать с заданными значениями.
Предварительные условия должны быть проверены перед началом выполнения. Также предполагается, что ответственность за выполнение предварительных условий лежит на вызывающих этот метод. Любой сбой в предварительном условии метода должен привести к исключению. Если при выполнении все идет правильно, метод должен гарантировать, что постусловие выполнено. Если по какой-либо причине Постусловие не может быть выполнено, опять же, это должно привести к исключению.
Типичной реализацией метода insert может быть:
void insert(TreeNode node){
//Pre-Condition 1
if(node == null)
throw new IllegalArgumentException("Parameter 'node' cannot be null");
//Pre-Condition 2
if(node.getName() == null)
throw new IllegalArgumentException("'Name' of the node cannot be null");
//Execution
try(get the Database Connection){
// insert into record into the database
}catch(DuplicatePrimaryKeyException e){
throw new InvalidDataException("A node with the name '"+node.getName()+"' already exists");
}
}
Если вы заметили, в то время как мы проверяем предварительные условия, оценка Постусловия (существует ли запись в базе данных с заданными значениями) здесь не выполняется. Обычно это проверяется в модульных тестах для этого метода.
Выбор правильного типа данных:
Выбор правильного типа для заданных данных имеет важное значение для уменьшения ошибок. Много раз я сталкивался со строками, используемыми вместо перечислений. Если список значений конечен и известен во время компиляции, зачем использовать string? Пример: Пол.
Список – это еще один широко и ошибочно используемый тип, помимо перечислений. Подумайте об этом:
List excludedExtensions = ReadFromConfig();
Список используется для хранения упорядоченной коллекции значений. Значения также могут быть продублированы. В этом случае исключаемые расширения файлов будут уникальными, и порядок не имеет значения. Они зачем используют список? Здесь Set – это более подходящий тип данных, чем список.
Краткое примечание по работе с устаревшими кодовыми базами:
Если вы работаете над устаревшей кодовой базой, скорее всего, это не самый чистый код, который вы видели (если вы хотите, чтобы он был без сахарной глазури: это полный беспорядок). Единственный способ очистить его – делать это постепенно. Каждый коммит, который вы делаете, должен оставлять кодовую базу более чистой, чем это было раньше. Если это невозможно, по крайней мере, убедитесь, что он не стал еще более грязным. Требуется много терпения и времени, чтобы медленно это исправить.
Оригинал: “https://dev.to/grprakash/code-review-guidelines-3fad”