👨‍🎓

クリーンコード【関数編】

2024/07/13に公開

Java

はじめに

本記事では、Robert C. Martinの名著『Clean Code』の第３章「関数」に関して、自分用にまとめました。コード例は差し替えたり追加したりしています。本書の第３章にはさらに詳細な関数に関するベストプラクティスが含まれていますので、興味がある方はぜひお読みください。

イントロダクション

関数はあらゆるプログラムの最も基本的な構成要素であり、その設計やリファクタリングの方法を理解することは、クリーンコードを書くための重要なステップです。この記事では、具体的な例を用いて、関数の設計やリファクタリングの方法について詳しく解説します。

例

以下の関数を見てください。

public void processOrder(Order order) {
    if (order == null) {
        throw new IllegalArgumentException("Order cannot be null");
    }
    for (OrderItem item : order.getItems()) {
        if (inventory.get(item.getProductId()) < item.getQuantity()) {
            throw new IllegalArgumentException("Not enough stock for product: " + item.getProductId());
        }
    }

    for (OrderItem item : order.getItems()) {
        int currentStock = inventory.get(item.getProductId());
        inventory.put(item.getProductId(), currentStock - item.getQuantity());
    }

    PaymentResult result = paymentProcessor.process(order.getPaymentDetails());
    if (!result.isSuccess()) {
        throw new PaymentFailedException("Payment failed for order: " + order.getId());
    }

    Invoice invoice = invoiceGenerator.generate(order);
    emailService.sendOrderConfirmation(order, invoice);
}

上記の関数では、一つの関数で多くのことが行われており、一目で理解しにくいです。こちらを簡単にリファクタリングしてみましょう。

public void processOrder(Order order) {
    validateOrder(order);
    checkInventory(order);
    updateInventory(order);
    processPayment(order);
    Invoice invoice = generateInvoice(order);
    sendConfirmationEmail(order, invoice);
}

このようにリファクタリングすることで、注文処理の流れが明確になります。それでは、実際に読みやすい関数を書くための具体的なポイントを見ていきます。

小さくする

最も重要な規則は、関数を小さくすることです。関数の長さは20行を超えないようにするべきです。関数を短くするために、以下のポイントを考慮します。

より詳細な処理は他の関数に細かく抽出していく。そうすることで関数は自然と一つのことを行うようになり、それぞれの関数の処理の流れが明確になる。
if文、else文、while文のブロック内は関数の呼び出し一行にする。呼び出す関数に上手く処理を表す名前を付けることで、コードがドキュメントのように読みやすくなる。

１つの関数に１つの抽象レベル

「関数では、１つのことを行う」の「１つのこと」とは、「１つの抽象レベル」を表します。次の例を見てください。

public class HTMLBuilder {
    public String buildUserProfilePage(User user, boolean isAdmin) {
        StringBuilder htmlBuilder = new StringBuilder();

        htmlBuilder.append("<html>");
        htmlBuilder.append("<head>");
        htmlBuilder.append("<title>").append(user.getName()).append("'s Profile</title>");
        htmlBuilder.append("</head>");

        htmlBuilder.append("<body>");
        htmlBuilder.append("<h1>").append(user.getName()).append("</h1>");
        htmlBuilder.append("<p>Email: ").append(user.getEmail()).append("</p>");
        htmlBuilder.append("<p>Age: ").append(user.getAge()).append("</p>");

        if (user.getActivityLog() != null && !user.getActivityLog().isEmpty()) {
            htmlBuilder.append("<h2>Activity Log</h2>");
            htmlBuilder.append("<ul>");
            for (String activity : user.getActivityLog()) {
                htmlBuilder.append("<li>").append(activity).append("</li>");
            }
            htmlBuilder.append("</ul>");
        }

        if (isAdmin) {
            htmlBuilder.append("<h2>Admin Actions</h2>");
            htmlBuilder.append("<ul>");
            htmlBuilder.append("<li><a href=\"/editProfile?userId=").append(user.getId()).append("\">Edit Profile</a></li>");
            htmlBuilder.append("<li><a href=\"/deleteUser?userId=").append(user.getId()).append("\">Delete User</a></li>");
            htmlBuilder.append("</ul>");
        }

        htmlBuilder.append("</body>");
        htmlBuilder.append("</html>");

        return htmlBuilder.toString();
    }
}

