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,5단계 제출합니다. #148

Merged
merged 43 commits into from
Apr 21, 2020

Conversation

ksy90101
Copy link

@ksy90101 ksy90101 commented Apr 8, 2020

안녕하세요. 이번 리뷰도 잘 부탁드리겠습니다.
코드가 너무 지저분 해서 ... 계속해서 리팩토링 진행하겠습니다..ㅠㅠ
따끔한 피드백 부탁드리겠습니다!

moonyoungCHAE and others added 30 commits April 2, 2020 23:50
* docs: 1단계 요구사항 작성

* feat: 위치 정보 생성

* feat: 체스 말 생성

* feat: 체스판 초기화 및 테스트

* feat: 명령어 입력 기능 구현

* feat: 체스판 출력 기능 구현

* refactor: 2중 for문 stream으로 수정

* docs: Level2 요구사항 추가

* feat: 기물이 움직이는 방법을 나타내는 MoveRule Interface 구현

* feat: 각 기물에 맞는 움직임 상태 추가

* feat: move 명령어를 입력했을 때, King 이동 가능한 범위인지 확인

* feat: 사용자 추가, 체스 기물 분리

* feat: 이동 시 장애물 처리를 위한 이동 경로 확인

* refactor : 이동 경로 장애물 확인 수정

* feat: Knight 이동 구현

* faet : Pawn 이동 기능 구현

* refactor: Pawn 예외 처리

* refactor: Pawn 예외 추가 처리 및 전반적인 수정

* test : 킹 기물에 대한 테스트 수정 및 테스트 추가, empty 테스트 추가

* test : 체스 기물에 대한 테스트 케이스 추가

* test: Chess Board 리팩토링

* docs: 3단계 요구 사항 추가

* feat: 체스 점수 계산 구현

* refactor: 불필요한 import 제거

* feat : Pawn 점수 계산 예외 처리

* refactor: 상태를 저장하는 Status 생성

* feat : 승패결과를 구하는 기능 구현

* feat : status 명령 시, 각 플레이어의 점수와 승패 결과를 출력 하는 기능 구현

