「キミのコードが汚い理由」という記事に例示された「クリーンな」コードが汚い件

　http://www.atmarkit.co.jp/im/carc/serial/redge51/redge51.html

　テニスのゲームカウントを表示するJavaのコード。きれいなコードの例として出されている。

public class SetScorer {
      private int[] gamesWon = {0, 0};

      public void gameWon(int player) {
            gamesWon[player - 1]++;
      }

      public String getScore() {
            int leader = gamesWon[0] > gamesWon[1] ? 1 : 2;
            int leadersGames = gamesWon[leader - 1];
            int opponentsGames = gamesWon[leader == 1 ? 1 : 0];
            String setScoreMessage = null;
            if ((gamesWon[0] < 6 && gamesWon[1] < 6)
                  || (leadersGames == 6 && opponentsGames == 5)) {
                      setScoreMessage = "Player" + leader + " leads " +
                        leadersGames + " - " + opponentsGames;
            } else if (gamesWon[0] == gamesWon[1]) {
                      setScoreMessage = "Set is tied at " +
                              leadersGames;
            } else if ((leadersGames - opponentsGames >= 2)
                  || (leadersGames == 7)) {
                  setScoreMessage = "Player" + leader +
                        " wins the set " + leadersGames + " - " +
                        opponentsGames;
            }
            return setScoreMessage;
      }
}

　なんつうか、読めない。「詰まりすぎ」「インデントの法則が決まってない」「テニスの知識がないと理由が分からない処理だらけ」といった問題が一目で見て取れると思うのだが、何より生データをごりごり使いすぎだ。俺にはウンコみたいなコードに見える。

　直していこう。まず絶対にありえないのが、

      public void gameWon(int player) {
            gamesWon[player - 1]++;
      }

　引数にintを使っていること。これは数値じゃない。プレイヤーを指定する列挙値だ。直接1とか2とか入れさせちゃいけない。数値が数値以外の意味を持っている場合はマジックナンバーにしない。

　でもJavaには列挙体がないのだよな。ちょっとがんばらないといけないけど、クリーンなコードにするべきだろう。

public class Player
{
  int num;

  private Player(int _num){
    num = _num;
  }

  public static Player Player1 = new Player(0);
  public static Player Player2 = new Player(1);

  public int getPlayerNum(){
    return num + 1;
  }

  //配列のインデックス用。プレイヤー番号-1を返す
  public int toIndex()
  {
    return num;
  }
}
...

public void gameWon(Player player)
{
  gamesWon[player.toIndex()]++;
}

　外部からは使いやすくなったが、そもそもint配列を直接いじるのはおかしい。複雑なアルゴリズムを書くなら、簡単に出来る物は簡単にして、簡単なものの組み合わせでなるたけ構成するようにしなきゃいけない。

　メインの処理もほとんどが実装の詳細で埋まっていて何がしたいのか分からない。読んで意味がわかるコードにしなきゃいけない。

public String getScore()
{
  int leader = getLeader();
  int leaderGames = getLeaderGames();

  //負けてるほうのカウント
  int opponentsGames = getOpponentsGames();

  int p1Games = getPlayer1Games();
  int p2Games = getPlayer2Games();

  if (p1Games < 6 && p2Games < 6)
        || (leadersGames == 6 && opponentsGames == 5))
  {
     return "Player" + leader + " leads " +
        leadersGames + " - " + opponentsGames;
  }

  if (p1Games == p2Games)
  {
    return "Set is tied at " + p1Games;
  }

  if ((leadersGames - opponentsGames >= 2)
      || (leadersGames == 7))
  {
      return "Player" + leader +
          " wins the set " + leadersGames + " - " +
          opponentsGames;
  }
  return null;
}

　配列と?を消して、意味分からん変数名に説明を加えて、詰まりすぎを直した。これでとりあえずメインルーチンが何をやってるかはわかっただろう。コメント書かないならこのくらいまでは意味化しないと。

　しかしテニスを知らない人のためにコメントも書かないとダメだ。

