勝手にプログラムレビュー① Java編

こんにちは、Hibikiです。

今日はいつもと趣向を変えて、プログラムのソースコードレビューをしてみました。

あなたはコードレビューを受けること、好きですか？

必要なことと分かっていても、自分が書いたコードを他人にとやかく言われるのは楽しくないですよね？

分かります。私も好きではありません。

しかし、コードレビューを受けることは、プログラミングスキルの向上にあたって最高の方法であることは確かです。

プログラミングに100％正しい書き方というのは、存在しません。
この書き方しかない、という定型があればどれだけ楽か．．．

要件通りに動いたとしても、、、

などのように、より効率良く、より保守しやすい改善点というのは出てくるものです。

だからこそ、第三者に見てもらい、自分に足りない視点を学ぶことは貴重な経験なのです。

今回対象としたプログラムは、ネット上で公開されているポートフォリオを使わせて頂きました。

勝手に使わせて頂いて、本当にすいません。

ボリューム的にも皆さんに見て頂く上でちょうど良いサイズでしたので、使わせて頂くことにしました。

Javaのプログラムですが、指摘はJavaに限らず、最近の高級言語で当てはまるものだけ、指摘を挙げています。Javaを知らなくても、何かしらプログラム言語を読んだことがあれば、理解頂ける内容にしています。

【この記事をオススメの人】

【この記事をオススメしない人】

では、誌上コードレビューを通して、"ワンランク上のプログラマ"を目指しましょう。

📚️対象プログラム

今回のコードレビューは、上記サイトで公開されているコードを使わせて頂きました。

Javaを使ったWebアプリです。

ソースコードはGitHubからダウンロードできます。

https://github.com/Takthustra/learning_stopwatch

📚️良いプログラムとは？

ソースコードレビューに入る前に「良いプログラム」について考えてみたいと想います。

皆さんは何が「良いプログラム」か考えたことありますか？

パフォーマンスが良いプログラム
バグの無いプログラム
拡張しやすいプログラム
etc

きっと他にも様々な「良いプログラム」の定義が出てくることだと思います。そして、どれも間違いではありません。

しかし、一番大事なのは『システムの目的に沿ったプログラム』であるかどうかです。

例えば、2024年11月に東京証券取引所の新システム「arrowhead4.0」がリリースされましたが、このシステムの目的は簡単に言うと次の2点に集約されます。

あなたがarrowhead4.0のプログラマだったとして、「保守性を考慮して、多少パフォーマンスは落ちますが、こういうプログラムを書きました」と言ったら怒られますよね。なぜならパフォーマンスはシステムの主要要件の1つだからです。

「パフォーマンスは良いが可読性の落ちるソースコード」
「可読性は良いがパフォーマンスが落ちるソースコード」

どちらが良いかはシステムの目的によって変わるのです。

プログラムを書く前、もっと言うと設計する前に、エンジニアはシステムの目的を確認して、すべての作業において目的に合致しているかどうかをチェックすることが大事です。

「良いプログラムとは何か」について、別の記事にて詳細を書く予定です。

📚️レビュー指摘 【効率性】

レビューに入っていきましょう。

まずは効率性観点から2つ指摘を挙げました。

form.getName()を呼び出すと、内部で何が起こるか見てみましょう。
01. formオブジェクトにアクセス
02. formオブジェクトからgetNameメソッドを探す
03. getNameメソッド内のコードを実行し、名前の値を返す

簡単に書いても3つの処理が実行されます。
（メモリ上での値の取り出し、保管も含めるともっと複雑です）

一方、ローカル変数を使った場合は、処理が1つだけです。
01. 変数から名前の値を取得する

1回目のform.getName()呼び出しで、値をローカル変数「name」にコピーしているので、2回目、3回目もローカル変数を使用すべきでしょう。

今回のように処理数が少ないメソッドの場合、その違いはほとんどありません。

が、これがループ内で繰り返される処理だった場合にはチリツモでパフォーマンスに影響することがあります。

メソッド内で繰り返し使用される値は、ローカル変数にコピーしておく、が鉄則です。

条件分岐にif文を使用することは間違いではありません。
ただ、このケースではif文よりもswitch文が効率良いです。

なぜif文が効率悪いのでしょうか？

上記forループ処理の中で変数「i」が"2"の場合の動きを追ってみましょう。
01. 「if (i == 0)」これは一致しないので、次の条件判定に進む
02. 「else if (i == 1)」 これも一致しないので、次の条件判定に進む
03. 「else if (i == 2)」 やっと一致したので、中身の処理に入る

これがswitch文だった場合、

