-
Notifications
You must be signed in to change notification settings - Fork 6
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
feat-be: 지원서 수정 기능 구현 #266
Conversation
📌 Test Coverage Report
|
1722864831.806699 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
수고하셨습니다! 수정할 사항 없어보입니다.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
수고 많으셨습니다. 👍
간단한 코멘트 남겼으니 확인 부탁드립니다.
// then | ||
Optional<Applicant> changedApplicant = applicantRepository.findById(applicant.getId()); | ||
assertAll( | ||
() -> assertThat(changedApplicant).isNotNull(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
isPresent() vs isNotNull()
옵셔널이 비어있지 않은지 확인하려면 어느 것이 더 명확하다고 생각하시나요?
저는 전자요. ㅎ ㅎ 초코칩 생각도 궁금합니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
assertThat에서도 있는지 몰랐네요. isPresent는 Optional에 한정헤서 쓴다는 가정하에 있기에, 저도 전자가 가독성이 좋은것 같아요.
String toChangeName = "도비"; | ||
String toChangeEmail = "dev.dobby@gmail.com"; | ||
String toChangePhone = "010111111111"; | ||
applicantRepository.save(createApplicantDobby()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
음 save를 생략해도 된다고 생각하는데, 어떻게 생각하시나요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
필요없는 로직이네요. 삭제했습니다.
작업 내용
관련 이슈
PR 체크리스트