実録！あるある！ソースダメポイントはどこ！！？

StudioZ Advent Calendar 8日目の記事です。

最近、日本の若者の読解力が低くなってきてるそうです。
※日本の15歳読解力が世界15位に～ネットリテラシーを子どもにどう教えるか(ねとらぼ)

国語の「作者の気持ちを答えよ」みたいな問題、苦手な子が増えているのかな。

エンジニア育成やソースレビューをしていると「想像力が足りてないねぇ！」と思う事がときどきあります。
今日はそんな例をクイズ形式で。
プログラムの気持ちになって読解力を鍛えてみてください。

問題1：下記プログラムに共通する問題点を答えよ

例①

$data = CardDb->getCardData( $cardId );
$name = $data['name'];

例②

$cardList = Deck->getCardList($deckNum);
foreach ( $cardList ) {
  //なんかの処理
}

答え

• データがなかった時の考慮がされていない
• 例①は$data['name']にアクセスした時にwarning
• 例②はnullをforeachで回そうとしてwarning

こうしましょう

• 外から受け取ったものは存在しないケースを想像しよう
• 存在チェックやエラーチェックするの大事

例えばこう書く

$name = "????"; //デフォ値を入れておく
$data = CardDb->getCardData( $cardId );
if ( $data ) { //データが存在すれば
  $name = $data['name'];
}

$cardList = Deck->getCardList($deckNum);
if ( $cardList ) {
  foreach ( $cardList ) {
    //なんかの処理
  }
}

問題2：下記プログラムに存在する問題点を答えよ

public function getStageSetting( $type, $stageId ) {
   if ( $type == TYPE_NORMAL) {
      $settingData = Stage->getSettingData($stageId);
   } else {
      $key = __METHOD__."_".$stageId;
      $settingData = Cache->read($key);
      if (empty($settingData)) {
         $settingData = Event->getSettingData($stageId);
         Cache->save($key, $settingData);
      } 
   }
   return $settingData;
}

答え

• 無駄なifの入れ子はやめよう
• 後続処理があるわけでもないのでNormalならデータとったら早期リターンしたほうが保守性が高い
• 単純に見づらい
• 常に「Normalでない」が意識スタックに必要になる(早期で抜けても同じなんだけど)

こうしましょう

• 早期リターンを使おう

例えばこう書く

public function getStageSetting( $type, $stageId ) {
   if ( $type == TYPE_NORMAL) {
      $settingData = Stage->getSettingData($stageId);
      return $settingData;
   }
   $key = __METHOD__."_".$stageId;
   $settingData = Cache->read($key);
   if (empty($settingData)) {
      $settingData = Event->getSettingData($stageId);
      Cache->save($key, $settingData);
   } 
   return $settingData;
}

この程度のソースだとやる必要ない感じだけど、1000行くらいになってくると、
早期リターンの有無はボディーブローのように効いてきます

問題3：下記プログラムに存在する問題点を答えよ

public function getTotalRecord($eventId) {
	$rankingListAll = $this->selectRankingList($eventId)
	$totalRecord = count($rankingListAll);
	return $totalRecord;
}

public function selectRankingList($eventId) {
	$rankingList = RankingDb->select($eventId);
	return $rankingList;
}

答え

• phpのメモリの気持ちになって！
• 欲しいのはcountだけ。
• Listを全件拾って覚えておくコストが無駄

こうしましょう

• count(*)でいいじゃん

例えばこう書く

public function getTotalRecord($eventId) {
	$options['select'] = 'count(*)';
	$totalRecord = RankingDb->select($eventId, $options);
	return $totalRecord;
}

問題4：下記プログラムに存在する問題点を答えよ

foreach ($deckList as $deck) { //1デッキに5体
	$eventDeckDb->updateLockFlg($eventId, $userId, $selectDeckNum, $lockFlg);
}

答え

• 行ごとにupdateする必要なくない？
• 5回updateクエリがDBに対して発行される

こうしましょう

• eventId + userId + deckNumでまとめて複数行updateすればsqlは1発だけでいい

問題5：下記プログラムに存在する問題点を答えよ

const SPECIAL_QUEST_FLG = true;
const NOT_CLEAR_FLG = true;

答え

• 定数名のつけかた
• デフォルト値がどっちなのか実数みないと伝わらない

こうしましょう

const IS_SPECIAL_QUEST = true;
const IS_CLEAR = false;

booleanならisが使いやすい。
あと否定フラグはバグの元なので肯定形でOFFにしましょうね・・・

問題6：下記プログラムに存在する問題点を答えよ

$shopList　= Shop->openShop();
if ( empty($shopList)) {
   return [];
}
$itemList = [];
foreach($shopList as $shopData) {
   $shopItemList = item->selectShopItemList($shopData['shopId'];
   if ( empty($shopItemList ) {
      continue;
   }
   itemList = array_marge($itemList, $shopItemList);
}
return $itemList

答え

• N+1問題というやつです
• Railsライブラリ紹介: N+1問題を検出する「bullet」
• 開いているshopの数だけ紐づく商品を拾うSQLが発行されます
• 1000件shopがあれば1000回商品問い合わせにいきます
• in句を使えばSQL一発でとれる(やたら長いinになるが)

こうできる

$itemList = [];
$shopList　= Shop->openShop();
if ( empty($shopList)) {
   return [];
}
$shopIds = [];
foreach( $shopList as $shopData ){
   $shopIds[] = $shopData['shopId'];
}

$itemList = item->selectShopItemListByShopIds( $shopIds ); //Idsをinでselectするメソッド
return $itemList;

まとめ

というわけで、うちの若手につっこんだレビューのコメントを元に記事にしてみました。
DBの気持ち、プログラムの気持ちになる読解力があると負荷やコストも少なくできるので、
やっぱりエンジニアもごんぎつねを1000回くらい読むべきだと思いました。