Edited at

数多の先輩を殺しかけてきたクソコーディング集(Java編)


はじめに

 本記事は、数多の先輩を殺しかけてきた私のクソコードを紹介し、

 笑いのネタにしつつも

 そのクソコードの有効なリファクタリング方法を記述していくものです。


1.インデント・スペースの不揃い


fizzbuzz.java


public class fizzbuzz {

public static void main(String[] args) {
for(int i = 0 ; i < 500 ; i ++) {
if(i % 3 == 0 && i % 5 == 0) {
System.out.println("FizzBuzz");
}
else if(i % 3 == 0) {
System.out.println("Fizz");
} else if (i % 5 == 0) {
System.out.println("Buzz");
}
}
}
}


このコードは正常に動作しますが、説明不要なくらいきったないコードですよね。

インデントやスペースの不揃いは意外と軽視しがちな要素ではありますが、しっかりと揃えてから質問やレビューに出しましょう。

処理ブロックに入ったら1段下げる、抜けたら1段戻る。を意識するとやりやすいです。


fizzbuzz.java


public class fizzbuzz {

public static void main(String[] args) {
for(int i = 0 ; i < 500 ; i ++ ) {
if(i % 3 == 0 && i % 5 == 0 ) {
System.out.println("FizzBuzz");
} else if(i % 3 == 0) {
System.out.println("Fizz");
} else if (i % 5 == 0) {
System.out.println("Buzz");
}
}
}
}



2.変数名・定数名がやったら長い。

 これは、先輩に「名前から用途がわかる変数名にしてくれ」という指摘を受けてやらかしたものです。

※本コードの処理の内容には誤りがあります。


fizzbuzz.java


public class fizzbuzz {

public static final boolean CONFIRM_OR_ADD_BUTTON_NAME_FLG = true;

public static void main(String [] args) {
String screenShowButtonNameText = "";
if(CONFIRM_OR_ADD_BUTTON_NAME_FLG) {
screenShowButtonNameText = "confirm";
} else {
buttonNameText = "confirm";
}
System.out.println("ButtonName : " + screenShowButtonNameText);
}
}


定数:CONFIRM_OR_ADD_BUTTON_NAME_FLG は、おそらくボタン名がConfrimかAddかを判定するのでしょう。

そして判定した結果をおそらく画面に表示する文字列である、

screenShowButtonNameTextに格納するのでしょう。

ですが、長すぎます。具合が悪くなります。

命名規則は職場でのコーディングであれば規約等があるかもしれませんが、個人で開発するなら1単語 + 1動詞くらいに抑えておくと後々すぐわかります。


fizzbuzz.java


public class fizzbuzz {

public static final boolean BUTTON_NAME_FLG = true;

public static void main(String [] args) {
String buttonNameText = "";
if(CONFIRM_OR_ADD_BUTTON_NAME_FLG) {
buttonNameText = "confirm";
} else {
buttonNameText = "confirm";
}
System.out.println("ButtonName : " + buttonNameText);
}
}



3.急にテクニックに走る。

少し慣れてきたころに、その構文の有用性がわからないまま急にテクニックに走り出しました。


fizzbuzz.java


public class fizzbuzz {

public static void main(String [] args) {
List<String> retItemList = new ArrayList<String>();
fizzbuzz fizzbuzz = new fizzbuzz();
fizzbuzz.checkResultList(retItemList);
}

public List<String> checkResultList(List<String> retItemList) {
List<String> checkList = retItemList == null ? new ArrayList<>() : retItemList.isEmpty() ? new ArrayList<>() : retItemList;
return checkList;
}
}


三項演算子の使用そのものは悪ではないです。(私の中では)しかし、上記のようにネストをさせると可読性が落ちます。

三項演算子の使用は、短い条件かつ1個の条件のみとし、それ以外はif -elseで判定するとするのが有効でしょうか。


fizzbuzz.java


import java.util.ArrayList;
import java.util.List;

public class fizzbuzz {

public static void main(String [] args) {
List<String> retItemList = new ArrayList<String>();
fizzbuzz fizzbuzz = new fizzbuzz();
fizzbuzz.checkResultList(retItemList);
}

public List<String> checkResultList(List<String> retItemList) {
List<String> checkList = new ArrayList<String>();
if(checkList != null && !(checkList.isEmpty())) {
checkList = retItemList;
}
return checkList;
}
}



4.不必要すぎるコメント


fizzbuzz.java


import java.util.ArrayList;
import java.util.List;
// クラス名の「fizzbuzz」は嘘。本当はリストの中身をチェックする処理。
public class fizzbuzz {
// mainメソッドはJavaのエントリーポイントで、ここから処理が開始される。
// コマンドライン引数は今回は渡さない。
public static void main(String [] args) {
// String型を格納する空のListを生成する。
List<String> retItemList = new ArrayList<String>();
// TODO:「fizzbuzz」は正しくない名前、修正が必要。
fizzbuzz fizzbuzz = new fizzbuzz();
// checkResultListメソッドを呼び出す。判定を行う。
fizzbuzz.checkResultList(retItemList);
}
// リストのnullチェックをする。
public List<String> checkResultList(List<String> retItemList) {
// チェック用の空のリストを宣言する。
List<String> checkList = new ArrayList<String>();
// checkListがnull又は空の場合は、空のリストを返却する。
if(retItemList != null && !(retItemList.isEmpty())) {
checkList = retItemList;
}

return checkList;
}
}


・・・はい。ごめんなさい。

コメントの書き方のコツはいくつかありますが、今回の例でいくと、

・見ればわかるものは書かない。

・不要なTODOは削除しておく。

この二点を意識すると、まだ意味のあるコメントが残ります。


fizzbuzz.java


import java.util.ArrayList;
import java.util.List;
public class fizzbuzz {
public static void main(String [] args) {
List<String> retItemList = new ArrayList<String>();
fizzbuzz fizzbuzz = new fizzbuzz();
fizzbuzz.checkResultList(retItemList);
}
// 引数で渡されてきたリストのnullチェックをする。
public List<String> checkResultList(List<String> retItemList) {
List<String> checkList = new ArrayList<String>();
// checkListがnull又は空の場合は、処理を行わない。
if(checkList != null && !(checkList.isEmpty())) {
checkList = retItemList;
}

return checkList;
}
}


条件分岐のコメント、「checkListがnull又は空の場合は、処理を行わない。」は、

処理の変更によってnull、空以外をチェックするようになった時に、コメントを変更しないと処理とコメントが乖離する危険性をはらんでいますが、

一目見て条件分岐の内容がわかることの利点もあります。

今回は利点の方を重視しました。


おわりに

 以上です。ここで挙げたことは、基本的な内容ですが、意識するのは最初の内は難しいです。

可読性を上げるメリットは数多くあるので、処理の内容がわからなくて物が作れなくても可読性だけは意識しましょう。

それだけでも先輩エンジニアからのアドバイスの受けやすさや、余計なダメージ、話の流れが飛ぶといったことが減り、成長につながるはずです。