Плохой код #3

Евгений Бовыкин Задача 13 Мая 2016

Как мы уже могли убедиться, правильные имена переменных могут избавить от лишних комментариев. Так же, качественно имя может объяснить смысл переменной или функции, облегчить понимание кода или даже помочь в определении интерфейса функции. Но часто программисты в спешке забывают давать объектам правильные имена - “потом при рефакторинге все приведу в порядок”. Но часто про рефакторинг с радостью забывается, но если он все-таки проводится - работа это непростая, потому что понять, что же делает код и как его переработать становится очень сложно.

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

Имена переменных еще более субъективная вещь, чем комментарии, и иногда нет однозначного ответа на то, какое же имя лучше. Однако для очень многих случаев существуют стандартные практики, которых стоит придерживаться. Я старался подобрать примеры кода так, чтобы затронуть именно “классические” ошибки.

int n = A.length; // Количество сотрудников, которые еще не получили зарплату
function sumAllElementsInArray(arrayOfElements){
    let overallSum = 0;
    for(let indexOfElement; indexOfElement < arrayOfElements.length; indexOfElement++){
        overallSum += arrayOfElements[indexOfElement];
    }
    return overallSum;
}
class Board {
    
    constructor(){
        this.mainList = [];
    }

    getFl(){
        let a = [];

        for(let i = 0; i < this.mainList; i++){
            if(this.mainList[i].value === 4){
                a.append(this.mainList[i]);
            }
        }

        return a;
    }
}
function copy(a1, a2){
    for(let i = 0; i < a1.length; i++){
        a2[i] = a1[i];
    }
}
let a = l;
if(0 == l){
    a = O1;
} else {
    l = O1;
}
class HTTP {
    fetchURL(url){ /* ... */ }
    getDataFromURL(url){ /* ... */ }
}
class Queue {
    addValueToQueue(value) { /* ... */ }
    isQueueEmpty() { /* ... */ }
}
class Time {
    getTimeInDDMMYYFormat(){ /* ... */ }
}
function checkoctal(c){
    return '0' <= c && c <= '7';
}

Решение

Первый пример

int n = A.length; // Количество сотрудников, которые еще не получили зарплату

Комментарий пытается объяснить то, для чего нужна переменная n. При этом само имя n ничего нам не говорит. Это явные сигналы того, что переменной стоит дать более содержательное имя. employessNumberWithoutSalary вполне подойдет. Длинно, зато понятно.

С другой стороны, массив, содержащий этих сотрудников, называется А. Это тоже не несет в себе никакой информации. Переименуй мы его в employeesWithoutSalary, объяснять, что такое n скорее всего не пришлось бы.

Итак, у нас есть два варианта исправления

int employeesNumberWithoutSalary = A.length;
int n = employeesWithoutSalary.length;

Второй вариант предпочтительнее, так как массив сотрудников может использоваться где-то еще. Однако использовать короткие имена вроде n стоит только в коротких участках кода. В ином случае ее стоит опять же назвать employeesNumberWithoutSalary, однако это имя становится очень похоже на employeesWithoutSalary, что тоже плохо. Тогда лучшее ее переименовать в numberOfEmployeesWithoutSalary, что выглядит громоздко, зато понятно, и не конфликтует с другими именами.

Второй пример

function sumAllElementsInArray(arrayOfElements){
    let overallSum = 0;
    for(let indexOfElement; indexOfElement < arrayOfElements.length; indexOfElement++){
        overallSum += arrayOfElements[indexOfElement];
    }
    return overallSum;
}

В этом примере все наоборот. Функция занимает пять строк, включая заголовок, при этом имена в ней очень длинные. Функция суммирования всех чисел в массиве используется довольно часто, поэтому имя ее должно быть короткое, но от этого не менее содержательное. Хорошими вариантами являются sum или sumArray.

