1조 레포 리뷰 남깁니다.

[lukeqwaszx]

안녕하세요. 1조입니다.

2조 레포 리뷰하면서 대부분 깔끔하게 작성되어 있어서 배우는 점이 많았습니다.
소셜로그인을 구글뿐만 아니라 카카오, 네이버까지 모두 완성도 있게 구현하시면서정말 수고 많으셨을 것 같습니다.
읽으면서 몇 가지 이슈를 추려봤습니다. 혹시라도 잘못 읽은 부분이나 이상한 지적 있으면 양해해주시면 감사하겠습니다.

1. Access, Refresh Token에 대한 처리

access, refresh 토큰을 JWT 내부 검증으로만 이루어졌을 때, 토큰 탈취 시의 보안 위험이나 로그아웃 구현 등을 어떻게 처리하실 계획인지 궁금합니다. Redis 컨테이너를 하나 더 띄워서 거기에 토큰과 이메일 인증 코드 같은 임시 정보를 저장하면 좋을 것 같습니다. 미리 설정해둔 유효기간이 지나면(JWT와 별도) 알아서 키-값 쌍을 삭제해주기 때문에 작성해두신 스케쥴링도 없앨 수 있습니다

2. DRY한 코드

몇몇 부분에서 DRY하지 않은 코드를 확인할 수 있었습니다. 예를 들어 EditMemoResponse, CreateMemoResponse 등의 DTO는 Memo와 동일하여, 그냥 Memo DTO를 바로 리턴하는 방식으로 구현하실 수 있을 듯합니다. 또한 UpdateMemoRequest, CreateMemoRequest에서도 UpdateMemoRequest의 memoId는 path paramete로 가져올 수 있기에, 사실상 동일한 DTO입니다. DRY하면서도 명료한 코드 작성을 원하신다면 typealias를 사용하셔도 될 것 같습니다. UpdateTagRequest와 CreateTagRequest 또한 동일합니다.

3. 모호한 네이밍

isAdmin()의 return type이 void이며 error throw를 하는 부분, WrongUserException이 tag의 Permission 관련된 부분이라는 것을 이름을 가지고는 쉽게 확인할 수 없습니다. 보다 디테일하게, checkIfAdmin()이나 TagForbiddenException(), TagNoPermissionException()등 네이밍을 추천드립니다.

4. Validation API를 통한 dto 이중 확인

DTO 정의에서

      @field:NotBlank(message = "snuMail is required")
      @field:Pattern(
          regexp = "^[a-zA-Z0-9._%+-]+@snu\\.ac\\.kr$",
          message = "snuMail must be a valid SNU email address.",
      )
      val snuMail: String,
      @field:NotBlank(message = "password is required")
      @field:Pattern(
          regexp = "^(?=.*[a-z])(?=.*[A-Z])(?=.*\\d)(?=.*[@#\$%^&+=!]).{8,20}$",
          message = "Password must contain at least one uppercase letter, one lowercase letter, one digit, and one special character.",
      )
      val password: String,

와 같이 하고, Controller parameter로

        @Valid @RequestBody request: SignUpRequest,

Valid annotation을 달아줄 수 있습니다. 이를 통해, 프론트엔드 단에서만 제약조건을 확인하는 것이 아니라 백엔드 단에서 또한 제약 조건을 더블 체크하실 수 있습니다.

5. 기타 사소한 부분들

• 함수 parameter가 너무 많으면, request dto를 service 단에 바로 넘기는 것도 좋은 방법입니다. controller가 담당하는 로직을 줄여주며 controller와 service의 tight coupling을 줄여줍니다.
• user를 업데이트하는 controller의 함수명이 getUsers로 되어 있습니다.
• social login에서 계정 생성되는 경우 201 Created 반환하는 것 고려해보시면 좋을 것 같습니다.
• MemoTags entity를 만들기보다, ManyToMany annotation을 이용하는 방법도 있습니다. tradeoff 고려하셔 결정하시면 좋을 것 같습니다.
• social, user는 package를 분리하시기보다 합치는 것이, 둘 다 유저라는 동일한 영역을 다루는 코드이기에 깔끔한 방법 같습니다. 너무 user 패키지가 복잡해진다고 느끼신다면 user.admin, user.normal과 같이 subdomain을 만드시거나, user.utils를 넣어 utils를 정리하시는 것도 좋은 방법이실 것 같습니다.

[kih00]

정성스러운 피드백 감사합니다! 말씀해주신 내용 전부 반영할 순 없더라도, 추후 프로젝트를 이어 나가게 될 때 JWT라든지 더블 체크 같은 요소들을 반영할 수 있도록 해보겠습니다.