-
Notifications
You must be signed in to change notification settings - Fork 0
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] 북토크 개설 관련 API 구현 #36
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.
DTO 부분은 사람마다 견해가 다르긴 한데 일단 저는 웬만하면 분리하는게 좋다고 들었습니다!
코드의 가독성(명확성)과 유지보수 측면에서 장점이 있다 해서요!
그 외에는 하나의 공통된 Dto 내에 Inner Class를 만들어 관리하는게 있는데 그거는 논의해보고 정하면 좋을 것 같아요~
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.
아 그리고 이거는 추가로 dto 폴더링 할 때 고민했던 건데 도메인과 관련된 dto는 도메인 폴더 아래에 dto에 폴더를 파서 넣고, 그 외의 것들은 Controller 등 해당 계층 내 dto 폴더에 두는게 좋을 것 같은데 어떻게 생각하시나요?
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.
우선 저도 북토크로 작업해야하니 승인하고 merge하겠습니다~
추후 수정내용은 다시 refactor 해주세요!
그리고 TODO가 나중에 해야할 일 이라는 의미로 쓰인건지도 개인적으로 궁금했습니다 :)
@Transactional | ||
public BooktalkCreateResponseDto createBooktalk(BooktalkRequestDto booktalkRequestDto) { | ||
Place place = getPlaceById(booktalkRequestDto.getPlaceId()); | ||
//작가인지 확인할 필요가 있는지? |
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 | ||
public BooktalkUpdateDto updateBooktalk(Long booktalkId, BooktalkUpdateDto booktalkUpdateDto) { | ||
Booktalk booktalk = getBooktalkById(booktalkId); | ||
if (booktalkUpdateDto.getPlaceId() != booktalk.getPlace().getId()) { |
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.
이거는 장소 업데이트로 이해 했는데,
- 북토크의 기존 정보도 Dto에 같이 넘어오는 구조인지?
- 그렇다면 장소가 기존과 달라지는지를 체크 안 하고 그냥 장소 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.
- 기본 정보도 같이 넘어오는게 맞습니다!
- 오 그렇네유 ㅋㅋㅋㅋ 수정하겠습니다!
public BooktalkDeleteResponseDto deleteBooktalk(Long booktalkId) { | ||
Booktalk booktalk = getBooktalkById(booktalkId); | ||
//TODO soft delete? | ||
//공간이 거절 됐거나 공간 매칭중일 때만 삭제가능 |
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.
z클라 단에서 북토크 상태를 보고 삭제 버튼을 안보이게 한다고 해서 delete 요청 자체가 안들어오게 될거 같아서 따로 처리를 안했습니다!
DTO 부분 제외하고 코드리뷰 반영했습니다! |
📌 관련 이슈
close #33
✨ 과제 내용
궁금증