この関数は、低い抽象度（htmlBuilder.append）と高い抽象度（user.getBio）の処理が混在しています。以下に改善案を示します。

public class HTMLBuilder {
    private StringBuilder htmlBuilder;
    private User user;
    private boolean isAdmin;

    public HTMLBuilder(User user, boolean isAdmin) {
        this.user = user;
        this.isAdmin = isAdmin;
    }

    public String buildUserProfilePage() {
        initializeHtmlBuilder();
        appendUserProfilePageContent();
        return buildHtml();
    }

    private void initializeHtmlBuilder() {
        htmlBuilder = new StringBuilder();
    }

    private void appendUserProfilePageContent() {
        appendHtmlHeader();
        appendUserProfile();
        appendUserBio();
        if (isAdmin) {
            appendAdminActions();
        }
        appendHtmlFooter();
    }

    private void appendHtmlHeader() {
        htmlBuilder.append("<html>");
        htmlBuilder.append("<head>");
        htmlBuilder.append("<title>").append(user.getName()).append("'s Profile</title>");
        htmlBuilder.append("</head>");
    }

    private void appendUserProfile() {
        htmlBuilder.append("<body>");
        htmlBuilder.append("<h1>").append(user.getName()).append("</h1>");
        htmlBuilder.append("<p>Email: ").append(user.getEmail()).append("</p>");
        htmlBuilder.append("<p>Age: ").append(user.getAge()).append("</p>");
    }

    private void appendUserBio() {
        if (user.getBio() != null && !user.getBio().isEmpty()) {
            htmlBuilder.append("<h2>Bio</h2>");
            htmlBuilder.append("<p>").append(user.getBio()).append("</p>");
        }
    }

    private void appendAdminActions() {
        htmlBuilder.append("<h2>Admin Actions</h2>");
        htmlBuilder.append("<ul>");
        htmlBuilder.append("<li><a href=\"/editProfile?userId=").append(user.getId()).append("\">Edit Profile</a></li>");
        htmlBuilder.append("<li><a href=\"/deleteUser?userId=").append(user.getId()).append("\">Delete User</a></li>");
        htmlBuilder.append("</ul>");
    }

    private void appendHtmlFooter() {
        htmlBuilder.append("</body>");
        htmlBuilder.append("</html>");
    }

    private String buildHtml() {
        return htmlBuilder.toString();
    }
}

こうすることで、関数が１つの抽象レベルに留まり、それぞれの処理が明確になります。そして、抽象度の高い順で関数を並べ、それぞれの関数に適切な名前を付けることで、コードを物語のように上から下へ読めるようになります。

switch文のデメリット

switch文を使うと、メソッドがどうしても長くなりがちです。以下の例を見てください。

public void processTask(Task task) throws InvalidTaskTypeException {
    switch (task.getType()) {
        case EMAIL:
            processEmailTask(task);
            break;
        case SMS:
            processSmsTask(task);
            break;
        case PUSH_NOTIFICATION:
            processPushNotificationTask(task);
            break;
        default:
            throw new InvalidTaskTypeException(task.getType());
    }
}

この関数にはいくつかの問題があります。

関数が大きくなりがち。
単一責務の原則（SRP）に反する。
開放／閉鎖原則（OCP）に反する。

更に問題なのは、他の関数（例えば、タスクをアサインする関数 assignなど）にも同じswitch文の構造が際限なく作成されてしまうことです。これらの関数は全て同様の有害な構造を持つことになります。
改善案の一つは、switch文を抽象レベルの最下層である抽象ファクトリクラスに置き、processはTaskインターフェースを通してディスパッチすることです。

