PHP ローカルスコープ内で変数名を使い回すのはなるべくやめようと先輩から教わった

目的

• 先輩から教わったことをメモ的にまとめる

ご注意

• 本ソースは他のエンジニアの方が書いてくださったものです。
• ソースの設計思想はいくつか存在し、正解はいくつもあります。
• 本記事は筆者個人の意見であり、登場人物である先輩の考えも実際とは異なる可能性があります。
• 例として記載するソースは元のものからかなり修正をしております。

情報

• 下記の様なソースが記載されていた。

  public function foo()
  {
      $adminUserInfos = $this->userService->getAllAdminUserInfo();
      foreach($adminUserInfos as $adminUserInfo) {
          $user_id = $adminUserInfo->id;
          $user_name = $adminUserInfo->name;
          $this->mailService->sendMail($user_id, $user_name);
      }
  
      $userInfos = $this->userService->getAllUserInfo();
      foreach($userInfos as $userInfo) {
          $user_id = $userInfo->id;
          $user_name = $userInfo->name;
          $this->mailService->sendMail($user_id, $user_name);
      }
  }
  

先輩が言っていたことと筆者の初見の所感

• 「ローカルスコープ内で変数は使い回さないほうが良いかもしれない」
• どういうことかというと、上記のソースは１つ目のforeachで$user_idと$user_nameを定義して他メソッドの引数として使っている。
• 更に2つ目のforeachで$user_idと$user_nameを更に定義している。
• 筆者は問題ないと思ってしまった。何なら処理に一貫性があって良いソースだと思った。
• 先輩曰く「1つ目のforeachが終わった後に$userInfos = $this->userService->getAllUserInfo();エラーかなにかでスルーされてしまったら事故になる可能性がある」ということだ。
• 上記のソースに限っては$userInfosがnullなどだった場合、foreachが動作することは無い。
• しかしながら、先輩はこのソースに限った話ではなく、そもそも「ローカルスコープ内で変数名を使い回すと事故につながる可能性になるのでやめたほうが良い」と言っていると筆者は受け取った。
• 個人的に非常に学びになった。

どのように記載すると事故は防げるのか

• 今回のソースで言うと2個回避策があると思う。

• 下記に先輩が提案されていた回避策を記載する。

  public function foo()
  {
      $adminUserInfos = $this->userService->getAllAdminUserInfo();
      foreach($adminUserInfos as $adminUserInfo) {
          $this->mailService->sendMail($adminUserInfo->id, $adminUserInfo->name);
      }
  
      $userInfos = $this->userService->getAllUserInfo();
      foreach($userInfos as $userInfo) {
          $this->mailService->sendMail($userInfo->id, $userInfo->name);
      }
  }
  

• 先輩は変数に入れずにそのまま直接引数として使用する方法を挙げられていた。その後使う予定の無い変数を増やすより事故は減りそうである。

• 下記に筆者が考えた回避策を記載する。

  public function foo()
  {
      $adminUserInfos = $this->userService->getAllAdminUserInfo();
      foreach($adminUserInfos as $adminUserInfo) {
          $admin_user_id = $adminUserInfo->id;
          $admin_user_name = $adminUserInfo->name;
          $this->mailService->sendMail($admin_user_id, $admin_user_name);
      }
  
      $userInfos = $this->userService->getAllUserInfo();
      foreach($userInfos as $userInfo) {
          $user_id = $userInfo->id;
          $user_name = $userInfo->name;
          $this->mailService->sendMail($user_id, $user_name);
      }
  }
  

• 筆者は単純に変数を使い回さないようにした。ただそれだけ。

感想

• 先輩とソースを一緒に読むと非常に学びになることがわかった。
• どうにかして一緒にソースを読んで意見を出し合う時間を作りたいのだけれど、、、なかなか難しい。