С именами внутри функции та же проблема. Сравните arrayOfElements и array. Есть ли разница между этими двумя именами? Первое имя по сути является тавтологией, ведь чего еще может быть массив, как ни каких-то элементов? indexOfElement ничем не лучше, чем index, а в таком коротком цикле имеет смысл использовать просто i, поскольку любой программист знает, что в цикле под i обычно означают индекс элемента. Имя overallSum не является таким уж плохим, однако замена его на sum или _sum или даже s ничего не ухудшит.

В итоге получаем такой код.

function sumArray(array){
    let sum = 0;
    for(let i; i < array.length; i++){
        sum += array[i];
    }
    return sum;
}

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

Третий пример

class Board {
    
    constructor(){
        this.mainList = [];
    }

    getFl(){
        let a = [];

        for(let i = 0; i < this.mainList; i++){
            if(this.mainList[i].value === 4){
                a.append(this.mainList[i]);
            }
        }

        return a;
    }
}

Метод getFl не дает даже намека на то, что в нем происходит. Только по имени класса Board можно понять, что мы имеем дело с какой-то доской, скорее всего игровой. Такой код может понять только человек, который разобрался во всех тонкостях этой программы и знает, что такое mainList, почему 4 такое важное число, и что вообще такое value. Это очень яркий “запах” плохого кода. Попробуем его переработать, предварительно узнав, что же вообще этот метод делает у его разработчика.

Выясняем, что mainList это массив всех клеток игровой доски. Поле клетки value означает его тип, который кодируется от 1 до 4, и 4 означает клетку с поднятым флажком. Теперь становится понятно, что getFl возвращает все клетки с флажком. Начнем переработку.

Прежде всего, изменим getFl на getFlaggedCells, в крайнем случае - getFlagged, хотя из такого именни не совсем понятно, что такое Flagged. Если этот метод будет использоваться только внутри класса, такое именование еще имеет смысл, если нет - точно нужно выбрать более понятный вариант. Затем переименуем a во flagged. Имя получилось достаточно коротким, и тем не менее понятным.

class Board {
    
    constructor(){
        this.mainList = [];
    }

    getFlaggedCells(){
        let flagged = [];

        for(let i = 0; i < this.mainList; i++){
            if(this.mainList[i].value === 4){
                flagged.append(this.mainList[i]);
            }
        }

        return flagged;
    }
}

Теперь нужно изменить класс Cell. Его внешнему пользователю не стоит помнить о том, что value === 4 означает, что в этой клетке поднят флажок. Вынесем различные его состояния в отдельную статическую переменную, и напишем несколько вспомогательных методов.

class Cell {

    getState(){
        return this.state;
    }

    isFlagged(){
        return this.getState() === Cell.STATE.FLAGGED;
    }
}

Cell.STATE = {
    EMPTY: 1,
    MINED: 2,
    NUMBERED: 3
    FLAGGED: 4
};

Осталось придумать, что делать с Board.mainList. Это название смущает, особенно учитывая слово List в названии, что может сбить с толку и заставить думать, что здесь используется какой-то список, хотя на самом деле это обычный массив. Имя вроде cells или cellsArray будет намного лучше объяснять, что же в этом массиве хранится.

class Board {
    
    constructor(){
        this.cells = [];
    }

    getFlaggedCells(){
        let flagged = [];

        for(let i = 0; i < this.cells; i++){
            if(this.cells[i].isFlagged()){
                flagged.append(this.cells[i]);
            }
        }

        return flagged;
    }
}

Четвертый пример

function copy(a1, a2){
    for(let i = 0; i < a1.length; i++){
        a2[i] = a1[i];
    }
}

На первый взгляд все хорошо - короткая функция, короткие имена. С другой стороны, взглянув на заголовок функции function copy(a1, a2) совсем не понятно, откуда и куда происходит копирование. Особенно это важно, если пользователь нашего кода использует IDE, и при введении имени copy он увидит подсказку copy(a1, a2). Придется идти в наши исходники, смотреть, что же куда копируется, а без дополнительных комментариев это и видно не сразу. Мы тратим время и терпение другого программиста. Однако, простое изменение на src и dest спасет время и нервы, как наше, так и чужое.

function copy(src, dest){
    for(let i = 0; i < src.length; i++){
        dest[i] = src[i];
    }
}

