-
Notifications
You must be signed in to change notification settings - Fork 5
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
[Refactor] - 여행 계획 Service 코드 리팩터링 #507
Conversation
Test Results 32 files 32 suites 8s ⏱️ Results for commit 4da1253. ♻️ This comment has been updated with latest results. |
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 빠진 곳이랑 사용 안하는 의존성 삭제 요청이 있어서 RC 드려요.
파이팅입니다
} | ||
|
||
@Transactional | ||
public void updateTravelPlanById(Long id, MemberAuth memberAuth, PlanRequest planUpdateRequest) { |
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.
void 관련해서 이 코멘트랑 동일한 얘기 한 번만 더 하겠습니다! ^_^
테스트 코드를 보니 이번에는 update 테스트를 위해 findById
를 통해 꺼내오게 테스트를 작성하셨네요.
이렇게 되면 단위 테스트가 아니라 다른 유닛의 성공 여부에 의존하게 되는 테스트 같습니다.
여전히 void보다는 사용되지 않더라도 반환타입을 갖는게 좋을 것 같다는 생각이라 코멘트 한 번만 더 달아둘게요~
리비의 의견은 어떤지 말해주세요!
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.
update는 상세 조회를 반환하도록 수정했고요! delete는 void를 반환하되 다른 방식으로 테스트 하도록 수정했습니다!
} | ||
|
||
private TravelPlan getTravelPlanByShareKey(UUID shareKey) { | ||
public TravelPlan getTravelPlanByShareKey(UUID shareKey) { |
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(readOnly=true)
삭제하신 이유가 있나요?
호출하는 상위 파사드에 걸려있긴한데 다른 곳에서 또 불릴 수 있는만큼 걸어두면 좋을 것 같습니다.
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.
누락했네요.. 다시 붙였습니다!
|
||
private void updateTravelPlanContents(PlanRequest request, TravelPlan travelPlan) { | ||
travelPlan.update(request.title(), request.startDate()); | ||
public void updateTravelPlan(TravelPlan travelPlan, Member member, PlanRequest updateRequest) { |
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.
여기도 void형 반환이네요. TravelogueService
에서는 반환값이 있는데 update쪽이 전반적으로 컨벤션이 어긋난 것 같습니다.
한 번 인지해두시면 좋을듯해요 👍
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.
수정했습니다~
@@ -38,175 +21,56 @@ public class TravelPlanService { | |||
|
|||
private final MemberRepository memberRepository; |
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.
제거!
클로버 코멘트 누락되었던 양방향 연관관계 테스트 코드 추가되었습니다! 리뷰 잘 부탁드려요~ |
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.
정말 많은 코드를 리팩토링해주셔서 감사합니다 리비씨! 🙏
덕분에 코드의 가독성과 유지보수성이 많이 향상된 것 같아요. 💯
테스트 코드 수정도 상당히 번거롭고 시간이 많이 드는 작업인데, 꼼꼼하게 챙겨주셔서 너무 고생 많으셨습니다. 👏
이번 PR 덕분에 코드 퀄리티가 한층 더 좋아졌습니다!
구두로 함께 코드를 보며 이야기를 나누었고, 여행기 쪽 리뷰와 같기 때문에 따로 드릴 리뷰는 없습니다!
@@ -30,6 +31,7 @@ public class TravelPlaceTodo extends BaseEntity { | |||
@GeneratedValue(strategy = GenerationType.IDENTITY) | |||
private Long id; | |||
|
|||
@Setter |
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.
좋습니다.
확실히 양방향 연관관계 도입하고 나니 엔티티 생성 과정이 간단해졌네요.
고생하셨습니다 리비!
.body("message", is("여행 계획 삭제는 작성자만 가능합니다.")); | ||
.body("message", is("여행 계획은 작성자만 접근 가능합니다.")); |
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.
어우 정말 수고 많으셨습니다..
여기도 어제 같이 얘기 나눴던 것처럼 추후에 업데이트 로직을 개선할 수 있을 것 같습니다!
이번 목표는 너무 잘 달성한 것 같아서 approve하겠습니다!
해당 코멘트 맥락과 비슷하게 세터의 메서드 명을 바꾸어 사용처를 제한하는 수정만 이루어졌습니다. |
✅ 작업 내용
🙈 참고 사항