public String getScore()
{
  int leaderNum = getLeaderNum();
  int leaderGames = getLeaderGames();

  //負けてるほうのカウント
  int opponentsGames = getOpponentsGames();

  int p1Games = getPlayer1Games();
  int p2Games = getPlayer2Games();

  //どちらも6点に達していない or 6点対5点ならば
  if ( (p1Games < 6 && p2Games < 6)
        || (leadersGames == 6 && opponentsGames == 5))
  {
     //勝負の真っ最中
     return "Player" + leaderNum + " leads " +
        leadersGames + " - " + opponentsGames;
  }

  //六点以上で同点の場合(6点未満で同点の場合は上のでいいのか？)
  if (p1Games == p2Games)
  {
    return "Set is tied at " + p1Games;
  }

  //六点以上で二点差開いた場合 or 七点に達した場合
  if ((leadersGames - opponentsGames >= 2)
      || (leadersGames == 7))
  {
      //決着
      return "Player" + leaderNum +
          " wins the set " + leadersGames + " - " +
          opponentsGames;
  }
  //コメントもなんもなくnullを返すとかありえない。例外にする。
  throw new InvalidTennisGamesException("テニスのルール上ありえないスコアになりました");
}

　テニスの知識があまりない俺でもなんかおかしいなという感じがする。ここのところが。

  //六点以上で同点の場合(6点未満で同点の場合は上のでいいのか？)
  if (p1Games == p2Games)
  {
    return "Set is tied at " + p1Games;
  }

　七点に達したら終了なんだから、ここは六点対六点しかありえない。

  if(p1Games == 6 && p2Games == 6)
  {
    return "Set is tied at 6";
  }

　こう書いてないということは、多分バグだろう。俺の写し間違いでないことは確認した。俺のテニスの知識が間違っているのかもしれないけど、多分こうだ。

  //どちらも6点に達しておらず、同点ではない or 6点対5点ならば
  if ( (p1Games < 6 && p2Games < 6 && p1Games != p2Games) 
        || (leadersGames == 6 && opponentsGames == 5))
  {
     //勝負の真っ最中
     return "Player" + leaderNum + " leads " +
        leadersGames + " - " + opponentsGames;
  }

　ここも意味が分からない。

  //六点以上で二点差開いた場合 or 七点に達した場合
  if ((leadersGames - opponentsGames >= 2)
      || (leadersGames == 7))
  {
      //決着
      return "Player" + leaderNum +
          " wins the set " + leadersGames + " - " +
          opponentsGames;
  }

　二点差以上開いた場合というのがここに出てくるのは変だろう。意味が通りにくい。本当に七ゲームに達したら終了というルールなんだろうか。

　まあそういう前提でいこう。こっちの条件が先だろう。

  //六点以上で二点差開いた場合 or 七点に達した場合
  if ( (leadersGames >= 6 && leadersGames - opponentsGames >= 2)
      || (leadersGames == 7))
  {
      //決着
      return "Player" + leaderNum +
          " wins the set " + leadersGames + " - " +
          opponentsGames;
  }

  //同点
  if (p1Games == p2Games)
  {
    return "Set is tied at " + p1Games;
  }

  //どちらも6点に達していない or 6点対5点ならば
  if ( (p1Games < 6 && p2Games < 6)
        || (leadersGames == 6 && opponentsGames == 5))
  {
     //勝負の真っ最中
     return "Player" + leaderNum + " leads " +
        leadersGames + " - " + opponentsGames;
  }

  throw new InvalidTennisGamesException("ありえないスコアになりました");

　こっちの方がシンプルに書ける。「六点以上で」という重要な勝利条件がコードに明示できたし、無駄な!=も消せた。バグも取れた。多分もともとこうだったものをひっくり返そうとしてバグを混入したんじゃねえかな。

　引用元のやつの、リスト１という筆者が汚いと書いてあるコード、これなんかは読めば意味がわかる。はるかに、何十倍もリスト2のやつよりは優れている。バグもないし。しかし無効な状態で例外が返らないのが厳しいがリスト2は無言でnullを返すだけだからな。死ねっての。なぜこんな記事を和訳して乗っけたんだろう@ITは。狂ってるな。