Пятый пример

let a = l;
if(0 == l){
    a = O1;
} else {
    l = O1;
}

Конечно, имена здесь как минимум бессмысленные, но в этом примере я хотел показать, почему не стоит называть переменные так, чтобы их трудно было отличить от чисел. Иногда в качестве имени локальной переменной хочется использовать l или O, однако их бывает сложно отличить от цифр 1 и 0, особенно в некоторых шрифтах. Если уж вы используйте короткие имена для переменных, используйте i, или k, или j. Если у вас уже есть переменные с такими именами, но вам нужна еще одна однобуквенная - подумайте, скорее всего вы что-то делаете не так.

Шестой пример

class HTTP {
    fetchURL(url){ /* ... */ }
    getDataFromURL(url){ /* ... */ }
}

Можете ли вы сказать, в чем разница между этими двумя методами? Скорее всего нет. Их имена говорят одно и тоже, просто другими словами. Но, скорее всего, делают они разные вещи, иначе зачем бы понадобилось два метода?

Опять приходится говорить с автором кода. Мы делаем это уже второй раз, отвлекая его от других дел, и тратя свое время. А ведь от всего этого можно было избавиться, просто делая свой код более понятным.

Из разговора выясняется, что fetchURL на самом деле проверяет, доступна ли страница по нужному адресу. Имя isURLAvailable вполне подойдет, потому что в данном контексте оно может означать лишь это.

class HTTP {
    isURLAvailable(url){ /* ... */ }
    getDataFromURL(url){ /* ... */ }
}

Седьмой пример

class Time {
    getTimeInDDMMYYFormat(){ /* ... */ }
}

Кроме вопроса, почему для преобразования в конкретный формат ddmmyy нужен отдельный метод, а не метод, который принимает формат в качестве аргумента (но и этот вопрос спорный), кажется, что к коду больше претензий нет. Однако, попробуйте произнести его имя вслух. “гет тайм ин д д м м уай уай формат”. Звучит как минимум глупо. А ведь если над проектом работаете не вы один, вам часто приходится говорить о коде, в том числе и о методах. Стоит задуматься о том, чтобы давать переменным и методам имена, которые будут читать более менее легко.

Например, ddmmyy является вполне стандартным форматом, поэтому и назвать его можно соответсвующе.

class Time {

    /** 
     * Возвращает время в формате "DDMMYY"
     */
    getTimeInStandardFormat(){ /* ... */ }
}

Тем не менее, снабдим наш метод комментарием, чтобы не возникало лишних вопросов, что же такое “стандартный формат”.

Восьмой пример

function checkoctal(c){
    return '0' <= c && c <= '7';
}

Этот пример иллюстрирует проблему единообразия именования. Представим такой код.

...
if(isNumber(c) && checkoctal(c)){
    ...
}
...

Что делает isNumber понятно сразу. Однако с checkoctal возникают сомнения. Почему она названа именно так? Почему не isOctal? В этом скрыт какой-то смысл, или это лишь невнимательность разработчика? Увидев реализацию все встает на свои места, однако для такой простой функции необходимость заглядывать в реализацию для того, чтобы ее вообще понять, должна вызывать стыд.

function isOctal(c){
    return '0' <= c && c <= '7';
}

Существуют определенные стандарты наименования подобных функций. Имена тех, что возвращают булевые значения и говорят о каком-то состоянни или принадлежности к какому-то множеству, стоит начинать с is - isOctal, isFlagged, isInRange и так далее. Имена геттеров и сеттеров стоит начинать с get и set - getPosition, setRange и подобное.

Итог

Правильно выбранное имя переменной, метода или класса может помочь вам и другим пользователям вашего кода понять, для чего они предназначаются, что делают, какие типы возвращают, или хотя бы просто не мешать прочитать код. Думайте над именами, и не бойтесь их менять, если вы не “заморозили” свой API. С возможностями рефакторинга IDE изменить имя метода по всему коду можно буквально в два клика.

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

Спасибо!

Комментарии