フルスタックエンジニアのノウハウ
2021.02.06    2021.12.14

「添削」ソースコードレビューの様子を共有します。ノウハウを吸収せよ!

この記事の動画版はこちら(画像クリックでYoutubeに飛びます)

チャンネル登録お願いします!


今回は、僕が日々講座の受講生さんに対して行っているソースコードレビュー(添削)の様子を少し共有したいと思います。


講座の上級レベルの中で作る課題アプリの一部分なので、少し分かりづらい部分もあるかと思いますが、ソースコードを書くときに、こういう所に気を付けた方が良いという細かいテクニックなどが伝わればと思いますので、是非参考にしてみてください。



機能の概要


まずは、「どんな機能を作っているのか?」ということを簡単にご説明します。


今回添削するソースコードは、ユーザーがデータベースに登録した「1行メモ」を、毎日ランダムに1件抽出して自動的にメールで送ってくれるサービスのバックエンド処理部分です。




データベースには、ユーザー情報を保存している「user」テーブルと、それぞれのユーザーの1行メモを保存している「item」テーブルがあります。


userテーブルには、ユーザーの名前やメールアドレスの他に「メールを何時に送って欲しいか」という送信希望時間が登録されています。




このプログラムは、1時間に1回自動的に実行され


①まず、その時間にメールを送るべきユーザーを「user」テーブルから抽出
②次に、そのユーザーの1行メモを「item」テーブルからランダムに1件抽出
③最後に、抽出した1行メモをユーザーにメール送信


これを、対象ユーザー分ループして処理する、といった流れです。



受講生さんのソースコード


では、受講生さんが書いた実際のソースコードを見ていきましょう。




上から見ていくと、


①まずは、必要な外部ライブラリをインポートして、データベースにコネクションを張っています。


②次に、そのデータベースコネクションを使って、「user」テーブルから全ユーザーのデータを取得し、「$data」という配列に入れて、foreachでループしています。


③ループの中では、まずは「そのユーザーが送信対象ユーザーかどうか?」を判断するために、if文を使ってチェックしていますね。

ユーザーの送信希望時間は「delivery_hour」というカラムに登録されていますので、その値と現在の時刻を比較しています。


④送信対象者だった場合は、「item」テーブルから、そのユーザーの1行メモをランダムに1件取得しています。


⑤そして、取得したデータを「$data」という変数に入れて、ループを終了。


⑥データベースコネクションを切断し、最後にメール送信処理を書いています。


このような感じですね。


こちらのソースコードは、全体的な流れや処理の組み立て方としてはとても良く書けており、完成まであと1歩というところなんですが、いくつか問題のある部分と、もっとこうした方が良いという部分があるので、その辺りを見ていってみましょう。



ソースコード添削


まず、問題のある部分は「2箇所」あります。


1つ目は、送信対象のユーザーが決まった後、そのユーザーのitemをランダムに1件取得している部分です。



ランダムに1件取得するのは、この「ORDER BY RAND() LIMIT 1」という指定で良いんですが、これだと、そのユーザーのitemだけでなく、itemテーブル全体からランダムに1件取得してしまっています。


別のユーザーが登録したitemが取得されてしまう可能性があるということですね。


これは、以下のようにして、WHERE句でそのユーザーのitemだけに絞り込んでから、そこからランダムに1件取得する必要があります。



2点目は、最後にメールを送信している部分ですね。




この の処理がループの外側に出てしまっていますが、これだとループが全て終了した後、一番最後のユーザーにしかメールが送信されません。




メール送信処理は、このようにループの中に入れて、各ユーザー毎にメール送信を行うようにしてあげる必要がありますね。




これで、動作上は問題なく動くと思います。


次に、より良いソースコードにするために「もっとこうした方が良い」という部分を添削していきます。


まずは、最初のuserを取得している部分で、全ユーザーを一旦取得して、次にif文で対象を判定していますが



ここはWHERE句で絞り込んで、そもそも送信対象者だけを取得するようにした方が無駄がなく良いです。


 


また、取得したデータを「$data」という配列に入れていますが、このような変数名だと、中にどんなデータが入っているのか、単数なのか複数なのかがパッと見で分かりづらいです。



正しい変数名の付け方の動画でもご説明していますが、データベースから取り出したデータを入れる変数名は、そのテーブル名などを付けると分かりやすいのでお薦めです。


関連記事

正しい変数名の付け方


また、複数件のデータが入っている場合は、「_list」などそれが連想出来る名前を付けましょう。


今回の場合は、$user_listとすると良いですね。




そして、ループして1件ずつ取り出す際は、「$user」とすれば、このように「user」テーブルの「id」カラムを取り出しているんだ、ということがパッと見で分かりやすいですよね。




こちらの変数名も分かりやすく変更しておきましょう。




itemテーブルから1件のデータ(単数)を取得しているので、$itemと付けると分かりやすいと思います。



あとは、このメールを送信する処理の前に、itemが取得出来たかどうかの判定を付けると良いですね。


ユーザーがitemを1件も登録していない場合など、データが取得されない可能性もあるため、正常に取得出来た場合のみ、メール送信処理を行うようにすれば完璧です。


このソースコードの添削はこんなところですね。


ちなみに、こういったメール送信を行うようなプログラムを作る際は、実際にメール送信テストを行う前に、正しいデータが取得出来ているかどうかをデバッグして確認しましょう。


このように、メール送信処理の部分をコメントアウトして、送信しようとしている文面を画面にデバッグ出力します。




そして、このプログラムをブラウザから直接実行すると、画面に表示して確認することが出来ます。


確認して問題なければ、次に実際のメール送信をテストすると開発作業がスムーズだと思います。



まとめ


ということで今回は、講座のソースコード添削の様子をご紹介しました。


今回のソースコードは上級カリキュラムの課題となるため、内容は少し難しかったですが、講座では、未経験からプロレベルに到達するまで、1つずつステップアップしながら学んでいきます。


本気でプログラミングスキルを身につけていきたい方は、バナーの方から是非ご参加ください。


おすすめ記事