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

[team-13 새리] API 구현 #27

Open
wants to merge 66 commits into
base: team-13
Choose a base branch
from

Conversation

sallyjellyy
Copy link
Collaborator

안녕하세요 새리입니다.

게임관련 프로젝트가 처음이어서인지 아님 혼자하는 프로젝트가 처음이어서인지 이번 프로젝트는 설계부터(항상 설계가 제일 어렵다고 느끼지만) 너무 복잡하게 느껴졌습니다. 어차피 설계를 꼼꼼히 한다고 해도 구현하다보면 요구사항 분석으로 돌아가 빠진부분을 찾게될 것 같아 처음에는 테이블을 최소한으로 잡고(Game, Team, Player) 기능도 최소한으로 잡고(ㅠㅡㅠ) 구현을 시작했습니다.

  • 질문
    • 게임 서비스는 게임 레포지토리를 가지고 팀 서비스는 팀 레포지토리만을 가집니다. GameController의 browseAllScore()(line 55-59)를 구현하면서 GameScoreDTO를 만드는 부분까지 서비스단에서 해결하고 싶었지만 gameId로 팀을 불러올 수 있는 상황이 되지 못해 컨트롤러에서 해결하도록 했습니다. 해당 메서드는 gameId를 PathVariable로 받지만 바로 위의 addScore()에서는 teamId를 PathVariable로 받는 부분또한 같은 원인이라고 생각됩니다. 이런 부분은 설계, 관계설정의 문제일까요?
    • 컨트롤러의 구분이 헷갈리기도 했습니다. 서비스는 레포지토리 단위로 구분지었지만 컨트롤러에서는 (위 질문의 메서드와 같이) 두 서비스를 모두 사용하는 부분이 있어 게임서비스, 팀서비스를 다 넣었습니다. 아니면 공통된 엔드포인트 단위로 컨트롤러를 나누는 게 맞는 방법인가요?

감사합니다!

sallyjellyy added 30 commits May 3, 2021 15:52
Game에 id, 게임이 가진 두 팀 아이디를 바탕으로 조인을 이용해 팀 이름 가져오기
에러코드 등 구체적인 예외 구현은 나중에 할 예정
플레이어 테이블이 팀 테이블을 가리킬 수 있도록 스키마도 변경
우선은 아이디로 팀을 찾아주는 메서드만 구현했다
TeamDTO가 팀의 아이디, 팀 이름, 해당 팀의 선수들을 다 불러오고 TeamRecordDTO로 감싸서 내보낸다
기존의 게임, 팀, 플레이어 NotFoundException들이 상속받을 수 있도록 구현
에러코드만 넣으면 그에 맞는 http status와 메세지를 꺼내올 수 있는 static 메서드 구현
만들어 둔 커스텀 예외들을 처리하는 핸들러
close #9
맞춰둔 형식에 맞게 반환하기 위해 GameStatusDTO를 반환한다
어차피 ErrorCode를 통해 생성될 객체이므로 인자로 ErrorCode만 받도록 수정했다
선택한 팀의 isPlayable 상태가 false일 때 발생시키는 예외
average 계산이 필요해서 추가했다
@wheejuni wheejuni self-assigned this May 11, 2021
sphilee pushed a commit that referenced this pull request May 12, 2021
dahun-lee-daji pushed a commit that referenced this pull request May 12, 2021
Copy link

@wheejuni wheejuni left a comment

Choose a reason for hiding this comment

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

어려운 프로젝트를 혼자 진행하고 있으시군요. 응원합니다 👍
GameServiceTeamRepository 를 가져도 된다고 생각하고, 그런 구현을 시도해 보시면 좋겠다는 생각이 드네요.
컨트롤러 설계에 대한 제 생각을 포함해서 이외에도 몇 가지 피드백 남겨보았습니다.
확인하시고, 질문이 있다면 다시 남겨주세요! 감사합니다. 💯


import org.springframework.data.relational.core.mapping.Column;

