레디 미션 제출#23
Open
reddevilmidzy wants to merge 61 commits intowoowacourse-6th-code-review-study:reddevilmidzyfrom
Open
레디 미션 제출#23reddevilmidzy wants to merge 61 commits intowoowacourse-6th-code-review-study:reddevilmidzyfrom
reddevilmidzy wants to merge 61 commits intowoowacourse-6th-code-review-study:reddevilmidzyfrom
Conversation
* docs: 기능 요구 사항 작성 * feat : 8 * 8 의 체스 판 생성 * feat: 체스 말 객체 생성 * feat : 한 칸의 정보를 가진 클래스 생성 * refactor: 패키지 변경 * feat: point equals & hashCode 구현 * feat: 체스 말 배치 * feat : 검은색 말은 대문자, 흰색 말은 소문자로 표현 * refactor: message 패키지 생성 * feat: 체스 게임 시작 메시지 출력 * feat: 게임 시작(start)/종료(end) 입력 받기 * feat : end 가 입력되면 프로그램 종료 * feat: 예외 시 재입력 기능 구현 * refactor: gameBoard의 필드 객체를 Piece로 변경 * feat : 보드의 해당 위치의 기물 정보 반환 기능 구현 * refactor: 테스트 코드 메서드 명 및 디스플레이 네임 수정 * feat: move 시 기물이 없는 위치일 경우 예외 발생 기능 구현 * feat: 이동할 말의 현재 위치, 이동할 위치 입력 받기 및 말의 위치 이동 구현 * refactor: gameBoard 필드를 List<List<>> 대신 Map으로 변경 * refactor : piece에서 posotion 삭제 * feat : Pawn 움직임 가능 여부 기능 구현 * feat : Pawn 이동 기능 구현 대각선 제외 * feat : 기물 이동 GameBoard에 업데이트 * fix: 출력 시 NPE 발생하던 문제 수정 * refactor: 사용하지 않는 클래스 삭제 * feat: 시작 메시지 출력 구현 * fix: target 위치에 아무 기물이 없을 때 예외 발생 수정 * feat : Knight 이동 기능 구현 * feat: Bishop 이동 기능 구현 * feat : 게임 보드 출력 시 좌표 안내 기능 추가 * feat : Rook 이동 기능 구현 * feat: Queen 이동 기능 구현 * feat : King 이동 기능 구현 * refactor : 테스트 케이스 가독성 향상 * refactor: 추상 클래스 Piece의 불필요한 메서드 제거 * refactor: 패키지 구조 변경 * refactor: moving 클래스에서 레코드로 변경 * docs: 구현 기능 목록 추가 * feat: 현재 턴의 해당하는 진영만 이동 가능 * refactor : Position 생성자 가독성 향상을 위한 매개변수 순서 변경 * refactor : 인뎁스 1로 변환 * refactor: GameBoardDto 생성 * feat : Pawn 대각선 이동 구현 * feat : 처음에 start외 다른 명령어 입력시 예외 처리 구현 * refactor: turn 변경 camp 에 메서드 추가 * fix: 운영체제 별 줄바꿈 맞춤 * refactor: application 메서드 분리 * refactor: moving 레코드 안에 움직임 방향 확인 메서드 추가 * refactor : toString 사용 대체 * refactor : toString 재정의 * refactor : 방향 찾는 기능 Moving으로 이동 * test : Queen checkRoute Test * test: 퀸 이동 경로 확인 테스트 * test: 룩 이동 경로 확인 테스트 * test: 비숍 이동 경로 확인 테스트 * refactor : Pawn의 이동 검증 기능 GameBoard에서 Pawn으로 이동 * refactor: 테스트 하나로 합침 * refactor: getRoute 메서드 명 변경 및 board 메서드 분리 * refactor : 이동 경로 생성 메서드 중복 제거 * refactor: 이동 방향 인덱스 구하는 과정을 Direction enum 으로 이동 * refactor : PieceType 패키지 이동 * refactor : 인뎁스 감소 * refactor : Moving을 class로 변경 * feat: 재입력 기능 구현 * test : Knight 테스트 케이스 추가 * style : TODO 주석 제거 * refactor: controller 코드 메서드 분리 * style: todo 주석 제거 * refactor : 테스트 불변화 적용 * refactor: 메서드 및 변수에 final 추가 * docs: 기능 요구 사항 수정 * docs: 리팩터링 목록 추가 * test: Camp의 toggle 테스트 추가 * refactor: dto 메서드 분리 * test: moving의 방향 확인 테스트 추가 * feat: 유효하지 않은 포지션일 경우 예외 발생 기능 구현 * docs: 우선순위 낮은 구현 기능 목록 추가 * refactor: Position fixtures 생성 * feat: 커스텀 예외 추가하여 IllegalArgumentException 대체 * refactor: 게임 진행을 상태로 만들어 리팩터링 * refactor: row, column enum 명을 rank, file로 변경 * refactor: dto 변환 스트림 활용 * test: 상태 테스트 추가 * refactor: king 의 움직임 가능 확인 책임을 Moving으로 이동 * feat: 예외 시 재입력 기능 구현 * feat: 예외 메시지 출력 기능 구현 * docs: 게임 상태 및 명령어 설명 추가 * refactor: GameBoard 클래스명 ChessBoard로 변경 * refactor: Knight의 움직임 가능 확인 책임을 Moving으로 이동 * test: command 테스트 추가 * refactor: dto 변환 로직 스트림 활용 * feat: File 입력 시 대문자도 허용 * refactor: 컨트롤러의 메서드 책임 분리 * test: 기물들의 시작 위치 확인 * refactor: 파라미터, 변수 앞에 final 추가 * test: chessboard 테스트 보충 * test: pawn 움직임 테스트 추가 * test: running 예외 발생 테스트 추가 * style: 코드 포맷팅 수정 * refactor: ChessBoard 생성 시 바로 체스 기물 세팅 * refactor: 변수에 final 추가 * test: 움직이지 않을 경우 예외 테스트 추가 * refactor: 테스트 명 및 displayName 변경 * refactor: 명령어 인덱스 매직넘버 제거 * refactor: command 사이즈를 상수 대신 enum 필드로 변경 * refactor: dto에서 사용되는 매직넘버 상수화 * refactor: printCurrentCamp 메서드 명 변경 * docs: 완료한 리팩터링 목록 수정 * refactor: dto에 현재 턴 정보 포함 * refactor: List<String>이었던 커맨드들을 일급 컬렉션으로 변경 * refactor: dto 변환 스트림 대신 메서드 분리 * refactor: 불필요한 emtpy 커맨드 제거 * refactor: board를 일급 컬렉션으로 변경 및 책임 분리 * refactor: 게임 상태 생성 클래스 이름 변경 * refactor: pawn을 추상클래스로 변경하고 색마다 클래스 상속받는 형태로 변경 * fix: toString 메서드 제거 * refactor: 각 기물마다 예외 코드 세분화 * test: command body 확인 테스트 추가 * feat: 도착 지점에 아군이 있는 경우 ErrorCode 추가 * test: 해당 위치에 기물이 없는 경우 예외 발생 테스트 추가 * refactor: 점연산자 한줄에 여럿 있는 부분 분리 --------- Co-authored-by: unifolio0 <boho82ha@gmail.com>
seokmyungham
reviewed
Apr 2, 2024
Member
seokmyungham
left a comment
seokmyungham
left a comment
There was a problem hiding this comment.
레디 전체적으로 깔끔하게 잘 구현하셨군용 👍👍
그런데 더 눈길이 가는건 미친 꼼꼼함의 테스트 코드...
한 치의 빈틈도 허용하지 않으려는 노력이 보입니다 🤣
미션 1 수고 많았습니다 (오예 방학~)
Comment on lines
+15
to
+19
| public class Repository { | ||
|
|
||
| private final MovingDao movingDao; | ||
| private final TurnDao turnDao; | ||
| private final BoardDao boardDao; |
Member
There was a problem hiding this comment.
컨트롤러로부터 데이터를 전달 받고, Dao 로부터 반환된 데이터를 이용하여
하나의 메서드가 한 트랜잭션으로 관리될만한 기능을 수행하고 있네요
해당 클래스가 하는 역할은 Repository보다 Service에 가까운 것 같은데 왜 이름을 Repository로 지었는지 레디의 생각이 궁금해요
Comment on lines
+69
to
+71
| private boolean unsaved(final TurnDto findTurn, final List<MovingDto> findMoving) { | ||
| return findTurn.count() < findMoving.size(); | ||
| } |
Member
There was a problem hiding this comment.
우선 움직임 사이즈가 턴의 개수보다 클 때 저장되지 않았다. 라는 의미를 바로 이해하는게 힘듭니다
그리고 Turn이 Count..?가 왜 필요한지 테이블 시그니처를 봐도 이해가 잘 되지 않네용
턴이 진행된 횟수를 Count라고 생각해봐도 메서드 네이밍때문인지 이해가 안되는 것 같아요
Comment on lines
+53
to
+55
| if (unsaved(findTurn, findMoving)) { | ||
| restore(findTurn, findMoving, board); | ||
| } |
Member
There was a problem hiding this comment.
게임을 찾는다 -> 턴과 움직임이 저장되지 않았으면 -> 게임을 복구한다. 라는 의미가 무슨 뜻일까요?
시그니처만 봤을 때 저장되었으면 복구하고, 저장되지 않았으면 새로운 게임을 시작하는게 자연스러운 흐름이라고 느낍니다
| private static final Camp STARTING_CAMP = Camp.WHITE; | ||
|
|
||
| private Camp camp; | ||
| private Turn turn; |
Member
There was a problem hiding this comment.
클래스 이름이 GameTurn이고, 필드로 Camp가 이미 존재한다면
Turn 이름을 그냥 Count로 해도 괜찮겠다는 생각이 드네용 (그냥 이건 소소한 의견입니다)
Comment on lines
+12
to
+19
| public enum PieceScore { | ||
|
|
||
| KING(King.class, new Score(0)), | ||
| QUEEN(Queen.class, new Score(9)), | ||
| ROOK(Rook.class, new Score(5)), | ||
| BISHOP(Bishop.class, new Score(3)), | ||
| KNIGHT(Knight.class, new Score(2.5F)), | ||
| PAWN(Pawn.class, new Score(1)); |
Member
There was a problem hiding this comment.
각 Piece가 점수 상태를 갖고 있지않고 따로 Enum으로 정의한 이유가 궁금합니다
| repository.save(boardDto, turnDto); | ||
|
|
||
| //then | ||
| final ChessGame game = repository.findGame(); |
| } | ||
|
|
||
| @Test | ||
| @DisplayName("저장된 기보를 확인한다.") |
| assertThat(chessGame.getCamp()).isEqualTo(Camp.WHITE); | ||
| } | ||
|
|
||
| @DisplayName("후공은 BLACK이다.") |
| import model.piece.Piece; | ||
| import model.position.Position; | ||
|
|
||
| public record BoardDto(Map<PositionDto, PieceDto> pieces) { |
Member
There was a problem hiding this comment.
저는 지금 미션까지 Dto를 쓰다가 이번 미션에서는 사용하지 않았습니다
Dto를 하나 만들면 코드 일관성을 지키기 위해 다 만들어야 하는 번거로움이 있는데
레디는 다 만들어버리셨군용 ㅋㅋ 뭔가 움직임을 저장하려다 생긴 MovingDto 때문인 것 같기도 하고,,
| import model.position.Position; | ||
| import model.position.Rank; | ||
|
|
||
| public class BlackPawn extends Pawn { |
robinjoon
reviewed
Apr 3, 2024
Member
robinjoon
left a comment
robinjoon
left a comment
There was a problem hiding this comment.
DAO 와 Repository(Service 가 더 어울릴 것 같아요.)가 인터페이스인 것이 좀 더 테스트에도 유리할 것 같아요.
단위 테스트 라는 책을 읽어보면 좋을 것 같네요
| public static Connection getConnection(final String database) { | ||
| try { | ||
| final String url = "jdbc:mysql://" + SERVER + "/" + database + OPTION; | ||
| return DriverManager.getConnection(url, USERNAME, PASSWORD); |
Member
There was a problem hiding this comment.
Connection은 비싼 자원인데, 매번 새로 생성하는 것 보단 재사용할 수 있을 때 까지 재사용하는 것은 어떤가요?
Comment on lines
+3
to
+10
| import constant.ErrorCode; | ||
|
|
||
| public class ConnectionException extends DBException { | ||
|
|
||
| public ConnectionException(final ErrorCode errorCode) { | ||
| super(errorCode); | ||
| } | ||
| } |
Member
There was a problem hiding this comment.
DB 관련된 예외 클래스를 한 곳에 모아두셨는데, ErrorCode도 마찬가지로 분리하는게 좋을 것 같아요.
Comment on lines
11
to
12
| public class ChessBoardDto { | ||
|
|
Member
There was a problem hiding this comment.
클래스 네이밍이 ChessGameDto 로 변경되는게 더 나을 것 같아요.
Comment on lines
+27
to
+31
| public void removeAll() { | ||
| turnDao.remove(); | ||
| boardDao.remove(); | ||
| movingDao.remove(); | ||
| } |
| create table turn | ||
| ( | ||
| camp varchar(5) not null, | ||
| count int not null |
|
|
||
| @BeforeEach | ||
| void beforeEach() { | ||
| boardDao.remove(); |
Member
There was a problem hiding this comment.
- 매번 데이터베이스를 초기화 하는 것이 유일한 방법일까요?
- boardDao.remove 가 잘못 동작한다면 다른 모든 테스트가 깨질 가능성이 있지 않을까요?
| } | ||
| final Piece piece = pieces.get(nextPosition); | ||
| if (KINGS.contains(piece)) { | ||
| throw new KingDeadException(ErrorCode.KING_DEAD); |
Member
There was a problem hiding this comment.
킹이 죽는 것을 예외를 던져서 처리하는 것이 적절할까요?
Comment on lines
+36
to
+38
| } catch (KingDeadException exception) { | ||
| return new Checkmate(); | ||
| } |
Member
There was a problem hiding this comment.
체크메이트는 킹이 잡힌 상황을 뜻하는 도메인 용어가 아니기 때문에, 적절하지 않은 것 같아요.
Comment on lines
+11
to
+16
|
|
||
| boolean isCheck(); | ||
|
|
||
| default boolean isQuit() { | ||
| return false; | ||
| } |
Member
There was a problem hiding this comment.
default 메서드를 왜 isQuit 메서드에만 사용했나요?
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
꾸벅