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 체스 - 3단계] 다니(이다은) 미션 제출합니다. #299

Merged
merged 26 commits into from
May 2, 2021

Conversation

da-nyee
Copy link

@da-nyee da-nyee commented Apr 25, 2021

안녕하세요, 김고래! 다니입니다 🙌

지난 1, 2단계에 이어 3단계 미션 제출합니다~!
추가 미션은 따로 진행하지 않았어요. 대신 개인 공부를 더 하려고 합니다!

기존에 방 관련 기능이 하나도 없어서 구현에 시간이 약간 걸렸습니다...!
그래도 지난번 마지막에 남겨주신 피드백을 모두 반영했고, 이번에 추가해야 하는 기능도 다 완성했습니다!

요 친구는 다음 단계를 진행하시면서 자연스럽게 없어질것 같은데 thead safe한 방향을 고민해봐주세요^_^

이 피드백은 서비스가 상태를 가지고 있으면 안 된다고 해석했는데 올바르게 이해한 게 맞을까요??
그동안 서비스가 상태를 가지는 게 마음에 걸렸는데, DB 데이터로 객체를 매번 새로 생성하는 방식으로 해결했습니다!

이번에도 부족한 부분은 전부 지적해주세요 !
코드 리뷰를 통해 더 많이 배우고 성장하고 싶습니다 ✨

항상 귀한 시간 내주셔서 감사합니다 🙇‍♀️❣️

@da-nyee da-nyee changed the base branch from master to da-nyee April 25, 2021 19:13
Copy link

@ep1stas1s ep1stas1s left a comment

Choose a reason for hiding this comment

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

넵 코멘트 남긴 의도대로 잘 변경해주셔서 감사합니다 👍
이전보다 많이 깔끔해졌네요 ^_^
몇가지 코멘트 남겼으니 확인 부탁드립니다~!

@@ -19,13 +18,23 @@ public ChessApiController(final ChessService chessService) {
this.chessService = chessService;
}

@PostMapping("/room")
public Long start(@RequestBody final String req) {

Choose a reason for hiding this comment

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

아래 MoveRequestDto처럼 dto를 만들어서 받는 방식이 더 괜찮아보이네요ㅎㅎ

내려줄 때도 마찬가지로 dto를 만들어서 내려주면 클라이언트에서 받기 더 용이할 것 같아요~

return chessService.addRoom(req.split(":")[1].split("\"")[1]);
}

@GetMapping("/board/{roomId}")

Choose a reason for hiding this comment

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

url이 깔끔해졌네요 👍

} catch (Exception e) {
e.printStackTrace();
}
public List<ChessResponseDto> showAllPieces(final Long roomId) {

Choose a reason for hiding this comment

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

DTO, entity의 차이를 짚고 넘어가시면 미션 진행하는데 도움이 되실거에요 ^_^

Copy link
Author

Choose a reason for hiding this comment

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

키워드 감사합니다~! 개인 블로그에 정리하고 학습 로그에 기록했어요! 🙌

filterPieces(whitePieces, blackPieces, chessBoardEntry);
}
return new PiecesDto(whitePieces, blackPieces);
public ChessBoardDto chessBoardFromDB(final Long roomId) {

Choose a reason for hiding this comment

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

일반적으로 DB에서 가져오기 때문에 굳이 db라고 명시하지 않아도 될 것 같습니다 :)

