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

[Spring 체스 - 1단계] 써머(최혜원) 미션 제출합니다. #383

Merged
merged 23 commits into from
Apr 27, 2022

Conversation

hyewoncc
Copy link

안녕하세요 핀! 웹체스 미션 매칭 된 써머입니다
도메인 변경 없이 spring framework를 적용해, 정상 작동하게 만드는 것을 중점으로 진행했습니다
이번 미션동안 잘부탁드려요 🙇

Copy link

@lalize lalize left a comment

Choose a reason for hiding this comment

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

써머, 안녕하세요!
1단계 스프링 적용 잘 해주셨네요 👍
간단히 코멘트 남겨두었으니 다음 단계 진행하면서 함께 봐주시면 좋을 것 같아요!
추가로 궁금한 점 있으시면 편하게 DM, 코멘트 남겨주세요~

@@ -0,0 +1,8 @@
package chess.console;
Copy link

Choose a reason for hiding this comment

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

스프링 적용 미션이니 관련 없는 코드는 모두 제거해도 좋을 것 같아요!


@Override
public int save(int roomId, GameStateDto gameStateDto) {
return insertActor.executeAndReturnKey(Map.of("room_id", roomId, "turn", gameStateDto.getTurn()))
Copy link

Choose a reason for hiding this comment

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

SimpleJdbcInsert로 간단히 id를 가져올 수 있네요 👍

Comment on lines +25 to +39
@PostMapping(value = "/{boardId}", params = "command=move")
public ResponseEntity<BoardDto> movePiece(@PathVariable int boardId, @RequestBody CommendDto commendDto) {
gameService.move(boardId, commendDto);
return ResponseEntity.ok(gameService.gameStateAndPieces(boardId));
}

@GetMapping(value = "/{boardId}", params = "command=result")
public ResponseEntity<ResultDto> result(@PathVariable int boardId) {
return ResponseEntity.ok(gameService.gameResult(boardId));
}

@GetMapping(value = "/{boardId}", params = "command=end")
public ResponseEntity<ResultDto> end(@PathVariable int boardId) {
return ResponseEntity.ok(gameService.gameFinalResult(boardId));
}
Copy link

Choose a reason for hiding this comment

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

@PostMapping, @GetMapping 등 각각의 매핑을 어떤 기준으로 사용하신 건지 궁금해요!
HTTP 요청 메서드들이 어떤 의미를 나타내는지 아래 문서를 참고해봐도 좋을 것 같아요~

Copy link
Author

Choose a reason for hiding this comment

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

@GetMapping은 DB에서 데이터를 조회만 하는 메서드에 사용했고, @PostMapping은 요청으로 받은 데이터로 DB에 변화가 일어나는 경우 사용했습니다
movePiece의 경우 프론트에서 클릭한 체스말의 기존 위치, 이동할 위치를 받아, 이동 실행 후 DB에 반영하는 역할을 하고 있는데요,
예를 들어 a1에 있는 말을 a2로 옮길 경우, fk인 boardId와 위치 컬럼값인 a1이 일치하는 말을 piece 테이블에서 조회해 위치를 a2로 업데이트 합니다
그래서 연속으로 같은 요청을 보낼 경우 두 번째에는 해당하는 말을 조회할 수가 없기에 멱등성이 성립하지 않는다고 생각해 put이 아닌 post로 요청을 보냈어요
그런데 올려주신 링크를 읽어보니 부분적인 수정에 해당하는 것 같아서 patch로 수정해야 겠네요

import org.springframework.web.bind.annotation.ControllerAdvice;
import org.springframework.web.bind.annotation.ExceptionHandler;

@ControllerAdvice
Copy link

Choose a reason for hiding this comment

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

컨트롤러의 예외를 한 곳에서 관리하도록 하셨네요 👍

추가로 @RestControllerAdvice와는 어떤 차이가 있는지 알아보시면 좋을 것 같아요!

<h2 class="login-title">Welcome Chess</h2>
<img src="images/black_k.png" class="icon-image">
</div>
<form action="http://localhost:8080/rooms" onsubmit="return checkNameNotEmpty();" method="post">
Copy link

Choose a reason for hiding this comment

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

방을 생성하는 것만 form 방식으로 하게된 의도가 궁금해요~

Copy link
Author

Choose a reason for hiding this comment

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

2단계에서 비밀번호도 입력받아야 한다는 걸 미리 확인해서, 여러 값을 받기 편하게 form을 사용했어요 😅

return "/board.html";
}

@GetMapping(value = "/{roomId}", params = "command=start")
Copy link

Choose a reason for hiding this comment

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

params로 command를 받아 구분하기보다는 url을 /{roomId}/start 처럼 변경해서 의도를 드러내면 더 좋을 것 같다는 생각이 들어요 :)

Copy link

Choose a reason for hiding this comment

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

HTTP API와 RESTful API에 대해 학습하여 비교해보시고 url 디자인을 조금 더 고민해보면 어떨까요?

import org.springframework.stereotype.Repository;

@Repository
public class BoardRepositoryImpl implements BoardRepository {
Copy link

Choose a reason for hiding this comment

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

저는 Dao는 하나의 테이블에 대한 저수준 CRUD 책임을 갖고, Repository는 하나 이상의 Dao를 의존하여 객체 컬렉션의 집합 처리의 책임을 갖는다고 생각하는데요.
써머는 어떤 의미로 Repository라는 이름을 부여 했는지 궁금하네요!

Copy link
Author

Choose a reason for hiding this comment

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

부끄럽지만 이제까지 Dao랑 Repository가 동명이인 패턴이라고 생각했어요 😳
그래서 방금 찾아봤는데 핀이 말하신 것 처럼 다른 패턴이더라고요
이 부분은 더 고민해보고 2단계에 적용하도록 하겠습니다

Comment on lines +36 to +37
int boardId = boardRepository.save(roomId, getGameStateDto(board));
saveNewPieces(board, boardId);
Copy link

Choose a reason for hiding this comment

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

Board를 저장할 때 항상 Pieces도 같이 저장해야 하는 것을 알고 그것을 처리하는 게 서비스의 책임과는 조금 맞지 않은 부분이 있는 것 같아요.

서비스는 단순히 Board를 조회하여 비즈니스 로직을 처리하고 Board를 저장하는 단순한 흐름으로 변경해보면 어떨까요?
(삭제, 수정, 조회도 마찬가지구요!)

위에서 남긴 Dao, Repository 코멘트와 함께 보시면 좋을 것 같네요!

@@ -0,0 +1,23 @@
create table room
Copy link

Choose a reason for hiding this comment

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

테스트를 위한 테이블 자동 생성 👍


@SpringBootTest(webEnvironment = SpringBootTest.WebEnvironment.RANDOM_PORT)
@Import(TestConfig.class)
class SpringChessControllerTest {
Copy link

Choose a reason for hiding this comment

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

각 컨트롤러마다 테스트 클래스를 만들어서 테스트 해보면 어떨까요?

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.

None yet

2 participants