エンジニアのコードレビューで指摘された事に対しての振り返り

まえがき

「株式会Ancar ~Advent Calendar 2019~」 20日目を担当させていただきます。

中古車の個人間売買をオンラインで行うサービス
全国の中古車からお買い得な車を検索・比較できるサービス を展開しております。

軽い自己紹介

株式会AncarのWebエンジニアをしている、keiと申します。
私は以前、メーカーで電気設計エンジニアとしてインフラ整備を行っていました。
主な業務内容は、フローチャートに示す処理手順にしたがって処理を行うようにシーケンス図プログラムを書いていました。
そこから現職に転職をしました

当記事の内容

社内のコードレビューで先輩方からご指摘いただいた一部をまとめました。
主にLaravel、Symfonyのコードです。

本記事の修正前のコードは、実際に自分が書いたイケてないコードです。(一部簡略化)
失敗を次に活かすためにも、この場をお借りして書いていきたいと思います！
どんな仕事をする上でも当たり前の事ですが、同じ失敗は二度としないようにします！！

行ったことの一部を紹介

台数のカウントが0の車両は表示しないコマンド作成
Dockerで環境構築する際にテーブルに初期値が設定された状態で、環境構築できるように設定
ソート　（お買い得度、価格順、走行距離順、年式順）
ラジオボタンでAT／MT/指定なしを選択でき、ミッション種別で検索
年式、走行距離、価格のソートでnullのデータを最後にもってくる
ページ切り替え時にソートクエリストリングを引き継ぐ
デザイン修正　ソート＆ページネーション
お買い得度外れ値のロジック
Sentry設置&テスト
コマンド取得時間の短縮
コマンド作成
売却予想価格の査定担当は、かんたん査定の係数を調整できる
カバレッジエラー の対応
マスタースレーブDBによる切り分け
絞り込み検索
Cognito

確認漏れによる失敗

migrationsファイルのdownメソッドで元の型にそぐわない定義をしてしまった
今回の場合、downメソッドではカラムにNULL値を許容してはいけなかったのです。（既存のテーブル構造をしっかりと確認していなかったのがミスの要因となりました）

変更した点

• nullableの引数にfalseを記載することで、NULLを許容しない形にしました。

####修正前

   public function down()
   {
       Schema::table('items', function (Blueprint $table) { 
           $table->unsignedInteger('price')->nullable()->change(); 
           $table->unsignedInteger('odd')->nullable()->change(); 
       });
   } 

####修正後

   public function down()
   {
       Schema::table('items', function (Blueprint $table) { 
           $table->unsignedInteger('price')->nullable(false)->change(); 
           $table->unsignedInteger('odd')->nullable(false)->change(); 
       });
   } 

些細な事に気を配れずにした失敗

####修正前
デザイン修正の際に余分なスペースが入っていた
インデントがおかしい

####修正後
拡張機能を使ってkillしました！
ex: http://baba-s.hatenablog.com/entry/2016/04/26/100000
お恥ずかしい話、ご指摘前は拡張機能の存在も知りませんでした

npm run scss-lint をするとlintが起動してコーディング指摘をしてくれるのもこの際に教えていただきました！

Mysqlに対して思いやりがなかった事による失敗

####修正前
maxとminの値を取ってくる処理で、yearとpriceとoddそれぞれに対して行っていた。
つまり3カラム×2=6回クエリを発行していたのです。

 $maxYear = Item::where('id', $item->id)	                    
                  ->where('week', $lastWeek)	                    
                  ->where('year', '<=', date("Y"))	                
                  ->max('year');	    
 $minYear = Item::where('id', $item->car_model_id)	                    
                  ->where('week', $lastWeek)	                
                  ->where('year', '<=', date("Y"))	                
                  ->min('year');	            

####修正後
6回クエリを発行していたのが、1回のクエリに抑えることができました！
ただ、rawメソッドはクエリを文字列として挿入するため、SQLインジェクションの脆弱性を生まないように気をつけたいですね

$getMinMax = Item::where('id', $item->id)
                    ->where('week', $lastWeek)
                    ->where('year', '<=', date("Y"))
                    ->select(\DB::raw("MAX(price) AS max_price, MIN(price) AS min_price, MAX(year) AS max_year, MIN(year) AS min_year, MAX(odd) AS max_odd, MIN(odd) AS min_odd"))
                    ->first();

エラーケースを考慮できていなかった事による失敗

ユーザープールに存在しないユーザーが退会するケースで、エラーになっていました。
if文を１行追加することで、修正後はユーザープールに存在しないユーザーに対して、Cognitoへ退会リクエストがされないようにしました。

####修正前

$cognitoUserName = current($cognitoUser['Users'])['Username'];
$client->adminDeleteUser([
      'UserPoolId' => $this->container->getPrameter('cognito_user_pool_id'),
      'Username'   => $cognitoUserName,
]);            

####修正後

if (!empty($cognitoUser['Users'])) {
　　$cognitoUserName = current($cognitoUser['Users'])['Username'];
　　$client->adminDeleteUser([
   　　   'UserPoolId' => $this->container->getPrameter('cognito_user_pool_id'),
      　　'Username'   => $cognitoUserName,
　　]);
}            

最も多かったミス

今回全てご紹介することは出来なかったですが、全体を通して目立ったミスは、エラーケースを全て考慮できていなかった事による失敗が最も多かったです。

考えられる要因

• エラーケースを考えながら、実装を行っていたのが１つの原因だと感じました。

失敗によって得られたもの

人によっては、エラーケースを考えながら実装することは容易かもしれませんが、私には事前にエラーケースを全て把握した上で、実装に取りかかることが必要でした。

何より考えながら実装すると、その分工数もかかってしまうので、現在はコメントアウトして、どんなコードを書く必要があるか全てメモしてから書くようにしています。

そして、テストコードの重要性を知りました。

これからの事

漠然と書いてもいいコードは書けないので、すぐに手をつけず一度整理してから取りかかることで、より工数を減らし、社会に影響を与えることができるコードを書いていきたいと思います。
そして、同じ失敗は２度しないように仕組み化していきたいと思います。