[Feature/#240] 팝업 관련 api 구현 #241
Conversation
There was a problem hiding this comment.
저희 성공에 대한 메시지를 각 api마다 다르게 내려주고있는데 이게 프로젝트가 커지면 엄청 많아질수도 있어서 성공인 경우에는 CREATED, OK 인경우만 나눠서 단순하게 내려준다고하는데 성공인 경우에는 굳이 어떤 것에 성공인지 메시지 없어도 될거같긴한데 혹시 각 api 마다 성공메시지를 따로 만들지않고 공통으로 쓰는거에대해 어떻게 생각하시나용?
There was a problem hiding this comment.
아 저도 최근에 하는 프로젝트에서 분기처리하는거보니까 상태코드만 보는 것 같더라고요.. 저도 동의합니다. 한번 싹 리팩토링 해볼까요?
|
|
||
| @PatchMapping("/{userId}") | ||
| public ApiResponse<InvisibleResponseDto> updateInvisible(@PathVariable Long userId, @RequestBody PopUpRequestDto popUpRequestDto){ | ||
| return ApiResponse.success(Success.UPDATE_POPUP_SUCCESS, popupService.updatePopupInvisible(userId,popUpRequestDto)); |
There was a problem hiding this comment.
RequestDto를 그대로 service에 넘겨주면 controller과 service가 분리되기보다 의존성이 높아진다고 controller -> service에 넘길때도 따로 dto를 만든다고해서 이런식으로 리펙토링 진행해보려고하는데 어떠신가요??
There was a problem hiding this comment.
오 이해했습니다. 좋은 생각인 것 같아요.! 덕분에 저도 요런 링크를 찾았습니당
https://ksh-coding.tistory.com/102
약간 이해한대로 요약해보면 현재 저희는 여기서 설명하는 1번 방법인
service레이어에서 requestDto를 컨트롤러에서 받은 그대로 사용하고 있는데, 2번 방법인 requestCommandDto라는 service레이어에서 사용할 때 service와 controller의 결합을 약하게 만들 수 있다는 것이라고 이해했습니다!
혹시 그러면 이 리팩토링을 어느정도 사용하시는 걸 생각하고 계시나욤? 일단 리팩토링을 적용하는 것이 저도 좋다고 생각하는데
전체적으로 사용한다고 하면 dto 관리측면이 많이 늘어날 수 있다는 단점이 있을 수 있을 것 같습니당
위 링크의 필자분은 처음에 들어온 requestDto와 컨트롤러에서 외부 api등을 호출해야해서 service와 controller레이어에서 원하는 requestDto 포멧이 바뀌는 경우에만 적용하는 것이 좋아보인다고 해요!
물론 의존성을 분리한다는 장점이 있는 것은 맞지만 단점과 비교했을 때 많은 이득을 볼 수 있을지 사실 체감이 잘 되지는 않는 것 같아서 여쭤봅니당..! (근데 시도 자체는 좋은 것 같아서 좋은 고민인 것 같아요!
There was a problem hiding this comment.
음 일단 기존꺼보다 새로운거 개발할때 적용해보려고 생각했는데 기존꺼는 어떻게 해야할지 잘 모르겠네용...
There was a problem hiding this comment.
기존꺼 리팩토링.. 어디부터 손대야할지 빡세긴하네용 ㅜ
|
|
||
| PopupInvisibleManager manager = findManagerWithUserIdAndPopup(userId, popup, popUpRequestDto.invisible()); | ||
|
|
||
| manager.updateInvisible(popUpRequestDto.invisible()); // 어차피 더티체킹시에 똑같으면 쿼리 안나가겠지? |
There was a problem hiding this comment.
트랜잭션안에서 엔티티값이 변경되지않으면 쿼리는 실행되지않을거같긴합니다...!
🚩 관련 이슈
📋 구현 기능 명세
📌 PR Point
엔티티 관련해서 현재 user - popupInvisibleManager-popup와 같이 중간테이블 관계로 되어있습니다.
popupManager에서 userId외에 유저 정보를 하나도 가지고 있을 필요가 없어보여서 userid는 엔티티로 안엮은 다음에
user삭제 로직에 → popupmanagerrepository.deleteByUserId() 한줄 추가할 생각입니당. -> 코드 상 고칠 거 없으면 추가 커밋으로 올리겠습니다.
popup과 popupmanager는 생명주기를 cascade설정을 통해 삭제되게 구현해봤고, 양방향 연관관계까지는 필요없어서
on_delete옵션으로 처리했습니다.📌 삭제 쿼리로 popup 제거시 manager 같이 삭제 확인함.
현재 컨트롤러부는 테스트를 위해 pathVariable로 해놨고, 추가 커밋에서 한꺼번에 엑세스토큰을 받아 userId를 반환하는식으로 수정하겠습니다.
어떤 부분에 리뷰어가 집중해야 하는지
저희가 패키지 구조를 원래 한번에 몰아넣는 식으로 구성했었는데, 이번 팝업과 같이 도메인 별로 분리해보는 것이 어떨까싶어서 팝업부분을 다음과 같은 예시로 해봤습니다..! 오히려 파일을 찾기 어렵다 판단되면 추가 커밋에서 구조 원래대로 옮겨서 푸시하겠습니다!
개발하면서 어떤 점이 궁금했는지
예외 상황이 없을까요? 일단 관리자 페이지를 나중에 만든다는 가정하에 서버단에서 생성, 삭제 api도 추가로 구현해놓는게 좋을지?
📸 결과물 스크린샷
{ "code": 200, "message": "팝업 정보 조회 성공", "data": { "popupList": [ { "id": 1, "image": "ddd", "active": true, "linkUrl": "dfsadf" }, { "id": 2, "image": "dddd", "active": true, "linkUrl": "dfsadf" } ] } }🛠️ 테스트
🚀 API Endpoint