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단계 미션 제출합니다. #184

Merged
merged 8 commits into from
May 26, 2020
Merged

[터틀] 체스 스프링 4단계 미션 제출합니다. #184

merged 8 commits into from
May 26, 2020

Conversation

begaonnuri
Copy link
Member

마지막까지 잘부탁드려요!
감사합니다 :)

Copy link

@minuyim minuyim left a comment

Choose a reason for hiding this comment

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

안녕하세요 터틀!
4단계까지 체스 구현하시느라 수고많으셨습니다.
스프링으로 구현한 웹 코드에 대해 리뷰하면서 저도 많이 배울 수 있었습니다.

3단계에서 코드만 보고 리뷰하느라 몇가지 놓친 점이 있어 그 점 코멘트로 남겨볼게요.

  1. schema.sql 파일 이름이 scheme.sql로 잘못 설정된 것 같습니다. 만약 이 부분 사용하신다면 고쳐주셔야 할 것 같습니다.
  2. 테스트 중 update 부분이 깨지는 것 같습니다. 이름을 update를 안하는데 비교를 new_name으로 하다보니 그런 것 같습니다.

Comment on lines +25 to +27
this.board = board;
this.turn = turn;
this.finishFlag = finishFlag;
Copy link

Choose a reason for hiding this comment

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

3개의 인자가 chessGame과 관련된 정보이니만큼 하나로 묶어보면 어떨까요?
https://docs.spring.io/spring-data/jdbc/docs/current/reference/html/#jdbc.entity-persistence.embedded-entities
이 부분을 참고하시면 좋을 것 같아요!

Copy link
Member Author

Choose a reason for hiding this comment

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

ChessGame 엔티티를 만들기엔 비용이 커서 기존 구조대로 하겠습니다!

String board = BoardConverter.convertToString(chessGame.getBoard());
roomRepository.save(new Room(roomName, board, Side.WHITE, FinishFlag.GOING.getSymbol()));
roomRepository.save(
new Room(roomName,
Copy link

Choose a reason for hiding this comment

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

개인적인 생각인데 entity를 만들거나 dto로 변환할 때 chessGame을 인자로 받는 생성 메서드 같은게 있으면 좀 더 편할 것 같네요

Copy link
Member Author

Choose a reason for hiding this comment

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

Room의 생성자로 ChessGame을 받아 내부에서 처리하도록 변경했습니다!

$targetDiv = e.target.closest('div');
targetId = $targetDiv.id;

const movedInfo = await getMovedInfo();
Copy link

Choose a reason for hiding this comment

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

여기 부분인지 잘 판단이 안되긴 하는데
서버 로직 상 예외처리는 되는데 웹에서는 자유자재로 움직이네요

Copy link
Member Author

Choose a reason for hiding this comment

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

검증 로직 추가했습니다!

await getRooms();

const $rooms = document.querySelectorAll(`.room`);
$rooms.forEach(room => room.addEventListener('click', onRoomClick));
Copy link

Choose a reason for hiding this comment

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

이 부분을 이벤트 위임으로 바꾸어보면 어떨까요?

Copy link
Member Author

Choose a reason for hiding this comment

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

이벤트 위임으로 변경했습니다!


function onCreateBtnClick(e) {
let roomName = prompt('방 이름을 입력해주세요.');
$createInput.setAttribute('value', roomName);
Copy link

Choose a reason for hiding this comment

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

이 부분에서 사용자가 입력을 취소하면 null 이름을 가진 방이 생성되네요
입력을 안하면 생성이 안되도록 바꿔보면 어떨까요?

Copy link
Member Author

Choose a reason for hiding this comment

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

입력했을 경우만 방이 생성되도록 변경했습니다!

@woowahan-pjs woowahan-pjs merged commit e53a149 into woowacourse:begaonnuri May 26, 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