Skip to content
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

[레거시 코드 리팩터링 - 3단계] 현구막(최현구) 미션 제출합니다. #197

Closed
wants to merge 12 commits into from

Conversation

Hyeon9mak
Copy link
Member

술샘! 조영호님 강의 3회독하고 삘받아서 줄기차게 리팩토링 해봤습니다!!
리팩토링 진행하면서 진짜 고민이 많았던거 같아요.

1. Validator 클래스 사용

처음에는 아래 그림과 같이 패키지를 나누고, Validator 클래스를 만드려하지 않았어요.
2단계에서처럼 여전히 비즈니스로직 책임을 가진 객체가 검증도 진행하는게 올바른거 같았어요.

image

그런데 패키지를 나누어도 결국 다른 패키지의 Repository 조회 때문에 계속 의존성이 유지되더라구요.
특히 비즈니스 로직은 어떻게든 의존성을 분리할 수 있었지만, 검증로직은 대놓고 다른 패키지의 Repository 의존을 필요로 했어요.
그제서야 조영호님 영상 속 Validator 클래스의 의의가 이해가 되더라구요.
"검증 로직만큼은 어쩔 수 없이 다른 패키지의 Repository를 의존하고, 비즈니스 로직만이라도 의존성을 분리한다!" 였던거 같아요.


2. 패키지 내부 양방향 연관관계 매핑, Table 패키지 통합

Validator 클래스를 이해하고 패키지 분리 및 리팩토링을 진행하니까 아래와 같아졌어요.

image

우선 패키지 내부의 엔티티끼리는 양방향 매핑을 진행했는데요, 패키지로 나누어진 엔티티간 의존도를 최대한 끊는 만큼
반대로 '패키지 내부의 엔티티끼리는 의존도를 갖고 있다는 걸 인정하는거 아닌가?' 라는 생각도 들었고,
이전에 술샘의 피드백이 생각나서 양방향 매핑을 사용해봤어요!!

그리고 OrderTableTableGroup이 하나의 패키지로 묶였는데요,
처음엔 OrderTableTableGroup의 생명주기가 다르다는 이유로 둘을 나누었는데,
TableGroupService의 모든 로직이 OrderTable에 의존하더라구요.
모든 로직이 의존한다는 건 사실 하나의 엔티티로 봐야하는거 아닐까 싶기도 해서, 패키지를 묶어버렸습니다.
여기서도 내부에 양방향 매핑을 진행할까 했는데, OrderTableTableGroup을 크게 필요로 하는 경우가 없어서
양방향 매핑은 진행하지 않았고, 서로 조금 소원한 상태(?)로 놔두어 보았어요 😅


3. 이벤트 핸들러 사용

지금 다시 문제 지문을 읽어봐도 도저히 이해가 안가서요...

메뉴의 이름과 가격이 변경되면 주문 항목�도 함께 변경된다. 메뉴 정보가 변경되더라도 주문 항목이 변경되지 않게 구현한다.

거짓말 안하고 20번은 읽어본거 같은데 도저히 지분이 이해가 안돼서, 다른 크루들은 어떻게 해석했나 코드를 살펴봤는데
여전히 잘 모르겠더라구요...
우선은 사용해본 것에 의의를 두기로 했고, 다시 지문이 이해되면 이벤트 핸들러가 사용된 부분 테스트 코드를 완성할 생각에
TODO 주석을 남겨두었습니다!!

이번 리뷰도 잘 부탁드림돠!!!!!!!!!!!! 술샘 화이팅!!!!!!!!!!!!!!!!

Copy link

@DWL5 DWL5 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

현구막 대단해요!! 전 아직 진행중인데 ㅜ 제 미션을 하다가 코드리뷰 하러 왔습니다!!!

클래스 간의 방향도 중요하고 패키지 간의 방향도 중요하다. 클래스 사이, 패키지 사이의 의존 관계는 단방향이 되도록 해야 한다.

이번 미션의 요구사항이 클래스 사이, 패키지 사이의 의존관계는 단방향이 되어야 한다. 더라구요

그래서 제가 이전 단계에서 양방향 매핑도 괜찮지 않을까? 라고 드렸던 피드백을 한번 더 고민해봤어요.
예를 들어 현재 MenuProduct가 다대일로 매핑 되어있는 Menu는 사실 필요없어보여요.
Menu가 가지고 있는 일대다로 매핑되어 있는 MenuProduct는 가격 유효성 검증에 필요해요.
이 부분에서 일대다 단방향 매핑만 하면 어떨까라고 고민을 해보았는데, 현재 DDL에 외래키들이 NotNull로 설정되어 있기도하고, insert문이 불린 후 외래키 설정하려고 update문도 불려서.. 양방향 매핑이 나은듯 합니다!

그리고 현구막의 패키지 연관관계를 살펴보았는데, order와 table패키지 간에 양방향 의존관계가 발생해요.
Validator쪽에서요! OrderTable을 Order패키지로 옮기면 의존성이 사라지려나요?

메뉴의 이름과 가격이 변경되면 주문 항목�도 함께 변경된다. 메뉴 정보가 변경되더라도 주문 항목이 변경되지 않게 구현한다.

이건 저도 아직 잘 모르겠어요!! 크루들한테 물어보고 알게되면 답을 달겠습니답

저도 지금 미션 진행 중인데.. 저 미션 다하고 피드백 드리면 조금 늦을 것 같아 미리 할 수 있는 피드백 드려보아요!! 현구막 화이팅~

@yaho99 yaho99 deleted the branch woowacourse:hyeon9mak November 2, 2022 07:53
@yaho99 yaho99 closed this Nov 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants