Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
[1단계 - 장기] 차니(이동찬) 미션 제출합니다. #71
[1단계 - 장기] 차니(이동찬) 미션 제출합니다. #71
Changes from all commits
03c73ec
4726844
0bb6140
b35e6b2
7c3cc88
248a6bc
d0fd565
da4566c
a1f8ee3
fb95774
9443472
7d0593d
1e03a13
4f75b0b
cbe9296
5e96303
0dcc7c8
9855aaa
7a14f39
6aba16d
52dc337
d710e96
0d38e84
caa798b
45f304f
496e4ba
91f2625
5c7b98b
ab79815
3c8af9b
211d248
f259b0d
5a6af00
e82967f
5790ae7
f228648
e263dc6
97231e7
5eba43f
98dbb2a
f4ac3e9
a9385af
efbc7d8
1752f6f
b7ceb0f
2468b88
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
장기를 생성할 때 장기말들과
Turn
을 정하는 것이 유동적이지 않다는 것에 동의하여 외부에서 의존성을 주입하는 것으로 변경하였습니다. 👍지금은
Game
에서 주입을 하지만Game
의 책임을 덜어주고, 나중에 상차림관련 로직을 위해 따로 클래스를 만드는 것을 고려하고 있습니다.Game
에서 주입을 하는 것이 자연스러운지, 따로 클래스를 생성하는 것이 자연스러운지 로키의 의견이 궁금합니다.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.
Turn을 외부에서 주입하는 것이 조금 더 유연성이 높다고 생각해요. 😃
추후 어떤 팀이 선공을 할 것인지 입력을 받는다거나하는 변경이 생겼을 때도 유연하게 대처할 수 있을 것 같아요.
질문 주신 내용에 대해서도 생성자를 비즈니스 규칙에 맞춰 강하게 의존하도록 만들 필요는 없다고 생각해서 외부에서 주입받는 방식이 더 좋다고 생각해요.
다만 Game 객체의 역할이 조금 불분명하게 느껴집니다. 🤔
View에도 의존하고 도메인 객체에도 의존하다보니 일반적으로는 Controller의 역할을 할 것으로 보이는데, 도메인 객체를 또 상태로 가지고있다보니 Game 객체가 어떤 의도로 만들어진 것인지 조금 헷갈리는 것 같아요. 👀
Game객체는 View와의 의존성을 가지고있는 것 같은데, domain 패키지에 두신 것은 domain 모델을 의도하신걸까요?
Game 객체는 확실하게 (view 의존성 없이) 장기 게임을 진행하는 역할을 위임해보면 어떨까요?
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.
말씀하신것처럼 1,2 둘 다 장단점이 확실히 보이는 것 같아요.
저는 3의 방법을 제안드려보고 싶은데요.
Map<Position, Unit> units
를 일급 컬렉션으로 랩핑해보면 어떨까요? 😃그렇다면 Janggi 객체의 막중한 책임이 조금은 해소되지 않을까 싶어요. 🤔
또한
findMovableRoutesFrom
의 결과와 같이List<Route>
또한 Routes와 같은 일급 컬렉션을 만들어서 Janggi 객체가 가지고 있는 역할을 일부 위임해줄 수도 있을 것 같아요.지금처럼 Janngi 객체가 과한 책임이나 역할을 가지고있다면 다른 객체에게 적절히 위임해볼 수는 없을지 고민해보고
제가 제안드린 것과 같이 새로운 객체 혹은 이미 존재하지만 별다른 역할이 없는 객체 등을 훑어보며 적절히 역할을 위임하게된다면 차니가 고민하고 계신 부분이 어느정도 해결될 것 같아요.
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.
한번 할당한 변수를 재할당할 필요 없이 지금처럼 filterRoutesByUnitType 메서드를 수행한 결과를 한번만 변수로 선언하면 될 것 같아요. 😉
(final은 꼭 안써도되지만, 해당 코멘트를 조금 더 강조하기위해 선언했어요 😃)
추가적으로
filterRoutesByUnitType
지금처럼 바꾸게되면 행위가 조금 달라지니 네이밍도 바뀌는게 좋을 것 같네요 👀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.
point와 position은 엄연히 다른 용도로 만드셨다고 했던 것 같은데요.
네이밍도 혼용되게 사용되지 않는게 좋을 것 같아요. 😃
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.
1이 어떤 의미를 가지는건지 이해하기가 조금 어려운 것 같네요. 🤔
상수로 뽑고 네이밍을 통해서 의도를 드러내보면 어떨까요?
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.
unit 객체에게 메시지를 보내볼 수도 있을 것 같아요. 😃
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.
남겨주신 코멘트에 대한 답변 여기서 이어서 진행할게요. 😃
초기 좌표로 y가 꼭 하나가 아닌 경우에는 어떻게 되는걸까요?
지금은 y 좌표가 무조건 동일할거라는 가정이 깔려있다고 생각해요.
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.
로키의 의견 이제 완전히 이해했어요! 감사합니다 👍
초기 y 좌표에 변경이 생기면 변경이 생길 수 있다는 것에는 저도 동의합니다. 확장성을 고려하는 것은 필수적이지만, 너무 미래에 대한 확장을 열어 놓는 것은 불필요하다고 생각해요.
관련해서 두 가지 의견이 있습니다. 들어보시고 로키의 의견 편하게 말씀해주세요! 😊
첫번째로는 장기의 초기 Y위치가 변경될 여지가 현저하게 적다는 점입니다. X좌표는 추후 상차림이라는 규칙으로 인하여 바뀔 가능성이 있지만 같은 유닛이 다른 Y좌표를 가질 수 있다는 규칙은 존재하지 않기 때문입니다.
두번째로는 사이드 이펙트가 거의 없다고 생각합니다. 추후에 Y좌표를 한 팀의 장기말에서 동일하지 않는다는 규칙이 생길 때 바뀌어도 늦지 않을 것이라고 생각해요.
만약 요구사항이 변경되어 로키가 제시해준 방법으로 바꾸었을 때 다른 클래스들이 영향을 받는다면 문제가 심해질 것이지만, 나중에 그런 상황이 발생해도
DefaultUnitPoisition
이라는 클래스 하나만 수정해도 외부에 영향이 없기 때문이에요.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.
저도 DRY 원칙을 참 좋아하기 때문에 매우 공감하는 말이에요.
먼저 첫번째 말씀은 저도 어느정도 동의합니다만, 너무 순수하게 전통적인 장기 게임으로만 생각해도 안될 문제라고 생각해요. 😃
온라인 게임으로 장기 게임을 서비스회사라면 꼭 전통적인 방식의 장기게임만 제공하라는 법은 없으니깐요. 😉
지금은 규모가 작기 때문에 상대적으로 리팩터링이 쉬울 수 있지만, 실제로 이미 운영되고있는 서비스이고 또 규모도 커진다면...
생각한 것보다 변경이 쉽지 않을 수도 있다고 생각해요.
그래서 어느정도는 확장성을 고려하며 코드를 구현하는 것도 중요하다고 생각해요.
그래도 DefaultUnitPosition의 (getter를 써서 직접 처리하는 로직없이) 공개 메서드만 사용하고있는 상황이라면 추후 수정하는 것이 그리 어렵지 않을 것 같다는 말에는 동의합니다. 😃 (단지... 다른 팀원들이 공개 메서드만 쓴다는 보장은 없기 때문에... 이렇게 이상적으로 코드가 유지보수될지는 확신하기는 어려울 것 같네요)
그리고 무엇보다도...
저희는 미션 요구사항에서
3개 이상의 인스턴스 변수를 가진 클래스를 만들지 않는다.
라는 요구사항을 만족해야하기도하죠. 😅저는 인스턴스 변수를 줄이는 방면을 고민하다보니 각팀의 y좌표와 x좌표들을 따로 관리하는 것보다는 묶음으로 관리하는게 낫지 않나 싶었어요. 😃
차니의 의견도 충분히 납득이되기 때문에 제가 제안드린 방식이 꼭 아니더라도 상관없으니 미션 요구사항에 맞게 인스턴스 변수 갯수를 줄여보면 좋을 것 같아요. 😃