01. 「i == 2」の処理にジャンプし、中身の処理に入る

だけです。

分岐の数が多くなればなるほど、switch文の方が効率よく動きます。

条件分岐は、まずswitch文を使えないか確認する、が鉄則です。

※私が未経験の言語では分からないのですが、おそらく多くの言語で同じはずです。コンパイル言語の場合、コンパイラが最適化してswitch文と同じ処理に書き換えてくれる可能性もあります。

📚️レビュー指摘 【保守性】

保守性観点から2つ指摘を挙げました。

return文は、可能な限り末尾に1回のみにすべきです。

あとで、誰か別の人がこのソースコードをメンテした時のことをイメージしてみましょう。

例えば仕様変更により、毎回処理したいコード（=処理A）を3個目のreturn文の直前に追加したとします。

当然、上2つのreturn文が実行されると、処理Aが実行されることがありません。そして、それはバグとなるのです。

経験のあるプログラマは保守性の観点からreturn文を末尾にしか書かないので、経験のある人ほど思い込みでこのようなバグを埋め込んでしまうのです。経験のあるプログラマからすると、「まさかreturn文が上にいくつも書いてあるなんて！」と思うのです。

一方で、1個目のreturn文は、書くこともあります。

エラーが発生している場合に後続の処理をしたくないので、メソッドの最初にエラー判定とreturn文を書いてしまうのです。

この書き方をすると、後続のコードがすっきりするので、可読性が上がります。

経験のあるプログラマにとってトラップとなってしまう心配ですが、すべてのメソッドで同じように書いてあれば、おそらく問題ありません。

逆に他のメソッドではこのような書き方をしていないのに、あるメソッドだけ先頭にreturn文が書いてあると、思い込みを誘発し、バグを埋め込む危険性が高まります。

return文は、基本体に末尾に1回、が鉄則です。
そして、出来るだけ書き方はすべてのメソッドで統一、も鉄則です。

ここはアスタリスク「*」ではなく、全カラムの名前を書くべきです。

プログラムの動きとして何の問題も無いですし、コードもすっきりするのですが、後で面倒な事態になります。

将来何かバグが見つかった場合に、他に同じような問題がないのか「横展開調査」を行うのですが、どのカラムのデータをどの処理で使用しているのか調べることがあります。

例えば、「"従業員"テーブルの"名字"カラムを参照している処理をすべて洗い出す」などのケースです。

その際に「*」で書いてあると、わざわざテーブル定義書を参照してカラムを確認しなくてはいけないのです。

1回使ってお役御免のプログラムであれば問題ないのですが、将来に渡って保守しなくていけないプログラムでは、SQLにてアスタリスク「*」の使用を避けましょう。

SQLのSELECT文ではアスタリスク「*」の使用を避ける、が鉄則です。

📚️技術的に不安があるときは

どんなに経験のあるプログラマでも、経験のない技術を使ったプログラムを書くことがあります。

そのような時には、「技術検証用のソースコード」を別に作って試すことが大事です。

本番用のソースコードを使って試行錯誤してしまうと、誤って試行錯誤した時のソースコードが残ってしまい、余計なバグを生み出してしまうことがあります。

試行錯誤したソースコードは、可読性の観点からも問題ありますので、ソースコードも「下書き」と「清書」を分ける習慣を持ちましょう。

今回、題材に選んだソースコードは、未経験の方が17日間で作成したものと説明がありました。

色々指摘を挙げましたが、作成者の名誉のために書いておくと、1ヶ月もかからず未経験者がこれだけのソースコードを書くことは通常出来ないと思います。

私が所属する会社にも毎年新卒者が入ってきて3ヶ月間の研修を受けますが、3ヶ月経ってもここまでのソースコードは書けない人がほとんどです。

きっと相当努力したのでしょう。その努力に敬意を表します。

プログラマとしての職を得て、有意義な経験を積んでいってくれることを祈ってます。

いかがでしたか。

プログラミングの学習をしている人からすると、そんなことを考えてプログラムを書くのか！？と驚くようなことばかりではないかと思います。

仕事としてプログラムを書く際には、本当に色々なことを考えなくてはいけません。

しかし、誰でも最初は初心者であり、とりあえず動くコードを書くことだけで精一杯です。

色々なプログラムを書き、様々な指摘を受けて、少しずつ学んでいくのです。

あなたにとって何か学びがあれば、スキ、コメント、フォローをお願いします。🙇‍♀️

#プログラムレビュー
#コードレビュー
#初心者
#Java
#Web
#SpringBoot
#良いプログラム
#プログラマ
#レビューア
#Hibiki