getXXX..のような名前の処理の中で値をセットするのはやめよう

はじめに

何番煎じだよ・・って内容ですが、遂に実際に目の当たりにすることができたので記念カキコです。
こういうコードを書く人はqiitaなんか見てない気がしますが、これから生まれる初心者さんたちが少しでもこういうコードを書かずに育てるように書き残しておきます。

どういうコードだったの？

public class testClass {
    private Map<String, String> userInfo;

    public void main() {
        userInfo = new Map<String, String>();

        userInfo.add('userId', '123456');
        String parentId = getParentId();

        // 色々な処理
    }

    private String getParentId() {
        String parentId = '';
        parentId = UserDao.getParentId(userInfo.get('userId'));
        userInfo.put('parentId', parentId);
        return parentId;        
    }
}

ざっくりですが、まあこんな感じでした。

(個人的に)だめなところ

getXXXの中で値をセットしているところが良くないです！！！¹

これにつきます。
コードを書いたあなた以外の人の9割9分は、getXXXという関数を見たときに
「ああ、値を渡せば目当ての値を返してくれるんだな」
と考えます。

ところがどっこい、実は中で勝手に値をセットしている。

まあびっくりする程度で済めばいいですが、そのほかの人がどんなコードを書きたいのかはその時にならないとわかりません。
このparentIdがこっそりセットされている仕様に気づかず書ききって、後でめちゃくちゃロスが発生する可能性もあります。
(使う前に他人が書いたコードは確認しよう)

ちなみに僕は今回3時間ほど無駄にしました。

まとめ

関数名以上の処理を関数の中でやらないようにしよう。
ほんとうに。お願いします。

個人的にこちらの記事は一回読む価値があると思います。
内容もユーモアも全部お気に入りです。

あっと驚かせるJavaプログラミング(をやめよう）

1. プロパティのフィールドとかじゃなくて、たんに何かしらの値を取得するような関数のときもgetterって呼んでいいのかな？ ↩