public interface Task {
    void process();
    void assign();
}
public interface TaskFactory {
    public Task createTask(TaskRecord r) throws InvalidTaskTypeException;
}
public class TaskFactoryImpl implements TaskFactory {
    @Override
    public Task createTask(TaskRecord r) throws InvalidTaskTypeException {
        switch (r.type) {
            case "EMAIL":
                return new EmailTask(r);
            case "SMS":
                return new SmsTask(r);
            case "PUSH_NOTIFICATION":
                return new PushNotificationTask(r);
            default:
                throw new InvalidTaskTypeException(r);
        }
    }
}

内容をよく表す名前を使う

関数が何をするのかよく表す名前をつける必要があります。内容をよく表す長い名前は、不可解な短い名前や、内容を説明する長いコメントよりも優れています。

関数の引数は少なくする

引数は少なくするべきです。理想は0です。多くの引数があると、コードの意図を理解するのが難しくなります。

引数１つのパターン

引数が1つの関数で一般的なのは、照会や変換を行う関数です。

public boolean userExists(String userId) {
    return userDatabase.contains(userId);
}

public String toUpperCase(String text) {
    return text.toUpperCase();
}

もう一つのケースは、イベントです。関数呼び出しをイベントの発生とみなし、引数はシステムの状態を変更する目的で使用されます。以下は、ユーザーがATMのPINを誤って入力した場合に、誤入力の回数を追跡し、一定回数を超えた場合にカードをロックする例です。

public class ATMService {
    private int failedPinAttempts = 0;
    private static final int MAX_PIN_ATTEMPTS = 3;

    public void pinAttemptFailed(int attempts) {
        failedPinAttempts += attempts;
        if (failedPinAttempts >= MAX_PIN_ATTEMPTS) {
            lockATMCard();
        }
    }

    private void lockATMCard() {
        System.out.println("ATM card is locked due to too many failed PIN attempts.");
    }
}

これらのどのパターンなのか、それぞれ読み手にはっきり伝わるような名前や文脈を選ぶ必要があります。

不適切な1引数の関数

以下の様に、値の変換に戻り値ではなく引数を用いるのは混乱を招くので避けるべきです。

public void convertToUpperCase(StringBuffer inputText) {
    String originalText = inputText.toString();
    inputText.setLength(0);
    inputText.append(originalText.toUpperCase());
}

引数が２つのパターン

２つの引数は必ずしも悪ではないですが、以下のような方法で引数を減らすことを検討することができます。 writeField(outputStream, name)というメソッドを考えてみましょう。

writeFieldを outputStreamのメンバーにすると、 outputStream.writeField(name) と書ける。
outputStream をこのクラスのメンバー変数にすると、引数で渡さなくてよくなる。
FileWriter のような新しいクラスを抽出し、コンストラクタで outputStream を受け取り、 write メソッドを提供する。

public class FieldWriter {

    private OutputStream outputStream;

    public FieldWriter(OutputStream outputStream) {
        this.outputStream = outputStream;
    }

    public void write(String field) throws IOException {
        outputStream.write(field.getBytes());
    }
}

もちろん、 Point p = new Point(0, 0); のように２つで１つのコンポーネントのような自然な引数の場合は問題ないです。

引数が３つのパターン

引数が３つは２つに比べても格段に理解が難しくなるので、よほど慎重になるべきです。 assertEquals(message, expected, actual) のような簡単な関数でさえ、間違えてmessage に expected を渡してしまいます。

以下のように引数オブジェクトにラップすることも検討すべきです。

public void startServer(ServerConfig config) {
    System.out.println("Starting server at " + config.host + ":" + config.port());
    System.out.println("Max Threads: " + config.maxThreads);
    System.out.println("Use HTTPS: " + config.useHttps);
}

フラグ引数は使わない

フラグ引数を使うと、関数が複数のことを行うことになりますし、呼び出す側も分かりにくくなります。以下の例を見てください。

