[reviews] 全体

[sumiredc]

Comments

課題お疲れ様でした👍 せっかくなのでコードを見させてもらい、勝手ながら簡単に気づいた箇所にレビューさせていただきます

表記揺れ

句読点は揃えて置くと良さそうです https://github.com/LunaInsidious/chess-alpha/blob/511082c26bf36b5dd074762a0c4a1e06d91921cd/src/translation/locales/ja.json#L19-L26 https://github.com/LunaInsidious/chess-alpha/blob/511082c26bf36b5dd074762a0c4a1e06d91921cd/src/translation/%40types/resources.d.ts#L20-L27

"error": {
  "failed": "{{action}}に失敗しました。",
  "maxSelect": "{{selectedName}}は{{maxSelect}}つまで選択できます。",
-  "minMaxWrongNumber": "{{targetName}}は{{minNumber}}以上{{maxNumber}}以下の数値を入力してください",
+  "minMaxWrongNumber": "{{targetName}}は{{minNumber}}以上{{maxNumber}}以下の数値を入力してください。",
  "minWrongFormat": "{{targetName}}は{{minLength}}文字以上で入力してください。",
  "required": "{{requiredName}}を入力してください。",
  "requiredSelect": "{{requiredName}}を選択してください。"
}

厳格な比較

ここは !== で比較した方が良さそうに思いましたがいかがでしょうか？

https://github.com/LunaInsidious/chess-alpha/blob/511082c26bf36b5dd074762a0c4a1e06d91921cd/src/usecases/errors/handling.ts#L13

定数化

e.name と 比較している文字列ですが、 enum や const で定数化させておいてはいかがでしょうか？ https://github.com/LunaInsidious/chess-alpha/blob/511082c26bf36b5dd074762a0c4a1e06d91921cd/src/usecases/errors/handling.ts#L14 https://github.com/LunaInsidious/chess-alpha/blob/511082c26bf36b5dd074762a0c4a1e06d91921cd/src/usecases/errors/BadRequestError.ts#L5

Typo ?

ここは <br />っぽいですね https://github.com/LunaInsidious/chess-alpha/blob/511082c26bf36b5dd074762a0c4a1e06d91921cd/src/components/features/home/ui/modal.tsx#L17

複数のモーダルコンポーネントが定義されているようですが、ここは１ファイル１つのコンポーネントへ分割しても良さそうに思いました https://github.com/LunaInsidious/chess-alpha/blob/511082c26bf36b5dd074762a0c4a1e06d91921cd/src/components/features/home/ui/modal.tsx#L52

ファイル名（tsx）

PascalCase と そうでない表記が混在しているようなので、揃えて置くのが良いと思いました

凝集度

この辺りの処理は分量が多くなっており、どこで return が実行されたか見やすくするために、もう少し細分化して関数へ切り出しても良さそうですね クラス化してしまうのも良いと思います https://github.com/LunaInsidious/chess-alpha/blob/511082c26bf36b5dd074762a0c4a1e06d91921cd/src/domains/piece/common.ts#L55

コード全体を通して、しっかりと作られている印象でとても良いと思いました ファイル分割して整理されている印象もあり、その点も良いと思った一方、アーキテクチャの部分が少し定まっていないようにも感じました

( ^ω^)b ｽﾊﾞﾗｯ

[LunaInsidious]

レビューありがとうございます！

厳格な比較

こちらは TypeScript Deep Dive に従った意図的なものです。

Typo ?

これも意図的です。 https://htmlcss.jp/html/wbr.html これですね。できるだけ改行してほしくないけど横幅狭い時にはいい感じの改行になるようにここで改行してねーのやつです。

その他

その他は完全におっしゃる通りだと思います。ありがとうございます！後ほど修正します。 アーキテクチャは持ってきたテンプレートがClean Architectureベースだったけど、今回の実装でClean Architecture使ってテストも書いて…ってしてたら間に合わん！捨てよう！ってなったのがアーキテクチャごちゃついてる印象抱かせちゃったのかなぁって感じです

[sumiredc]

@LunaInsidious

こちらは TypeScript Deep Dive に従った意図的なものです。

なるほどなるほど、参考情報ありがとうございます 確かにこれだと 緩やかな比較 で良さそうですね👍

https://htmlcss.jp/html/wbr.html これですね。できるだけ改行してほしくないけど横幅狭い時にはいい感じの改行になるようにここで改行してねーのやつです。

