【フルボッコ】コードレビューの振り返り

はじめに

プロトタイプ実装1週間ちょっとで行いましたが、レビューではボコボコにしていただきました。

感じたことは、前は指摘されなかったからこれでいいと思ってたけど違ったんや！！ってことが山ほど。今までのが逆に大丈夫だったかなと不安になるくらいでした。

言われてみるとホントや・・となったりすることも多く、もう少しコメント数減らせたなという思いもあります。

コードレビューについて

ありがたい限りの文化で、完全に教えて頂いてるということなので、ありがたいという気持ちと、出来てなくてごめんなさいの気持ちです💦

当然のことながらオリアプ開始したての方への指摘事項とタマネギ先生への指摘事項と私への指摘事項が同じわけがないと思うので、「難しいこと要求されている＝期待値が上がっている」ってことだよなと、自分自身の能力が上がってきたものだととらえています。

もし違ってもここは否定しないでください！！(切実)

こうしたら成立はするけどホントにこれでいいのだろうかって思うことが多々ありますし、コメントの数は私の伸びしろであることは間違いないので、ありがたや～ありがたや～と思いながら私は修正しています。

そもそもここはこうした方がいいと言われているだけで、「お前、ほんと使えないな！」と私の人格否定されてはいないわけなので(思われてるかもしれないけどw)、反省することは多々ありますが特に落ち込むことは全くなく。。

淡々と吸収していけばよいのではと思っています。

学んだこと

今回100はいかないと思いますが数えるのがめんどくさいと思うくらいの大量コメントを付けていただきました。

その中で、これは知らなかった！と思ったことをピックアップして振り返っていきたいと思います。

enum型データの使い方

回答のステータスデータを出力時には「下書き」「合格」「再提出」にしたくてその処理を行っていました。

スキーマで定義しているDB上の型

enum StatusType {
  DRAFT
  PASSED
  REVISION_REQUIRED
}

変換後の型

export type Status = "未提出" | "合格済み" | "再提出";

この変換を行う関数の書き方についてコメントいただいて修正しました。

レビュー前

import { StatusType } from "@prisma/client";
import { Status } from "../_types/Status";

export const status = (status: StatusType | null): Status => {
  switch (status) {
    case "DRAFT":
      return "未提出";
    case "PASSED":
      return "合格済み";
    case "REVISION_REQUIRED":
      return "再提出";
    default:
      return "未提出";
  }
};

レビュー後

import { StatusType } from "@prisma/client";
import { Status } from "../_types/Status";

export const status = (status: StatusType | null): Status => {
  switch (status) {
    case StatusType.DRAFT:
      return "未提出";
    case StatusType.PASSED:
      return "合格済み";
    case StatusType.REVISION_REQUIRED:
      return "再提出";
    default:
      return "未提出";
  }
};

このように使えることを知りませんでした。。

今まで似たような処理したことあったと思いますが、この書き方したのは初めてでした。

(status: StatusType | null): Status

ここで型安全担保してるから大丈夫だと思っていました。

複数のIDの組合せが一意になる場合にfindUniqueする方法

完全にしらなくて、すげぇぇぇそうなんやぁぁぁってなったやつです。

ユーザー1人に対して各問題のanswerテーブルのレコードは一意です。

でも、answerとquestionが1対1ではない。ユーザーの数だけanswerはある。というケースです。

const answers = await prisma.answer.findMany({
  　where: {
　　　 AND: [{ userId, questionId: parseInt(questionId, 10) }],
　　},
});

こんな処理をしていたのです。

これanswersの長さって絶対1になるのに、配列になっちゃうからちょっと嫌だったんです。

@@unique([userId, questionId])

これをanswerモデルに書くと

const answer = await prisma.answer.findUnique({
      where: {
        userId_questionId: {
          userId: userId,
          questionId: parseInt(questionId, 10),
        },
      },
    });

findUnique出来ちゃうんです。便利・・！！

引数をオブジェクトにする

データを取得するフックを作っていて、lessonIdという引数を受け取りたいのですが、

レビュー前

import { useFetch } from "./useFetch";
import { QuestionsResponse } from "../api/lessons/[lessonId]/_types/QuestionsResponse";
export const useQuestions = (lessonId:string) => {
  return useFetch<QuestionsResponse>(`/api/lessons/${lessonId}`);
};

レビュー後

import { useFetch } from "./useFetch";
import { QuestionsResponse } from "../api/lessons/[lessonId]/_types/QuestionsResponse";
export const useQuestions = ({ lessonId }: { lessonId: string }) => {
  return useFetch<QuestionsResponse>(`/api/lessons/${lessonId}`);
};

これで取得する側で何を渡せばよいかわかりやすくなりました。

const { lessonId } = useParams();
const { data, error } = useQuestions({ lessonId: lessonId as string });

以前ぶべさんがブログに書いていましたね。なるほどと思いながら使っていなかったですが、今後は絶対使おうと思いました！

おわりに

一旦再度レビュー依頼をしたので手待ち時間みたいな感じで書いてみました。(って書いていたら少し修正したらマージOK来ました～😭)

今回3つしか書いてないですが、この20～30倍の数のご指摘をいただいているので、また続編書こうと思います。

プロトタイプ実装、問題一覧と詳細画面だけの予定でしたが、ユーザー登録機能もやっぱりつくる！って言われたので、引き続き頑張ります💪