見ろ！！コードがゴミのようだ！！

まずは以下のjavaのコードをみてもらおう。

class SampleClass {
  public int i;
  public String s;
}

public class Test {
  static public void main(String[] args) {
    Test test = new Test();


    SampleClass a = new SampleClass();
    a.i = 1;
    a.s = "a";
    
    SampleClass b = new SampleClass();
    b.i = 2;
    b.s = "b";

    System.out.println("== before ===");
    System.out.println("a= " + a.i + ":" + a.s);
    System.out.println("b= " + b.i + ":" + b.s);

    SampleClass c = test.antiPattern(a,b);

    System.out.println("== after ===");
    System.out.println("a= " + a.i + ":" + a.s);
    System.out.println("b= " + b.i + ":" + b.s);
    System.out.println("c= " + c.i + ":" + c.s);
  }

  public SampleClass antiPattern(SampleClass a, SampleClass b) {
    a.i = 11;
    a.s = "A";

    SampleClass b2 = new SampleClass();
    b2.i = 12;
    b2.s = "B";
    b = b2;

    SampleClass c = new SampleClass();
    c = b2;
    return c;
  }
}

mainメソッドでantiPatternメソッドを呼ぶ前後で変数値を出力している訳だが何が出力されるか分かるだろうか。

結果はこうである。

== before ===
a= 1:a
b= 2:b
== after ===
a= 11:A
b= 2:b
c= 12:B

変数cは良いとしても、aやbまでもmainメソッドだけを見ていては決して分かりようがない結果が出力されるのである。さらに言うとantiPatternメソッドを見てみると直感的にはbも更新されてよさそうな気がするのだが実際はそうはならない。この辺はjavaのメソッドはポインタの値渡しであるという言語仕様によるものである。

しかしこれは言語仕様が許しているからと言って書いてはいけないコードの代表格と言えるだろう。mainメソッドを読んでいるのにantiPatternメソッドがどうなっているかまで意識しなくてはいけない。

上の例ではantiPatternメソッドが実行ステップにして10行足らずで完結しているのでまだよいが、antiPatternメソッドも同じように別のメソッドAを呼びだしてAの中でも渡した変数の中身が変わって返ってくるとなったらどうだろうか。さらにAの中でもBを呼び出し、Bの中でもCを呼び出し・・・。
結局全部のコードを理解しないと高々数十行のmainメソッドすら読み解くことができない。
これでは保守性が悪くてどうしようもない。
これをゴミコードと呼ばずにいられるだろうか。いや、いられまい（反語）。

さて、突然こんなことを書いたのは仕事でこんなコードを見たからである。
実際はもっと処理があるが、ポイントだけ抜粋した。

public class Y {
  public void methodA() {
    ・・・
    XListGenerator generator = new XListGenerator(a,b,c);
    List<X> xList = new ArrayList<X>();
    ・・・
    List<Something> somethingList = new ArrayList<Something>();
　　・・・
    generator.set(xList, somethingList);
  }
}

クラス名が悪い、変数名が悪いとか言いたいわけではない。
methodAが何をやっているか分かるだろうか。
「generatorに２つのlistを設定している。」と読みとるのが自然ではないだろうか。

しかしgenerator.set(list,somethingList)の中を見て愕然とした。以下のような内容なのであった。

   public set(List<X> xList, List<Something> somethingList) {
       for(i=0; i<somethingList.size(); i++) {
          String s= somethingList.get(i).getString();
          xList.get(i).setS(s);
       }
   }

なんなんでしょうかこれ。

完全に依存関係があるんだしメソッドを分けたって百害あって一利なしだ。setの主体もgeneratorなのかなんなのかよく分からない。
これでいいじゃない。

public class Y {
  public void methodA() {
    ・・・
    List<X> xList = new ArrayList<X>();
    ・・・
    List<Something> somethingList = new ArrayList<Something>();
　　・・・
    for(i=0; i<somethingList.size(); i++) {
       String s= somethingList.get(i).getString();
       xList.get(i).setS(s);
    }
  }
}

どうしてもgeneratorを使いたいなら、なるべく原型を残したとしてこうだ。

public class Y {
  public void methodA() {
    ・・・
　　XListGenerator generator = new XListGenerator(a,b,c);
    ・・・
    List<Something> somethingList = new ArrayList<Something>();
　　・・・
    List<X> xList = generator.generate(somethingList);
  }
}

プログラムは動けばよい！というのは自分一人で作っている場合に限るのである。
こんなコードを読まなければいけない他人の身にもなってコードは書くべきである。