現在とある会社に拾っていただきエンジニアとして研修を受けています。その研修の中で作成したコインゲームの出来がとても悪く講師の方に意見をいただいて修正したものを自分への戒めとして残しておきます。
書いたコード
まず書いたコードがこれです。
public Integer execute() {
if(this.possessionCoin == 0) {
return 0;
} else {
System.out.println("You have" + this.possessionCoin + "Coin, Start the game? y / n");
while(true) {
try {
int getCoin = 0;
BufferedReader br = new BufferedReader(new InputStreamReader(System.in));
String inputChoice = br.readLine();
System.out.println(inputChoice);
if(inputChoice.equals("y")) {
System.out.println("Please bet Coin 1 ~ " + this.maxBetCoin);
while(true) {
try {
BufferedReader br2 = new BufferedReader(new InputStreamReader(System.in));
String inputStr = br2.readLine();
Integer inputBetCoin = Integer.parseInt(inputStr);
if(0 < inputBetCoin && inputBetCoin <= this.possessionCoin && inputBetCoin <= this.maxBetCoin) {
this.possessionCoin -= inputBetCoin;
int sumCardScore = this.getCard();
if(this.judgeCard(sumCardScore)) {
getCoin = inputBetCoin * 2;
System.out.println("You Win! Get" + getCoin + "Coin!");
DoubleUpChanceGame doubleUpChanceGame = new DoubleUpChanceGame(getCoin, this.deckSetCount);
int winCoinCount = doubleUpChanceGame.execute();
if(winCoinCount == 0) {
return 0;
} else {
System.out.println("You got " + winCoinCount + "Coin !!");
this.execute();
}
} else {
this.possessionCoin = 0;
break;
}
} else {
break;
}
} catch(IOException | NumberFormatException e) {
}
}
} else if(inputChoice.equals("n")) {
return this.possessionCoin;
} else {
System.out.println("Please enter y or n.");
}
if(getCoin > 0) {
System.out.println("You got " + getCoin + "Coin !!");
} else {
System.out.println("You are losing");
}
PlayLogs playLog = new PlayLogs();
playLog.export();
this.execute();
} catch(IOException | NumberFormatException e) {
}
}
}
}
プログラムを書きなれている方でなくともいかにひどいコードかお分かりになるでしょう。
驚異の10段ネストです。（笑）
しかし2日前の自分は平気でこう書いていました。多分この記事を見た方はこのコードを見た瞬間に戻るボタンを押下することでしょう。ちなみにネストが深くなりすぎて自分でも手出しができなくなり、ところどころ完成していないコードですのでその辺はご了承ください。
それでは過去にタイムスリップしてから自分をぶんなぐって修正させましょう。
ちなみに処理が長すぎるので本来であればメソッドを切り分けるべきですが、今回着目したいのがネストを浅くすることなのでメソッド切り出しに関しては言及していません。そちらもご了承ください。
ポイント1 アーリーリターンを使いelseを削除する
まずは一番上のif文から
こちらはもし現在のコインが0だったら0を返しそれ以外なら次の処理といった動作です。
before
public Integer execute() {
if(this.possessionCoin == 0) {
return 0;
} else {
System.out.println("You have" + this.possessionCoin + "Coin, Start the game? y / n");
こちらを次のように修正します。
after
public Integer execute() {
if(this.possessionCoin == 0) {
return 0;
}
System.out.println("You have" + this.possessionCoin + "Coin, Start the game? y / n");
if文の結果によってtrueだったらreturnとしているのでその下のelseは必要ありません。これだけで1つネストが浅くなりました。
このように処理に当てはまったらreturnしてしまう場合はelseを書く必要はありません。
ポイント2 tryの中は例外が起こりうる処理に絞って書く
before
try {
int getCoin = 0;
BufferedReader br = new BufferedReader(new InputStreamReader(System.in));
String inputChoice = br.readLine();
System.out.println(inputChoice);
if(inputChoice.equals("y")) {
System.out.println("Please bet Coin 1 ~ " + this.maxBetCoin);
while(true) {
try {
BufferedReader br2 = new BufferedReader(new InputStreamReader(System.in));
String inputStr = br2.readLine();
Integer inputBetCoin = Integer.parseInt(inputStr);
if(0 < inputBetCoin && inputBetCoin <= this.possessionCoin && inputBetCoin <= this.maxBetCoin) {
this.possessionCoin -= inputBetCoin;
int sumCardScore = this.getCard();
if(this.judgeCard(sumCardScore)) {
getCoin = inputBetCoin * 2;
System.out.println("You Win! Get" + getCoin + "Coin!");
DoubleUpChanceGame doubleUpChanceGame = new DoubleUpChanceGame(getCoin, this.deckSetCount);
int winCoinCount = doubleUpChanceGame.execute();
if(winCoinCount == 0) {
return 0;
} else {
System.out.println("You got " + winCoinCount + "Coin !!");
this.execute();
}
} else {
this.possessionCoin = 0;
break;
}
} else {
break;
}
} catch(IOException | NumberFormatException e) {
}
}
} else if(inputChoice.equals("n")) {
return this.possessionCoin;
} else {
System.out.println("Please enter y or n.");
}
if(getCoin > 0) {
System.out.println("You got " + getCoin + "Coin !!");
} else {
System.out.println("You are losing");
}
PlayLogs playLog = new PlayLogs();
playLog.export();
this.execute();
} catch(IOException e) {
}
すさまじいですね（笑）tryの中だけで50行ぐらいあります。ここをtryは例外が起こりえる場所に絞って記述するといった意識で取り組みます。
まず最初のtryの中で例外が起こりえるのは
String inputChoice = br.readLine();
この一文のみです。そのため以下のように修正します。
try {
inputChoice = br.readLine();
} catch(IOException e) {
}
他の記述は外に出してしまいます。そうすると変数のスコープの関係上エラーが出ますが、外に変数を作ればいいだけなので作ります。
tryする記述を絞ることでこれまたネストを浅くすることができます。
after
try {
inputChoice = br.readLine();
System.out.println(inputChoice);
} catch(IOException e) {
}
if(inputChoice.equals("y")) {
System.out.println("Please bet Coin 1 ~ " + this.maxBetCoin);
while(true) {
tryの中からif文を出せたことで少しすっきりしました。
次は外に出したif文をすっきりさせていきましょう。
before
// この上にwhile文があります。
if(inputChoice.equals("y")) {
System.out.println("Please bet Coin 1 ~ " + this.maxBetCoin);
while(true) {
try {
BufferedReader br2 = new BufferedReader(new InputStreamReader(System.in));
String inputStr = br2.readLine();
Integer inputBetCoin = Integer.parseInt(inputStr);
if(0 < inputBetCoin && inputBetCoin <= this.possessionCoin && inputBetCoin <= this.maxBetCoin) {
this.possessionCoin -= inputBetCoin;
int sumCardScore = this.getCard();
if(this.judgeCard(sumCardScore)) {
getCoin = inputBetCoin * 2;
System.out.println("You Win! Get" + getCoin + "Coin!");
DoubleUpChanceGame doubleUpChanceGame = new DoubleUpChanceGame(getCoin, this.deckSetCount);
int winCoinCount = doubleUpChanceGame.execute();
if(winCoinCount == 0) {
return 0;
} else {
System.out.println("You got " + winCoinCount + "Coin !!");
this.execute();
}
} else {
this.possessionCoin = 0;
break;
}
} else {
break;
}
} catch(IOException e) {
}
}
} else if(inputChoice.equals("n")) {
return this.possessionCoin;
} else {
System.out.println("Please enter y or n.");
}
そしてさらにこれはelseを削除できるのでelseを削除します。
ついでにelse ifもifに直しましょう。（ここはお好みで）
after
if(inputChoice.equals("y")) {
break;
}
if(inputChoice.equals("n")) {
return this.possessionCoin;
}
System.out.println("Please enter y or n.");
ここまでくるとだいぶ見やすくなりました。
if(inputChoice.equals("y")) {
break;
}
if(inputChoice.equals("n")) {
return this.possessionCoin;
}
System.out.println("Please enter y or n.");
while(true) {
int getCoin = 0;
BufferedReader br = new BufferedReader(new InputStreamReader(System.in));
String inputChoice = "";
try {
inputChoice = br.readLine();
System.out.println(inputChoice);
} catch(IOException e) {
}
if(inputChoice.equals("y")) {
break;
}
if(inputChoice.equals("n")) {
return this.possessionCoin;
}
System.out.println("Please enter y or n.");
if(getCoin > 0) {
System.out.println("You got " + getCoin + "Coin !!");
} else {
System.out.println("You are losing");
}
PlayLogs playLog = new PlayLogs();
playLog.export();
}
// ここまで修正完了
//--------------------------------------------------------------
while(true) {
System.out.println("Please bet Coin 1 ~ " + this.maxBetCoin);
try {
String inputStr = br.readLine();
Integer inputBetCoin = Integer.parseInt(inputStr);
if(0 < inputBetCoin && inputBetCoin <= this.possessionCoin && inputBetCoin <= this.maxBetCoin) {
this.possessionCoin -= inputBetCoin;
int sumCardScore = this.getCard();
if(this.judgeCard(sumCardScore)) {
getCoin = inputBetCoin * 2;
System.out.println("You Win! Get" + getCoin + "Coin!");
DoubleUpChanceGame doubleUpChanceGame = new DoubleUpChanceGame(getCoin, this.deckSetCount);
int winCoinCount = doubleUpChanceGame.execute();
if(winCoinCount == 0) {
return 0;
} else {
System.out.println("You got " + winCoinCount + "Coin !!");
this.execute();
}
} else {
this.possessionCoin = 0;
break;
}
} else {
break;
}
} catch(IOException e) {
}
あとは下のwhile文のtryの中を外に出します。
try {
String inputStr = br.readLine();
Integer inputBetCoin = Integer.parseInt(inputStr);
} catch(IOException | NumberFormatException e) {
System.out.println("You typed incorrect value, please type of correct number");
}
after
while(true) {
System.out.println("Please bet Coin 1 ~ " + this.maxBetCoin);
try {
String inputStr = br.readLine();
Integer inputBetCoin = Integer.parseInt(inputStr);
} catch(IOException | NumberFormatException e) {
System.out.println("You typed incorrect value, please type of correct number");
}
//次はこの下を修正
if(0 < inputBetCoin && inputBetCoin <= this.possessionCoin && inputBetCoin <= this.maxBetCoin) {
this.possessionCoin -= inputBetCoin;
int sumCardScore = this.getCard();
if(this.judgeCard(sumCardScore)) {
getCoin = inputBetCoin * 2;
System.out.println("You Win! Get" + getCoin + "Coin!");
DoubleUpChanceGame doubleUpChanceGame = new DoubleUpChanceGame(getCoin, this.deckSetCount);
int winCoinCount = doubleUpChanceGame.execute();
if(winCoinCount == 0) {
return 0;
} else {
System.out.println("You got " + winCoinCount + "Coin !!");
this.execute();
}
} else {
this.possessionCoin = 0;
break;
}
} else {
break;
}
}
ポイント3 はじける条件は先に書く（continueの使用）
if文を修正します。今回の条件は入力値（ベット額）が0より大きくベットできる最大額以下の時、かつ現在の所持コイン以下だった場合に処理を行いたいので、逆に考えてこの条件以外の時はその後の処理ははじいて入力を再度求める処理を書けばよさそうです。ここでcontinueを使います。
if(0 < inputBetCoin && inputBetCoin <= this.possessionCoin && inputBetCoin <= this.maxBetCoin) {
処理
}
この部分を
if(inputBetCoin <= 0) {
continue;
}
if(inputBetCoin > this.possessionCoin) {
continue;
}
if(inputBetCoin > this.maxBetCoin) {
continue;
}
処理
このように書き換えました。
こうすることで入力値が0以下の時はやり直し、所持コインよりも大きいときはやり直し、ベット可能最大額より大きいときはやり直しといった記述ができました。このほうがシンプルで読みやすいと思います。
ポイント4 複雑な条件の時は処理を小分けにする。
&&でつなげてもよいのですが、人間の目は横より縦に長いほうが読みやすく条件1つずつのほうが前の条件を気にしなくていいので上記のような書き方にしました。
次はif文の中にif文が書いてあるのでここを修正します。
before
if(this.judgeCard(sumCardScore)) {
getCoin = inputBetCoin * 2;
System.out.println("You Win! Get" + getCoin + "Coin!");
DoubleUpChanceGame doubleUpChanceGame = new DoubleUpChanceGame(getCoin, this.deckSetCount);
int winCoinCount = doubleUpChanceGame.execute();
if(winCoinCount == 0) {
return 0;
} else {
System.out.println("You got " + winCoinCount + "Coin !!");
this.execute();
}
} else {
break;
}
まず中のif文を外に出します。
after
int winCoinCoint = 0;
if(this.judgeCard(sumCardScore)) {
getCoin = inputBetCoin * 2;
System.out.println("You Win! Get" + getCoin + "Coin!");
DoubleUpChanceGame doubleUpChanceGame = new DoubleUpChanceGame(getCoin, this.deckSetCount);
winCoinCount = doubleUpChanceGame.execute();
} else {
break;
}
if(winCoinCount == 0) {
return 0;
} else {
System.out.println("You got " + winCoinCount + "Coin !!");
break;
}
1つ目のif文がelseになったらbreakするようにすればbeforeの時と同じ動きになります。変数のスコープでエラーが発生するので修正します。
おなじみifでアーリーリターンしているので下のほうのelseを削除します。
if(this.judgeCard(sumCardScore)) {
getCoin = inputBetCoin * 2;
System.out.println("You Win! Get" + getCoin + "Coin!");
DoubleUpChanceGame doubleUpChanceGame = new DoubleUpChanceGame(getCoin, this.deckSetCount);
winCoinCount = doubleUpChanceGame.execute();
} else {
break;
}
if(winCoinCount == 0) {
return 0;
}
System.out.println("You got " + winCoinCount + "Coin !!");
break;
ポイント5 条件を逆に考える
上記のif文にはもう一つ直せそうな箇所があります。
それはelseに処理が移った時にbreakしかしていないということです。
before
if(this.judgeCard(sumCardScore)) {
getCoin = inputBetCoin * 2;
System.out.println("You Win! Get" + getCoin + "Coin!");
DoubleUpChanceGame doubleUpChanceGame = new DoubleUpChanceGame(getCoin, this.deckSetCount);
winCoinCount = doubleUpChanceGame.execute();
} else {
break;
}
this.judgeCardというメソッドはbooleanを返すメソッドです。今はfalseが返ってきたときはbreakで処理を抜けるのでこうも書けます。
after
if(!this.judgeCard(sumCardScore)) {
break;
}
getFromCardPickCoin += (inputBetCoin * 2);
System.out.println("You Win! Get" + (getFromCardPickCoin) + "Coin!");
DoubleUpChanceGame doubleUpChanceGame = new DoubleUpChanceGame((getFromCardPickCoin), this.deckSetCount);
getFromDoubleUpCoin = doubleUpChanceGame.execute();
こうすればfalseになった時にはbreakして処理を抜ける、それ以外は処理を継続するということができます。これでまたelseを削除することができました。
ただ、!でのbooleanをひっくり返す判定は逆にわかりづらくなることがあるので変数名の工夫をしたり、ケースバイケースで使ったほうがよさそうです。
修正完了
修正前
before
public Integer execute() {
if(this.possessionCoin == 0) {
return 0;
} else {
System.out.println("You have" + this.possessionCoin + "Coin, Start the game? y / n");
while(true) {
try {
int getCoin = 0;
BufferedReader br = new BufferedReader(new InputStreamReader(System.in));
String inputChoice = br.readLine();
System.out.println(inputChoice);
if(inputChoice.equals("y")) {
System.out.println("Please bet Coin 1 ~ " + this.maxBetCoin);
while(true) {
try {
BufferedReader br2 = new BufferedReader(new InputStreamReader(System.in));
String inputStr = br2.readLine();
Integer inputBetCoin = Integer.parseInt(inputStr);
if(0 < inputBetCoin && inputBetCoin <= this.possessionCoin && inputBetCoin <= this.maxBetCoin) {
this.possessionCoin -= inputBetCoin;
int sumCardScore = this.getCard();
if(this.judgeCard(sumCardScore)) {
getCoin = inputBetCoin * 2;
System.out.println("You Win! Get" + getCoin + "Coin!");
DoubleUpChanceGame doubleUpChanceGame = new DoubleUpChanceGame(getCoin, this.deckSetCount);
int winCoinCount = doubleUpChanceGame.execute();
if(winCoinCount == 0) {
return 0;
} else {
System.out.println("You got " + winCoinCount + "Coin !!");
this.execute();
}
} else {
this.possessionCoin = 0;
break;
}
} else {
break;
}
} catch(IOException | NumberFormatException e) {
}
}
} else if(inputChoice.equals("n")) {
return this.possessionCoin;
} else {
System.out.println("Please enter y or n.");
}
if(getCoin > 0) {
System.out.println("You got " + getCoin + "Coin !!");
} else {
System.out.println("You are losing");
}
PlayLogs playLog = new PlayLogs();
playLog.export();
this.execute();
} catch(IOException | NumberFormatException e) {
}
}
}
}
修正後
after
public Integer execute() {
if(this.possessionCoin == 0) {
return 0;
}
BufferedReader br = new BufferedReader(new InputStreamReader(System.in));
while(true) {
System.out.println("You have" + this.possessionCoin + "Coin, Start the game? y / n");
String inputChoice = "";
try {
inputChoice = br.readLine();
} catch(IOException | NumberFormatException e) {
System.out.println("You typed incorrect value, please type of correct number");
}
if(inputChoice.equals("n")) {
return this.possessionCoin;
}
if(inputChoice.equals("y")) {
break;
}
System.out.println("Please enter y or n.");
}
int inputBetCoin = 0;
int getFromCardPickCoin = 0;
int getFromDoubleUpCoin = 0;
while(true) {
System.out.println("Please bet Coin 1 ~ " + this.maxBetCoin);
try {
String inputStr = br.readLine();
inputBetCoin = Integer.parseInt(inputStr);
} catch(IOException | NumberFormatException e) {
System.out.println("You typed incorrect value, please type of correct number");
}
if(inputBetCoin <= 0) {
continue;
}
if(inputBetCoin > this.maxBetCoin) {
continue;
}
if(inputBetCoin > this.possessionCoin) {
continue;
}
this.possessionCoin -= inputBetCoin;
int sumCardScore = this.getCard();
if(!this.judgeCard(sumCardScore)) {
break;
}
getFromCardPickCoin += (inputBetCoin * 2);
System.out.println("You Win! Get" + (getFromCardPickCoin) + "Coin!");
DoubleUpChanceGame doubleUpChanceGame = new DoubleUpChanceGame((getFromCardPickCoin), this.deckSetCount);
getFromDoubleUpCoin = doubleUpChanceGame.execute();
if(getFromDoubleUpCoin == 0) {
System.out.println("You are losing");
return this.possessionCoin;
}
break;
}
if(getFromDoubleUpCoin > 0) {
System.out.println("You got " + getFromDoubleUpCoin + "Coin !!");
this.possessionCoin += getFromDoubleUpCoin + inputBetCoin;
} else {
System.out.println("You are losing");
}
PlayLogs playLog = new PlayLogs();
playLog.export();
return this.execute();
}
ネストを3段にまで減らすことができました。行数が多く読みにくいかもしれませんがそれでも修正前よりだいぶ読みやすくなったと思います。
ここからメソッドを切り分ければさらに読みやすくなると思います。
（修正前はアプリが未完成の状態で修正後がアプリの完成版なのでところどころロジックや変数名が変わっていますが、今回の主眼はアプリの作成ではなくネストを浅くすることだったのでそちらについてはご容赦ください。）
最後に
ネストが深くなっているときには絶対に何かしら他の方法があるといった考えを持つことが必要だと感じました。
とりあえず2日前の自分のことはぶっ飛ばしたのでよしとします（笑）
もしもっと見やすくするにはこうしたほうが良いといった意見やこういったところにも気を配ったほうが良いなどありましたらぜひコメントいただけると幸いです！