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

[레거시 코드 리팩터링 - 4단계] 제이미(임정수) 미션 제출합니다 #805

Merged
merged 4 commits into from
Oct 29, 2023

Conversation

JJ503
Copy link
Member

@JJ503 JJ503 commented Oct 29, 2023

안녕하세요 오리!
이제 정말 마지막 미션의 마지막 단계네요!

2단계의 추가적인 피드백도 수정 맟 답변달아 두었습니다.

이번 미션에서는 각 컨텍스트 별로 모듈을 분리해 진행해 보았습니다.
그런데 생각보다 모듈 설정이 어려워 분리에 계속 구조를 바꾸는 바람에
적절한 위치에 대해선 집중을 좀 못 하게 되어 잘한 것인지 잘 모르겠네요..!

특히, 테스트의 위치와 ui 패키지의 경우 app 모듈로 이동해야 하는 가에 대한 고민을 하고 있습니다.
테스트의 경우 원래 각 도메인 모듈에 넣으려고 했는데 이는 fixture를 어떻게 해야 할지 몰라 실패했습니다.

좀 우당탕탕 진행하긴 했지만 이번 리뷰도 잘 부탁드립니다!

Copy link

@carsago carsago left a comment

Choose a reason for hiding this comment

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

고생하셨습니다 제이미 ~~

Comment on lines +1 to +2
bootJar { enabled = false }
jar { enabled = true }
Copy link

Choose a reason for hiding this comment

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

루트 gradle에도 이 코드가 있었으면 좋았겠네용

Comment on lines 1 to +11
rootProject.name = 'kitchenpos'

include 'app'
include 'core'
include 'domain-menu'
include 'domain-menugroup'
include 'domain-order'
include 'domain-ordertable'
include 'domain-ordertable'
include 'domain-product'
include 'domain-tablegroup'
Copy link

Choose a reason for hiding this comment

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

고생하셨습니다.
사실 모듈을 나눠야할 필요가 있어서 나눴다기 보다는 미션 요구사항으로 나누라고해서 나눈거다 보니 어떻게 나누는 것보단 한 번 해본게 중요한 경험이라고 생각합니다 !!

그리고 이 과정에서 고민한 것들이 이후에 쓸모가 있지않을까용

@carsago carsago merged commit 7669edd into woowacourse:jj503 Oct 29, 2023
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.

2 participants