[初心者向け]レビューアーから見る「こんなコードは書かないで!」(その1)

2024年のGWも終わりましたが。。。

みなさん、お元気ですか？
大丈夫？遊びすぎて体調崩したりしてない？
休みすぎてやる気無くなったりしてない？
4月に教えてもらったこと全部忘れるとかいう、とんでもないボケをかましてない？

社会人だったら、、、そうだね、 【自己責任】 だね(ｴﾋﾞﾊﾞﾃﾞｨﾊﾟｯｼｮﾝ

そんなこんなで、いよいよネタが。。。

いろんなプロジェクトを同時並行とかでやらせてもらえてれば、きっとそれぞれのプロジェクトでネタになるような出来事(炎上ネタ)がたくさんあるはずなのに、縛りプレイな諸事情でメインのプロジェクトしかやらせてもらえてないので、もうネタが無くて辛い。。。
ネタになりそうな開発のお仕事ください(要約：実装したいし給料も上げて欲しい

お前は本当にそのコードでレビューに出すんだな？

偉く(色々と一人で出来るように)なる、と言うか他に見る人がいなかったりすると、巡り巡ってくるレビューと言う、他人が書いたコードを 【読んで、ダメ出しをする】 お仕事が回ってくるわけで。。。

そんな自分もこれまでに何度かコードを読んでは指摘、読んでは指摘なことをやってきたわけですが、
だいたいプログラマ3年生ぐらいまでが必ずやるであろう、
レビューアーを イラッ とさせる細かい指摘を紹介。

新人や初心者はこれを反面教師として、レビューアー嫌いなヤツだからといって、決して真似して書いちゃ駄目だぞ!!
※仮に真似して書いたのにスルーされたら、そのレビューアーが　☆ク★ソ☆　というのは内緒だぞ!!

まずはこんなコード

関数の概要としては、
引数のStringの値の必須チェックと桁数チェックを行い結果(エラー時にtrue)を返す
※外部ライブラリは使わないものとする(生のJava
※引数のStringはnull or 半角文字しか絶対来ないという想定で話を進めます

クソコード.java

private boolean isError(String value){
  // チェック処理:エラーフラグ
  boolean a = false;

  // 必須チェック
  boolean b = true;
  if(value == null || value.isEmpty()) {
    a = true;
    
    // valueが必須じゃなかったらbをfalseにする
    b = false;
  }
  
  // 桁数チェック
  if(b == true){
    int c = value.length();
    if(c > 10){
      a = true;
    }
  }

  if(a == true) {
    return true;
  } else {
    return false;
  }
}

いいよいいよ、インデント以外は完璧なクソだよ(ちゃんと概要通りには動くはずだよ
書いてて吐き気がするほど、新人感出てるよ!!
※気持ち悪すぎてインデントだけはちゃんとやった、みんなもツールの整形機能を使おうNE☆

SHI・TE・KI 1：変数名の雑さよ

クソコード.java(抜粋)

private boolean isError(String value){
  // チェック処理:エラーフラグ
  boolean a = false;

コメントはちゃんとしてるのに、なんで変数名まで気を使わないのか(Why?Japanese People!?

まだクソコード.java

private boolean isError(String value){
  // チェック処理:エラーフラグ
  boolean errorFlg = false;

  // 必須チェック
  boolean valueInputed = true;
  if(value == null || value.isEmpty()) {
    errorFlg = true;
    
    // valueが必須じゃなかったらbをfalseにする
    valueInputed = false;
  }
  
  // 桁数チェック
  if(valueInputed == true){
    int valueLength = value.length();
    if(valueLength > 10){
      errorFlg = true;
    }
  }

  if(errorFlg == true) {
    return true;
  } else {
    return false;
  }
}

いいよいいよ、変数の役割が名前からなんとなく見えてきたよ。

SHI・TE・KI 2：無駄な変数の多さよ

valueに値が入ってたら…を管理する【valueInputed】や、
valueの文字列長…を管理する【valueLength】だけど、
後続処理で1回しか使われてないんだから、はじめから要らなくね？
もっとスマートに書いてよね!!

まだまだクソコード.java

private boolean isError(String value){
  // チェック処理:エラーフラグ
  boolean errorFlg = false;

  // 必須チェック
  if(value == null || value.isEmpty()) {
    errorFlg = true;
  } else {
    // 桁数チェック
    if(value.length() > 10){
      errorFlg = true;
    }
  }

  if(errorFlg == true) {
    return true;
  } else {
    return false;
  }
}

いいよいいよ、だいぶスッキリしてきたよ。
必須チェックでエラーにならないってことは、elseでは必ず値が入ってるってことだもんね。
あと、ifの()内にメソッドを書いちゃ駄目なんていうルールはないから、いちいち値を変数に格納せず、()内にvalue.length()書いちゃえばいいんだYO!!

SHI・TE・KI 3：ifの()内に== trueをKA・KU・NA☆

errorFlgの型ってな～んだ？

まだまだクソコード.java(抜粋)

private boolean isError(String value){
  // チェック処理:エラーフラグ
  boolean errorFlg = false; // boolean型なんだぜ

  // ...省略...
  
  if(errorFlg == true) {
    return true;
  } else {
    return false;
  }
}

知ってるか？ifの()の中には、boolean判定出来るものが入ってればいいんだぜ？
おいおい、じゃあ「errorFlg == true」でもいいじゃないかと思ってるんだろ？
いいかよく見ろerrorFlgは 【boolean型】 なんだ、つまりはこいつだけで判定が出来るんだZE(ﾋｭｰ
ということは「errorFlg == true」は「true == true」or「false == true」と言ってるのと同じ事なんだ。
わかったか、boolean型の変数を比較するときに「== true」や「== false」を書くことは、二郎で食べきれない注文をするのと同じくらい ギルティ ってことさ。

まだまだまだクソコード.java

private boolean isError(String value){
  // チェック処理:エラーフラグ
  boolean errorFlg = false;

  // 必須チェック
  if(value == null || value.isEmpty()) {
    errorFlg = true;
  } else {
    // 桁数チェック
    if(value.length() > 10){
      errorFlg = true;
    }
  }

  if(errorFlg) {
    return true;
  } else {
    return false;
  }
}

SHI・TE・KI 4：returnがKU☆SO

errorFlgがboolean型なのに、errorFlg == trueの時にreturn trueするとか。。。

やっとまともコード.java

private boolean isError(String value){
  // チェック処理:エラーフラグ
  boolean errorFlg = false;

  // 必須チェック
  if(value == null || value.isEmpty()) {
    errorFlg = true;
  } else {
    // 桁数チェック
    if(value.length() > 10){
      errorFlg = true;
    }
  }
  return errorFlg;
}

そのままerrorFlg返せばよくね？なんのためのフラグなの？
記憶力ないの？馬鹿なの？死ぬの？

追記【@fujitanozomuさん】指摘

コメントでerrorFlgもいらないのではという指摘を頂きましたので、それ版も↓

まともコード.java

private boolean isError(String value){
  // 必須チェック
  if(value == null || value.isEmpty()) {
    return true;
  } else {
    // 桁数チェック
    if(value.length() > 10){
      return true;
    }
  }
  return false;
}

指摘的にはごもっともなところではありますが、
自分的には処理の途中で値を返すような書き方は、保守とかでコードを見直しているときになんか見落としそうな気がしてあまり好きじゃないんですよね。。。
今回のコードみたいに短い関数とかでアレば別に気にならないレベルですが、短編小説レベルに長ったらしいコードで、ラムダのforEachとかガンガン書いてあった場合、処理の真ん中らへんで急にreturnとか書かれてると「ん？」ってなっちゃうので、ここはケースバイケースなのかなってっていうのが個人的な見解でした。
※決して指摘が間違ってるというわけではなく、一個人の主観です。

そんなわけで。。。

今回は変数名・if文・return周りによくあるクソみたいなコードをピックアップしてみましたが、いかがでしたでしょうか？
Javaに限らず、どの言語であっても同じような書き方をするとは思うので、ちゃんと言語仕様や参画プロジェクトのコーディング規約に沿った書き方を心がけましょうね!!

また【== true】は割とどのプロジェクト(言語問わず)でも見かけるもので、これを見つけるたびに「あっ、このプロジェクトではコードレビューしてない or レビューアーのレベルがゴミのようだ(ﾊﾞﾙｽ」と、参画初日にうんざりすることも。。。

まぁクソクソと言ってますが、一人で実装してると、どうしても気が付かない書き方のクセや間違いなどがあると思うので、
新人や初心者の方々は身近の出来る人にコードを見てもらっていっぱいダメ出し貰って成長してください。

いいか、誰でも最初はどうしようもない無駄なコードを書いてしまうんだ!!
いきなり清書レベルで書けるプログラマーなんか10年選手でも半分いればいいレベルだと思うので、失敗を挫けずにどんどんクソなコードを書いて、しっかりとレビューで洗練されていけばいいんだ!!

なお、俺スゲーwwwなプログラマーが書くとこうなる

俺スゲーwwwコード.java

private boolean isError(String value){
  return value == null || value.isEmpty() || value.length() > 10;
}

ね、簡単でしょ(某絵描き風

||(or条件)なので、最初がtrueだったら後ろの条件は無視してtrueを返すんだ。
valueがnullだったら、value.isEmpty()とvalue.length() > 10 は呼ばれることなくtrueを返すんだ。
ただし可読性がクソなので、コメントしっかり残してねと指摘はするだろう。

可読性の指摘についてはそのうち書きたい(願望