駆け出しエンジニアが既存のコードを読んで追加実装をするときに気をつけたいこと

はじめに

実務でコードを書いていると、こういうもの！と型のように覚えてしまっているものが結構あります。
今回は、意識せずになんとなくで覚えて書いてしまっていたよくある実装で、意図しない動きになってしまったことで「そういうことなんだ！」と納得することができたので、共有したいと思います。

この記事で伝えたいこと・結論

既存のコードを真似して書くと実装スピードは上がるが、その分考える機会が減り、理解が深まらない。
理由を考えながら実装することが大切！

具体例で説明

背景

プロダクトの海外対応に向けて、住んでいる国を選択できるようにする。
国を選択するselectボックスを実装することになった。

この実装の目的

国名を選択したら、言語表示が切り替わるようにしたい。

設計

• 国名はCOUNTRY_LIST: {code: string; countryName: string; }[]という型の配列が実装済みなので、それを使う
• 国名の選択はselect要素を使う
• 国名を選択するときに、formValueというstateを更新する
• 更新するstateはformValueのcountryCodeとlanuguage
• 日本が選択されたら、言語は日本語にする。それ以外の国が選択されたら言語を英語にする

実装

最初に私が書いたソースコードです。
国を選択するselect要素のonChangeイベントで、formValueというstateを更新できるように書きました。

<select
  value={formValue.countryCode}
  onChange={(event) => {
    setFormValue({
      ...formValue,
      countryCode : event.target.value,
      language : formValue.countryCode === 'JP' ? Launguage.jp : Language.eng,
    });
  }}
>
  {COUNTRY_LIST.map((opt) => (
    <option value={opt.code} key={opt.code}>
      {opt.countryName}
    </option>
  ))}
</select>

問題発生

これで実装できた！と思ったのですが、想定外の動きになってしまいました。

要件通り、日本を選択したら日本語表示、日本以外を選択したら英語表示にしたかったのですが…

• 日本以外を選択しても英語表示にならない
• 次に日本を選択すると英語表示になる
• その次に日本以外を選択すると日本語表示になる

つまり、1つ前で選択した国によって言語が切り替わるようになってしまいました。

何が原因なのでしょうか。
Reactを触ったことがある方なら、すぐにお分かりになるかもしれません。

残念ながら私は自力では気づくことができず…😞
メンターさんに質問しました。

原因

結論からいうと、表示したい言語を判定するロジックに更新する前のstateを使っていることが原因でした。
わかりやすくするために、問題の箇所だけ確認します。

onChange={(event) => {
    setFormValue({
      ...formValue,
      countryCode : event.target.value,
      language : formValue.countryCode === 'JP' ? Launguage.jp : Language.eng,
    });
  }}

languageのバリューを見てください。
三項演算子の条件がformValue.countryCode === 'JP'となっています。

一見良さそうなのですが、これでは比較する時に、更新する前(1つ前)のstateを参照してしまうことになります。
そのため、1つ前に選択した国にあてはまる言語に変わってしまうという、予想外の動きになってしまっていたのでした。

イメージしにくかったのですが、setFormValueする時に最初にformValueオブジェクトをスプレッド構文で展開している理由を考えるとわかりやすいと思いました。
formValueをスプレッド構文で展開するのは、更新するstateで上書きするためです。
つまり、展開されたformValueは、まだ更新していないstate、1つ前のstateということです。

ではどうすれば良いのか。
選択された国によってlanguageを切り替えたいので、ユーザーがそのとき選択した国によって判定すれば良いことになります。それはつまり、event.target.valueです。

解決

以下が修正したコードです。
ついでにDRY原則に則って、共通するcountryCodeを変数に切り出しました。

onChange={(event) => {
    const countryCode = event.target.value;
    setFormValue({
      ...formValue,
      countryCode,
      language:
        countryCode === 'JP' ? Language.jp : Language.eng,
    });

これによって、日本を選択した場合は言語が日本語、日本以外を選択した場合は言語が英語という要件を満たすことができました。

学んだこと

• onChangeなどのイベントでは、実行されて初めてstateが更新される
• イベントの中でstateを参照するときは、いつの時点のstateなのかを意識する！

終わりに

実務でコードを書くとき、すでにプロダクトは動いているものなので、大体はどこかで同じような実装がすでにされていて、それを参考にして書いています。
言い方を悪くすると、コピペです。
わざわざ自分でロジック考えるよりも、先輩エンジニアが書いたコードの方が良いコードのはずですし、速いですから…

しかし、コピペしまくるのはよくないですね。
なんとなくでコードを書いてしまって、考える機会を減らしてしまいます。

今回の実装もこういうもの！と型のように覚えてしまっていて特に意識していなかったところですが、自分でコードを書いて間違えることで、なぜそう書いてあるのかを気づくことができました。

理由がわかって意識して書くのと、なんとなく書いてしまうのとでは、理解度に大きな差が出ると思うので、今後も考えながら実装することを忘れないようにします。