Hannes Schneidermayer Java・Web開発者

私のベストプラクティス – 全般

1. コメント――呪いであり、恵みでもある

良いコメントを書くのはひとつの芸です。よく考えて書けば読み手への贈り物になりますが、骨折り損に終わることも少なくありません。

自己文書化コード（self-documenting code）とは、「コメントを書かない」という意味ではありません。どのように動くかはできるだけコード自身に語らせ、コメントではなぜそうするのか、自明ではない前提条件、そして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("_", " ");
}

自己文書化：privateメソッド

悪い例：読み手は、バリデーション、計算、業務ルールを一度に読み解かなければなりません。

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メソッドが処理の流れを語ります。ステートレスなprivateメソッドがそれぞれの意図に名前を付け、依存関係を引数と戻り値で見えるようにしています。

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のローディングスピナーを表示・非表示にするメソッドですが、なぜかメソッドが4つあります。その理由は、ぱっと見ただけではよくわかりません：

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) {}

2021-06-21のワークショップでの議論：開発者全員の意見が一致したのは、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. 大きな処理ルーチンの中のprivateメソッドは危険

要注意！現代のオブジェクト指向の世界では、privateメソッドはいくらでもあります。普通はそれで問題ありません。問題になるのは、そうしたメソッドがフィールド、つまり状態（ステート）にアクセスし、しかも大きなルーチンの中にある場合です。こうしたサブメソッドはprivateかつステートレスであるべきです。そうでないと、単独で呼び出されたときにトラブルの元になりがちです。開発者はいずれ、そのサブメソッドを呼び出そうと考えるものです。不正な状態はバグの主な原因のひとつです。この点については、John Carmack（id Software、Doom）の発言を紹介した興味深い記事があります。コードに非常に高い安定性が求められる分野（宇宙飛行など）ではサブルーチンが禁止されていることも多く、それによってコードがより決定論的になる、という話です：http://number-none.com/blow/john_carmack_on_inlined_code.html

この話は、ステートレスが主流の現代のJava/Springの世界にはそのまま当てはまるわけではありませんが、考えるきっかけとしては十分です。