これは知らなかったですね、ありがとうございます！ であればこちらも問題なさそうです👍

[sumiredc]

アーキテクチャは持ってきたテンプレートがClean Architectureベースだったけど、今回の実装でClean Architecture使ってテストも書いて…ってしてたら間に合わん！捨てよう！ってなったのがアーキテクチャごちゃついてる印象抱かせちゃったのかなぁって感じです

この辺りは課題という時間の制限があったことも起因してそうですね README や コミットメッセージ等からも、まずはカタチにすることを優先したという点が伝わってくるので全く問題ないと思います👍 むしろ、初期フェーズでのプロダクト・プロトタイプと考えて見れば、良い落ち着きどころだと思いました👏

[wf9a5m75]

== nullを使ってundefinedと nullを両方ともチェックすることを推奨します。一般的に2つを区別する必要はありません。

これは間違いではないですが、推奨はしません。 まず「===」は、JavaScript特有の比較演算子です。他言語から参加した人からすると「==」と「===」の違いは、一見しただけでは理解できません。

また「undedined」もJavaScript特有の値です。これも他言語から参加した人からすると、「nullとundefinedは何が違うだ～」となります。

そのような背景を踏まえると、「e == null」 は nullとundefindに対して trueとなりますが、他の条件式では「a === "3"」と書いてあり、「==」と「===」の違いがさらに分からなくなります。 これはバグを引き起こす要因になります。

なので基本的には「===」で統一しておくのが望ましく、どうしても使いたいならコメントをしておくのが良いです。

[wf9a5m75]

ご存知だとは思いますが、一応、書いておきます。

a = "3";
console.log(a == 3); // true

[LunaInsidious]

なるほど…他言語から参加した人の視点は考えられていませんでした…。 ただ、nullとundefinedや==と===との違いは調べればすぐに分かることですし、 調べれば「nullとundefinedは些細な違いしかないが、文字列型と数値型は扱いに大きな違いがあるからa === "3"は厳密等価で比較しているんだな」、と思ってもらえるのではないかなと思っています。

[wf9a5m75]

それは、他人の時間を奪っていませんか？

@LunaInsidious さんが他の全く知らない言語のコードを読まなければならないときを想像してください。 本当に些細なことでも、１つ１つ調べなければなりません。

過去に実体験として、全くObjective-Cを知らないのに、他人が書いた iPhoneのコードをその日のうちにデバッグしてください、と頼まれたことがあります。夕方6pm に。

すでに疲れ果てている 6pm に、全く知識のない Objective-C と iOS のコードを読んで、デバッグしなければならないストレスは想像できますか？ ちなみに、深夜までかかりました。

コードを書く人が、ほんの少しだけ他人に配慮をすれば、なんの苦労もなく読めるところを、敢えてJavaScriptの特性を使って書くことで、その誰かに5分、10分調べさせることが、どれだけストレスか想像したことがありますか？

それでも @LunaInsidious さんは、「いや、調べるのが正しい！」と言えますか？

[wf9a5m75]

もちろん、それぞれのプログラミング言語特有の書き方やコンセプトはあります。 それらを全く使わない、というのはないでしょう。 しかし、「他人を惑わす可能性がある書き方」は避けるべきでしょう。

チームワークをする上で大切なのは、「自分が分かる」＝「他人が分かる」ではない、ということを認識することです。

[wf9a5m75]

少なくとも TypeScript がある程度分かる @sumiredc さんは、 e != null に対して、その意図を読み解くことはできませんでしたが、どうでしょうか。

[LunaInsidious]

しかし、「他人を惑わす可能性がある書き方」は避けるべきでしょう。

これでなるほどとなりました。 そうですね。確かに知らない人から見たら惑わせる原因にしかならない書き方でした。 今から===に統一したら思わぬデグレが出る可能性があるのでこのリポジトリでは逐次コメントを入れる形で修正しようと思います。

[wf9a5m75]

関数に切り出すのはどうですか？

function isNullOrUndefined(value: any) {
  // `!=` で nullとundefinedが判定できる
  return value != null;
}

[LunaInsidious]

感動です！！！ 多分これが最適解ですね。(null or undefinedのときにtrueであるはずなので比較演算子は逆だと思いますが)

[wf9a5m75]

null or undefinedのときにtrueであるはずなので比較演算子は逆だと思いますが

あ、そうですね