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단계] 연로그(권시연) 미션 제출합니다. #331

Merged
merged 28 commits into from
Apr 24, 2022

Conversation

yeon-06
Copy link

@yeon-06 yeon-06 commented Apr 22, 2022

안녕하세요 미르!
미션 잘 부탁드립니다~!! 😄

미션 요구사항

  • Spring Framework를 활용하여 애플리케이션을 구동하기
  • Spark를 대체하여 Spring MVC를 활용하여 요청을 받고 응답하기
  • Spring JDBC를 활용하여 DB 접근하던 기존 로직을 대체하기

참고사항

  • 기존 요구사항이었던 콘솔과 Spark는 사용하지 않는 것으로 간주하고 관련 파일을 모두 삭제함
  • 로컬에서 3306 포트가 사용중이라 13306으로 구동되게끔 파일이 수정됨

기타 질문 사항은 코멘트로 달겠습니다!!

bugoverdose and others added 28 commits April 19, 2022 16:58
- implement home.hbs
- setup new SuccessResponseDto
- remove unused files and methods
 - remove SuccessResponseDto and its usages
- fix test fixtures
- update configuration file
- remove unnecessary inner class
- change method and constant names
- map game creation request to GameController method
- remove unnecessary annotation
- wrap String values
- setup db with schema.sql
- rollback each test result with @transactional
final String sql = "SELECT COUNT(*) FROM game WHERE id = :game_id";

MapSqlParameterSource paramSource = new MapSqlParameterSource("game_id", gameId);
int existingGameCount = namedParameterJdbcTemplate.queryForObject(sql, paramSource, Integer.class);
Copy link
Author

Choose a reason for hiding this comment

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

쿼리문상 0이 발생할 수는 있어도 null이 절대 발생할 수 없습니다!
헌데 queryForObject() 메서드의 결과로는 null이 발생할 수도 있다고 노란 전구가 뜨고 있어요
이 경우 매번 null 처리를 해주어야 할까요? 🤔

Copy link

Choose a reason for hiding this comment

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

클라이언트 입장에선 null이 아님을 보장할 수 있지만, 해당 내용을 처리하는 queryForObject는 null이 아님을 보장할 수 없기 때문이 아닐까요 ?_?
@nullable로 선언되어 있기 때문에 null처리를 해주거나 Optional로 감싸보면 어떨까요?


public enum PieceType {

PAWN(PieceType::isPawnMovable, 1, "♟"),
Copy link
Author

@yeon-06 yeon-06 Apr 22, 2022

Choose a reason for hiding this comment

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

요 부분은 이번 미션과 관련된 것은 아니지만 페어와 의견이 갈린 부분이라 미르의 의견을 참고하고 싶어요!
마지막 파라미터로 존재하는 String에 대한 고민입니다!

페어의 의견

  • view를 위해 만들어졌으니까 해당 클래스에 존재하면 안된다. domain과 view가 분리되지 않은 것이다.

저의 의견

  • 기호 하나를 나타내기 위해 util 을 만드니까 너무 많은 문제가 존재한다.(enum과 상수가 같은 순서라고 보장된 상태로 로직이 돌아가고 있음) 하나의 상수라고 생각하고 여기 두어도 괜찮을 것 같다.

Copy link

Choose a reason for hiding this comment

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

저는 domain과 view가 결합되었다고 생각하지 않아요. 클라이언트는 해당 값을 받아서 다른 내용으로 변경하고 있기 때문이죠.

다만, 기호로 표현되어 있기 때문에 해당 문제가 발생하지 않을까요?
♟을 사용하기 보다 PAWN Enum을 그대로 사용하여 Pawn이라면 Pawn 사진을 보여주는 형태로 진행해도 괜찮지 않을까요?

비즈니스 로직상에서 사용하고 있지 않기 때문에 다른 방식으로 풀어나가보면 어떨까요?

return new GameCountDto(totalCount, runningCount);
}

@Transactional
Copy link
Author

Choose a reason for hiding this comment

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

Spring으로 바꾸면서 너무너무 감동 먹었던 부분입니다🥺

서로 다른 Dao에서 둘 중 하나라도 sql 수행이 실패하면 두 개 다 모두 롤백되고 있는데요

이전 미션에서 이 부분을 어떻게 처리할지 참 많이 고민하고 아직까지도 결론이 안났어요
어떤 키워드로 검색해봐야 참고 자료를 찾을 수 있을지조차 감이 좀 안잡히더라고요ㅠㅠ
만약 Spring을 안쓰고 순수 Java와 Jdbc를 사용한다면 이 경우 어떻게 처리하실 것 같으신가요!?

Copy link

Choose a reason for hiding this comment

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

이전 미션에서는 어떤 방식을 고려했었을까요?
JDBC Template Transaction을 이용해보는건 어떨까요?

return new GameResultDto(gameId, game);
}

