Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[멘토님] 코드리뷰PR #43

Open
wants to merge 168 commits into
base: main
Choose a base branch
from
Open

[멘토님] 코드리뷰PR #43

wants to merge 168 commits into from

Conversation

jinakim8
Copy link
Collaborator

@jinakim8 jinakim8 commented Aug 23, 2023

  1. 테스트 기능 접근(현재 배포사이트 중지중)
  1. 하단내용은 깃 커밋히스토리입니다.
  2. 코드담당자마다 따로 코드리뷰에 대한 건의가 없어 코멘트를 달지 않은 상태,
    Issues가 세분화되어 있는 링크로 가서 보시는게 편하실거 같습니다..

dali186 and others added 30 commits July 24, 2023 16:54
공통 시간 entity 생성
암호화를 먼저 적용하면 헷갈릴 것 같아 주석처리 해뒀습니다
응답코드가 추가된 형태입니다
@jinakim8 jinakim8 changed the title Review [멘토님] 코드리뷰 Aug 23, 2023
@jinakim8 jinakim8 changed the title [멘토님] 코드리뷰 [멘토님] 코드리뷰PR Aug 23, 2023
@jinakim8 jinakim8 added the question Further information is requested label Aug 23, 2023
Copy link

@Jungdahee Jungdahee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

여러분 모두 미니 프로젝트 진행하시느라 고생 많으셨습니다.
따로 코드 리뷰에 대한 니즈가 없어서 제가 코드를 대략적으로 보고, 전체적인 리뷰를 진행하려고 해요!

먼저, 공통 응답에 대한 클래스를 util로 만들어 활용한 부분 잘하셨네요!
Exception도 다양한 상황에 맞게 잘 구성해주셨는데, 저라면 CustomException 클래스를 하나만 만들어서 그 안에서 HttpStatus를 활용하여 처리할 것 같습니다! 지금처럼 Exception 400, 401 등 별도의 클래스로 분리하여 운용하려고 한다면 status가 생기거나 삭제될 때마다 파일 개수가 수정이 되기 때문에 유지보수 측면에서 불리하게 작용할 수 있어요!

그리고 validation 체크 부분이 꼼꼼하게 잘 작성되어 있어 좋았습니다.

모두 수고 많으셨어요! 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested
Projects
None yet
Development

Successfully merging this pull request may close these issues.

관리자 기능 구현 개인 연차/당직 등록 조회 개인정보수정 로그인 회원가입
6 participants