[레거시 코드 리팩터링 - 2단계] 현구막(최현구) 미션 제출합니다. #170
Conversation
OrderService 테스트 오타 수정
Menu MenuGroup MenuProduct MenuProducts Product Price
Order Orders OrderLineItem OrderLineItems OrderTable OrderTables
There was a problem hiding this comment.
구막구막현구막! 바쁘실 텐데 미션 구현 잘 해 주셨어요.
몇 가지 코멘트 남길게요!
특히 엔티티간 단방향 매핑에 대해서 수리의 의견도 궁금해요!!
단방향 매핑으로도 이미 연관관계 매핑은 되어있으니까, 필요할 때 양방향 매핑을 추가해주는 것은 좋아보여요. 현구막은 이번 미션에서 극단적으로 만들어보았다고 했는데 그렇게 극단적일 필요는 없다고 생각합니다!!
그런데 데이터베이스 의존적인 형태는 어느것을 말하는 것일까요? 이 부분을 계속 고민해보았는데 제가 답을 찾지 못했네요 ㅜㅜ OrderTableService에서 OrderRepository의 데이터를 얻어오는 부분일까요?
그랬더니 리팩토링이 조금 더 쉬웠던거 같아요!! 물론 객체지향보단 데이터베이스 의존적인 형태가 보이기 시작했지만요...
결국 객체지향이라는 것도 유지보수를 편하게 하기 위함이니까, 트레이드오프가 있는 걸까요??
이 질문도 위의 예시처럼 도메인단의 양방향 매핑을 하는 대신, 서비스 단에서 레포지토리 의존성을 추가하는 것을 말한 것인가요??
어려워요 현구막....
제가 질문의 내용을 이해하고 있는게 맞을까요..
| return productId; | ||
| public BigDecimal productTotalPrice() { | ||
| return product.getPrice().multiply(BigDecimal.valueOf(quantity)); | ||
| } |
There was a problem hiding this comment.
해당 코드는 한번더 캡슐화가 가능할 것 같아요!
getPrice()로 얻어온 값을 multiply하지 않고, product에 해당 계산을 하는 메시지를 보내는 건 어떨까요? 👀
There was a problem hiding this comment.
오 세상에 홀리몰리 그게 무조건 좋겠네요!!!!!!!
객체에 메세지를 보내라는 피드백을 레벨5 에서도 받다니!!!!!!!!!!!!!!!! 🥊 🥊 🥊 🥊
| package kitchenpos.domain; | ||
|
|
||
| import java.time.LocalDateTime; | ||
| import java.util.List; |
There was a problem hiding this comment.
너잘한테 Ctrl + Option + O 배워서 바로 싹다 없앴어요!!!!
| public Long getMenuGroupId() { | ||
| return menuGroupId; | ||
| private void validateBlank(String name) { | ||
| if (name.replaceAll(" ", "").isEmpty()) { |
There was a problem hiding this comment.
공백을 체크하는 로직은 중복이 많이 될 것 같은데, Util로 빼도 될 것 같아요.
Java11을 사용한다면 isBlank()로 대체 할 수 있을 것 같아요
There was a problem hiding this comment.
아!! isBlank()!!!! 감사합니다 새까맣게 잊고 있었어요!!!!!!!!!!!!!!!!!!!!!!
자바 버전 1.8 사용중이라 isBlank()가 없네유 흐허헝..
There was a problem hiding this comment.
Null과 공백을 체크하는 것 모두 각각의 값 객체가 가져야할 책임이라 생각하기도 하고,
3단계에서 의존성을 끊어내는게 주 목적이다보니까 Util 클래스는 지양하고 싶어유!!
| OrderTable orderTable = findById(orderTableId); | ||
| orderTable.changeNumberOfGuests(request.getNumberOfGuests()); | ||
|
|
||
| return OrderTableResponse.from(orderTable); |
There was a problem hiding this comment.
만약 양방향 매핑을 한다면, OrderTableService에서 OrderRepository의존을 지울 수 있을 것 같네요.
There was a problem hiding this comment.
이 부분은 추가 PR 멘트에서 설명해보겠습니다!!!
| import java.util.Optional; | ||
|
|
||
| @Repository | ||
| public class JdbcTemplateMenuDao implements MenuDao { |
There was a problem hiding this comment.
Dao들이 지워졌는데, Dao 테스트가 남아있어서 테스트가 안돌아가고 있어용!
There was a problem hiding this comment.
으?! 다 지웠다 생각했는데 커밋이 섞여들어갔나봐요 😭 🥊
답변해주신 내용들이 모두 맞아요!!!!!!
양방향 매핑을 진행하게 되면 말 그대로 클래스간 사이클이 발생하게 되는거니까, 이 부분을 만들지 않고자 했습니다!! 결국 서비스 단에서 레포지토리에 의존하는 코드가 작성됐고, '이 부분이 트레이드 오프인가?' 하는 고민이었어요!! 저도 수리가 피드백 주신대로 필요한 곳에서는 양방향을 사용하는게 백번 맞다고 생각합니다!! 나머지 부분들은 수리가 피드백 주신대로 반영해보았습니다!!! |
There was a problem hiding this comment.
양방향 매핑을 진행하게 되면 말 그대로 클래스간 사이클이 발생하게 되는거니까,
이 부분을 만들지 않고자 했습니다!! 결국 서비스 단에서 레포지토리에 의존하는 코드가 작성됐고,
'이 부분이 트레이드 오프인가?' 하는 고민이었어요!!
객체참조를 통한 매핑보다 Repository를 통해서 객체를 탐색하는것이 더 낮은 결합도를 갖는다고 해요.
결국 결합도를 낮추느냐 vs 서비스 단이 방대해지느냐의 트레이드 오프 이겠네요.
트레이드 오프이긴 하지만, 조금만 건드려도 결합도가 높아 예상치 못한 오류터지는 애플리케이션 vs 읽기 힘든 서비스 단 코드 하면 전자가 조금 더 힘들지 않을까요 ㅜ..
저도 미션 진행중인데, 3단계를 진행하면서 더 고민해봅시다. 화이팅!!!
| return productId; | ||
| public BigDecimal productTotalPrice() { | ||
| return product.getPrice().multiply(BigDecimal.valueOf(quantity)); | ||
| } |
3 participants
수리!!! 2단계 진행했습니다!!! 죽을거같아요!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!
JPA를 도입하면서 서비스 계층에 남아있던 비즈니스 로직을 최대한 엔티티쪽으로 이동시키고,
서비스 계층에서는 로직의 순서만 담당하도록 변경해보았습니다!
도메인끼리 결합도가 너무 높아서 동시에 3~5개의 엔티티를 리팩토링하는 경험도 해보고...
왜 결합도를 낮춰야하는지 피부로 느끼는 미션 매우 굳 ^^:+1:
원시값 포장도 한꺼번에 진행해볼까 했는데, 3단계 의존성을 변경할 때 엔티티간의 관계가 계속 변경될거 같아서
우선 2단계 먼저 코드리뷰 받아보고 싶습니다!!!
중점적으로 진행한 부분은
입니다!!
특히 엔티티간 단방향 매핑에 대해서 수리의 의견도 궁금해요!!
저는 기존 팀프로젝트에서 양방향 매핑을 자주 사용하는 편이었어요.
처음에는 양방향 매핑이 굉장히 편리했는데, 프로그램이 복잡해진 이후부터는 머리가 아프더라구요.
그래서 이번에는 아예 극단적으로 단방향으로만 연관관계를 만들어보았어요!!
그랬더니 리팩토링이 조금 더 쉬웠던거 같아요!! 물론 객체지향보단 데이터베이스 의존적인 형태가 보이기 시작했지만요...
결국 객체지향이라는 것도 유지보수를 편하게 하기 위함이니까, 트레이드오프가 있는 걸까요??
리뷰 잘 부탁드림돠!!!!!!! 🙇♂️ 🙇♂️ 🙇♂️ 🙇♂️ 🦅