CleanCode Chapter 3:Functions

Clean Code: A Handbook of Agile Software Craftsmanship (Robert C. Martin Series)

Chapter 1 は実際に買って読んでもらうとして、Chapter 2 の Meaningful Names は当たり前すぎて飛ばすとして、Chapter 3 の Functions の感想 (感想？)。
Java 以外の言語に当てはまる部分も多いけど、とりあえずカテゴリは Java で。

行数

Kent Beck は全ての関数 (Every function) が 2〜4 行なんだそうな。
多分 1 行の関数や 5 行以上の関数も書くことはあるんだろうけど、割合としては無視できるくらい少ないんだろうな。
1 行の関数が含まれてないってことは、単純な getter/setter はほとんど書かない、ってことだと思う。
実装パターンでもそんなこと言ってたし。

抽象度の違う処理をまぜない

読みにくくなるから。これはパターン指向リファクタリング入門でもあったはず。
でも徹底しようとしてもなかなか難しいことなんだよねー。

switch 文、もしくは if-else if の連鎖

switch 文は本質的に N 個のことをするので、Do One Thing に反する。よって代りにポリモーフィズム使え、と。
例外として、ポリモーフィックなオブジェクトの生成を行う関数内部での使用を挙げている。

名前

短くてワケワカラン名前より、長くて説明的な名前を付けろ、と。
ただ、英語をネイティブに使ってる人だったら気にならんのかもしれないけど、英語で書かれた長い名前を見ただけで読む気が失せるとか言うような人もいるわけで、何事もほどほどがいいと思わなくも無い。

キーワード引数がある言語だと、長ったらしい関数名の一部をキーワードに押し付けることができるんだけど、IDE の支援が受けにくくなりそうでこれも微妙。
流れるようなインターフェイスにするのも手だけど、確実に手間は増える上、呼び出し側は分かりやすいかもしれないけど定義は難読化がすすむような気がする。
IDE の支援も受けれて、手間もそれほど気にならず、定義も読みやすい方法となると、引数用のクラスを作る、とかになるんだろうか。
例えば、

// static Start _(int value)みたいなメソッド作った方がいいかも
StringUtils.substring("hogehoge", new Start(4), new Length(2));
StringUtils.substring("hogehoge", new Start(2), new End(4));

・・・手間、気になるな*1。

あと、write(name) より writeField(name) の方がいいらしい。
理由として、"write name" じゃなくて、 "written name" だからってことを言ってるんだけど、日本人には write(name) の方が分かりやすいんじゃないかと思うんだけどどうだろう。
で、writeField(name) はキーワードを名前に含めてる (field == name) 例になってるそうなんだけど、同じ理由から assertEquals は assertExpectedEqualsActual の方がいいんだそうな。・・・ごめん無理。

引数

引数の個数*2としては、理想は 0、次に 1 つと 2 つ*3、3 つはできれば避けたいらしい。それ以上はよほどの理由が無い限り却下。

それと、フラグ引数は醜いと断言している。
まぁこれはその通りだと思う。java.io.FileOutputStream のコンストラクタに第二引数として渡せる boolean だけど、上書きするのが true なのか、新規に作るのが true なのか、今でも迷うことあるし。
この本では、render(boolean isSuite) ってメソッドがあったら、renderForSuite() と renderForSingleTest() に分けるべき、としているけど、コンストラクタには名前が付けられないから、static なファクトリメソッドを用意することになる。
他にも、enum を作って適切な名前をつける方法もある。

// FileOutputStream(File/String, boolean)をパッケージプライベートにして、
// FileOutputStream(File/String, IfExistsThen)をpublicで追加するイメージ
// まぁ、標準APIを弄ることは現実的ではないのだけれど・・・
// OutputStreamかFileOutputStreamを継承した新しいクラス作るのが一番現実的かな
package java.io;
public enum IfExistsThen {
    APPEND {
        FileOutputStream create(File f) throws FileNotFoundException {
            return new FileOutputStream(f, true);
        }
    },
    OVER_WRITE {
        FileOutputStream create(File f) throws FileNotFoundException {
            return new FileOutputStream(f, false);
        }
    };
    // FileOutputStreamのコンストラクタから呼ばれるイメージ
    abstract FileOutputStream create(File f) throws IOException;
}