public String convertCase(String inputText, boolean toUpperCase) {
    if (toUpperCase) {
        return inputText.toUpperCase();
    } else {
        return inputText.toLowerCase();
    }
}

それぞれの場合の二つの関数に分けるべきです。

public String toUpperCase(String inputText) {
    return inputText.toUpperCase();
}

public String toLowerCase(String inputText) {
    return inputText.toLowerCase();
}

副作用を避ける

関数が１つのことを行うことを主張しつつ、隠れて他のことを行うことは避けるべきです。副作用は、渡された引数、あるいはシステムのグローバル状態に対して行われる変更です。
以下の例を見てください。

public class AuthService {
    public boolean checkPassword(String username, String password) {
        User user = userRepository.findUserByUsername(username);
        if (user != null && user.getPassword().equals(password)) {
            sessionManager.initializeSession(user);
            return true;
        }
        return false;
    }
}

checkPasswordという名前はパスワードの検査を行うことを示していますが、実際にはセッションの初期化も行っています。関数の呼び出し側は、ユーザーのパスワードを確認するだけのつもりで誤ってセッションを初期化してしまう恐れがあります。また、このメソッドはセッションを初期化しても良いタイミングでしか呼び出せない関数になっており、時間軸上の依存関係を生み出しています。
もし時間軸上の関連が必要であれば、名前をcheckPasswordAndInitializeSessionに変更するのが良いかもしれません。しかし、これは「1つのことを行う」というルールを破ってしまいます。

出力引数を避ける

下記のような出力引数も副作用なので避けるべきです。

public void appendFooter(StringBuffer report) {
    report.append("\n--- End of Report ---");
}

この関数を呼び出すコードは appendFooter(s); となりますが、これだけ見ると s をフッターとして追加するのか、 S にフッターを追加するのかが分かりません。オブジェクト指向では、出力引数の代わりに this を用いることで以下のように呼び出すのが妥当です。

report.appendFooter();

関数が状態を変更しなければならない場合は、自分自身の状態を変更すべきです。

コマンド・クエリ分離法則

オブジェクトのメソッドは、以下のどちらかのみを行うべきです

コマンド（Command）: 状態を変更する操作（副作用がある）
クエリ（Query）: 値を返す操作（副作用がない）

下記の関数を見てください。

public boolean set(String attribute, String value) {
    if (attribute == null ||value == null) {
        return false;
    }
    if (attributes.containsKey(attribute)) {
        attributes.put(attribute, value);
        return true;
    }
    return false;
}

この関数は、状態を変更しながら戻り値を返しているため、利用する側で以下のような記述につながります。

if(set("username", "taro")){
  ....
}

この if文は、「ユーザーネーム属性に名前を設定し、それが成功したら」なのか、「もしもユーザーネーム属性に指定の値が設定されていれば」なのかが不明確です。以下のように分離すべきです。

public void setAttribute(String attribute, String value) {
    if (attribute != null && value != null) {
        attributes.put(attribute, value);
    }
}

public boolean hasAttribute(String attribute) {
    return attributes.containsKey(attribute);
}

すると、呼び出し側も意図が明確になります。

if(hasAttribute("username")){
    setAttribute("username", "taro");
    ...
}

リターンコードよりも例外を

関数がリターンコードを返すと、呼び出し側のコードは複雑になります。

public void delete(Page page) {
    if (deletePage(page) == E_OK) {
        if (deleteReference(page.name) == E_OK) {
            if (deleteKey(page.name.makeKey()) == E_OK) {
                logger.log("page deleted");
            } else {
                logger.log("config key not deleted");
            }
        } else {
            logger.log("delete reference from registry failed");
        }
    } else {
        logger.log("delete failed");
        return E_ERROR;
    }
}

代わりに例外を返せば、エラー処理のコードと本来の処理コードが分離され綺麗になります。

public void delete(Page page) {
    try {
        deletePage(page);
        deleteReference(page.name);
        deleteKey(page.name.makeKey());
        logger.log("page deleted");
    } catch (Exception e) {
        logger.log(e.getMessage());
    }
}

