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

Merged
merged 26 commits into from
May 1, 2022

Conversation

yeon-06
Copy link

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

안녕하세요 미르~!
두번째 PR 제출합니다
마찬가지로 질문 사항은 코멘트로 남기겠습니다 😄

2단계 요구사항

  • 체스방 생성
    • 제목, 비밀번호 입력받기
    • 생성 시 만들어지는 고유값은 url에 활용
  • 체스방 목록 조회
    • 제목 클릭 시 체스방 참가
    • 삭제 버튼 클릭 시 체스방 삭제
      • 비밀번호 입력받기
      • (e) 진행중인 체스방은 삭제 불가능
      • 올바른 비밀번호라면 삭제

chessService.playGame(id, new MoveEvent(moveRoute));
GameDto gameDto = chessService.findGame(id);
@PostMapping("/move")
public ModelAndView playGame(@RequestBody MoveRouteDto moveRoute) {
Copy link
Author

@yeon-06 yeon-06 Apr 27, 2022

Choose a reason for hiding this comment

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

갑자기 @RequestBody로 받은 클래스에 대해 아래와 같은 오류가 뜨기 시작했어요

com.fasterxml.jackson.databind.exc.InvalidDefinitionException: Cannot construct instance of '클래스명' (no Creators, like default constructor, exist): cannot deserialize from Object value

나름대로 찾아봤는데 '원래' 빈 생성자가 있어야만 @RequestBody 또는 @RequestParam으로 받을 수 있다라고들 하던데 정확한 원인을 찾기 힘드네요💦
이전에는 왜 돌아갔는지도 이해가 안돼요ㅠㅠ

Copy link

Choose a reason for hiding this comment

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

저도 이 부분을 깊게 파헤치지 않아서 깊이 있는 내용은 잘 모르겠네요 😭
추측이지만, 필드가 전부 final로 선언된 경우는 기본 생성자가 없어도 잘 동작하는 것 같네요.
reflection을 이용하여 직렬화 / 역직렬화를 하는데, 만약 final이 없다면 기본 생성자를 사용하는 걸로 알고 있어요. 같이 공부해보아요 !_!

참고 내용 1, 참고 내용 2

try {
return jdbcTemplate.queryForObject(sql, paramSource, rowMapper);
} catch (EmptyResultDataAccessException e) {
return null;
Copy link
Author

@yeon-06 yeon-06 Apr 27, 2022

Choose a reason for hiding this comment

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

queryForObject는 query와는 다르게 조회 결과가 없으면 Exception이 발생해 처리한 부분입니다!
다만 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을 반환하는 방향이 꺼려지는 이유가 있을까요?

Copy link
Author

Choose a reason for hiding this comment

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

null을 반환할 가능성이 있으면 API를 호출하는 (ex: 프런트) 쪽에서 null에 대한 처리를 해줘야하기 때문입니다!

Copy link

Choose a reason for hiding this comment

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

반환을 꼭 해야할까하는 생각이드네요 ㅎ_ㅎ..
만약 에러가 발생하는거라면 에러를 발생해도 괜찮지 않을까요?

- 게임 삭제 post -> delete
- 피스 이동 /move -> /{id}
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.

안녕하세요 연로그! 2단계 미션도 빠르게 진행해주셨네요 ㅎ_ㅎ
간단히 코멘트 남겼으니 확인해주시면 감사하겠습니다 🙏🏻

@@ -12,11 +12,13 @@ public class ExceptionResolver {

@ExceptionHandler
public ResponseEntity<String> handleException(Exception e) {
e.printStackTrace();
Copy link

Choose a reason for hiding this comment

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

아직 로그는 배우기 전이니 해당 글 참고만 해주시면 감사하겠습니다 😄

Copy link
Author

@yeon-06 yeon-06 Apr 29, 2022

Choose a reason for hiding this comment

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

이번 미션과는 무관하지만 이전에 Log4j나 Slf4j 사용 경험은 있어서 궁금해서 한번 테스트 해보았습니다
(현재 import된 라이브러리 중 기본적으로 내장된 slf4j을 사용했습니다)

제가 작성해본 테스트 코드는 아래와 같습니다.
아래 코드가 실행되면 '메시지 부분만' 출력이 됩니다.

Logger logger = LoggerFactory.getLogger(ExceptionResolver.class);
logger.error(e.getMessage());

로그를 보는 대상은 개발자일 것이고 오류 메시지만 출력되는게 아닌 어디서 오류가 났는지 세세하게 알아야 유지보수가 편할 것 같아요 (e.printStackTrace() 처럼요)
실무에서 어떤 식으로 오류를 남기시는지 궁금합니다!

  • (추가)
logger.error("error: ", e);

이런 식으로 추가하니까 printStackTrace()처럼 모두 출력은 되네요!!😄

Copy link

@ddu0422 ddu0422 Apr 29, 2022

Choose a reason for hiding this comment

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

log.error()메서드에 Exception객체를 직접 넘기는 e.printStackTrace()처럼 Exception의 스택도 모두 남겨줍니다. 에러의 추적성을 높이기 위해서는 e.toString()이나 e.getMessage()로 마지막 메시지만 남기기보다는 전체 에러 스택을 다 넘기는 편이 좋습니다.


이런 내용이 있네요!
저도 logger.error("error: ", e)처럼 사용합니다 😄 연로그의 의견대로 어디서 오류가 났는지를 알아야 빠르게 파악하고 대처할 수 있기 때문입니다!

chessService.playGame(id, new MoveEvent(moveRoute));
GameDto gameDto = chessService.findGame(id);
@PostMapping("/move")
public ModelAndView playGame(@RequestBody MoveRouteDto moveRoute) {
Copy link

Choose a reason for hiding this comment

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

저도 이 부분을 깊게 파헤치지 않아서 깊이 있는 내용은 잘 모르겠네요 😭
추측이지만, 필드가 전부 final로 선언된 경우는 기본 생성자가 없어도 잘 동작하는 것 같네요.
reflection을 이용하여 직렬화 / 역직렬화를 하는데, 만약 final이 없다면 기본 생성자를 사용하는 걸로 알고 있어요. 같이 공부해보아요 !_!

참고 내용 1, 참고 내용 2

return ResponseUtil.createModelAndView(HTML_TEMPLATE_PATH, gameDto);
}

@DeleteMapping("/{id}")
public void delete(@RequestBody DeleteGameRequest deleteGameRequest) {
Copy link

Choose a reason for hiding this comment

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

@PathVariable에서 @RequestBody를 사용하는 객체 내부로 이동한 이유가 있을까요?
저는 해당 글을 참고하여 @PathVariable을 사용해요..!
객체안에 있으면, 가독성이 떨어진다고 생각하는데 연로그의 생각은 어떤가요?

Copy link
Author

@yeon-06 yeon-06 Apr 29, 2022

Choose a reason for hiding this comment

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

여러번의 수정을 거쳤던 메서드인데요

delete는 데이터를 쿼리 파라미터로 보내던 기억이 있어 post로 설정
-> move와 url과 status 코드 동일
-> url에서 가져오던 id를 json에서 받아오도록 수정
-> delete도 메시지 바디에 데이터를 받아올 수 있다는 것을 알고 delete로 롤백

하는 과정에서 id가 dto로 받아오도록 유지되었습니다
어차피 둘 다 클라이언트에서 보내주는 값이니 객체 안에 있으나 밖에 있으나 차이가 없다고 생각하여 별도로 수정하지 않았습니다!

id를 따로 빼서 다른 로직을 수행했어야 했다면 빼는 편이 나을거라 생각하나 지금의 경우에는 괜찮을거라는 생각을 한 것 같아요!

Copy link

Choose a reason for hiding this comment

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

객체안에 있다면 PathVariable 관련 내용을 사용했는지 사용하지 않았는지 한눈에 알기 어렵다고 생각해요!
기존 도메인을 파악할 때, 탐색을 한 단계라도 줄이기 위해 저는 따로 사용하는 편입니다 😄

Comment on lines 11 to 12
public MoveRouteDto() {
}
Copy link

Choose a reason for hiding this comment

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

private로 변경해도 괜찮지 않을까요?

Comment on lines 99 to 102

public List<GameInfoDto> selectAllGames() {
return gameDao.selectAll();
}
Copy link

Choose a reason for hiding this comment

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

트랜잭션의 read only 속성을 사용해보는 건 어떨까요?

Copy link
Author

@yeon-06 yeon-06 Apr 30, 2022

Choose a reason for hiding this comment

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

제가 공부하면서 이해하기로는 @Transactional은 트랜잭션의 단위를 메서드로 지정할 수 있게 도와주는 어노테이션인 것 같아요!
여기서 한가지 궁금한 점이 생겼습니다!
@Transactional을 기입하지 않는 경우에 여러번 쿼리문을 실행하면 모두 다 다른 트랜잭션을 통해 실행되나요?
이 부분을 디버깅 / 콘솔에 print하며 확인해볼 방법이 있을까요?

Copy link

Choose a reason for hiding this comment

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

우선 @Transaction이 없다면, Application에선 확인을 할 수 없을거예요.
Application에서 따로 설정하지 않았으니 DB의 기본 Transaction전략을 따를거예요..!

@Transactional을 있는 경우도 확인을 할 수 있을지 잘모르겠네요 😂
propagation을 확인해본다면 다른 트랜잭션으로 동작하는지 아닌지 생각할 수 있을 것 같아요!

저도 연로그의 질문을 받아 DB에서 처음으로 확인을 해봤네요 ㅎ_ㅎ..
performance_schema DB에서 확인해보면 될 것 같아요!

Application에선 PlatformTransactionManager로 확인을 해보려 했는데 생각대로 나오지 않네요.. 😭

try {
return jdbcTemplate.queryForObject(sql, paramSource, rowMapper);
} catch (EmptyResultDataAccessException e) {
return null;
Copy link

Choose a reason for hiding this comment

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

null을 반환하는 방향이 꺼려지는 이유가 있을까요?

}

int result = gameDao.delete(deleteGameRequest.getId());
if(result == 0) {
Copy link

Choose a reason for hiding this comment

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

공백 컨벤션 확인해주세요!

}

@PostMapping
@GetMapping(params = "game_id")
Copy link

Choose a reason for hiding this comment

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

저는 해당 글을 참고하여 밑줄대신 하이픈을 선호하긴 하는데 연로그의 생각이 궁금해요 ㅎ_ㅎ

Copy link
Author

@yeon-06 yeon-06 Apr 29, 2022

Choose a reason for hiding this comment

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

저는 다 카멜케이스나 스네이크 케이스를 사용하는줄 알았는데 하이픈을 사용하는 케이스도 있었군요!! ㄴ😮ㄱ
수정 시 반영하겠습니다ㅎㅎ

@@ -17,17 +17,17 @@ public EventDao(NamedParameterJdbcTemplate namedParameterJdbcTemplate) {
this.namedParameterJdbcTemplate = namedParameterJdbcTemplate;
}

private final RowMapper<Event> eventRowMapper = (resultSet, rowNum) ->
Event.of(resultSet.getString("type"),
private final RowMapper<EventEntity> eventRowMapper = (resultSet, rowNum) ->
Copy link
Author

Choose a reason for hiding this comment

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

테이블과 일대일 매핑되는 entity라는 클래스를 별도로 분리했습니다!
나중에 event의 type, description 반환 -> event의 type만 반환 식으로 반환해야하는 값이 달라지면 매번 쿼리문이나 dao의 메서드 등을 수정해야하는 상황을 방지하기 위함입니다!

@@ -12,5 +13,7 @@ public interface Game {

GameResult getResult();

GameDto toDtoOf(int gameId);
Copy link
Author

@yeon-06 yeon-06 Apr 30, 2022

Choose a reason for hiding this comment

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

도메인에서 dto를 생성한다 = dto 가 수정되면 도메인을 수정해야한다

라고 생각해서 약간의 로직을 바꿨습니다!
dto.of(도메인) 의 방식으로 dto를 생성할 수 있게 수정했습니다

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.

안녕하세요 연로그! 미르예요 !

이번 미션 정말 고생하셨습니다 ㅎ_ㅎ
이만 머지해도 될 것 같아서 머지할게요.
추가로 궁금한 점이 있으시다면 편하게 코멘트 남겨주시거나 DM주세요 !

스프링 세계에 오신 걸 진심으로 환영합니다 😎

Comment on lines +46 to +56
public ModelAndView playGame(@PathVariable int id
, @RequestBody MoveRouteDto moveRoute) {
chessService.playGame(id, moveRoute);
GameDto gameDto = chessService.findGame(id);
return ResponseUtil.createModelAndView(HTML_TEMPLATE_PATH, gameDto);
}

@DeleteMapping("/{id}")
@ResponseStatus(HttpStatus.NON_AUTHORITATIVE_INFORMATION)
public void delete(@PathVariable int id
,@RequestBody DeleteGameRequest deleteGameRequest) {
Copy link

Choose a reason for hiding this comment

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

기존 내용이 컨벤션처럼 보이는데 컨벤션이 바뀌었을까요?

Copy link
Author

@yeon-06 yeon-06 May 1, 2022

Choose a reason for hiding this comment

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

앗 아니요ㅠㅠ 값을 수정하다 미처 확인을 못한 것 같습니다😂
기존 내용이 맞습니다!

Copy link

Choose a reason for hiding this comment

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

아하..! 그럴 수 있져 ㅎ_ㅎ..

Comment on lines 22 to +28
return new ResponseEntity<>(DEFAULT_SERVER_EXCEPTION_MESSAGE, HttpStatus.INTERNAL_SERVER_ERROR);
}

@ExceptionHandler
public ResponseEntity<String> handleException(EmptyResultDataAccessException e) {
logger.error(EMPTY_STRING, e);
return new ResponseEntity<>(NO_DATA_EXCEPTION_MESSAGE, HttpStatus.NOT_FOUND);
Copy link

Choose a reason for hiding this comment

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

인스턴스 생성 or Builder 둘 중 하나로 통일하는 건 어떨까요?

Comment on lines +8 to +9
List<GameEntity> games;
GameCountDto gameCount;
Copy link

Choose a reason for hiding this comment

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

해당 필드들은 클래스 내부에서만 사용하는데 접근제한자를 가장 낮게 설정하는건 어떨까요?
final이여도 괜찮지 않을까요?

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