「キミのコードが汚い理由」は本質を理解していないから
昔、テニスのスコアに関するプログラムをしたことがある。テニスのスコアは複雑に見えるかもしれないが、実は単純な法則にしたがっている。しかし、そこに単純な法則を見出すことが出来ないと、めちゃくちゃよみにくいプログラムを書いてしまう。その例がこれだ。
http://www.atmarkit.co.jp/im/carc/serial/redge51/redge51.html
意図はわからないが、二つのリストが掲載されている。どっかの誰かもコメントしていたけど、前者の方がまだ読みやすい。（掲載している人は後者の方が読みやすいと主張している）前者の方は読みやすさではなくコードの重複が多いのが問題。ちょっとした修正が困難であろう。
このコードは読みにくい上に、このままではメンテナンス性も悪い。
■ 問題の本質を理解していない
「キミのコードが汚い理由」それは問題の本質を理解していないからだ。
このプログラマは本質を見誤って「テニスをされる方ならお分かりだろうが、このコードを開発しているうちに、われわれは一部のゲーム、セット、最終スコアにかなり複雑な条件があることに気付い」てしまった。この発想を続けている限りこのプログラマの書いたコードはきれいにならない。テニスのポイントの柱となる部分はいたって簡単で次の二つの法則でなりたっている。
- 勝つためには２ポイント（あるいはゲーム）以上はなさなければならない
- 勝つためにはあるポイント以上（あるいはゲーム）とらなければならない
セットを取るためには具体的には
- 2 ゲーム以上はなす
- 6 ゲーム以上とる
必要がある。さらに「複雑」と言っている部分のタイブレークで勝つというのがある。これは
13 ゲーム目を取る。ということである。
つまり 14 ゲーム目はない。
いたって簡単だ。多くのスポーツが採用しているシステムでもある。であるからこれをプログラムすると、次のようになる。
private boolean isEndOfSet() {
if (( games[0] + games[1] ) == 13 ) {
return true;
}
if ( Math.abs(games[0] - games[1] ) < 2 ) {
return false;
}
if ( Math.max(games[0], games[1] ) < 6 ) {
return false;
}
return true;
}isEndOfSet という英語が怪しいのは勘弁してもらって（プロとして本当はだめだけど）、こんな感じで簡単になる。引数に int games0, int games1 とするとより汎用的になる。
■ 適切な分離をしていない
「キミのコードが汚い理由」それは適切な分離をしていないからだ。
このコードのやるべきこと(要求されていること）は二つある
- 試合中のスコア表示
- 試合後の結果表示
この構造に気付き分離すればもっと簡単になる。
public String getSetScore() {
if ( isEndOfSet() ) {
// "決着がついた後の表示"
int leader = (games[0] > games[1] ? 0 : 1);
return getSetScoreFinished(leader);
} else {
// "試合中の表示"
return getSetScorePlaying();
}
}後は getSetScoreFinished と getSetScorePlaying を実装すればよい。ほら簡単で見やすくなった。日本語でコメントをつけたが、むしろコメントが邪魔になるくらいシンプルにかけた。あとは英語を適切な表現に直せばコメントすらいらない。
■ 変数の使い方に間違いがある
「キミのコードが汚い理由」それは変数の使い方に間違いがあるからだ。
参考になっているソースに次のようなミスリードさせるコードがある。
int leader = (games[0] > games[1] ? 1 : 2);
leader はその変数名からリードしている人という意味だろう。しかし、
この時点では games[0] == games[1] ということはありえる。
games[0] > games[1] → leader = 1 games[0] == games[1] → leader = 2 // 間違い！！leader はいない games[0] < games[1] → leader = 2
leader という変数を使うなら意味のある箇所でしか使ってはならない。それはgames[0] <> games[1] の条件のときだけだ。つまり次のようになる。
if ( games[0] == games[1] ) {
// 同スコアのときの処理
....
} else {
// leader が存在するときの処理
int leader = (games[0] > games[1] ? 0 : 1)
}leader = -1 で tie を表そうとしたりしてもだめ。leader = -1 という特殊条件はソースをよく読まないとわからないから。
■ 数字の意味が一貫していない
「キミのコードが汚い理由」それは一貫した意味で数字を使っていないからだ。
参考になっているソースでは一貫した意味で数字が使われていないため、ミスリードさせる。
games[0] ... player1 のゲーム数
games[1] ... plyaer2 のゲーム数
leader ... 1 のとき player1
2 のとき player2このコードでは games に与える添え字が 0 のときは player"1" のゲーム数なのだ。そして、leader という変数に 1 が入っているときに player"1" を示す。従って、leader を使ってその leader の games を表すと
games[leader - 1]
という摩訶不思議なコードを書かなければならない。このコードだけを見て、”なぜ” - 1 をしているのかわかる人はいない。下駄を履かせている理由はコードをよく読まないとわからない。本来であれば、player1, 2 ではなく player0, 1 として、0 という添え字が来たら常に player0 を意味するようなコードを書かなければならない。
player1 と表示したいなら、最後に表示するルーチンだけを変えればよい。例えばこんな風に。
private String getPlayerName(int player) {
return "Player" + (player + 1);
}
■ ソースの読みやすさとは？
このプログラムを書いた人は「読みやすい」の意味を理解していない。
ソースの読みやすさには３つのポイントがある。
- 見た目が見やすい
- 適切な関数名・変数名になっている
- 論理的に筋が通っている
○ 見た目が見やすい
これは本当に見た目。例えば、関数の区切り、処理の区切りで横線を入れる。
...
}
//--------------------------------------------------------
void func(void)
{
....
}これだけでソースは読みやすくなる。C 言語が読みやすいのはこの見た目のおかげだ。これが begin/end を使うような言語だと見にくくなってしまう。これは完全に人間の目の特性の問題だ。
○ 適切な関数名・変数名になっている
適切な関数名・変数名にするとそれを羅列するだけで処理の流れがおえる。このとき、ミスリードするような名称をつかってしまうと読みにくくなってしまう。前述の leader なんかがそれだ。また、「似て非なるもの」の名前をつけてしまうのもだめ。
あまりに抽象的あるいは意味の無い名称もだめ。だめなものの典型に dummy, value などがある。foo, bar なんてのはもってのほかだ。
また、一貫した命名方法も必要だ。notify という変数を用意したら、プログラム全域で同じ意味で使わないとならない。
○ 論理的に筋が通っている
当然、論理的に筋が通っているプログラムが見やすくなる。論理的に筋が通っていて、適切な名称が使われていれば、たいていの場合、”コメントはいらない”。
■ サンプルはこんな感じ
public class TennisScore {
private int games[] = {0, 0};
//-----------------------------------------------------
public void gameWon(int player) {
games[player]++;
}
//-----------------------------------------------------
private boolean isEndOfSet() {
if (( games[0] + games[1] ) == 13 ) {
return true;
}
if ( Math.abs(games[0] - games[1] ) < 2 ) {
return false;
}
if ( Math.max(games[0], games[1] ) < 6 ) {
return false;
}
return true;
}
//-----------------------------------------------------
private String getPlayerName(int player) {
return "Player" + (player + 1);
}
//-----------------------------------------------------
// みにくい、、、
// java.text を使えばもっとスマートになりそう
// "Player{0} leads {1} - {2}";
private String getSetScorePlaying() {
if ( games[0] == games[1] ) {
return "Set is tied at " + games[0];
} else {
int leader = (games[0] > games[1] ? 0 : 1);
String text;
text = getPlayerName(leader) + " leads " + games[leader] + "-" + games[1 - leader];
return text;
}
}
//-----------------------------------------------------
private String getSetScoreFinished(int leader) {
String text;
text = getPlayerName(leader) + " won by " + games[leader] + "-" + games[1 - leader];
return text;
}
//-----------------------------------------------------
public String getSetScore() {
if ( isEndOfSet() ) {
// "決着がついた後の表示"
int leader = (games[0] > games[1] ? 0 : 1);
return getSetScoreFinished(leader);
} else {
// "試合中の表示"
return getSetScorePlaying();
}
}
//-----------------------------------------------------
public static void main(String args[]) {
TennisScore theScore;
theScore = new TennisScore();
theScore.gameWon(0);
System.out.println(theScore.getSetScore());
theScore.gameWon(1);
System.out.println(theScore.getSetScore());
theScore.gameWon(0);
System.out.println(theScore.getSetScore());
theScore.gameWon(1);
System.out.println(theScore.getSetScore());
theScore.gameWon(0);
System.out.println(theScore.getSetScore());
theScore.gameWon(1);
System.out.println(theScore.getSetScore());
theScore.gameWon(0);
System.out.println(theScore.getSetScore());
theScore.gameWon(1);
System.out.println(theScore.getSetScore());
theScore.gameWon(0);
System.out.println(theScore.getSetScore());
theScore.gameWon(1);
System.out.println(theScore.getSetScore());
theScore.gameWon(1);
System.out.println(theScore.getSetScore());
theScore.gameWon(1);
System.out.println(theScore.getSetScore());
}
}
