[1,2,3단계 - 체스] 현구막(최현구) 미션 제출합니다. #207
Conversation
open ScoreCalculator constructor add const variable for delete magic number fix stream methods add check pawn method for delete negative operator
|
안녕하세요 닉!! 피드백 주신 내용 반영이 많이 늦었습니다 😭 상태패턴에 빠뜨렸던 테스트들을 추가했고, 유틸클래스를 제거하는 등 Command에 대해 Enum으로 변경을 시도하다가, 각 Command 별 반환이 서로 달라서 감사합니다! |
|
|
||
| public interface MoveStrategy { | ||
|
|
||
| Set<Position> moveStrategy(Board board, Position source); |
There was a problem hiding this comment.
더 나은 메서드명은 없을까요? :)
@Hyeon9mak Author
바로 위에 있는 메서드에서 valid라는 표현을 사용해서, 이 메서드도 isInvalid 표현을 사용하게 바꿨어요! 👍
앗, 아뇨 저는 Board에 있는 메서드가 아니라 moveStrategy에 대한 피드백이었는데, 착오가 있었네요.
제가 여기다가 코멘트를 남길걸 그랬나봐요 😢
There was a problem hiding this comment.
😱 현재 source 좌표에서 이동좌표를 구하는 것이니까, currentPositionMoveStrategy로 변경했어요!
| private long countOfPawnsInVertical(Xpoint xpoint, Color color) { | ||
| List<Position> verticalPositions = verticalPositions(xpoint); | ||
|
|
||
| return this.ranks.stream() | ||
| .map(Rank::squares) | ||
| .map(Map::entrySet) | ||
| .flatMap(Collection::stream) | ||
| .filter(entry -> verticalPositions.contains(entry.getKey())) | ||
| .map(Map.Entry::getValue) | ||
| .filter(piece -> piece.isSameColor(color)) | ||
| .filter(Piece::isPawn) | ||
| .count(); | ||
| } | ||
|
|
||
| private List<Position> verticalPositions(Xpoint xpoint) { | ||
| return this.ranks.stream() | ||
| .map(Rank::squares) | ||
| .map(Map::keySet) | ||
| .flatMap(Collection::stream) | ||
| .filter(position -> position.isSameX(xpoint)) | ||
| .collect(Collectors.toList()); | ||
| } | ||
|
|
||
| private double scoreExceptPawns(Color color) { | ||
| return ranks.stream() | ||
| .map(Rank::squares) | ||
| .map(Map::values) | ||
| .flatMap(Collection::stream) | ||
| .filter(piece -> piece.isSameColor(color)) | ||
| .filter(Piece::isNotPawn) | ||
| .mapToDouble(Piece::score) | ||
| .sum(); | ||
| } |
There was a problem hiding this comment.
음 세 메서드 모두 가공이 상당히 복잡한데요.
일부분은 Rank에서 담당해줄 수 있는 로직도 있지 않을까요? :)
+) 여기 말고도 Rank를 사용하고 있는 다른 곳도 항상 flatMap을 통해 가공을 거치고 있네요 ㅎㅎ
There was a problem hiding this comment.
이것도 역시 일급컬렉션을 일급컬렉션 답게 사용하지 못한거 같네요!!
| import java.util.Map; | ||
| import java.util.stream.Collectors; | ||
|
|
||
| public class ScoreCalculator { |
There was a problem hiding this comment.
점수 계산도 중요한 도메인 로직인데, 테스트를 통한 검증이 필수일 것 같아요 :)
| } | ||
|
|
||
| public Map<Position, Piece> squares() { | ||
| return this.squares; |
There was a problem hiding this comment.
일급컬렉션에서는 담당한 컬렉션을 외부로 반환해야할 때, 외부에서 조작할 수 있으므로 항상 주의해야 합니다.
- 반환하지 않을 수 있다면 가장 Best
- 반환해야만 한다면 불변으로, 혹은 현재 필드와 관계없는 새로운 컬렉션으로 만들어서 반환
There was a problem hiding this comment.
일급컬렉션을 일급컬렉션답게 사용하지 못하고 있었네요 😢
squares를 요청하는 곳에서 Rank에게 메세지를 보낼 수 있는 방법에 대해 고민해보고 적용해보겠습니다!
[OOP] 커맨드 패턴 - 5내용
링크[JDK] flatMap - 3내용
링크 |
안녕하세요 닉! 현구막입니다!!
제출 마감에 쫒겨서 진행하다보니 유닛테스트를 빼먹은 곳도 많고, 일급 컬렉션으로 만들 수 있는 부분도 많이 빠뜨린 것 같아요..
우선 제출부터(😢) 하고 닉이 주시는 피드백과 함께 리팩토링 진행해볼게요!
미션을 진행하면서 궁금한 부분이 몇 가지 있었어요!
InitBoardGenerator의 역할을 InitPieces에 통합하는게 어떨까요?
현재 보드 초기상태를 제공하는 제네레이터 클래스가 존재하고 있어요. 얼마전 포비의 엘레강트 오브젝트 수업을 들으면서
-er명명이 붙는 클래스를 지양하라는 말을 들었는데, 이 클래스를 없애려면 InitPieces 라는 클래스에 통합시키는게 나을지, 아니면 현재 상태를 유지하는게 좋을지 고민 중이에요. 닉이 보기엔 어떠신가요?Controller 는 지나치게 많은 일을 하는 것 같고, ChessGame은 지나치게 하는 일이 단순한 것 같아요..
블랙잭 미션을 하면서도 비슷한 일을 겪었는데, View와 지속적인 대화를 하기 위해서 Controller가 비대해지고, ChessGame은 원시적인
명령만 전달받는 형태로 프로그래밍이 되었어요. 마냥 나쁘다고 하기엔 View와 지속적인 대화를 잇기 위해 어쩔수 없는건가 생각도 들고...
잘못된거면 확 잘못됐다는 확신을 얻고 싶어요!!
Command 안에 ChessGame을, ChessGame 안에 State를, State안에 Board
메세지가 일렬로 전달되도록 구현하는 것을 목표로 했는데, 결국 중간중간에 다른 객체들과 메세지 방향이 뒤섞인 것 같아요 😢
더 좋은 구상 방법이 있었을까 여전히 고민이 되는데, 힌트를 얻을 수 있을까요?!
감사합니다!