e.printStackTrace();
}
public void initializePieceStatus(final String pieceName, final String piecePosition, Long roomId) {
String query = "INSERT INTO piece (piece_name, piece_position, room_id) VALUE (?, ?, ?)";

Choose a reason for hiding this comment

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

시간이 되신다면 batch insert도 학습해보시면 도움되실 것 같아요
(적용하실 필요는 없습니다!!)

public List<ChessResponseDto> showAllPieces(final Long roomId) {
List<ChessResponseDto> pieces;
String query = "SELECT * FROM piece WHERE room_id = ?";
pieces = jdbcTemplate.query(

Choose a reason for hiding this comment

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

바로 return 해줘도 좋겠네요~

return Objects.requireNonNull(keyHolder.getKey()).longValue();
}

public List<RoomResponseDto> showAllRooms() {

Choose a reason for hiding this comment

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

취향차이일 수도 있지만 일반적으론 find, select, update, insert와 같은 키워드를 많이 사용합니다 ^_^

public String reset() {
chessService.resetRound();
return makeNewGame();
@GetMapping("/room-list")

Choose a reason for hiding this comment

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

음 제가 사용하는 url pattern 방식은 아래와 같이 api의 경우 앞에 api를 명시해주어 일반적인 컨트롤러와 분리합니다
ApiController -> /api/rooms
Controller -> /rooms
이렇게 사용하면 /room-list도 패턴에 맞게 적용할 수 있겠네요 ^_^

@@ -18,10 +19,19 @@

private Map<Position, Piece> board;
private Command command;
private boolean isEnd = false;
private String currentTurn;

Choose a reason for hiding this comment

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

요 친구도 역할이 있나요??

Copy link
Author

Choose a reason for hiding this comment

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

Round에서는 따로 사용하는 메소드가 없지만, Player에서 턴을 기준으로 각 플레이어의 상태를 세팅할 때 필요해서 넣어두었습니다!

@da-nyee
Copy link
Author

da-nyee commented Apr 30, 2021

📚 다니의 학습 로그

[Spring] Entity vs DTO - 3

내용

링크

@da-nyee
Copy link
Author

da-nyee commented Apr 30, 2021

안녕하세요 김고래! 다니입니다 🙌

피드백 감사합니다 ㅎㅎ
전부 참고해서 리팩토링 완료했습니다!

추가적으로,

테...스트가... 안보이네요ㅎㅎ
차차 추가될거라고 굳게 믿고있습니다!!

step1에서 남겨주신 코멘트도 반영했습니다! 테스트 코드도 한번 확인해주시면 감사하겠습니다 🤗
다만, 모든 레이어가 아닌 Controller에 한해서 테스트를 진행했어요. 특히, ApiController를 테스트 했습니다!
스프링 환경에서 테스트를 작성하는 건 처음이라 낯설고 어려웠어요. 그래도 스스로 공부하고 주변에 물어가며 코드를 완성할 수 있었습니다!

몇몇 크루들과 DTO를 어느 레이어까지 전달할 수 있을지 얘기를 하다 궁금한 점이 생겼어요!

Q.

저는 기존에 DTO를 모든 레이어 사이에서 전달하도록 코드를 작성했어요. 따로 레이어별로 DTO를 나누지 않았어요.
근데, 한 크루가 View <-> Controller 사이 DTO와 Repository <-> DAO 사이 DTO를 두는 게 맞다고 주장했어요.
이 크루의 의견을 듣다 보니 이게 맞는 방법인 것 같아 기존과 다르게 DTO를 분리시켰습니다.
김고래는 DTO가 어느 레이어까지 이동할 수 있다고 생각하시나요? View에서 받은 데이터를 Repository까지 전달해도 괜찮은 걸까요??

이번에도 코드 리뷰 잘 부탁드립니다 ~!
매번 귀한 시간 내주셔서 감사합니다 🙇‍♀️✨

@ep1stas1s
Copy link

개인적으론 너무 layer에 얽매이지 않으셨으면 해요~
말씀하신대로 view <-> controller사이에 dto를 가장 많이 사용하지만, 상황에 따라 repository에서 나온 dto가 view까지도 갈 수도 있을 것 같아요
그렇기 때문에 필요에 따라, 레이어 구분에 따라, dto가 어디서 생성되고 어디까지 가냐에 따라 패키지를 분리만 해서 잘 사용하시면 될 것 같습니다ㅎㅎㅎ

Copy link

@ep1stas1s ep1stas1s left a comment

Choose a reason for hiding this comment

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

피드백뿐만 아니라 테스트까지... 👍
이전보다 깔끔해진 게 느껴지네요ㅎㅎㅎ
스프링 적응하시느라 고생하셨습니다 추가적으로 궁금한점 있으시면 언제든 DM주세요~~

import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.status;

@WebMvcTest
public class ChessApiControllerTest {

Choose a reason for hiding this comment

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

MockMvc를 사용한 테스트를 해주셨네요 👍 👍
테스트가 선택사항인지 몰랐었어서...ㅎㅎㅎ;
앞으로도 다양한 테스트 방식을 경험해보셨으면 좋겠습니다 ^_^

public class RoomNameRequestDto {
private final String roomName;

@ConstructorProperties({"roomName"})

Choose a reason for hiding this comment

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

요 어노테이션도 필요한가요???

}

public List<TurnResponseDto> showCurrentTurn() {
return turnDao.showCurrentTurn();
public Map<Long, String> findAllRooms() {

Choose a reason for hiding this comment

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

RoomResponseDto를 반환해도 좋아보이는데 map을 사용하신 이유가 있을까요?

Comment on lines +36 to +38
double whiteScore = chessService.whiteScore(roomId);
double blackScore = chessService.blackScore(roomId);
ScoreResponseDto scoreResponseDto = new ScoreResponseDto(whiteScore, blackScore);

Choose a reason for hiding this comment

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

컨트롤러에서 서비스 호출이 처음보다 줄긴 했지만 요런 부분도 줄여볼 수 있을 것 같아요
서비스 메서드에 roomId를 넘기면 ScoreResponseDto를 반환해주는 느낌으로요 :)
(왠지 DTO때문에 이렇게 하신 것 같네요ㅎㅎ)

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