コードレビューだったり、調べものだったり、他の人のコードを読む機会って多いですよね。
そのときにこのコード読みにくいなぁとか、自分だったらこう書くけどなぁと思うこともあると思います。
そんな感じで気になったコードをまとめてみます。
個人的に読みにくいコード
その理由と、自分だったらこうやって書くであろうコードを紹介します。
カッコなしのif
if (num == 1)
return num;このように条件分岐した処理が{}で囲われていないif文。
どこまでがif文の処理なのかがわかりにくいです。
それでいて、省略できるのは{}だけなので省略する必要はないと思います。
また、必ずカッコありのif文と混在することとなるので、カッコありのif文に統一したいです。
if (num == 1) {
return num;
}単純なif else
int num;
if (true) {
num = 2;
} else {
num = 3;
}このように単純な処理であれば、三項演算子を使いたいです。
コードが無駄に長くなるだけで、読みにくくなります。
三項演算子を使えば１行で収まって良いですね。
int num = true ? 2 : 3;else if
int num1;
int num2 = 1;
if (num2 == 1) {
num1 = 1;
} else if (num2 == 2) {
num1 = 2;
} else if (num2 == 3) {
num1 = 3;
} else if (num2 == 4) {
num1 = 4;
}if elseと同様に、このような単純な処理であれば、switch文を使いたいです。
switch文は読みやすいので積極的に使っています。
int num1;
int num2 = 1;
switch (num2) {
case 1:
num1 = 1;
break;
case 2:
num1 = 2;
break;
case 3:
num1 = 3;
break;
case 4:
num1 = 4;
break;
default:
}for文・拡張for文
List<String> list = Arrays.asList("a", "b", "c");
for (int i = 0; i < list.size(); i++) {
System.out.println(list.get(i));
}List<String> list = Arrays.asList("a", "b", "c");
for (String s : list) {
System.out.println(s);
}forEachが使えるならforEachを使いたい。
forEachを使えば１行でかけてしまう上に読みやすいですよね。
Java8以降であれば積極的にラムダを使いたい。
List<String> list = Arrays.asList("a", "b", "c");
list.forEach(System.out::println);複数のコンストラクタ
public class UserDto {
public String name;
public Integer age;
UserDto(String name) {
this.name = name;
}
UserDto(Integer age) {
this.age = age;
}
}複数コンストラクタを定義するのは良くないと教わったので違和感があります。
別のメソッドでラップしてあげるのが良さそう。
public class UserDto {
public String name;
public Integer age;
UserDto() {
}
public static UserDto createByName(String name){
UserDto userDto = new UserDto();
userDto.name = name;
return userDto;
}
public static UserDto createByAge(Integer age){
UserDto userDto = new UserDto();
userDto.age = age;
return userDto;
}
}Boolean型
Boolean flg = true;基本的にフラグを扱うときには、NULLが入ることはないはずなので、boolean型を使いたいです。
プリミティブ型で良いならプリミティブ型を使いたい。
boolean flg = true;Lists.newArrayList()
List<String> list = Lists.newArrayList();こちらは明確な理由があるわけではなく、なんとなく使いたくないコードです。
私はListを初期化する場合、以下のコードに統一しています。
List<String> list = new ArrayList<>();NULLの判定
String s = null;
if (s == null) {
s = "abc";
}NULLの判定にはObjects.isNull()を使用したいです。
==で判定しても良いのですが、その場合はそちらに統一したいですね。
java.util.Objectsは便利なので積極的に使っていきたいです。
String s = null;
if (Objects.isNull(s)) {
s = "abc";
}カッコの場所
public class UserDto
{
public String name;
public Integer age;
}滅多に見ないけど、見ると気になっちゃうコード。
無駄な改行に感じてしまうので、できれば改行しないでほしい。
public class UserDto {
public String name;
public Integer age;
}importをまとめる
import javax.persistence.*;エディタが勝手にやってくれる部分だと思いますが、できればまとめないでほしい。
たまに同じ名前のメソッドを読み込んでいてエラーになるイメージ。
コード自体短くなるのですが、import部分を読む人はいないので、多少長くなってもすべて明記したいです。
import javax.persistence.CollectionTable;
import javax.persistence.Column;
import javax.persistence.ElementCollection;
import javax.persistence.Entity;
import javax.persistence.EnumType;
import javax.persistence.Enumerated;
import javax.persistence.FetchType;
import javax.persistence.GeneratedValue;
import javax.persistence.GenerationType;
import javax.persistence.Id;
import javax.persistence.JoinColumn;
import javax.persistence.ManyToOne;
import javax.persistence.Table;
import javax.persistence.Temporal;
import javax.persistence.TemporalType;まとめ
あくまでこれらはすべて個人的な意見なので、これらのコードが悪いとは思いません。
コードレビューでも指摘するほどの内容でもないと思います。
ただ、これらのコードを混在させるのではなく、どちらかに統一したほうが良いと思います。
複数人で開発する場合は開発方針として最初に明確にしておくと良いですね。
メンバー全員が開発しやすく、レビューしやすいコードだと効率が良くなると思います。