こんな感じ。フラグにしたんじゃ動作を追加しようとした場合にコンストラクタを増やさなきゃいけないけど、enum だと enum に追加するだけ。例えば、

public enum IfExistsThen {
    APPEND {
        FileOutputStream create(File f) throws FileNotFoundException {
            return new FileOutputStream(f, true);
        }
    },
    OVER_WRITE {
        FileOutputStream create(File f) throws FileNotFoundException {
            return new FileOutputStream(f, false);
        }
    },
    THROW_EXCEPTION {
        FileOutputStream create(File f) throws FileNotFoundException {
            if (f.exists())
                throw new IllegalStateException();
            return new FileOutputStream(f);
        }
    };
    abstract FileOutputStream create(File f) throws IOException;
}

出力引数

引数は関数への入力と解釈するのが一番自然なんだから、出力引数は使うな、と。
出力引数が使いたくなったら、オブジェクトにくくりだして obj.method() の形にしろ、と。

理由の一つに、それが出力か入力か分からない、ってのを挙げていたけど、C# みたいに呼び出し側でも定義側でもそれが出力引数と分かる場合は必要があれば使えばいいと思う。

ちなみに本では StringBuilder を appendFooter メソッドに渡す例を挙げてるんだけど、こういうやり方、よくやるなぁと反省。

オブジェクトの状態を変更する関数と、オブジェクトに関する情報を取得する関数は分ける

まぁ、基本分けるべきなんだろうけど、マルチスレッドを考えると、微妙。
本で例に挙げられていたのは set という名前のメソッドで check then act を行っていたので、どちらかというと名前が悪いような気がする。
詳しくはJava 並行処理プログラミング参照・・・って、この本もう絶版らしい。他に絶版にすべき本はいくらでもあるだろうに・・・

try-catch ブロックの抽出

関数は一つのことだけをやるべきで、例外処理はまさに「一つのこと」なので、関数がエラーハンドリングしてたらそれ以外のことはするな、というちょっと過激に見える意見。
つまり、

void hoge() {
    // tryを使うならtryの前には何も書かない
    try {
        // 例外が発生するコード。抽出するので呼び出すだけ
        hogehoge();
    } catch (SomeException e) {
        // 例外処理
        ...
    }
    // catch(もしくはfinally)の後ろにも何も書かない
}

こんな感じにしろということ。
でも、実際に試してみたらコードがとても読みやすくなったから、これはぜひともやるべき。

ちなみに実際に本で紹介されてる例はこんな感じ。

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

private void deletePageAndAlReferences(Page page) throws Exception {
  deletePage(page);
  registry.deleteReferences(page.name);
  configKeys.deleteKey(page.name.makeKey());
}

CleanCode (P.47)

依存性マグネット

Error.java とか、ErrorCode.java とか、そんなものを作るな、という話。
こんなものを作ってしまうと、そういったファイルは他のたくさんのクラスから import され使われるから、そのファイルに修正を加えた際にほとんど全部のクラスの再コンパイル、再配置が必要になってしまう (これを著者は dependency magnet と呼んでいる)。
で、このことから Error.java などといったファイルへの修正や追加を避けるようになって、何もかもが gdgd になる、と。

個人的には、Error.java だけに限らず、Message.java だとか、Constant.java だとか、とにかく何もかもを突っ込んだ定数クラスは作るな、といいたい。
でもなんか、「クラス化/共通化/共通部品化」の名の下に、真っ先に作られるクラスなんだよな。
そしてコード*4を Excel で管理、っと。○ねばいいのに。

実際には他にも色々と書いてあるから、気になった人は買うといいよ。

Clean Code: A Handbook of Agile Software Craftsmanship (Robert C. Martin Series)

*1:特に Start とか End とか Length とかの定義、ほとんど一緒になるし。D のみたいな強い typedef が欲しい。あれがあれば Start も End も Length も int を typedef しとけばいい

*2:可変長引数は 1 つと数える。例えば、String.format(String, Object...) なら引数の数は 2 つ

*3:1 つの方が 2 つよりも望ましい

*4:ソースコードじゃなくて、エラーコードとかメッセージコード