private Game currentSnapShotOf(int gameId) {
Copy link
Author

Choose a reason for hiding this comment

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

현재 DB에는 이동 기록들이 저장되어있어요
기존 플레이 기록이 있다면 초기 보드판에서 시작해서 이동 기록들을 하나하나 실행하며 마지막 실행 상태를 복구합니다!
페어는 snapshot의 개념을 생각하며 이런 방식을 도입했다고 하시더라고요

제가 이동 기록이 지나치게 많아지면 마지막 실행 상태를 복구하는 것도 일이지 않느냐, 라고 여쭤봤더니 페어가 현재의 상태인 snapshot을 DB에 따로 저장한다고 하더라고요.
어떻게 저장하느냐 라고도 여쭤봤는데 좋은 아이디어가 생각이 안나서 페어와 고민만 하다 끝이 났네요ㅎㅎ

실무에서 DB에 이런 snapshot의 개념을 도입하기도 하는지, 도입 한다면 이런 경우 어떤 방식으로 DB를 설계하는지 궁금해요!

Copy link

Choose a reason for hiding this comment

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

snapshot을 도입하는 목적이 중요할 것 같아요.
실무에서 snapshot을 도입한 경우가 있는데, history가 중요한 내용을 snapshot을 남기기도 합니다.
체스판의 현재 상태를 저장하면 안되는 특별한 이유가 있을까요?

해당 문제도 snapshot으로 발생한 일이 아닐까요? 체스판의 현재 상태를 저장한다면, 해당 내용도 불필요한 논의가 될 것 같긴하네요..!

Copy link

@ddu0422 ddu0422 left a comment

Choose a reason for hiding this comment

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

안녕하세요 연로그! 리뷰어 미르입니다 :)
비즈니스 로직 변경없이 스프링 프레임워크로 잘 변경해주셨군요 ! 💯 💯
@RestController와 @Controller를 적절히 잘 사용해주셨어요 ㅎ_ㅎ
질문사항은 각 코멘트에 답변 드렸어요 😄 비즈니스 로직 관련 내용은 2단계에서 깊이 나누어보아요 :)
level 2 첫 미션 고생하셨습니다 ! 이번에도 열심히 달려보아요 💪🏻

final String sql = "SELECT COUNT(*) FROM game WHERE id = :game_id";

MapSqlParameterSource paramSource = new MapSqlParameterSource("game_id", gameId);
int existingGameCount = namedParameterJdbcTemplate.queryForObject(sql, paramSource, Integer.class);
Copy link

Choose a reason for hiding this comment

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

클라이언트 입장에선 null이 아님을 보장할 수 있지만, 해당 내용을 처리하는 queryForObject는 null이 아님을 보장할 수 없기 때문이 아닐까요 ?_?
@nullable로 선언되어 있기 때문에 null처리를 해주거나 Optional로 감싸보면 어떨까요?

return new GameCountDto(totalCount, runningCount);
}

@Transactional
Copy link

Choose a reason for hiding this comment

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

이전 미션에서는 어떤 방식을 고려했었을까요?
JDBC Template Transaction을 이용해보는건 어떨까요?

return new GameResultDto(gameId, game);
}

private Game currentSnapShotOf(int gameId) {
Copy link

Choose a reason for hiding this comment

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

snapshot을 도입하는 목적이 중요할 것 같아요.
실무에서 snapshot을 도입한 경우가 있는데, history가 중요한 내용을 snapshot을 남기기도 합니다.
체스판의 현재 상태를 저장하면 안되는 특별한 이유가 있을까요?

해당 문제도 snapshot으로 발생한 일이 아닐까요? 체스판의 현재 상태를 저장한다면, 해당 내용도 불필요한 논의가 될 것 같긴하네요..!


public enum PieceType {

PAWN(PieceType::isPawnMovable, 1, "♟"),
Copy link

Choose a reason for hiding this comment

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

저는 domain과 view가 결합되었다고 생각하지 않아요. 클라이언트는 해당 값을 받아서 다른 내용으로 변경하고 있기 때문이죠.

다만, 기호로 표현되어 있기 때문에 해당 문제가 발생하지 않을까요?
♟을 사용하기 보다 PAWN Enum을 그대로 사용하여 Pawn이라면 Pawn 사진을 보여주는 형태로 진행해도 괜찮지 않을까요?

비즈니스 로직상에서 사용하고 있지 않기 때문에 다른 방식으로 풀어나가보면 어떨까요?

Comment on lines +32 to +35
@PostMapping
public SearchResultDto searchResult(@RequestParam(name = "game_id") int gameId) {
return chessService.searchGame(gameId);
}
Copy link

Choose a reason for hiding this comment

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

해당 API는 데이터를 가져오는 행위를 하고 있지 않을까요? 이 API만 PostMapping인 이유가 따로 있을까요?

@ddu0422 ddu0422 merged commit 64a71f1 into woowacourse:yeon-06 Apr 24, 2022
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

3 participants