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

Feat [#13] 이력서 조회 API 구현 #15

Closed
wants to merge 15 commits into from
Closed

Conversation

chaentopia
Copy link
Contributor

🍀 작업한 내용에 대해 설명해주세요

🍀 어떤 것을 중점으로 리뷰 해주길 바라시나요?

  • 지금 작성한 modifiedAt과 createdAt의 LocalDateTime은 어떤 로직으로 주게 되는지 잘 모르겠습니다 하하..
  • 도메인과 DTO의 이름이 같아지게 됐는데 이렇게 진행해도 되는지 궁금합니다.

🍀 공통 작업 부분에 대한 수정 사항이 있다면 적어주세요

  • 에러 메세지 추가

🍀 PR 유형

어떤 변경 사항인가요?

  • 새로운 기능 추가
  • 버그 수정
  • 코드에 영향을 주지 않는 변경사항 (오타 수정, 탭 사이즈 변경, 변수명 변경)
  • 코드 리팩토링
  • 주석 추가 및 수정
  • 문서 수정
  • 파일 혹은 패키지명 수정
  • 파일 혹은 패키지 삭제

🍀 Checklist

  • 코드 컨벤션을 지켰나요?
  • git 컨벤션을 지켰나요?
  • PR 날리기 전에 검토하셨나요?
  • 코드리뷰를 반영했나요?

🍀 Issue

Resolved #13

@chaentopia chaentopia self-assigned this May 18, 2024
Copy link
Contributor

@geniusYoo geniusYoo left a comment

Choose a reason for hiding this comment

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

너무 고생하셨습니다 :)) 코멘트만 확인해서 리팩토링 진행해주시면 더 깔끔해질 것 같아요 ~~! 화이팅 👏👏

궁금하신 사항에 대해 답변드려보자면,

지금 작성한 modifiedAt과 createdAt의 LocalDateTime은 어떤 로직으로 주게 되는지 잘 모르겠습니다 하하..

JpaAuditing을 사용하고 있긴 한데요 ,,
원래 modifiedAt과 createdAt을 일일이 domain에 넣지 않고,
추상 클래스를 정의해 @EntityListeners(AuditingEntityListener.class) 어노테이션을 붙이고,
각 도메인 클래스가 이 추상 클래스를 상속받게 만들면 !!
이를 상속받는 모든 도메인 클래스에 생성과 수정 시각을 LocalDateTime으로 찍어주게 됩니다 :)
(근데 아직 안했어요) ㅎㅎ,,

dto 관련 질문은 코멘트로 남겨놨으니 확인해주세요 :) 👍🏻👍🏻🔥

}

@GetMapping("/{userId}")
public ResponseEntity<Resume> findResumeById(@PathVariable Long userId) {
Copy link
Contributor

Choose a reason for hiding this comment

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

여기서 반환 dto의 이름이 Resume로 하게 되면, 지금은 문제가 없을 수도 있지만 Resume의 다양한 형태로 보내게 될때 혼동이 생길 수 있어 주의해야 합니다 :)!!

Resume 보다는, findResumeResponse요런 이름이 더욱 적절해 보이네요!

@GetMapping("/{userId}")
public ResponseEntity<Resume> findResumeById(@PathVariable Long userId) {
return ResponseEntity.ok(resumeService.findResumeById(userId));
}
Copy link
Contributor

Choose a reason for hiding this comment

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

SuccessResponse도 같이 활용해서 Response를 날리면 좋을 것 같아요 :)

public Resume findResumeById(Long resumeId) {
return resumeRepository.findById(resumeId).orElseThrow(
() -> new BusinessException(ErrorMessage.USER_NOT_FOUND_BY_ID_EXCEPTION)
);
Copy link
Contributor

Choose a reason for hiding this comment

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

위에서 말씀드렸듯이, Resume 엔티티 자체를 반환하는 것이 아니라 위에 만들어두신 ResumeSearchResponse를 활용해서 반환하면 좋을 것 같아요 :)

또한, 여기서도 BusinessException 대신 NotFoundException 커스텀 클래스를 이용하면 좋을 것 같네요 🙃

Comment on lines +27 to +28
@GetMapping("/{userId}")
public ResponseEntity<Resume> findResumeById(@PathVariable Long userId) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

도메인과 DTO의 이름이 같아지는 건 흔히 발생할 수 있는 상황인 것 같아요! 이럴 때 이름을 바꾸기 애매하다면 패키지 구조를 달리하여 구별하는 것도 좋은 방법일 것 같아요. 예를 들어 dto 들만 모아두는 패키지를 만드는 것이요!

@chaentopia chaentopia closed this May 21, 2024
@chaentopia chaentopia deleted the feat/#13-resumeGET branch May 21, 2024 07:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feat] 내 이력서 조회 GET API 구현
3 participants