Hannes Schneidermayer Java- и веб-разработчик

Мои Best Practices — общие принципы

1. Комментарии — проклятие и благословение

Хорошие комментарии — это искусство. Продуманный комментарий — подарок, но нередко он пишется впустую.

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

Ниже — несколько примеров самодокументируемого кода, а также хороших и плохих комментариев.

Частый аргумент: комментарии быстро устаревают. На самом деле это в той же мере касается имён переменных и методов!

Ой — никогда так не делайте:

int age = 32; // set the value of the age integer to 32

Самодокументируемость: осмысленные имена переменных — уже полдела

Плохо:

var dateTime = DateTimeUtil.getNow();

Хорошо:

var now = DateTimeUtil.getNow();

Ещё один пример:

Плохо:

User user = this.getBruce();

Плохо:

// testing user without permission
User user = this.getBruce();

Хорошо:

User userWithoutPermission = this.getBruce();

Самодокументируемость: осмысленные имена методов

Плохо:

public String sanitizeInput(String input) {
    // replace underscore with spaces
    return input.replaceAll("_", " ");
}

Хорошо:

public String replaceUnderscoreWithSpaces(String input) {
    return input.replaceAll("_", " ");
}

Самодокументируемость: приватные методы

Плохо: метод заставляет читателя одновременно разбираться в валидации, вычислениях и бизнес-правиле.

public Invoice createInvoice(Order order, Customer customer) {
    if (order.items().isEmpty()) {
        throw new IllegalArgumentException("Order needs at least one item");
    }

    BigDecimal subtotal = order.items().stream()
            .map(item -> item.unitPrice().multiply(BigDecimal.valueOf(item.quantity())))
            .reduce(BigDecimal.ZERO, BigDecimal::add);
    BigDecimal discount = customer.isPremium()
            ? subtotal.multiply(new BigDecimal("0.10"))
            : BigDecimal.ZERO;

    return new Invoice(order.id(), subtotal, discount, subtotal.subtract(discount));
}

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

public Invoice createInvoice(Order order, Customer customer) {
    requireItems(order);
    BigDecimal subtotal = calculateSubtotal(order.items());
    BigDecimal discount = loyaltyDiscountFor(customer, subtotal);

    return invoiceFor(order, subtotal, discount);
}

private static void requireItems(Order order) {
    if (order.items().isEmpty()) {
        throw new IllegalArgumentException("Order needs at least one item");
    }
}

private static BigDecimal calculateSubtotal(List<OrderItem> items) {
    return items.stream()
            .map(item -> item.unitPrice().multiply(BigDecimal.valueOf(item.quantity())))
            .reduce(BigDecimal.ZERO, BigDecimal::add);
}

private static BigDecimal loyaltyDiscountFor(Customer customer, BigDecimal subtotal) {
    return customer.isPremium()
            ? subtotal.multiply(new BigDecimal("0.10"))
            : BigDecimal.ZERO;
}

private static Invoice invoiceFor(
        Order order,
        BigDecimal subtotal,
        BigDecimal discount
) {
    return new Invoice(order.id(), subtotal, discount, subtotal.subtract(discount));
}

Не каждой строке нужен собственный метод. Выносится цельная мысль, которой можно дать точное имя. А имена вроде processData() или handleStuff() ничего не дают.

Эмпирическое правило: комментируйте, почему что-то происходит, а не что происходит

Здесь понятно, что происходит:

public Duration retryDelay(int retryCount) {
    long seconds = Math.min(1L << retryCount, 60);
    return Duration.ofSeconds(seconds);
}

Но почему это происходит?

Лучше:

public Duration retryDelay(int retryCount) {
    // Cap the delay so an outage never blocks queued jobs for more than one minute.
    long seconds = Math.min(1L << retryCount, 60);
    return Duration.ofSeconds(seconds);
}

Ещё один пример из реальной жизни: методы, которые показывают и скрывают Ajax-спиннер загрузки. Вот только методов четыре, и зачем — с первого взгляда не совсем ясно:

function showAjaxLoader() {
    $('.layout-ajax-loader').children().first().show();
}

function hideAjaxLoader() {
    $('.layout-ajax-loader').children().first().hide();
}

function suppressAjaxLoader() {
    getOnlyElementByClassOrNull('layout-ajax-loader').style.display = 'none';
}

function releaseAjaxLoader() {
    getOnlyElementByClassOrNull('layout-ajax-loader').style.display = '';
}

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

showAjaxLoader: function() {
    $('.layout-ajax-loader').children().first().show();
},

hideAjaxLoader: function() {
    $('.layout-ajax-loader').children().first().hide();
},

// take the control from primefaces and don't allow the spinner to show, no matter what
suppressAjaxLoader: function() {
    getOnlyElementByClassOrNull('layout-ajax-loader').style.display = 'none';
},

// reset the state after hiding, to give the control back to primefaces
releaseAjaxLoader: function() {
    getOnlyElementByClassOrNull('layout-ajax-loader').style.display = '';
},

Ещё один пример:

Плохо:

/**
 * Sets the tool tip text.
 *
 * @param text  the text of the tool tip
 */
public void setToolTipText(String text) {}

Хорошо:

/**
 * Registers the text to display in a tool tip. The text
 * displays when the cursor lingers over the component.
 *
 * @param text  the string to display. If the text is null,
 *              the tool tip is turned off for this component.
 */
public void setToolTipText(String text) {}

Обсуждение на воркшопе 21.06.2021: все разработчики сошлись во мнении, что Javadoc в целом лучше избегать (кошмар с параметрами, постоянно устаревает, неудобен в сопровождении) и применять его, только если параметр действительно требует пояснения или иначе пришлось бы читать весь длинный метод.

Исключение из правила: комментируйте, что происходит, — здесь это уместно

Если это заметно упрощает чтение, потому что иначе код непонятен:

let isAlwaysAllowedKey = (
    e.which === 8  || /* BACKSPACE */
    e.which === 35 || /* END */
    e.which === 36 || /* HOME */
    e.which === 37 || /* LEFT */
    e.which === 38 || /* UP */
    e.which === 39 || /* RIGHT*/
    e.which === 40 || /* DOWN */
    e.which === 46 || /* DEL*/
    e.ctrlKey === true && e.which === 65 || /* CTRL + A */
    e.ctrlKey === true && e.which === 88 || /* CTRL + X */
    e.ctrlKey === true && e.which === 67 || /* CTRL + C */
    e.ctrlKey === true && e.which === 90   /* CTRL + Z */
)

Комментарии — ругательства в коде ядра Linux

Напоследок забавная история из ядра Linux — сколько всего ругательств в его исходном коде:

https://www.vidarholen.net/contents/wordcount/#fuck*,shit*,damn*,idiot*,retard*

Пишите комментарии с удовольствием!

2. Приватные методы в больших процедурах опасны

Внимание! В современном объектно-ориентированном мире приватных методов всегда много. Обычно это не проблема. Проблемой это становится, когда такие методы обращаются к полям, то есть к состоянию, и находятся внутри больших процедур. Такие вспомогательные методы должны быть приватными и не иметь состояния. Иначе, если вызвать их по отдельности, они легко создают проблемы. А разработчикам рано или поздно придёт в голову вызвать такой вспомогательный метод напрямую. Недопустимые состояния — одна из главных причин багов. Интересная статья на эту тему разбирает высказывания John Carmack (id Software, Doom) и рассказывает, что в приложениях, где код должен быть предельно надёжным (например, в космических полётах), подпрограммы часто запрещены — это делает код более детерминированным: http://number-none.com/blow/john_carmack_on_inlined_code.html

Эта история лишь отчасти применима к современному миру Java/Spring, где сервисы не хранят состояния, но всё равно даёт хорошую пищу для размышлений.