[1단계 - 장기] 차니(이동찬) 미션 제출합니다. #71
Conversation
| @@ -1,3 +1,73 @@ | |||
| # java-janggi | |||
|
|
|||
| 장기 미션 저장소 | |||
|
|
|||
There was a problem hiding this comment.
PR에 프롤로그 링크를 잘못 거신 것 같아요.
아직 정리가 덜되신 것 같던데, 다음 리뷰 전까지는 한번 정리해서 작성해주시면 좋을 것 같아요.
(완료되시면 코멘트도 추가적으로 남겨주시면 감사하겠습니다. 🙇♂️)
There was a problem hiding this comment.
네 급하게 제출하느라 일단 PR을 올리고 수정했습니다!..
테스트 시 Fake 객체와 Jdbc관련한 내용은 2단계 학습한 이후에 작성하는게 더 좋을 것 같아서 2단계 리뷰요청과 함께 수정할게요!
README.md
Outdated
| - [ ] 상차림 선택지 대로 마와 상을 배치한다. | ||
| - [ ] 어떤 좌표에 기물이 존재하는지 확인한다. | ||
|
|
||
| ### 기물, unit.Unit | ||
|
|
||
| - [ ] 기물이 갈 수 있는 경로를 모두 계산한다. | ||
| - [ ] 궁 : 궁은 아무 기능이 없다. | ||
| - [ ] 사 : 사는 아무 기능이 없다. | ||
| - [ ] 졸 : 직진(좌, 우, 상) 1번. | ||
| - [ ] 마 : 작진(좌, 우, 상, 하) 1번, 대각선 이동 1번. | ||
| - [ ] 차 : 직진(좌, 우, 상, 하)만 가능하다 | ||
| - [ ] 포 : 무조건 기물을 한칸 점프해야 하며, 직진만 가능하다. | ||
| - [ ] 상 : 직진(좌, 우, 상, 하) 1번, 대각선 이동 2번. |
There was a problem hiding this comment.
기능 구현 목록표는 잘 작성해주신 것 같은데요.
기능 단위로 구현하시면서 기능구현 목록표도 같이 체크해주셨으면 조금 더 살아있는 문서가 됐을 것 같아요. 🙂
| import java.util.stream.IntStream; | ||
| import java.util.stream.Stream; | ||
|
|
||
| public class CarUnitRule implements UnitRule { |
There was a problem hiding this comment.
장기 말 요소들을 영어로 네이밍하기에는 조금 어려우셨을 것 같은데요. 😃
미션 소개 페이지에서 언급한 장기 기획서에서 이런 장기 말 네이밍을 소개해주고있네요.
이런 기획서에 적힌 네이밍에 최대한 동일하거나 혹은 유사한 방식의 네이밍을 사용해보시면 어떨까 싶어요. 🤔
영어로 대체할 수 없는 상황이라면 소리나는대로 네이밍을 지을 수도 있을 것 같아요. 😉
There was a problem hiding this comment.
여기서 찾을 생각을 못하고 다른 방향으로 네이밍을 했네요..🤣
좋은 방향 제시해주셔서 감사합니다. 👍
| public class Unit { | ||
| private final Position position; | ||
| private final Team team; | ||
| private final UnitRule unitRule; |
There was a problem hiding this comment.
(유닛은 소통의 편의를 위해서 '장기말'로 대체해서 이야기할게요 😃)
장기말 객체가 각각의 별도의 룰 객체를 가지고있는 것 같은데요. 🤔
미션 요구사항 중에 3개 이상의 인스턴스 변수를 가진 클래스를 만들지 않는다. 라는 요구사항이 있는데,
이 요구사항을 지키도록 수정해보시면 좋을 것 같아요.
유닛을 역할(interface 혹은 추상 클래스)로 두고, 각각의 장기말 하위 구현체(포, 차, 마 Etc...)를 구현하는 방식이 인스턴스 갯수를 더 줄이면서도 동일한 처리를 할 수 있지 않을까요? 😃
There was a problem hiding this comment.
좋은 방향 제시해주셔서 감사합니다 👍
처음에는 말씀해주시는 방법과 같이 Unit(장기말)을 인터페이스로 두고, CarUnit과 같이 구현체에서 구체적인 움직이는 방법을 구현하였습니다. 7c3cc88
하지만 CarUnit, HorseUnit 등 구현체 장기말에서 공통된 기능들이 중복될 가능성을 우려해 지금 구현과 같이 UnitRule을 구현하는 구현체(CarUnitRule, HorseUnitRule 등)에서 움직임과 관련된 규칙을 정하고, Unit에서는 UnitRule을 조합하여 장기에 움직임에만 신경쓸 수 있도록 바꾸었습니다.
해당 객체에서 Position을 들고 있는 것이 객체의 책임이 많아지고, 장기말 자체의 역할과 맞지 않는 것 같아서 장기말들을 관리하는 Janggi 객체에서 Map<Position, Unit>과 같이 들고 있는 방향으로 개선해보려고 하는데 이러한 방향은 어떻게 생각하시는지 로키의 의견이 궁금합니다!
src/main/java/domain/Team.java
Outdated
| CHO, | ||
| ; | ||
|
|
||
| public boolean isFront(Position before, Position after) { |
| private final int hanY; | ||
| private final int choY; |
There was a problem hiding this comment.
hanY, choY라는 네이밍은 어떤 의미를 가지는걸까요?
아~ 각각의 팀에서의 y 좌표를 의미하시는 거였군요?
차라리 이런 경우에는 han과 cho에 대한 초기 Position들을 가지고있는게 어떨까요?
x,y 좌표를 따로 가지고있기 보다는, Position 묶음으로 가지고있는게, 조금 더 유동적인 대처가 될 것 같아요.
(ex. 현재는 초기 셋팅이 x가 고정되어있지만, x가 고정되지 않는다면...? )
There was a problem hiding this comment.
차라리 이런 경우에는 han과 cho에 대한 초기 Position들을 가지고있는게 어떨까요?
x,y 좌표를 따로 가지고있기 보다는, Position 묶음으로 가지고있는게, 조금 더 유동적인 대처가 될 것 같아요.
(ex. 현재는 초기 셋팅이 x가 고정되어있지만, x가 고정되지 않는다면…? )
위 피드백은 아래와 같이 초기 Position을 가지고 있는 것이 유동적이라는 뜻으로 이해했습니다.
하지만 예시로 들어준 내용에 대한 이해가 잘 가지 않습니다. List<Integer>로 들고 있는 것과 List<Position>으로 들고 있는 것이 왜 유동적인 면에서 차이가 나는 것일까요?
일단 Position으로 관리하면 반복되는 각 팀의 Y좌표가 더 관리하기 힘들다고 생각해서 로키의 피드백을 아직 반영하진 않았어요. 😢
public enum DefaultUnitPosition {
HORSE(
List.of(Position.of(0,2), Position.of(0,7)),
List.of(Position.of(9,2), Position.of(9,7)),
HorseUnitRule::new),
ELEPHANT(
List.of(Position.of(0,1), Position.of(0,6)),
List.of(Position.of(9,1), Position.of(9,6)),
HorseUnitRule::new),
;
private final List<Position> hanPositions;
private final List<Position> choPositions;
private final Supplier<UnitRule> rule;
}
src/main/java/domain/Janggi.java
Outdated
| private final List<Unit> units = new ArrayList<>(); | ||
| private Team turn; | ||
|
|
||
| public Janggi() { | ||
| turn = Team.CHO; | ||
| List<Unit> hanUnits = settingUnits(Team.HAN); | ||
| List<Unit> choUnits = settingUnits(Team.CHO); | ||
| this.units.addAll(Stream.concat(hanUnits.stream(), choUnits.stream()).toList()); | ||
| } |
There was a problem hiding this comment.
아래와 같이 주생성자는 생성자 파라미터를 통해서 이미 생성된 Units과 turn을 주입받도록해보면 어떨까요?
이런 초기 장기판을 만들어주는 역할을 다른 객체에게 위임해보면 좋을 것 같아요.
이렇게 변경하면, 생성하는 기능도 테스트가능하고 Janggi 객체에 대한 테스트도 작성하기 쉬워질 것 같아요.
| private final List<Unit> units = new ArrayList<>(); | |
| private Team turn; | |
| public Janggi() { | |
| turn = Team.CHO; | |
| List<Unit> hanUnits = settingUnits(Team.HAN); | |
| List<Unit> choUnits = settingUnits(Team.CHO); | |
| this.units.addAll(Stream.concat(hanUnits.stream(), choUnits.stream()).toList()); | |
| } | |
| private final List<Unit> units; | |
| private Team turn; | |
| public Janggi(Team turn, List<Unit> units) { | |
| this.turn = turn; | |
| this.units = new ArrayList<>(units) | |
| } |
There was a problem hiding this comment.
Janggi가 초기 세팅에 관련되어 있다는 생각에 빠져서 List<Unit>을 주입받을 생각을 못했네요.
로키의 말대로 List<Unit>같은 경우는 외부에서 주입을 하는 것이 더 깔끔할 것 같아요. 장기에는 상차림이라는 규칙이 있으니까요! 👍
하지만 장기 게임에서 초나라가 먼저 공격을 시작하는 것이 규칙이라면 생성자에서 turn = Team.CHO;와 같이 정해주는 것에 대해서는 어떻게 생각하시나요?
src/main/java/domain/Janggi.java
Outdated
| if (turn == Team.HAN) { | ||
| turn = Team.CHO; | ||
| return; | ||
| } | ||
| turn = Team.HAN; |
| return UnitType.CAR; | ||
| } | ||
|
|
||
| public List<Route> calculateAllRoute(Position start) { |
There was a problem hiding this comment.
가능한 오버라이드를 한 메서드는 @Override 애너테이션을 붙여주면 좋을 것 같아요.
| return routes; | ||
| } | ||
|
|
||
| public List<Position> calculateEndPoints(Position start) { |
There was a problem hiding this comment.
calculateEndPoints 메서드 는 해당 클래스 내부에서만 사용되는 것 같은데요.
접근 제한자를 private로 제한해주시면 좋을 것 같아요. 😃
(CarUnitRule 클래스 안에도 동일한 케이스의 메서드가 있네요)
Summary by CodeRabbit
Summary by CodeRabbit
Walkthrough이번 변경사항은 Janggi 게임의 전반적인 기능 구현 및 구조 개선을 포함합니다. 게임 보드 초기화, 유닛의 이동 경로 계산, 포지션 관리, 게임 턴 처리 등의 기능이 상세히 문서화되었으며, 다수의 신규 클래스와 메소드가 추가 또는 수정되었습니다. 또한, 게임 실행을 위한 Changes
Sequence Diagram(s)sequenceDiagram
participant App as JanggiApplication
participant In as InputView
participant Game as Game
participant Janggi as Janggi
participant Out as OutputView
App->>In: readUnitPosition(Team)
In-->>App: 선택한 유닛 위치 반환
App->>In: readDestinationPosition(Team)
In-->>App: 목적지 위치 반환
App->>Game: doTurn(pick, destination)
Game->>Janggi: doTurn(pick, destination)
Janggi-->>Game: 게임 상태 업데이트
Game->>Out: printJanggiUnits(units)
Game->>Out: printAvailableRoute(current, routes)
Possibly related PRs
Poem
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
안녕하세요 로키, 👍 구현하다보니 고민과 질문이 몇개 있어서 로키의 도움을 받고싶습니다.. 😭 질문1. UnitRule의 존폐 여부 로키의 리뷰에서는 그 이유는 인터페이스를 직접 구현하는 경우 인터페이스를 그룹핑으로 쓰는 느낌이 강하게 들었습니다. 움직인다는 것은 장기말의 특성인데 그 특성으로 장기말들을 묶기 때문에 어색하게 느껴졌습니다. 지금과 같이 조합으로 2. Janggi의 책임 분리에 대한 고민 지금 코드에서는
저는 두 가지 방법 중에서 첫 번째 방법으로 경로 탐색 관련된 역할을 |
| private final OutputView outputView; | ||
|
|
||
| public Game(InputView inputView, OutputView outputView) { | ||
| this.janggi = createJanggi(); |
There was a problem hiding this comment.
장기를 생성할 때 장기말들과 Turn을 정하는 것이 유동적이지 않다는 것에 동의하여 외부에서 의존성을 주입하는 것으로 변경하였습니다. 👍
지금은 Game에서 주입을 하지만 Game의 책임을 덜어주고, 나중에 상차림관련 로직을 위해 따로 클래스를 만드는 것을 고려하고 있습니다.
Game에서 주입을 하는 것이 자연스러운지, 따로 클래스를 생성하는 것이 자연스러운지 로키의 의견이 궁금합니다.
There was a problem hiding this comment.
Turn을 외부에서 주입하는 것이 조금 더 유연성이 높다고 생각해요. 😃
추후 어떤 팀이 선공을 할 것인지 입력을 받는다거나하는 변경이 생겼을 때도 유연하게 대처할 수 있을 것 같아요.
질문 주신 내용에 대해서도 생성자를 비즈니스 규칙에 맞춰 강하게 의존하도록 만들 필요는 없다고 생각해서 외부에서 주입받는 방식이 더 좋다고 생각해요.
다만 Game 객체의 역할이 조금 불분명하게 느껴집니다. 🤔
View에도 의존하고 도메인 객체에도 의존하다보니 일반적으로는 Controller의 역할을 할 것으로 보이는데, 도메인 객체를 또 상태로 가지고있다보니 Game 객체가 어떤 의도로 만들어진 것인지 조금 헷갈리는 것 같아요. 👀
Game객체는 View와의 의존성을 가지고있는 것 같은데, domain 패키지에 두신 것은 domain 모델을 의도하신걸까요?
Game 객체는 확실하게 (view 의존성 없이) 장기 게임을 진행하는 역할을 위임해보면 어떨까요?
| private final int hanY; | ||
| private final int choY; |
There was a problem hiding this comment.
남겨주신 코멘트에 대한 답변 여기서 이어서 진행할게요. 😃
왜 유동적인 면에서 차이가 나는 것일까요?
일단 Position으로 관리하면 반복되는 각 팀의 Y좌표가 더 관리하기 힘들다고 생각해서 로키의 피드백을 아직 반영하진 않았어요.
초기 좌표로 y가 꼭 하나가 아닌 경우에는 어떻게 되는걸까요?
지금은 y 좌표가 무조건 동일할거라는 가정이 깔려있다고 생각해요.
There was a problem hiding this comment.
로키의 의견 이제 완전히 이해했어요! 감사합니다 👍
초기 y 좌표에 변경이 생기면 변경이 생길 수 있다는 것에는 저도 동의합니다. 확장성을 고려하는 것은 필수적이지만, 너무 미래에 대한 확장을 열어 놓는 것은 불필요하다고 생각해요.
관련해서 두 가지 의견이 있습니다. 들어보시고 로키의 의견 편하게 말씀해주세요! 😊
첫번째로는 장기의 초기 Y위치가 변경될 여지가 현저하게 적다는 점입니다. X좌표는 추후 상차림이라는 규칙으로 인하여 바뀔 가능성이 있지만 같은 유닛이 다른 Y좌표를 가질 수 있다는 규칙은 존재하지 않기 때문입니다.
두번째로는 사이드 이펙트가 거의 없다고 생각합니다. 추후에 Y좌표를 한 팀의 장기말에서 동일하지 않는다는 규칙이 생길 때 바뀌어도 늦지 않을 것이라고 생각해요.
만약 요구사항이 변경되어 로키가 제시해준 방법으로 바꾸었을 때 다른 클래스들이 영향을 받는다면 문제가 심해질 것이지만, 나중에 그런 상황이 발생해도 DefaultUnitPoisition이라는 클래스 하나만 수정해도 외부에 영향이 없기 때문이에요.
There was a problem hiding this comment.
초기 y 좌표에 변경이 생기면 변경이 생길 수 있다는 것에는 저도 동의합니다. 확장성을 고려하는 것은 필수적이지만, 너무 미래에 대한 확장을 열어 놓는 것은 불필요하다고 생각해요.
저도 DRY 원칙을 참 좋아하기 때문에 매우 공감하는 말이에요.
첫번째로는 장기의 초기 Y위치가 변경될 여지가 현저하게 적다는 점입니다. X좌표는 추후 상차림이라는 규칙으로 인하여 바뀔 가능성이 있지만 같은 유닛이 다른 Y좌표를 가질 수 있다는 규칙은 존재하지 않기 때문입니다.
먼저 첫번째 말씀은 저도 어느정도 동의합니다만, 너무 순수하게 전통적인 장기 게임으로만 생각해도 안될 문제라고 생각해요. 😃
온라인 게임으로 장기 게임을 서비스회사라면 꼭 전통적인 방식의 장기게임만 제공하라는 법은 없으니깐요. 😉
두번째로는 사이드 이펙트가 거의 없다고 생각합니다. 추후에 Y좌표를 한 팀의 장기말에서 동일하지 않는다는 규칙이 생길 때 바뀌어도 늦지 않을 것이라고 생각해요.
만약 요구사항이 변경되어 로키가 제시해준 방법으로 바꾸었을 때 다른 클래스들이 영향을 받는다면 문제가 심해질 것이지만, 나중에 그런 상황이 발생해도 DefaultUnitPoisition이라는 클래스 하나만 수정해도 외부에 영향이 없기 때문이에요.
지금은 규모가 작기 때문에 상대적으로 리팩터링이 쉬울 수 있지만, 실제로 이미 운영되고있는 서비스이고 또 규모도 커진다면...
생각한 것보다 변경이 쉽지 않을 수도 있다고 생각해요.
그래서 어느정도는 확장성을 고려하며 코드를 구현하는 것도 중요하다고 생각해요.
그래도 DefaultUnitPosition의 (getter를 써서 직접 처리하는 로직없이) 공개 메서드만 사용하고있는 상황이라면 추후 수정하는 것이 그리 어렵지 않을 것 같다는 말에는 동의합니다. 😃 (단지... 다른 팀원들이 공개 메서드만 쓴다는 보장은 없기 때문에... 이렇게 이상적으로 코드가 유지보수될지는 확신하기는 어려울 것 같네요)
그리고 무엇보다도...
저희는 미션 요구사항에서 3개 이상의 인스턴스 변수를 가진 클래스를 만들지 않는다.라는 요구사항을 만족해야하기도하죠. 😅
저는 인스턴스 변수를 줄이는 방면을 고민하다보니 각팀의 y좌표와 x좌표들을 따로 관리하는 것보다는 묶음으로 관리하는게 낫지 않나 싶었어요. 😃
차니의 의견도 충분히 납득이되기 때문에 제가 제안드린 방식이 꼭 아니더라도 상관없으니 미션 요구사항에 맞게 인스턴스 변수 갯수를 줄여보면 좋을 것 같아요. 😃
|
|
||
|
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 62
♻️ Duplicate comments (2)
src/main/java/domain/position/DefaultUnitPosition.java (1)
33-33: 🧹 Nitpick (assertive)불필요한 줄바꿈이 있습니다.
33번 줄의 불필요한 줄바꿈을 제거하세요.
private final Supplier<UnitRule> rule; - DefaultUnitPosition(int hanY, int choY, List<Integer> xPositions, Supplier<UnitRule> rule) {src/test/java/domain/JanggiTest.java (1)
33-34: 🧹 Nitpick (assertive)불필요한 줄바꿈을 제거하세요.
이전 리뷰 코멘트에서도 언급되었던 불필요한 줄바꿈이 여전히 존재합니다.
- List<Route> routes = janggi.findMovableRoutesFrom(targetPosition); + List<Route> routes = janggi.findMovableRoutesFrom(targetPosition);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (27)
README.md(1 hunks)src/main/java/JanggiApplication.java(1 hunks)src/main/java/domain/Game.java(1 hunks)src/main/java/domain/Janggi.java(1 hunks)src/main/java/domain/position/DefaultUnitPosition.java(1 hunks)src/main/java/domain/position/Point.java(1 hunks)src/main/java/domain/position/Position.java(1 hunks)src/main/java/domain/position/Route.java(1 hunks)src/main/java/domain/unit/Direction.java(1 hunks)src/main/java/domain/unit/Team.java(1 hunks)src/main/java/domain/unit/Unit.java(1 hunks)src/main/java/domain/unit/UnitType.java(1 hunks)src/main/java/domain/unit/rule/CannonUnitRule.java(1 hunks)src/main/java/domain/unit/rule/ChariotUnitRule.java(1 hunks)src/main/java/domain/unit/rule/ElephantUnitRule.java(1 hunks)src/main/java/domain/unit/rule/HorseUnitRule.java(1 hunks)src/main/java/domain/unit/rule/NoneUnitRule.java(1 hunks)src/main/java/domain/unit/rule/SoldierUnitRule.java(1 hunks)src/main/java/domain/unit/rule/UnitRule.java(1 hunks)src/main/java/view/InputView.java(1 hunks)src/main/java/view/OutputView.java(1 hunks)src/test/java/domain/JanggiTest.java(1 hunks)src/test/java/domain/position/PositionTest.java(1 hunks)src/test/java/domain/position/RouteTest.java(1 hunks)src/test/java/domain/unit/UnitTest.java(1 hunks)src/test/java/domain/unit/rule/ChariotUnitRuleTest.java(1 hunks)src/test/java/domain/unit/rule/HorseUnitRuleTest.java(1 hunks)
🧰 Additional context used
🧬 Code Definitions (15)
src/main/java/domain/unit/rule/NoneUnitRule.java (1)
src/main/java/domain/position/Route.java (1)
Route(7-35)
src/main/java/JanggiApplication.java (3)
src/main/java/domain/Game.java (1)
Game(15-69)src/main/java/view/InputView.java (1)
InputView(6-29)src/main/java/view/OutputView.java (1)
OutputView(11-79)
src/test/java/domain/position/RouteTest.java (2)
src/main/java/domain/position/Position.java (1)
Position(5-75)src/main/java/domain/position/Route.java (1)
Route(7-35)
src/test/java/domain/position/PositionTest.java (1)
src/main/java/domain/position/Position.java (1)
Position(5-75)
src/main/java/domain/position/DefaultUnitPosition.java (7)
src/main/java/domain/unit/Unit.java (1)
Unit(8-34)src/main/java/domain/unit/rule/CannonUnitRule.java (1)
CannonUnitRule(11-69)src/main/java/domain/unit/rule/ChariotUnitRule.java (1)
ChariotUnitRule(11-64)src/main/java/domain/unit/rule/ElephantUnitRule.java (1)
ElephantUnitRule(11-39)src/main/java/domain/unit/rule/HorseUnitRule.java (1)
HorseUnitRule(11-39)src/main/java/domain/unit/rule/NoneUnitRule.java (1)
NoneUnitRule(8-19)src/main/java/domain/unit/rule/SoldierUnitRule.java (1)
SoldierUnitRule(10-34)
src/main/java/domain/unit/rule/ElephantUnitRule.java (2)
src/main/java/domain/position/Point.java (1)
Point(3-24)src/main/java/domain/position/Route.java (1)
Route(7-35)
src/test/java/domain/JanggiTest.java (7)
src/main/java/domain/unit/Unit.java (1)
Unit(8-34)src/main/java/domain/unit/rule/CannonUnitRule.java (1)
CannonUnitRule(11-69)src/main/java/domain/unit/rule/ChariotUnitRule.java (1)
ChariotUnitRule(11-64)src/main/java/domain/unit/rule/ElephantUnitRule.java (1)
ElephantUnitRule(11-39)src/main/java/domain/unit/rule/HorseUnitRule.java (1)
HorseUnitRule(11-39)src/main/java/domain/unit/rule/SoldierUnitRule.java (1)
SoldierUnitRule(10-34)src/main/java/domain/Janggi.java (1)
Janggi(12-164)
src/main/java/domain/unit/rule/SoldierUnitRule.java (2)
src/main/java/domain/position/Point.java (1)
Point(3-24)src/main/java/domain/position/Route.java (1)
Route(7-35)
src/main/java/domain/Game.java (6)
src/main/java/domain/position/Position.java (1)
Position(5-75)src/main/java/domain/position/Route.java (1)
Route(7-35)src/main/java/domain/unit/Unit.java (1)
Unit(8-34)src/main/java/view/InputView.java (1)
InputView(6-29)src/main/java/view/OutputView.java (1)
OutputView(11-79)src/main/java/domain/Janggi.java (1)
Janggi(12-164)
src/main/java/domain/unit/Unit.java (2)
src/main/java/domain/position/Position.java (1)
Position(5-75)src/main/java/domain/position/Route.java (1)
Route(7-35)
src/test/java/domain/unit/UnitTest.java (6)
src/main/java/domain/unit/rule/CannonUnitRule.java (1)
CannonUnitRule(11-69)src/main/java/domain/unit/rule/ChariotUnitRule.java (1)
ChariotUnitRule(11-64)src/main/java/domain/unit/rule/ElephantUnitRule.java (1)
ElephantUnitRule(11-39)src/main/java/domain/unit/rule/HorseUnitRule.java (1)
HorseUnitRule(11-39)src/main/java/domain/unit/rule/SoldierUnitRule.java (1)
SoldierUnitRule(10-34)src/main/java/domain/unit/Unit.java (1)
Unit(8-34)
src/main/java/domain/unit/rule/ChariotUnitRule.java (2)
src/main/java/domain/position/Position.java (1)
Position(5-75)src/main/java/domain/position/Route.java (1)
Route(7-35)
src/main/java/domain/unit/rule/HorseUnitRule.java (2)
src/main/java/domain/position/Point.java (1)
Point(3-24)src/main/java/domain/position/Route.java (1)
Route(7-35)
src/main/java/domain/unit/rule/CannonUnitRule.java (2)
src/main/java/domain/position/Position.java (1)
Position(5-75)src/main/java/domain/position/Route.java (1)
Route(7-35)
src/main/java/view/OutputView.java (3)
src/main/java/domain/position/Position.java (1)
Position(5-75)src/main/java/domain/position/Route.java (1)
Route(7-35)src/main/java/domain/unit/Unit.java (1)
Unit(8-34)
🪛 markdownlint-cli2 (0.17.2)
README.md
12-12: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
13-13: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
18-18: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
19-19: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
32-32: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
66-66: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
🔇 Additional comments (25)
src/main/java/domain/position/DefaultUnitPosition.java (1)
19-25:❓ Verification inconclusive
NoneUnitRule에 의존하는 코드가 있습니다.
GENERAL과 GUARD에 NoneUnitRule이 사용되고 있습니다. 이는 코드의 의도를 파악하기 어렵게 만들 수 있습니다. 만약 이 기물들이 움직일 수 없도록 의도한 것이라면, 좀 더 명확한 이름과 목적을 가진 규칙을 만드는 것이 좋습니다.
현재 GENERAL과 GUARD가 NoneUnitRule을 사용하는 이유가 무엇인지 확인해보세요. 이 기물들이 장기에서 움직일 수 없는 것이 맞는지, 아니면 아직 구현되지 않은 것인지 명확히 하면 좋겠습니다.
🏁 Script executed:
#!/bin/bash # 관련 UnitRule 클래스 파일 확인 echo "NoneUnitRule 구현 확인:" if [[ -f "src/main/java/domain/unit/rule/NoneUnitRule.java" ]]; then cat "src/main/java/domain/unit/rule/NoneUnitRule.java" fi echo -e "\n장기에서 장군(General)과 사(Guard)의 이동 규칙 관련 주석이나 TODO 확인:" rg -i "todo|fixme|general|guard" --type java src/main/java/domain/unit/Length of output: 840
확인: GENERAL과 GUARD의 NoneUnitRule 사용 의도 재검토 필요
NoneUnitRule의calculateAllRoute메서드는 빈 리스트만 반환하므로, 현재 GENERAL과 GUARD 기물은 이동할 수 없는 것으로 보입니다.- 단, 이 기물들이 실제로 움직이지 않아야 하는지 아니면 아직 이동 규칙이 구현되지 않은 상태인지를 명확하게 해주시면 좋겠습니다.
- 만약 의도된 대로 이 기물들이 움직이지 않아야 한다면, 혼동을 줄이기 위해
NoneUnitRule대신 보다 명확한 이름(예:ImmobileUnitRule)을 고려하거나, 관련 주석을 추가하여 이유를 명시하는 것이 좋겠습니다.src/main/java/domain/unit/Unit.java (4)
14-17: 생성자 인자 검증 로직 고려 권장
team혹은unitRule이null로 주어질 경우 예외 처리가 누락된 듯 보입니다. 필요하다면, 방어적 코드를 추가하여 안전성을 확보할 수 있을지 검토해보세요.
19-21: 정적 팩터리 메서드 사용에 대한 긍정적 평가
사용법을 보다 명확히 해주어 가독성이 좋아집니다. 현재 구현 상태도 충분히 적절해 보입니다.
23-25: 에러 처리 검토 요청
unitRule.calculateAllRoute(position)호출 시 발생할 수 있는 예외(유효하지 않은 위치 등)에 대한 처리가 없습니다. 상위 레벨에서 별도 예외 처리가 필요한지 확인해보시는 것을 권장드립니다.
27-33: Getter 메서드 간결성 확인
getType()및getTeam()메서드는 명확하고 가독성이 좋습니다. 현재 상태로 충분히 만족스럽습니다.src/main/java/view/OutputView.java (1)
43-50: 루트 출력 로직 적절성
searchDestination호출 시 예외가 발생할 수 있으나, 이미 내부적으로 빈 경로 예외를 던지므로 별도의 추가 방어가 크게 필요해 보이지 않습니다. 현재 로직은 명확한 책임 분리를 보여주어 좋습니다.src/main/java/domain/unit/rule/ElephantUnitRule.java (2)
14-18: 시작 위치 검증 필요성 고려
calculateAllRoute가 호출될 때start가 유효한 위치인지 사전에 확인할 필요가 있을 수 있습니다. 현재는 DFS를 진행하는 과정에서 유효하지 않은 위치를 자체적으로 필터링하지만, 초기에 방어적으로 검증한다면 예외 처리를 좀 더 명확히 할 수 있습니다.
35-38: 유닛 타입 반환 로직에 대한 긍정적 평가
getType()에서ELEPHANT를 명확히 반환하고 있어 일관성 있고 이해하기 쉽습니다.src/test/java/domain/JanggiTest.java (1)
160-179: 예외 메시지 검증에 상수를 사용하는 것은 좋습니다.예외 메시지 검증에 상수를 사용한 것은 좋은 방법입니다. 이렇게 하면 메시지가 변경되었을 때 테스트도 함께 업데이트되어야 하므로 일관성이 유지됩니다.
src/main/java/domain/unit/rule/HorseUnitRule.java (3)
13-18: 코드 작성이 잘 되었습니다.
calculateAllRoute메서드가 간결하게 구현되어 있으며, DFS 알고리즘을 효과적으로 사용하여 가능한 경로를 계산합니다.
20-33: 백트래킹 로직이 미묘한 버그를 포함할 수 있습니다.
route.remove(next)호출이 의도한 대로 작동하는지 확인하세요.List.remove(Object)는 객체의 첫 번째 발생을 제거하기 때문에, 복제된Point객체가 있는 경우 잘못된 객체를 제거할 가능성이 있습니다..remove(int index)를 사용하거나 리스트의 마지막 요소를 제거하는 것이 더 안전할 수 있습니다.- route.remove(next); + route.remove(route.size() - 1); // 항상 가장 최근에 추가된 요소를 제거
35-38: 타입 반환 메서드에 문제가 없습니다.
getType메서드가 적절하게 구현되어 있습니다.src/main/java/domain/position/Route.java (2)
7-17: 불변 객체 패턴이 잘 적용되었습니다.
Route클래스가 불변 객체로 설계된 것은 좋은 선택입니다. 생성자를 private으로 선언하고 정적 팩토리 메서드를 제공하는 패턴도 잘 적용되었습니다.
32-34: 불변 컬렉션 반환이 좋은 방식입니다.불변 컬렉션을 반환하여 객체의 내부 상태를 보호하는 것은 좋은 방식입니다.
src/main/java/domain/unit/rule/ChariotUnitRule.java (1)
13-21: calculateAllRoute 구현이 잘 되었습니다.메서드 구현이 명확하고 목적에 맞게 잘 작성되었습니다.
src/main/java/domain/unit/rule/CannonUnitRule.java (3)
42-63: 수직/수평 경로 계산 로직 정상 동작 확인
calculateRoute()에서 수직/수평 방향을 판단하여 경로를 생성하는 로직은 명료하고,IntStream.range사용으로 경계를 정확히 포함하고 있습니다. 오탈자나 오프바이원(Off-by-one) 에러도 없어 보이며, 포의 직선 이동 특성에 부합합니다.
65-68: 유닛 타입 반환 로직 문제없음
getType()에서UnitType.CANNON을 반환하는 부분은 간단하고 명확합니다. 추가 검증 로직이나 필요 사항은 없어 보입니다.
14-25:❓ Verification inconclusive
포 이동 경로 중 1칸짜리 경로 스킵 로직 검토 제안
포의 이동 경로를 구할 때, 단 하나의 위치만 포함하는 경로(1칸 이동)를 제외하고 있습니다. 실제 장기 규칙에서 포가 1칸 이동을 전혀 허용하지 않는지, 혹은 특정 상황에서 허용해야 하는지 다시 확인해 보시는 게 좋겠습니다.필요하다면 아래 스크립트를 사용해 포가 실제로 1칸 이동할 수 있는 호출이 있는지 검색해 보세요:
🏁 Script executed:
#!/bin/bash rg -A 5 'calculateAllRoute'Length of output: 4249
포 1칸 이동 경로 제외 로직 재검토 요청
src/main/java/domain/unit/rule/CannonUnitRule.java의calculateAllRoute메서드에서 1칸 이동(단일 위치) 경로를 제외하는 로직을 사용하고 있습니다. 실제 장기 규칙에서 포의 이동에 대해 1칸 이동이 전혀 허용되지 않는지 아니면 특정 상황에서 허용되어야 하는지 다시 한 번 확인해 주시기 바랍니다. 만약 1칸 이동이 가능하다면, 해당 조건 처리 로직을 수정해야 할 수도 있습니다.
- 대상 코드:
if (route.getPositions().size() == 1) { continue; }(src/main/java/domain/unit/rule/CannonUnitRule.java, 라인 14-25)src/main/java/domain/position/Position.java (2)
50-52: 수평 또는 수직 정렬 검증 로직 적절
isHorizontalOrVertical()구현은this.x == opposite.x || this.y == opposite.y식으로 명확히 수평·수직을 확인하고 있어, 오탈자나 불필요한 중복 없이 간결해 보입니다.
24-26:✅ Verification successful
Point 객체 사용 시 구현/참조 확인 필요
from(Point point)메서드가Point객체를 사용하지만, 현재 코드 단편에Point클래스에 대한 import나 구체 구현이 보이지 않습니다. 실제 프로젝트에서Point가 잘 정의되어 있고 참조 가능한지 확인이 필요합니다.
🏁 Script executed:
#!/bin/bash # 프로젝트 내 Point 클래스 및 import 여부를 검색 rg 'class Point'Length of output: 80
검증 결과: Point 클래스 구현 및 위치 확인됨
src/main/java/domain/position/Point.java파일에public class Point가 정의되어 있으며,Position.java와 같은 패키지 내에 있어 별도의 import 없이 참조 가능합니다.따라서, 해당 리뷰 코멘트에 제시된 문제는 해결된 것으로 보입니다.
src/main/java/domain/Janggi.java (5)
26-31: 두 팀 유닛 맵 병합 시 위치 충돌 검증 권장
of()메서드에서Map<Position, Unit>을 단일units맵으로 합칠 때, 동일한Position이 두 맵에 존재하면 덮어써질 가능성이 있습니다. 중복 키가 있으면 예외를 던지거나 로그로 남기는 처리가 필요할 수도 있으니 확인해 보시기 바랍니다.
33-45: 턴 진행(doTurn) 로직 적절
pick,destination을 검사하여 이동 가능 여부를 확정하고, 상대 말이 있으면 제거 후 이동, 그리고switchTurn()까지 전반적 흐름이 일관되어 보입니다.
88-97: 병사 이동 로직에서getFirst()사용 확인 필요
filterSoldierMoves메서드에서route.getPositions().getFirst()로 병사의 이동 가능 여부를 필터링하고 있습니다. 하지만List<Route>의 내부positions가List<Position>인 경우 기본List에는getFirst()가 존재하지 않습니다. 별도의 자료구조(예:LinkedList)나 확장 메서드를 사용 중인지 확인해 주세요.
99-120: 포 점프 로직(canCannonJump) 정상 동작
포가 점프하려면 경로상 기물이 정확히 하나만 있어야 한다는 장기 규칙을 이 로직에서 일관되게 처리합니다. 가독성도 좋아 유지보수하기 수월해 보입니다.
129-141: 경로 차단 여부 체크 로직 간결
filterBlockedRoutes()내부에서isClearRoute와isClearDestination두 조건을 모두 만족하는지 확인해, 경로 중간 혹은 도착지에 아군 기물이 있으면 이동을 제한하는 방식은 적절합니다.
| public enum UnitType { | ||
|
|
||
| GENERAL, | ||
| GUARD, | ||
| CHARIOT, | ||
| HORSE, | ||
| ELEPHANT, | ||
| CANNON, | ||
| SOLDIER, | ||
| NONE, | ||
| ; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
UnitType 열거형이 명확히 구현되었네요.
장기 게임에 필요한 모든 유닛 타입이 포함되어 있습니다.
다만 다음과 같은 개선사항을 고려해 보시는 것은 어떨까요?
- 각 유닛 타입에 대한 Javadoc 문서화 추가 (장기에 익숙하지 않은 사람들을 위해)
- 4번 줄의 불필요한 공백과 13번 줄의 불필요한 세미콜론 제거
public enum UnitType {
-
GENERAL,
GUARD,
CHARIOT,
HORSE,
ELEPHANT,
CANNON,
SOLDIER,
NONE,
- ;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public enum UnitType { | |
| GENERAL, | |
| GUARD, | |
| CHARIOT, | |
| HORSE, | |
| ELEPHANT, | |
| CANNON, | |
| SOLDIER, | |
| NONE, | |
| ; | |
| } | |
| public enum UnitType { | |
| GENERAL, | |
| GUARD, | |
| CHARIOT, | |
| HORSE, | |
| ELEPHANT, | |
| CANNON, | |
| SOLDIER, | |
| NONE | |
| } |
| public enum Team { | ||
|
|
||
| HAN, | ||
| CHO, | ||
| ; | ||
|
|
||
| public Team getOpposite() { | ||
| if (this == HAN) { | ||
| return CHO; | ||
| } | ||
| return HAN; | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
팀 구현이 깔끔하고 getOpposite 메서드가 유용해 보입니다.
장기 게임의 두 팀(HAN, CHO)을 명확하게 정의하고, 반대 팀을 쉽게 찾을 수 있는 유틸리티 메서드를 포함한 것이 좋습니다.
다음과 같은 사소한 개선을 고려해 보세요:
- 각 팀에 대한 Javadoc 문서화 추가 (한이 무엇이고 초가 무엇인지 설명)
- 4번 줄의 불필요한 공백 제거
public enum Team {
-
HAN,
CHO,
;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public enum Team { | |
| HAN, | |
| CHO, | |
| ; | |
| public Team getOpposite() { | |
| if (this == HAN) { | |
| return CHO; | |
| } | |
| return HAN; | |
| } | |
| } | |
| public enum Team { | |
| HAN, | |
| CHO, | |
| ; | |
| public Team getOpposite() { | |
| if (this == HAN) { | |
| return CHO; | |
| } | |
| return HAN; | |
| } | |
| } |
| @Test | ||
| @DisplayName("경로의 도착지를 반환한다.") | ||
| void test1() { | ||
| // given | ||
| List<Position> positions = List.of( | ||
| Position.of(0, 0), | ||
| Position.of(0, 1), | ||
| Position.of(0, 2), | ||
| Position.of(0, 3)); | ||
| Route route = Route.of(positions); | ||
|
|
||
| // when | ||
| Position destination = route.searchDestination(Position.of(0, 4)); | ||
|
|
||
| // then | ||
| assertThat(destination).isEqualTo(Position.of(0, 0)); | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
searchDestination 메서드 테스트 로직이 직관적이지 않습니다.
테스트는 잘 구성되어 있지만, searchDestination 메서드의 동작이 이름과 일치하지 않는 것 같습니다. 현재 구현을 보면 가장 가까운 위치가 아닌 가장 먼 위치(최대 거리)를 찾고 있습니다.
- 메서드 이름이
searchDestination이면 일반적으로 목적지(가장 가까운 위치)를 찾을 것으로 예상됩니다. 하지만 테스트와 실제 구현은 가장 먼 위치를 찾고 있습니다. - 두 가지 해결 방법이 있습니다:
- 메서드 이름을
searchFarthestPosition으로 변경 - 구현을 변경하여 가장 가까운 위치를 반환하도록 수정
- 메서드 이름을
🏁 Script executed:
#!/bin/bash
# Route 클래스의 searchDestination 메서드 구현 확인
cat src/main/java/domain/position/Route.java | grep -A 10 "searchDestination"Length of output: 710
메서드 네이밍 불일치 및 구현 의도 검토 필요
- 현재
searchDestination메서드는 전달받은 기준 위치에 대해, 리스트 내 위치들 중 최대 거리를 가진(즉, 가장 먼) 위치를 반환하도록 구현되어 있습니다. - 일반적으로 메서드 이름이
searchDestination이라면 목적지, 즉 가장 가까운 위치를 반환할 것으로 기대됩니다. - 두 가지 해결 방법을 고려해 주세요:
- 메서드 이름을
searchFarthestPosition으로 변경하여 구현과 일치시키거나, - 구현을 수정해 기준 위치에 가장 가까운 위치를 반환하도록 변경
- 메서드 이름을
추가 검토:
- 해당 구현은
positions.stream().max(Comparator.comparingDouble(...))를 사용해 최대값을 찾고 있으므로, 의도한 로직에 대한 명확한 재검토가 필요합니다.
| @Test | ||
| @DisplayName("경로의 도착지를 제외한 위치들을 반환한다.") | ||
| void test2() { | ||
| // given | ||
| List<Position> positions = List.of( | ||
| Position.of(0, 0), | ||
| Position.of(0, 1), | ||
| Position.of(0, 2), | ||
| Position.of(0, 3)); | ||
| Route route = Route.of(positions); | ||
|
|
||
| // when | ||
| List<Position> exceptDestination = route.getPositionsExceptDestination(Position.of(0, 4)); | ||
|
|
||
| // then | ||
| assertThat(exceptDestination).hasSize(3) | ||
| .contains( | ||
| Position.of(0, 1), | ||
| Position.of(0, 2), | ||
| Position.of(0, 3) | ||
| ); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
getPositionsExceptDestination 메서드 테스트 로직도 직관적이지 않습니다.
test2에서의 테스트 로직도 메서드 이름과 일치하지 않는 것 같습니다. 현재 구현은 가장 먼 위치를 제외한 모든 위치를 반환하고 있습니다.
- 메서드 이름이
getPositionsExceptDestination이면 목적지(일반적으로 가장 가까운 위치)를 제외한 위치들을 반환할 것으로 예상됩니다. - 두 가지 해결 방법:
- 메서드 이름을
getPositionsExceptFarthestPosition으로 변경 - 구현을 변경하여 가장 가까운 위치를 제외하도록 수정
- 메서드 이름을
또한, 테스트 이름(test1, test2)을 좀 더 설명적으로 바꾸는 것이 좋습니다:
- void test1() {
+ void searchDestinationShouldReturnFarthestPosition() {- void test2() {
+ void getPositionsExceptDestinationShouldReturnAllPositionsExceptFarthest() {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @Test | |
| @DisplayName("경로의 도착지를 제외한 위치들을 반환한다.") | |
| void test2() { | |
| // given | |
| List<Position> positions = List.of( | |
| Position.of(0, 0), | |
| Position.of(0, 1), | |
| Position.of(0, 2), | |
| Position.of(0, 3)); | |
| Route route = Route.of(positions); | |
| // when | |
| List<Position> exceptDestination = route.getPositionsExceptDestination(Position.of(0, 4)); | |
| // then | |
| assertThat(exceptDestination).hasSize(3) | |
| .contains( | |
| Position.of(0, 1), | |
| Position.of(0, 2), | |
| Position.of(0, 3) | |
| ); | |
| } | |
| @Test | |
| @DisplayName("경로의 도착지를 제외한 위치들을 반환한다.") | |
| void getPositionsExceptDestinationShouldReturnAllPositionsExceptFarthest() { | |
| // given | |
| List<Position> positions = List.of( | |
| Position.of(0, 0), | |
| Position.of(0, 1), | |
| Position.of(0, 2), | |
| Position.of(0, 3)); | |
| Route route = Route.of(positions); | |
| // when | |
| List<Position> exceptDestination = route.getPositionsExceptDestination(Position.of(0, 4)); | |
| // then | |
| assertThat(exceptDestination).hasSize(3) | |
| .contains( | |
| Position.of(0, 1), | |
| Position.of(0, 2), | |
| Position.of(0, 3) | |
| ); | |
| } |
| public interface UnitRule { | ||
|
|
||
| List<Route> calculateAllRoute(Position start); | ||
|
|
||
| UnitType getType(); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Janggi 클래스의 책임 분배에 대한 의견
PR 설명에 있는 Janggi 클래스의 책임에 대한 질문에 답변드립니다:
Janggi 클래스가 너무 많은 책임을 가지고 있다면, 첫 번째 접근 방식(각 게임 말이 자체적으로 이동 경로 필터링)이 단일 책임 원칙(SRP)에 더 부합합니다. 이렇게 하면:
- 각 유닛(Unit)은 자신의 이동 규칙(UnitRule)을 통해 가능한 경로를 계산하고 유효하지 않은 경로를 필터링합니다.
- Janggi 클래스는 게임 상태 관리와 규칙 적용에만 집중할 수 있습니다.
- 새로운 유닛 타입을 추가하거나 기존 유닛의 규칙을 변경할 때 Janggi 클래스를 수정할 필요가 없어집니다.
결론적으로, 유닛이 자신의 경로 필터링을 담당하는 방식을 채택하는 것이 객체 지향 설계 원칙에 더 부합합니다. 다만, 이동 경로가 다른 유닛에 의해 차단되는 등의 게임 전체 상태에 의존하는 규칙이 있다면, 이는 Janggi 클래스에서 관리하는 것이 적절할 수 있습니다.
🧹 Nitpick (assertive)
UnitRule 인터페이스 설계가 잘 되었습니다.
UnitRule 인터페이스는 단위 규칙을 정의하는 좋은 방법입니다. 각 유닛 타입의 이동 규칙을 별도의 클래스로 분리하면 코드 구조가 개선되고 객체 지향적인 설계가 됩니다.
PR 설명에서 언급한 UnitRule 관련 질문에 대한 제 의견은 다음과 같습니다:
- Unit이 UnitRule을 가지는 구조는 객체 지향적 원칙(특히 전략 패턴)을 잘 따르고 있습니다.
- 이렇게 하면 다형성을 활용하여 각 유닛의 움직임 로직을 캡슐화할 수 있습니다.
- 또한 각 유닛의 이동 규칙을 독립적으로 테스트하기 쉽습니다.
다음과 같은 개선사항을 제안합니다:
- 메서드에 Javadoc 추가
calculateAllRoute메서드가 반환하는 리스트가 변경 불가능한지(immutable) 명시
/**
* 유닛의 이동 규칙을 정의하는 인터페이스입니다.
*/
public interface UnitRule {
/**
* 주어진 시작 위치에서 이동 가능한 모든 경로를 계산합니다.
* @param start 시작 위치
* @return 가능한 모든 경로 리스트 (불변)
*/
List<Route> calculateAllRoute(Position start);
/**
* 이 규칙이 적용되는 유닛의 타입을 반환합니다.
* @return 유닛 타입
*/
UnitType getType();
}| private List<Position> calculateEndPoints(Position start) { | ||
| int x = start.getX(); | ||
| int y = start.getY(); | ||
| List<Position> xPositions = IntStream.range(0, Position.X_MAX + 1) | ||
| .filter(element -> element != x) | ||
| .mapToObj(element -> Position.of(element, y)) | ||
| .toList(); | ||
| List<Position> yPositions = IntStream.range(0, Position.Y_MAX + 1) | ||
| .filter(element -> element != y) | ||
| .mapToObj(element -> Position.of(x, element)) | ||
| .toList(); | ||
| return Stream.concat(xPositions.stream(), yPositions.stream()) | ||
| .toList(); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
효율적 합성을 위한 스트림 통합 고려
calculateEndPoints() 메서드에서 x축, y축 위치를 각각 스트림으로 만든 뒤, Stream.concat으로 최종 합치는 방식을 사용 중입니다. 가독성은 좋지만, x/y를 동시에 처리하는 단일 스트림 로직으로 합치는 방법도 고려해볼 수 있습니다. 다만 현재 코드도 충분히 명확하므로 가독성 측면에서 유지하셔도 괜찮습니다.
|
|
||
| ### 출력 형식 | ||
|
|
||
| ``` |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
코드 블록 언어 표기 권장 (MD040)
32번 라인에 fenced code block(```)이 언어 없이 사용되었습니다. 정적 분석 툴에서 해당 블록에 언어를 명시하라고 합니다. 예:
```bash
...
```
와 같이 작성하면 좋겠습니다.
🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
32-32: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
| - [x] 도착지가 규칙에 벗어나면 예외를 발생시킨다. | ||
| - [x] 도착지에 적군이 있다면 잡는다. | ||
|
|
||
| ### 장기말(기물), Unit | ||
|
|
||
| - [x] 해당 기물이 위치로부터 갈 수 있는 모든 위치를 반환한다. | ||
| - [x] 반환하는 경로는 다른 기물의 위치를 고려하지 않는다. | ||
| - [x] 장기판 내의 좌표인지는 고려한다. |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
마크다운 인덴트 규칙(MD007) 지적
정적 분석 도구에서 언급한 대로, 목록(-, *)의 들여쓰기가 표준 규칙(MD007)을 만족하지 않는 부분이 있습니다. 마크다운 문서의 가독성을 위해, 여기서 지적된 부분(예: 12번~19번 라인)들을 2칸 들여쓰기로 맞춰주시면 좋겠습니다.
🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
12-12: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
13-13: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
18-18: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
19-19: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
| - [x] `CannonUnitRule#calculateEndPoint()` 접근제한자 `private`으로 변경 | ||
| - [x] `OutputView`에서 if문에 도달하지 않는 경우 예외 발생시키기 | ||
| - [x] `Janggi` 외부에서 주입하기 | ||
| - [ ] 주입하는 위치 고민 |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
목록 들여쓰기 추가 수정 필요 (MD007)
66번 라인에서도 목록 들여쓰기를 2칸으로 조정하면 좋겠습니다.
🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
66-66: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
| public static boolean isCanBePosition(Point point) { | ||
| if (point.getX() < 0 || point.getX() > X_MAX) { | ||
| return false; | ||
| } | ||
| return !(point.getY() < 0 || point.getY() > Y_MAX); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
isCanBePosition 메서드 네이밍 개선 제안
isCanBePosition이라는 메서드 명이 다소 어색해 보일 수 있습니다. 예를 들어 canBePosition, isValidPositionRange 등 좀 더 직관적인 이름을 고려해 보세요.
There was a problem hiding this comment.
Actionable comments posted: 8
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Lite
📒 Files selected for processing (5)
src/main/java/domain/position/DefaultUnitPosition.java(1 hunks)src/main/java/domain/unit/rule/ElephantUnitRule.java(1 hunks)src/main/java/domain/unit/rule/HorseUnitRule.java(1 hunks)src/test/java/domain/unit/rule/ElephantUnitRuleTest.java(1 hunks)src/test/java/domain/unit/rule/HorseUnitRuleTest.java(1 hunks)
🧰 Additional context used
🧬 Code Definitions (3)
src/main/java/domain/position/DefaultUnitPosition.java (7)
src/main/java/domain/unit/Unit.java (1)
Unit(8-34)src/main/java/domain/unit/rule/CannonUnitRule.java (1)
CannonUnitRule(11-69)src/main/java/domain/unit/rule/ChariotUnitRule.java (1)
ChariotUnitRule(11-64)src/main/java/domain/unit/rule/ElephantUnitRule.java (1)
ElephantUnitRule(11-39)src/main/java/domain/unit/rule/HorseUnitRule.java (1)
HorseUnitRule(11-39)src/main/java/domain/unit/rule/NoneUnitRule.java (1)
NoneUnitRule(8-19)src/main/java/domain/unit/rule/SoldierUnitRule.java (1)
SoldierUnitRule(10-34)
src/main/java/domain/unit/rule/HorseUnitRule.java (2)
src/main/java/domain/position/Point.java (1)
Point(3-24)src/main/java/domain/position/Route.java (1)
Route(7-35)
src/main/java/domain/unit/rule/ElephantUnitRule.java (2)
src/main/java/domain/position/Point.java (1)
Point(3-24)src/main/java/domain/position/Route.java (1)
Route(7-35)
🔇 Additional comments (5)
src/main/java/domain/unit/rule/ElephantUnitRule.java (1)
20-33: 매직 넘버 제거 및 가독성 개선을 권장합니다.
depth == 3이라는 하드코딩된 값을 상수로 분리하면 의도가 더욱 분명해지며 유지보수가 쉬워집니다.아래 예시처럼 상수를 도입하는 방안을 제안드립니다:
public class ElephantUnitRule implements UnitRule { + private static final int MAX_DEPTH = 3; @Override public List<Route> calculateAllRoute(Position start) { List<Route> routes = new ArrayList<>(); dfs(0, Direction.NONE, new ArrayList<>(), Point.of(start.getX(), start.getY()), routes); return routes; } private void dfs(int depth, Direction before, List<Point> route, Point prevPoint, List<Route> routes) { - if (depth == 3) { + if (depth == MAX_DEPTH) { if (route.stream().allMatch(Position::isCanBePosition)) { routes.add(Route.of(route.stream().map(Position::from).toList())); } return; } ... }src/main/java/domain/unit/rule/HorseUnitRule.java (1)
20-33: 추상화 리팩토링 제안 (기존 중복 리뷰와 동일)
이dfs메서드는 과거 리뷰(Comment by coderabbitai[bot])에서 언급되었듯이, 다른 유닛 규칙(특히 ElephantUnitRule)과 매우 유사한 로직으로 보입니다. 추상 클래스를 활용해 공통 부분을 모아두면 코드 중복이 줄고 유지보수하기 쉬워질 것입니다.src/main/java/domain/position/DefaultUnitPosition.java (3)
28-31: Y 좌표 고정으로 인한 유연성 부족 문제가 다시 지적되었습니다.
이전 리뷰에서도 언급되었듯, 특정 팀(한, 초)에 대해 Y 좌표가 하나로 고정된 상태는 확장성 면에서 제약을 초래합니다. 여러 가지 시작 배치나 변형 규칙을 지원하려면 Y 좌표 목록이나Position객체 배열 등을 활용하는 방안을 고려해 보세요.
40-50: Team 유효성 검사가 누락되었습니다.
이전에 지적된 동일한 문제로,Team이CHO또는HAN이 아닌 경우 유닛 배치를 암묵적으로HAN으로 처리하게 됩니다. 안전한 사용을 위해 예외 처리를 추가하는 것이 좋습니다.public static Map<Position, Unit> createDefaultUnits(DefaultUnitPosition position, Team team) { + if (team != Team.CHO && team != Team.HAN) { + throw new IllegalArgumentException("팀은 CHO 또는 HAN만 유효합니다."); + } if (team == Team.CHO) { return position.xPositions.stream()
22-23:❓ Verification inconclusive
CANNON, SOLDIER 배치 좌표 검증이 필요합니다.
표준 장기 규칙에서 포와 졸(병)은 서로 다른 행(또는 열)에 배치되며, 팀에 따라 위치가 달라질 수 있습니다.hanY = 2,choY = 7/hanY = 3,choY = 6설정이 올바른지 최종적으로 확인해주세요.
🏁 Script executed:
#!/bin/bash # Description: CANNON(포), SOLDIER(졸/병) 배치 좌표가 장기 규칙과 일치하는지 확인 rg -A 5 'CANNON' rg -A 5 'SOLDIER' # 위 결과를 통해 관련 코드 사용처를 찾아, 장기 규칙에 맞는지 확인해 주세요.Length of output: 4549
CANNON과 SOLDIER 좌표 설정 재검토 필요
- 파일
src/main/java/domain/position/DefaultUnitPosition.java의 22-23 라인에서 CANNON은(2, 7), SOLDIER은(3, 6)으로 설정되어 있습니다.- 검색 결과, 해당 좌표들이 코드 전반(예:
Janggi.java,OutputView.java등)에서 일관되게 사용되고 있음을 확인했습니다.- 다만, 표준 장기 규칙에 따르면 포와 졸(병)은 서로 다른 행(또는 열)에 배치되어야 하므로, 현 설정이 게임 규칙 의도와 부합하는지 최종적으로 재검토해 주시기 바랍니다.
| List<Route> routes = elephantUnitRule.calculateAllRoute(Position.of(1, 9)); | ||
|
|
||
| // then | ||
| assertThat(routes).hasSize(2); |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
테스트 검증 범위를 넓혀주세요.
단순히 경로 리스트의 크기만 확인하면 불충분할 수 있습니다. 코끼리의 이동 특성에 맞는 구체적인 위치 검증(예: 실제 도착 지점 확인 등)을 추가하여 테스트 신뢰도를 높이는 것을 권장합니다.
| void test2() { | ||
| // given |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
테스트 메서드 이름을 좀 더 명확히 표현해주세요.
현재 test2라는 이름은 테스트 의도를 바로 파악하기 어렵습니다. 예를 들어 elephantUnitShouldCalculateAllPossibleRoutes 등 의미를 드러내는 이름을 권장합니다.
| @Test | ||
| @DisplayName("말") | ||
| void test1() { |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
테스트 메서드 및 @DisplayName을 더 구체적으로 작성해주세요.
test1, "말" 등은 테스트의 목적을 파악하기 어렵습니다. 의미를 담은 네이밍(ex: horseUnitShouldCalculateAllPossibleRoutes)을 사용하면 가독성과 유지보수성이 향상됩니다.
| // then | ||
| assertThat(routes).hasSize(4); |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
루트의 개수만 확인하는 테스트를 보강해주세요.
경로의 크기만 검증하면 말의 이동 규칙이 제대로 적용되는지 충분히 파악하기 어렵습니다. 도착 지점 좌표나 중간 경로 등의 구체적인 검증을 추가해 테스트 신뢰도를 높이는 것을 추천드립니다.
| @Override | ||
| public UnitType getType() { | ||
| return UnitType.HORSE; | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
명시적인 주석 추가 고려
getType()가 UnitType.HORSE를 반환한다는 것은 명확하지만, 본 메서드가 말 이동 규칙을 식별하는 핵심 역할을 하므로, 필요하다면 다른 곳에서도 용이하게 식별될 수 있도록 간단히 주석을 설명해 주시면 좋을 것 같습니다.
| public class HorseUnitRule implements UnitRule { | ||
|
|
||
| @Override | ||
| public List<Route> calculateAllRoute(Position start) { | ||
| List<Route> routes = new ArrayList<>(); | ||
| dfs(0, Direction.NONE, new ArrayList<>(), Point.of(start.getX(), start.getY()), routes); | ||
| return routes; | ||
| } | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick (assertive)
객체 책임 분리에 대한 고려사항
해당 메서드(calculateAllRoute)가 DFS 과정을 직접 호출하고 있는데, 이후 다른 말(코끼리 등)의 이동 규칙과도 유사한 로직을 공유한다면, 공통 로직을 추상화하여 재사용할 수 있는 구조를 고려해보세요. 현재는 단일 클래스 수준에서만 사용되고 있지만, 동일한 방식으로 경로를 탐색하는 다른 말이 늘어날수록 코드 중복이 발생할 가능성이 큽니다.
| private void dfs(int depth, Direction before, List<Point> route, Point prevPoint, List<Route> routes) { | ||
| if (depth == 2) { | ||
| if (route.stream().allMatch(Position::isCanBePosition)) { | ||
| routes.add(Route.of(route.stream().map(Position::from).toList())); | ||
| } | ||
| return; | ||
| } | ||
| for (Direction direction : before.getNext()) { | ||
| Point next = Point.of(prevPoint.getX() + direction.getX(), prevPoint.getY() + direction.getY()); | ||
| route.add(next); | ||
| dfs(depth + 1, direction, route, next, routes); | ||
| route.remove(next); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
파라미터 의미 주석 또는 메소드 분리 검토
DFS 로직이 비교적 복잡해 보이므로, depth, before, route, routes 같은 인자의 의미를 메서드에 주석으로 간단히 설명하거나, 일부 로직을 메서드로 분리해 읽기 쉽게 만들면 유지보수성이 향상될 것 같습니다.
| GENERAL(1, 8, List.of(4), NoneUnitRule::new), | ||
| GUARD(0, 9, List.of(3, 5), NoneUnitRule::new), |
There was a problem hiding this comment.
🛠️ Refactor suggestion
GENERAL, GUARD의 이동 규칙이 구현되지 않았습니다.
장기 규칙 상 장군과 사범(장기 규칙에서 사범은 별도 이동 규칙이 필요) 모두 특별한 이동 범위를 가져야 합니다. 현재 NoneUnitRule을 사용함에 따라 실제 게임 규칙이 반영되지 않으므로, 별도의 GeneralUnitRule, GuardUnitRule 등을 도입해 구현하는 방안을 검토해주세요.
| public static final String EMPTY_POINT_EXCEPTION = "해당 위치에 기물이 존재하지 않습니다."; | ||
| public static final String PICK_OPPOSITE_UNIT_EXCEPTION = "상대팀 말은 고를 수 없습니다."; | ||
| public static final String CANNOT_MOVE_EXCEPTION = "이동할 수 없는 도착지입니다."; |
There was a problem hiding this comment.
단순히 테스트를 위해서 접근제한자를 public 으로 두신 것 같은데요. 🤔
개인적인 생각으로는 접근 제한자를 private으로 제한하고 테스트 검증은 해당 문구를 직접 입력하여 검증하는게 낫다고 생각해요.
물론 예외 문구가 변경될 경우 지금의 방식이 편리할 수는 있지만, 추후 다른 곳에서 이 예외 메시지 상수를 실수로 사용할 수도 있을 것 같아요.
| final OutputView outputView = new OutputView(); | ||
| final Game game = new Game(inputView, outputView); | ||
|
|
||
| while (!game.isEnd()) { |
There was a problem hiding this comment.
사소할 수 있지만 부정문보다는 긍정문을 쓰는 것이 조금 더 가독성이 좋다고 생각해요. 😃
지금의 경우에도 isEnd의 부정보다는 isPlaying 같이 현재 게임이 진행중이라면 while 문 안의 로직을 수행한다고 코드가 자연스레 읽힐 것 같아요.
| while (!game.isEnd()) { | |
| while (game.isPlaing()) { |
| private final OutputView outputView; | ||
|
|
||
| public Game(InputView inputView, OutputView outputView) { | ||
| this.janggi = createJanggi(); |
There was a problem hiding this comment.
Turn을 외부에서 주입하는 것이 조금 더 유연성이 높다고 생각해요. 😃
추후 어떤 팀이 선공을 할 것인지 입력을 받는다거나하는 변경이 생겼을 때도 유연하게 대처할 수 있을 것 같아요.
질문 주신 내용에 대해서도 생성자를 비즈니스 규칙에 맞춰 강하게 의존하도록 만들 필요는 없다고 생각해서 외부에서 주입받는 방식이 더 좋다고 생각해요.
다만 Game 객체의 역할이 조금 불분명하게 느껴집니다. 🤔
View에도 의존하고 도메인 객체에도 의존하다보니 일반적으로는 Controller의 역할을 할 것으로 보이는데, 도메인 객체를 또 상태로 가지고있다보니 Game 객체가 어떤 의도로 만들어진 것인지 조금 헷갈리는 것 같아요. 👀
Game객체는 View와의 의존성을 가지고있는 것 같은데, domain 패키지에 두신 것은 domain 모델을 의도하신걸까요?
Game 객체는 확실하게 (view 의존성 없이) 장기 게임을 진행하는 역할을 위임해보면 어떨까요?
| private final int hanY; | ||
| private final int choY; |
There was a problem hiding this comment.
초기 y 좌표에 변경이 생기면 변경이 생길 수 있다는 것에는 저도 동의합니다. 확장성을 고려하는 것은 필수적이지만, 너무 미래에 대한 확장을 열어 놓는 것은 불필요하다고 생각해요.
저도 DRY 원칙을 참 좋아하기 때문에 매우 공감하는 말이에요.
첫번째로는 장기의 초기 Y위치가 변경될 여지가 현저하게 적다는 점입니다. X좌표는 추후 상차림이라는 규칙으로 인하여 바뀔 가능성이 있지만 같은 유닛이 다른 Y좌표를 가질 수 있다는 규칙은 존재하지 않기 때문입니다.
먼저 첫번째 말씀은 저도 어느정도 동의합니다만, 너무 순수하게 전통적인 장기 게임으로만 생각해도 안될 문제라고 생각해요. 😃
온라인 게임으로 장기 게임을 서비스회사라면 꼭 전통적인 방식의 장기게임만 제공하라는 법은 없으니깐요. 😉
두번째로는 사이드 이펙트가 거의 없다고 생각합니다. 추후에 Y좌표를 한 팀의 장기말에서 동일하지 않는다는 규칙이 생길 때 바뀌어도 늦지 않을 것이라고 생각해요.
만약 요구사항이 변경되어 로키가 제시해준 방법으로 바꾸었을 때 다른 클래스들이 영향을 받는다면 문제가 심해질 것이지만, 나중에 그런 상황이 발생해도 DefaultUnitPoisition이라는 클래스 하나만 수정해도 외부에 영향이 없기 때문이에요.
지금은 규모가 작기 때문에 상대적으로 리팩터링이 쉬울 수 있지만, 실제로 이미 운영되고있는 서비스이고 또 규모도 커진다면...
생각한 것보다 변경이 쉽지 않을 수도 있다고 생각해요.
그래서 어느정도는 확장성을 고려하며 코드를 구현하는 것도 중요하다고 생각해요.
그래도 DefaultUnitPosition의 (getter를 써서 직접 처리하는 로직없이) 공개 메서드만 사용하고있는 상황이라면 추후 수정하는 것이 그리 어렵지 않을 것 같다는 말에는 동의합니다. 😃 (단지... 다른 팀원들이 공개 메서드만 쓴다는 보장은 없기 때문에... 이렇게 이상적으로 코드가 유지보수될지는 확신하기는 어려울 것 같네요)
그리고 무엇보다도...
저희는 미션 요구사항에서 3개 이상의 인스턴스 변수를 가진 클래스를 만들지 않는다.라는 요구사항을 만족해야하기도하죠. 😅
저는 인스턴스 변수를 줄이는 방면을 고민하다보니 각팀의 y좌표와 x좌표들을 따로 관리하는 것보다는 묶음으로 관리하는게 낫지 않나 싶었어요. 😃
차니의 의견도 충분히 납득이되기 때문에 제가 제안드린 방식이 꼭 아니더라도 상관없으니 미션 요구사항에 맞게 인스턴스 변수 갯수를 줄여보면 좋을 것 같아요. 😃
| for (Direction direction : before.getNext()) { | ||
| Point next = Point.of(prevPoint.getX() + direction.getX(), prevPoint.getY() + direction.getY()); | ||
| route.add(next); | ||
| dfs(depth + 1, direction, route, next, routes); |
There was a problem hiding this comment.
재귀함수를 사용하고 계시네요. 😃
재귀함수는 단순해서 가독성이 좋다는 장점있지만, StackOverFlow가 발생할 여지가 있기 때문에 우려되는 방식인 것 같아요.
재귀함수를 가능하면 반복문으로 변경해볼 수는 없을까요? 🤔
| } | ||
| count++; | ||
| } | ||
| return (count == 1); |
There was a problem hiding this comment.
1이 어떤 의미를 가지는건지 이해하기가 조금 어려운 것 같네요. 🤔
상수로 뽑고 네이밍을 통해서 의도를 드러내보면 어떨까요?
| } | ||
|
|
||
| private boolean canCannonJump(Position current, Route route) { | ||
| Position endPoint = route.searchDestination(current); |
There was a problem hiding this comment.
point와 position은 엄연히 다른 용도로 만드셨다고 했던 것 같은데요.
네이밍도 혼용되게 사용되지 않는게 좋을 것 같아요. 😃
| int count = 0; | ||
| for (Position position : route.getPositionsExceptDestination(current)) { | ||
| if (isEmptyPosition(position)) { | ||
| continue; | ||
| } | ||
| Unit unit = units.get(position); | ||
| if (unit.getType() == UnitType.CANNON) { | ||
| return false; | ||
| } | ||
| count++; | ||
| } | ||
| return (count == 1); | ||
| } |
There was a problem hiding this comment.
조금 중복이될 수도 있지만, 지금 코드는 한번에 여러 계산을하려다보니 조금 복잡하게 느껴지는 것 같아요.
아래와 같이 변경해보는건 어떻게 생각하시나요? 😃 (혹시 해당 피드백을 반영하신다면 의도를 드러낼 수 있는 더 좋은 네이밍을 고려해주시면 좋을 것 같습니다 😉)
| int count = 0; | |
| for (Position position : route.getPositionsExceptDestination(current)) { | |
| if (isEmptyPosition(position)) { | |
| continue; | |
| } | |
| Unit unit = units.get(position); | |
| if (unit.getType() == UnitType.CANNON) { | |
| return false; | |
| } | |
| count++; | |
| } | |
| return (count == 1); | |
| } | |
| var positions = route.getPositionsExceptDestination(current); | |
| var hasCannon = positions.stream() | |
| .filter(units::containsKey) | |
| .map(units::get) | |
| .anyMatch(unit -> unit.getType() == UnitType.CANNON); | |
| return !hasCannon && onlyOne(positions); | |
| } | |
| private boolean onlyOne(List<Position> positions) { | |
| var count = positions.stream() | |
| .filter(units::containsKey) | |
| .map(units::get) | |
| .count(); | |
| return count == 1; | |
| } |
|
|
||
| import java.util.Objects; | ||
|
|
||
| public class Position { |
There was a problem hiding this comment.
Point는 실제 도메인 객체에서는 사용되지 않고 유효하지 않은 x,y 범위도 임시로 담기위한 용도로 이해를 했는데요. 🤔
이 부분은 Position 객체를 생성하는 정적 팩토리 메서드를 만들어보면 어떨까요?
유효하지 않은 범위의 x, y 값이 들어오면 null을 반환하고 유효한 범위가 들어온다면 Position을 생성하도록한다면 Point 객체가 불필요하지 않을까요?😃
| public List<Direction> getNext() { | ||
| if (this == NONE) { | ||
| return List.of(UPPER, UNDER, LEFT, RIGHT); | ||
| } | ||
| if (this == UPPER) { | ||
| return List.of(UPPER_LEFT, UPPER_RIGHT); | ||
| } | ||
| if (this == UNDER) { | ||
| return List.of(UNDER_LEFT, UNDER_RIGHT); | ||
| } | ||
| if (this == LEFT) { | ||
| return List.of(UPPER_LEFT, UNDER_LEFT); | ||
| } | ||
| if (this == RIGHT) { | ||
| return List.of(UPPER_RIGHT, UNDER_RIGHT); | ||
| } | ||
| return List.of(this); | ||
| } |
There was a problem hiding this comment.
이 부분은 Swtich Expression을 활용해보는 것도 좋을 것 같아요.😃
체크 리스트
test를 실행했을 때, 모든 테스트가 정상적으로 통과했나요?객체지향 생활체조 요구사항을 얼마나 잘 충족했다고 생각하시나요?
1~5점 중에서 선택해주세요.
급하게 코드를 작성하다보니, 세세한 디테일에 신경을 크게 쓰지 못하였습니다.
선택한 점수의 이유를 적어주세요.
어떤 부분에 집중하여 리뷰해야 할까요?
안녕하세요 로키, ✋
장기 미션을 구현하는데 상당히 애먹었습니다.
이번 미션에서 제가 가장 집중한 것은 객체간의 소통에 집중하고자 하였습니다.
급하게 구현을 하느라 도메인에 대해서 아직 예외 처리도 더 필요하고, 분리할 부분이 많다고 생각합니다.
그래서 첫 리뷰에서는 객체들 사이의 역할이 적절한지, 객체 사이의 소통에 이상한 부분이 있는지를 중점으로 중점적으로 리뷰해주셨으면 좋겠습니다.