public class GameListDTO {

Choose a reason for hiding this comment

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

DTO인데 @Column 이 있는 것은 좀 어색한데, DTO인가요? 엔티티인가요?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

아 이 부분도 질문으로 여쭤보려했는데 깜빡했어요! DTO입니다! GameListDTO를 받아오는 레포지토리 부분을 쿼리로 짜고 컬럼명 다 맞추고 MySQL에서 쿼리가 잘 작동하는 부분까지 확인했습니다. 하지만 json으로는 모두 null로 나왔고 @Column을 붙이고 해결했지만 관련부분 검색해보아도 나오는 게 없었습니다ㅠㅡㅠ

Choose a reason for hiding this comment

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

@min27604 댓글이 좀 늦었네요. 죄송합니다. ㅠㅠ
DTO를 받아오는 이라고 하셨는데 어디서 받아오시는 건가요? 보통의 설계로는, 레포지토리를 통해 DB row <-> Entity class 간의 정보 교환을 하고, entity <-> DTO는 다시 한 번 정보 전파 및 가공이 이뤄지는 부분인데요. 혹시 한 단계를 빼먹은 건 아니려나요?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@wheejuni 앗 네 맞아요 DB에서 가져올 때 game의 정보와 team의 정보가 조인으로 섞여있어 바로 DTO로 가져오게 했습니다.. 이렇게 하면 안 되는 거였군요ㅠㅡㅠ

Choose a reason for hiding this comment

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

@min27604 네 맞습니다. 서로 절대 존재를 알아차리지 못하게 해주세요. (DTO - 엔티티 간)
물론 작업하다보면 변환하는 로직이 필요해서 완전히 그렇게 고립시키기는 어렵지만, 최대한 노력해 보시는게 좋아요.

Comment on lines 22 to 23
private final GameService gameService;
private final TeamService teamService;

Choose a reason for hiding this comment

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

/baseball/game, /baseball/team 과 같은 스타일로 컨트롤러가 분리됐으면 좋겠다는 생각이 듭니다.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

네 수정했습니다!

ehgud0670 pushed a commit that referenced this pull request May 14, 2021
ehgud0670 pushed a commit that referenced this pull request May 14, 2021
ehgud0670 pushed a commit that referenced this pull request May 14, 2021
ehgud0670 pushed a commit that referenced this pull request May 14, 2021
ehgud0670 pushed a commit that referenced this pull request May 14, 2021
ehgud0670 pushed a commit that referenced this pull request May 14, 2021
ehgud0670 pushed a commit that referenced this pull request May 14, 2021
ehgud0670 pushed a commit that referenced this pull request May 14, 2021
GameService에 TeamRepository를 주입받아 컨트롤러 안의 로직을 최소화 시켰다
쿼리를 통해 DB에서 바로 DTO로 꺼내왔던 기존의 코드에서 DB에서 entity class로, 또 거기서DTO로 가공해 가져오도록 수정했다
@sallyjellyy sallyjellyy requested a review from wheejuni May 17, 2021 12:17
Copy link

@wheejuni wheejuni left a comment

Choose a reason for hiding this comment

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

9일이나 지났는데 못 보고 있었습니다. 죄송합니다 ㅠㅠ
간단한 피드백 하나 남겼는데 크게 문제는 없어 보이거든요.
스트림 API를 더 적극적으로 활용해서 리팩 해보면 어떨까 싶긴 한데요, 새리가 보시고 필요하다고 생각되면 진행해주세요.

일단 이 PR은 머지하지 않고 피드백만 남겨둘게요. 감사합니다.


import org.springframework.data.relational.core.mapping.Column;

public class GameListDTO {

Choose a reason for hiding this comment

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

@min27604 네 맞습니다. 서로 절대 존재를 알아차리지 못하게 해주세요. (DTO - 엔티티 간)
물론 작업하다보면 변환하는 로직이 필요해서 완전히 그렇게 고립시키기는 어렵지만, 최대한 노력해 보시는게 좋아요.

private Long awayTeamId;

public static GameListDTO of(Long gameId, Team homeTeam, Team awayTeam) {

Choose a reason for hiding this comment

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

👍 좋네요.

Team homeTeam = teamService.browseTeamById(game.getHomeTeamId());
Team awayTeam = teamService.browseTeamById(game.getAwayTeamId());
return new TeamRecordDTO(homeTeam, awayTeam);
return gameService.browseTeamPlayersByGameId(gameId);

Choose a reason for hiding this comment

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

훨씬 깔끔하네요. 이게 컨트롤러가 지향해야 할 모습입니다.

Comment on lines +36 to +41
Iterable<Game> games = gameRepository.findAll();
List<GameListDTO> gameList = new ArrayList<>();
for (Game game : games) {
Long id = game.getId();
gameList.add(GameListDTO.of(id, findHomeTeamByGameId(id), findAwayTeamByGameId(id)));
}

Choose a reason for hiding this comment

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

games 할당 없이 gameRepository.findAll() 에서 출발해서 쭉 뻗어나올 수 있을 것 같은 느낌이 드는데요?
어떻게 할 수 있을까요? .map() 적절히 활용하면 가능하지 않을까요?
이러면 .collect() 해서 바로 리턴까지 한 큐에 가능할 것 같은데요. 물론 취향 문제라 새리가 판단하시는 대로 하면 되긴 해요

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants