なぜダメなコードなのか

Java

仕事中どうしても気になって直してしまうコードがある。普通コードを修正すると、テストをする必要があるし、cvsにコミットしたり、付帯的な作業がたくさん発生する。それでもなお修正したくなるコードがあるわけで。
何でかと考えてみると、単にインデントがずれて読みにくいだとかではなく(その程度ならばウズウズしてもあきらめられる)、コードを読むのではなく読解する必要があるからだと思う。
その観点から考えて、今日のダメコード。

    /**
     * 使用するテーブルをクリアする。
     */
    private void clearUseTable() {
        setUseTable(null); //null を与えるとクリアできる。
    }
    /**
     * 使用するテーブルをセットする。
     * @param tablename 使用するテーブル
     */
    private void setUseTable(String tablename) {
        if(tablename == null) {
            _useTable.clear();
        } else {
            if(!_useTable.containsKey(tablename)) {
                _useTable.put(tablename, hoge(alias));
            }
        }
    }

みたいな。もっとひどいのになると、clearUseTable()がないとか。いやそっちのほうがマシなのかもしれない。
なぜダメかといえば、テーブルクリアみたいな重要な動作のタイミングがソースを流し読みしただけでは発見できないから。
「そんなことないじゃん。clearUseTable()を呼んでいるところでしょ」とも言われそうだが、クリアする方法はもう一つ。setUseTable(null)を呼ぶことだ。
まぁ普通に

setUseTable(tab + "0")

とかならばすぐ判るけど、

setUseTable(getHogeHage(col))

とかやられると面倒でしょ。しかもgetHogeHage()が、

    private String getHogeHage(String alias) {
        return getHogeHageReal(alias)[0];
    }
    private String getHogeHageReal(String alias) {
        return (String)_fugaMap.get(alias);
    }

だったりする訳で、コーヒーの一杯も欲しくなる状態。( (String[])_fugaMap.get(alias) )[0]がnullだったらどうするのという話。つまり、クリアならばクリアだけ、追加なら追加だけの機能にしておくべきという事。そうすればその特殊条件を探すのならば、メソッドを呼んでいるところを追えば済むという話になる。だからこういう風に書き直したい訳だ。

    /**
     * 使用するテーブルをクリアする。
     */
    private void clearUseTable() {
        _useTable.clear();
    }
    /**
     * 使用するテーブルをセットする。
     * @param tablename 使用するテーブル
     */
    private void setUseTable(String tablename) throws HogeException {
        if(tablename == null) {
            throw new HogeException("nullじゃん");
        }
        String val = hoge(alias);
        if(_useTable.containsKey(tablename)) {
            if (val.equals(_useTable.get(tablename))) {
//              throw new HogeException("最適化の余地はあるね");
                return;
            } else {
                throw new HogeException("違う値突っ込もうとしてるけど");
            }
        }
        _useTable.put(tablename, val);
    }

Exceptionはデバッグ用だけど、元のコードで既に格納済みだったらスキップしている理由もわからないし、それの検証用と考えればいいかな。(普通はExceptionを投げるべき部分だと思うし)
もしどっかで変数にnullが入っている事を利用してクリアしてたとすれば(本気じゃない事を祈りたいが)、それは面倒でもこう書くべきだ。

    String table = getHogeHage(col);
    if (table == null) {
        clearUseTable();
    } else {
        setUseTable(table);
    }

でもこの手の話はJavaに限らない話だね。普通の言語*1ならばどんな言語でも何でも当てはまりそうな気がする。

*1:APLとかだとどうなんだろう