* 리팩토링한 부분이에요! (#2)

* refactor : 입력 로직에 대한 리팩토링

* refactor : controller while문을 do while로 변경

* refactor : Custom Exception 생성 및 초기 가이드 출력 리팩토링

* refactor : isSamePlayer 메소드 if문 return으로 리팩토링

* refactor : 나이트 이동 방향 내부 클래스 리팩토링

* refactor : 폰 전진 기능 리팩토링

* reafactor : Direction에 있는 PositionsBetween 중복코드 제거 리팩토링

Co-authored-by: Seyun <ksy90101@gmail.com>

* refacotr: parameter null 체크 추가

* feat: 무승부일 때 추가

* feat: King이 잡혔을 때 게임 종료

* refactor: Column, Row 비교 수정

* test: Display Name 수정

* refactor: Pawn 이동 오류 수정

* refactor: Piece 및 move 검증하는 구조 수정

* refactor : Direction 리팩토링

* refactor : validateTileSize 중복 코드 제거

* refactor: Empty 제거 및 Chess Board Piece 생성 방식 수정

* refactor: 피드백 반영

- Optional 잘못 사용한 부분 수정
- 팩토리 수정
- enum 비교 수정

* refactor: Optional 수정

* refactor: Input 받는 부분 수정

* refactor: exception 분리

* style: formatting

* refactor: Controller 구조 수정

* refactor: Controller 구조 수정

* style: formatting

* refactor : PositionBetween 관련 Class 중복코드 제거

* refactor : 전반적인 컨벤션 수정 및 OutputView 체스판 출력 리팩토링

* 체리픽중 수정

* refactor : ChessController 리팩토링

Co-authored-by: Seyun <ksy90101@gmail.com>
Co-authored-by: SeYun <53366407+ksy90101@users.noreply.github.com>
Copy link

@hongsii hongsii left a comment

Choose a reason for hiding this comment

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

이번 단계도 잘 구현해주셨네요!
몇 가지 피드백 드렸으니, 확인하셔서 반영해주세요~

src/main/java/chess/controller/dao/GameDao.java Outdated Show resolved Hide resolved
src/test/java/dao/ChessBoardDaoTest.java Show resolved Hide resolved
private static final ChessBoardDao chessBoardDao = new ChessBoardDao();
private static final GameDao gameDao = new GameDao();
private static Gson gson = new Gson();
private static final WebController webController = new WebController();
Copy link

Choose a reason for hiding this comment

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

WebUIChessApplicationWebController 의 책임이 모호한 것 같은데요.
둘 다 DAO에 의존성을 맺고 있으며, 비지니스 로직도 처리하고 있습니다.

일반적인 3-tier 구조라면, 요청/응답 처리(controller) -> 비지니스 로직 처리(service) -> DB 로직 처리(dao 혹은 repository) 로 구성해 각 계층 간에 역할을 명확히 구분하며, 인접한 계층끼리만 의존성을 맺습니다.

아래와 같이 책임을 가지도록 바꿔보면 어떨까요?

  • WebUIChessApplication : 클라이언트의 요청을 controller로 전달 및 응답 처리(View 혹은 JSON 변환)
  • WebController : request / response 를 전달 받아 서비스에 필요한 형태로 요청을 가공해 Service로 전달 및 응답 코드 처리
  • Service : 게임 로직 flow 처리 및 DAO를 통한 DB 처리
  • DAO : DB 로직 처리

Copy link
Author

Choose a reason for hiding this comment

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

이 부분에 대해서 좀 더 공부해야 겠습니다! 감사합니다!

src/main/java/chess/WebUIChessApplication.java Outdated Show resolved Hide resolved
try {
int roomNumber = gameDao.findMaxRoomNumber();
ResponseDto responseDto = null;
if (gameDao.findState(roomNumber) == GameStatus.FINISH) {
Copy link

Choose a reason for hiding this comment

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

gameDao.findState(roomNumber) 를 변수로 선언하면 1번의 DB 호출로 로직을 처리할 수 있지 않을까요?

Copy link
Author

Choose a reason for hiding this comment

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

넵! 변경했습니다!

@ksy90101
Copy link
Author

웹에 대한 지식이 많이 부족하네요 ㅠㅠ 계속해서 웹에 대한 공부를 해야 될거 같습니다.. ㅠㅠ 피드백 감사합니다!

Copy link

@hongsii hongsii 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 15 to 16
private static final ChessBoardDao chessBoardDao = new ChessBoardDao();
private static final GameDao gameDao = new GameDao();
Copy link

Choose a reason for hiding this comment

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

아직 controller - service - dao 순으로 인접한 레이어끼리 의존성을 맺고 있는 것이 아니라 아래와 같은 의존성이 존재합니다. dao는 service를 통해서만 접근하도록 바꿔보면 어떨까요?

throw new IllegalArgumentException("게임을 불러올 수 없습니다.");
}

private ResponseDto loadInitGame() throws SQLException {
Copy link

Choose a reason for hiding this comment

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

도메인 모델과 DAO를 사용해 비지니스 로직을 처리하는 것은 service 가 담당해야할 역할입니다.
controller 에서는 요청을 파싱하는 역할만 하도록 바꿔보면 좋을 것 같아요.

Copy link

@hongsii hongsii left a comment

Choose a reason for hiding this comment

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

피드백 반영 잘해주셨네요!
추가로 피드백 드렸으니, 참고하셔서 반영해주세요!

Copy link

@hongsii hongsii left a comment

Choose a reason for hiding this comment

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

피드백 반영 잘 해주셨네요 👍
미션 진행하시느라 고생많으셨습니다!

@hongsii hongsii merged commit 2abb46f into woowacourse:ksy90101 Apr 21, 2020
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.

3 participants