try/catchブロックの分離

try/catchは通常の処理とエラーの処理が混ざっているので、それぞれブロックの中身を関数として外に出すべきです。

public void delete(Page page) {
    try {
        deletePageAndAllReferences(page);
    } catch (Exception e) {
        logError(e);
    }
}

private void deletePageAndAllReferences(Page page) throws Exception {
    deletePage(page);
    deleteReference(page.name);
    deleteKey(page.name.makeKey());
    logger.log("page deleted");
}

private void logError(Exception e) {
    logger.log(e.getMessage());
}

さらに、例外クラスを利用すれば、新たな例外クラスはExceptionの派生クラスとなるため、開放・閉鎖原則を満たします。

DRY原則

DRY（Don't Repeat Yourself）原則は、コードの重複を避けることを目的とした原則です。

public class UserService {
    public void createUser(User user) {
        if (user == null) {
            throw new IllegalArgumentException("User cannot be null");
        }
        if (user.getName() == null || user.getName().isEmpty()) {
            throw new IllegalArgumentException("User name cannot be empty");
        }
        // save user
        // ...
    }

    public void updateUser(User user) {
        if (user == null) {
            throw new IllegalArgumentException("User cannot be null");
        }
        if (user.getName() == null || user.getName().isEmpty()) {
            throw new IllegalArgumentException("User name cannot be empty");
        }
        // update user
        // ...
    }
}

上記の例では、ユーザーのバリデーション処理が修復しています。重複の分だけコードの量も多くなる上に、バリデーションロジックに変更が加わるたびに、バリデーション部分を全て探し出して修正しなければなりません。
共通の処理をメソッドとして抽出しましょう。

public class UserService {
    public void createUser(User user) {
        validateUser(user);
        // save user
        // ...
    }

    public void updateUser(User user) {
        validateUser(user);
        // update user
        // ...
    }

    private void validateUser(User user) {
        if (user == null) {
            throw new IllegalArgumentException("User cannot be null");
        }
        if (user.getName() == null || user.getName().isEmpty()) {
            throw new IllegalArgumentException("User name cannot be empty");
        }
    }
}

結論

初めから完璧なコードを書くことはできません。関数を抽出・分離し、名前を変更し、重複を排除し、並び替え、のようなリファクタを繰り返し行うことで、システムのストーリーを理解しやすくします。
以下のポイントを常に意識して、コーディングを行いましょう。

関数を小さくする
詳細な処理は他の関数に細かく抽出します。
1つの関数は1つの抽象レベルにする
関数が1つのことを行うようにし、異なる抽象レベルの処理を混在させないようにします。
switch文はできるだけ回避する
switch文を使うと関数が大きくなりがちなので、抽象ファクトリクラスに追いやり多態を利用するなどして、なるべく回避します。
内容をよく表す名前を使う
関数の名前はその内容をよく表すものにし、不可解な短い名前や説明的なコメントを避けます。
関数の引数はできるだけ少なくする
理想は0で、引数が多いとコードの意図が理解しにくくなるため、可能な限り少なくします。
フラグ引数は使わない
フラグ引数を使うと関数が複数のことを行うことになり、分かりにくくなるので避けます。
副作用を避ける
関数が1つのことを行うことを保証しながら隠れた副作用を持たないようにします。
出力引数を避ける
出力引数は副作用を引き起こすので避けます。
コマンド・クエリ分離法則を守る
関数は状態を変更する操作（コマンド）か、値を返す操作（クエリ）のどちらか一方のみを行うようにします。
リターンコードよりも例外を使う
エラー処理のコードと本来の処理コードを分離するために、リターンコードではなく例外を利用します。
try/catchブロックを分離する
通常の処理とエラーの処理が混ざらないように、try/catchブロックの中身を関数として分離します。
DRY原則を守る
コードの重複を避け、修正の量を減らすようにします。