-
Notifications
You must be signed in to change notification settings - Fork 1
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 [#10] 이력서 등록 POST API 구현 #12
Conversation
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.
전체적인 로직 구성은 잘하셨습니다 :) 가독성도 괜찮구요!
제가 체크한 몇가지 부분들은 다 코멘트로 남겨놓았으니, 적용해서 리팩토링하면 훨씬 깔끔한 코드가 될 것 같아요 !! 너무너무 고생하셨습니다 👍🏻👍🏻
모르겠는 부분은 언제든지 물어봐주세요 :) 화이팅!!!
import java.net.URI; | ||
|
||
@RestController | ||
@RequestMapping("/api/v1/resumes") |
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.
/api/v1
은 Controller 레벨에다 두고 /resumes
는 메소드 레벨로 낮추는 것이 좋을 것 같습니다 :))
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.
구체적으로 말씀드려보면,
@RequestMapping("/api/v1")
...
@PostMapping("/resumes")
public ResponseEntity<SuccessResponse> createResume
요렇게 수정해보면 어떨까요 :)!
return ResponseEntity.ok() | ||
.build(); | ||
} | ||
} |
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.
Controller를 잘 구성해주신 것 같은데요! 조금만 개선하면 더 좋은 코드가 될 것 같아요 :)
ResponseEntity<SuccessResponse>
보다는 반환값이 void라면 ResponseEntity<SuccessResponse<Void>>
로 반환값을 명확하게 지정해주면 좋을 것 같구요 !
이력서를 생성하는 API이기 때문에 201 Created 를 반환해야 합니다 !
추가적으로, 201 Created는 Request Header에 생성된 이력서의 pk값(기본키, 여기서는 id 값이겠죠?) 을 담아서 응답해줘야 합니다!
이 두가지를 한번 리마인드 해보시면서 리팩토링 해주면 더 멋진 코드가 될 것 같아요 🙂
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.
저도 같은 생각입니다! 😊
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.
궁금한 게 있는데 그렇게 되면 클라 단에서는 어차피 GET API를 사용해서 이력서 리스트를 받게 되는데 id 값을 반환해줘야 하는 이유를 알 수 있을까요!?
그리고 이렇게 바뀌게 되면 명세서의 Response 부분을 바꿔야 하는지도 궁금합니다!
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.
DB에 엔티티를 등록하는 API에서는 💡
RESTful API 설계 원칙에 따라, 새로운 리소스를 생성할 때는 201 Created 상태 코드와 함께 Location 헤더를 포함하는 것이 관례입니다! 이로써 API 사용자가 생성된 리소스의 위치를 쉽게 알 수 있도록 합니다 🙂
또한, 채은님의 질문처럼 명세서의 Response 부분이 수정되어야 하겠네요!
Response Body는 바뀌지 않지만, Header에 id값이 같이 전달된다는 것을 명시해야겠네요 :)
좋은 질문으로 한 단계 탄탄해지는 우리의 API 명세서 .,, 굿 ,,👍🏻
private final ResumeRepository resumeRepository; | ||
private final UserRepository userRepository; | ||
|
||
@Transactional |
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.
Transactional 키워드 좋네요 :)!!
) { | ||
User findUser = userRepository.findById(resumeCreateRequest.userId()).orElseThrow( | ||
() -> new BusinessException(ErrorMessage.USER_NOT_FOUND_BY_ID_EXCEPTION) | ||
); |
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.
Exception 처리를 적절하게 해주신 건 아주 좋은 포인트입니다!
하지만, BusinessException은 상위 Exception 클래스이기 때문에 NotFoundException 커스텀 클래스를 이용해 핸들링해주면 더 좋을 것 같아요 :)!
() -> new BusinessException(ErrorMessage.USER_NOT_FOUND_BY_ID_EXCEPTION) | ||
); | ||
Resume resume = Resume.create(findUser, "내 이력서"); | ||
resumeRepository.save(resume); |
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.
Controller 에서 말씀드렸듯이, id 값을 같이 반환해주는 로직을 마지막 여기에 추가해주면 되겠죠 ? 😆🔥
🍀 작업한 내용에 대해 설명해주세요
Jumpit-SERVER/jumpit/src/main/java/org/sopt/jumpit/resume/service/ResumeService.java
Lines 20 to 29 in bc4f323
Jumpit-SERVER/jumpit/src/main/java/org/sopt/jumpit/resume/domain/Resume.java
Lines 33 to 38 in bc4f323
Jumpit-SERVER/jumpit/src/main/java/org/sopt/jumpit/resume/controller/ResumeController.java
Lines 23 to 30 in bc4f323
🍀 어떤 것을 중점으로 리뷰 해주길 바라시나요?
🍀 공통 작업 부분에 대한 수정 사항이 있다면 적어주세요
🍀 PR 유형
어떤 변경 사항인가요?
🍀 Checklist
🍀 Issue
Resolved #10