[BE] TEAM-11 Jane & Jung PR드립니다. by janeljs · Pull Request #34 · codesquad-members-2021/baseball

janeljs · 2021-05-14T04:57:57Z

안녕하세요! Jung & Jane입니다.

너무 늦게 PR을 보내게 되어 죄송합니다😭

데이터 통신 주기

pitch가 일어날 때는 프론트에서 정보를 캐싱해서 저장하고 있다가, 선수 교체를 주기로 서버에 요청을 보내고 해당 정보를 데이터베이스에 저장하는 형식으로 통신했습니다. 선수교체, 팀교체 등 전체적인 로직은 백엔드에서 처리하였습니다. 복잡한 로직을 처리하다보니 한 메서드 안에 관리해야 할 변수가 많아져 적절하게 메서드로 추출하는 등 리팩토링을 진행하지 못하였고, GameService안에 다소 난잡하게 메서드들이 나열되어 있습니다😖

데이터베이스 설계

프론트엔드에서 데이터를 받기 편한 상태로 DTO를 설계한 것과 분리하여 테이블 간 관계를 설정하였어야하는데 그렇게 하지 못했습니다😢 뒤늦게 team과 player를 1:n으로 맵핑해두고, 게임에 따라 바뀌는 match, inning, history, team_game_score, player_game_info를 inning을 중심으로 묶었으면 좋겠다는 생각이 들었지만, 이미 게임 로직이 다 짜여져있어 전체적인 수정이 필요해 시간이 부족하여 그렇게 설계하지 못했습니다.

배포

AWS VPC를 학습하고 private subnet과 public subnet을 따로 생성한 뒤, 웹 서버용 인스턴스와 db용 인스턴스에 각각 지정해주었습니다.
프론트 코드를 포함하여 하나의 jar파일을 만들 수 있는 코드를 build.gradle에 추가한 뒤, 로컬에서 빌드한 jar파일을 s3버켓에 올렸습니다. 이후 작업은 자동화될 수 있도록 크론탭과 셸 스크립트를 이용해 s3 버켓의 변경을 자동으로 감지해 배포되도록 설정하였습니다. 배포 시 프론트와의 협업 채널에 web hook을 이용해 알림이 가도록 설정하였습니다.
웹 서버가 있는 인스턴스에 Nginx를 설치한 뒤 스프링 부트의 내장 톰캣과 연동하였고, 80포트만 외부에 개방해도 동작하도록 conf 파일을 설정해주었습니다.
배포한 주소로 들어가면 아래와같이 포트를 입력해주지 않아도 페이지가 잘 로딩됩니다!

- DTO 설계

- Team와 Player이 History와 각각 1:N관계를 맺도록 클래스에 List<History> 추가

DB 설계 및 mock API 구성

[BE] Controller 메서드 구현

- WebConfig에서 전역에 cross origin을 적용하는 방법에서 컨트롤러별로 적용하는 방식으로 변경

[iOS] Merge

wheejuni

우선 프로젝트를 일정에 맞게 마무리하고 기능 구현을 완료한 것은 칭찬합니다. 👍 💯 수고 많으셨습니다.
그러나 세부적인 구현 면에서 보면 다소 실망스러운 감이 있습니다.
특히 GameService 부분의 경우 제대로 들여다보지 못했는데요, 아무리 읽어도 코드가 잘 이해되지 않는 부분이 많네요.
GameService 뿐만이 아니라, 프로젝트 전반의 코드들을 다시 들여다보며, 아래 원칙에 입각하여 더 좋은 설계를 고민해 주시면 좋겠습니다.

원칙

분리한다. 객체가 비대해지지 않도록 관리한다.
- 3개 이상의 private 메소드가 만들어진다면 우린 무언가 잘못 하고 있는 것이다.
도메인 객체에게 더 많은 역할 을 부여한다.
감싼다, int, String 과 같은 기본 클래스에 의존하는 로직을 걷어낸다.
- 유효성 검증이 필요한 모든 값은 감싸는 것이 원칙이다.
Component, Service 선언하는 것에 부담감을 가지지 않는다. 책임 분리를 위해 필요하다면 과감히 그렇게 한다.
꺼내지 말고 가공한다. 가공하지 말고 물어본다.

수고 많으셨습니다. 👍

wheejuni · 2021-05-14T14:35:17Z

BE/src/main/java/codesquad/baseball/DTO/ApiResponse.java

+@Getter
+@AllArgsConstructor
+@JsonPropertyOrder({"matchId", "inning", "nextHitter", "expeditionTeam", "homeTeam", "pitcher", "hitter", "teamLog"})
+public class ApiResponse {


헐, 하나의 클래스에 이렇게 모든 정보가 담겨있어야 할까요?
클라이언트 개발자들과 협의한 결과겠죠? 그렇다면 어쩔 수 없겠지만요.

선수가 교체될때마다 업데이트된 정보를 반영해 (선수 히스토리, 공수교대, 득점 등) 백엔드에서 화면에 필요한 모든 정보를 response해주기로 협의했습니다. 필요한 모든 정보를 하나로 묶기 위한 DTO클래스라고 생각해주시면 좋을 것 같습니다 ㅜㅜ

wheejuni · 2021-05-14T14:35:39Z

BE/src/main/java/codesquad/baseball/DTO/ApiResponse.java

+import lombok.Getter;
+
+@Getter
+@AllArgsConstructor


조금 위험한 생성자인 것은 알고 있을 것 같네요.
이걸 개선하려면 어떻게 해야할지도 같이 고민해주셨으면 좋았겠는데요. 아쉽네요.

여기 에 따르면 같은 타입의 필드 변수의 순서가 바뀔 경우, @AllArgsConstructor로 생성자 매개변수의 순서도 자동으로 바뀌어 문제가 발생할 수 있다고 합니다.
애노테이션 사용없이 직접 생성자를 사용하거나, 빌더를 쓰면 좋을 것 같은데 생각하지 못했던 것 같습니다. 지적 감사합니다!

wheejuni · 2021-05-14T14:36:23Z

BE/src/main/java/codesquad/baseball/DTO/PlayerDTO.java

+        this.pitchCount = player.getPlayerGameInfo().getPitchCount();
+        this.plateAppearances = player.getPlayerGameInfo().getPlateAppearance();
+        this.hits = player.getPlayerGameInfo().getHits();


어려운 말로 디미터 법칙 위반 이라고도 하던데요.
이걸 개선하려면 어디서 어떤 수정이 이뤄져야할까요?

생성자에 PlayerGameInfo를 직접 넣어주면 게터를 연달아 사용하지 않을 수 있을 것 같습니다.

다만 이것은 임시방편인 것 같습니다. 사실 Player과 PlayerGameInfo가 설계상 엮여있으면 안되는 객체라고 (뒤늦게) 생각했습니다. Player에는 Match에 영향받지 않는, 무관한 정보만 담기도록하고, 선수의 타석, 안타 수 등 경기에 따라 다른 값들은 Match에 담겨있는 것이 맞는 것 같습니다.
만약 시간을 돌이킬수있다면, PlayerGameInfo를 Match를 aggregate root로 하도록 엮고, Player은 Team와 함께 Match와 별도의 aggregate가 되도록 수정하고 싶네요ㅜㅜ 위의 PlayerDTO 생성자에는 Player과 PlayerGameInfo를 받도록 하고 싶습니다.

wheejuni · 2021-05-14T14:37:18Z

BE/src/main/java/codesquad/baseball/DTO/PlayerGameScoreDTO.java

+import java.util.List;
+
+@Getter
+@NoArgsConstructor


모든 필드가 null 이어도 괜찮을까요? 흠...

잘못 넣은 것 같습니다;;ㅎㅎ JPA때랑 헷갈렸는지, 왜 추가해줬는지 모르겠네요. 심지어 Entity도 아니고 DTO인데! 바로 삭제하겠습니다

wheejuni · 2021-05-14T14:38:48Z

BE/src/main/java/codesquad/baseball/DTO/PlayerListPopUpDTO.java

+        for(Player player: team.getPlayerList()) {
+            playerGameScore.add(new PlayerGameScoreDTO(player));
+        }


List 와 같은 컬렉션 성격의 변수를 다루는 경우 원소를 따로 빼서 .add() 연산을 하는 것보다는, 리스트 자체를 통째로 갈아끼우는 구현이 사이드 이펙트를 방지하는 방향입니다.

Player 를 Team 에서 하나씩 꺼내서... DTO 로 변환한 후에 list 에 하나씩 밀어넣는 것보다, Team 이 PlayerGameScoreDTO 리스트를 통째로 딱 내려주는게 더 좋을 텐데요.

리스트 자체를 통째로 갈아끼우는 구현 -> 이렇게 하고 싶었는데 더 좋은 방법을 고민해보겠습니다.
Team 이 PlayerGameScoreDTO 리스트를 통째로 딱 내려주는게 더 좋을 텐데요. -> Team에서 DTO를 몰랐으면 하는데, 어떻게 구현할지 감이 잡히질 않네요. 한번 말씀대로 수정 시도해보겠습니다!

wheejuni · 2021-05-14T14:58:52Z

BE/src/main/java/codesquad/baseball/controller/GameController.java

+    public ResponseEntity<PlayerListPopUpDTO[]> showPlayerList(@PathVariable Long matchId) {
+        return new ResponseEntity<>(gameService.getPlayerInfo(matchId), HttpStatus.OK);
+    }
+
+    @GetMapping("/{matchId}/record/teams")
+    public ResponseEntity<TeamGameScoreDTO[]> showDetailScoreList(@PathVariable Long matchId) {


Generic 안에 List를 넣어도 됩니다. 배열을 써야 할 특별한 이유가 있는게 아니라면, 그렇게 개선해보시는게 좋겠어요.

public ResponseEntity<List<TeamScoreDto>>

배열로 하니까 add 과정이 없어 코드 두 줄을 절약할 수 있어 저렇게 했는데, list가 컨벤션인가 보네요! 지금은 배열에 원정팀 스코어, 홈팀 스코어 두개뿐이므로 문제가 없어 보일 수 있는데, 나중에 배열에 다른 값들이 추가된다면 리스트가 더 유연하게 대응할 수 있을 것 같습니다. 몰랐는데 감사합니다!

wheejuni · 2021-05-14T15:01:42Z

BE/src/main/java/codesquad/baseball/domain/Constants.java

+
+
+
+public class Constants {


상수를 분리해서 선언해둔 것은 좋습니다만...
아래 필드들을 보니, 별도의 enum으로 분리하여 관리해야 할 항목들이 좀 보이네요.

예를 들어 STRIKE, BALL... 의 경우, 플레이의 결과물이므로 분리되는 게 좋았을 것 같고,
PITCHER, HITTER 와 같은 것도 선수의 포지션에 관한 enum일 거고요.

설계상에 고려가 필요하지 않았나 생각됩니다.

검색했더니 JDBC에서 enum을 지원하지 않는다고 해서 DB에 저장할 방법이 없는 줄 알았습니다! 잘못 알았네요. 처음에 enum을 만들었다가 에러가 떠서 jdbc연동문제인줄 알고 다시 지웠었는데, 더 공부해보고 다른 분들 코드도 많이 참고할 걸 그랬습니다. enum으로 유연한 설계 해보겠습니다! 감사합니다.

wheejuni · 2021-05-14T15:06:37Z

BE/src/main/java/codesquad/baseball/domain/History.java

+    @Id
+    @JsonIgnore
+    private Long id;
+    private String actionName;


enum 으로 관리했으면 더 유연했겠죠.

wheejuni · 2021-05-14T15:23:45Z

BE/src/main/java/codesquad/baseball/domain/Inning.java

+
+    private int out;
+    private int inningNumber;
+    private String role;


사실 야구는 본디 홈 팀이 말 공격이기때문에, 초/말 구분만 보고도 공수를 알 수 있기는 합니다.

Inning이 사실 response(ApiResponse클래스)에 그대로 담기는 , 도메인 겸 DTO 클래스에 가깝습니다. 프론트에서 response에서 로직처리 없이 최대한 있는 그대로 값을 끌어오실 수 있게, 공수도 명시하면 좋을 것 같다고 생각했습니다.

janeljs and others added 30 commits May 3, 2021 23:05

feat: .gitignore 추가

d120d97

feat: 프로젝트 설정 및 schema.sql 추가

10f47b3

feat: History, Match 추가 #2

713965b

feat: 전체 조회 메서드 추가

774438c

임시 커밋

420b6af

- DTO 설계

feat: DB설계 수정 및 controller에 play 화면 추가

26e8c3b

- Team와 Player이 History와 각각 1:N관계를 맺도록 클래스에 List<History> 추가

Merge pull request #6 from janeljs/be-test

852bba9

DB 설계 및 mock API 구성

feat: Mock API 설계 #4

530ace5

feat: GameController 추가

13613a9

fix: 빌드 문제 해결

913df85

fix: 중복 아이디로 인한 DB 저장 에러 해결

2220d56

Merge pull request #12 from janeljs/be-test

c7336c6

[BE] Controller 메서드 구현

fix: CORS 문제 해결 위해 WebConfig 추가

ccf1f63

fix: mysql auto reconnection 설정 추가

8cc77ca

fix: cross origin 관련 설정 변경

3cf320b

- WebConfig에서 전역에 cross origin을 적용하는 방법에서 컨트롤러별로 적용하는 방식으로 변경

feat: Team,Match를 찾지 못할때 발생하는 예외 추가

897b53b

feat: Inning에 아웃 수를 더하고, 교체가 필요한지 검증하고, 교체하는 메소드 추가

0667d03

feat: 회의결과에 따라 leftTeam, rightTeam을 expeditionTeam, homeTeam으로 수정

ee137d8

feat: mysql data source url을 localhost에서 private ip로 변경

76df526

feat: team에게 투수, 첫 타자, 다음 타자를 물어보는 메소드 추가

e454702

feat: 찾는 Player없을 시 발생하는 예외 추가

6bf1a8c

feat: match에 홈팀, 원정팀 id를 리턴하는 메소드, 그리고 주어진 팀 id와 다른 id를 리턴하는 메소드 추가

64d4ff5

feat: TeamGameScore에 id없는 생성자 추가

c380ba1

feat: 사용하지 않는 repository삭제

1931f4e

feat: Player의 히스토리 삭제, 투수여부 묻는 메소드 추가

36ff62e

feat: 액션에 대한 스태틱 변수와 액션을 판별하는 메소드 추가

05d6127

feat: 안타/아웃/볼넷 중 어떤 액션으로 플레이어가 교체되었는지 확인하기 위해 필드에 lastAction추가

c7f0ce2

feat: 테이블 player에 필드 lastAction 추가

fbf6f42

feat: 불필요한 데이터 삭제

68a7c84

feat: request에 선수의 마지막액션과 팀의 현재 점수 받아올 수 있도록 해당 필드 DTO에 추가

841caf7

janeljs and others added 24 commits May 11, 2021 15:32

Merge branch 'be-14' of https://github.com/janeljs/baseball into be-test

bfc62cd

feat: 선수 추가

ff0b13c

feat: PlayerDTO 생성자 파라미터에 role추가

2e7594c

feat: totalScore을 기존 리스트에 append하는 메소드 추가

4febe7c

fix: TeamLogDTO에 베팅을 완료한 선수의 히스토리만 뜨도록 수정

a15c249

fix: 정렬 및 쓰이지 않는 코드 제거

9c8c7ca

fix: totalScore초기화 시 리스트 삭제 후 totalScore추가하도록 수정

44b57c4

fix: 바뀐 메소드 사용하도록 수정

56b7bbf

Merge branch 'be-14' of https://github.com/janeljs/baseball into be-14

d019912

feat: service 클래스 추가

eec1665

Merge branch 'be-14' of https://github.com/janeljs/baseball into be-14

f59d695

refactor: 전체 클래스 리팩토링

8d99756

fix: gameService 리팩토링

85c903a

fix: service 리팩토링

a76e162

fix: mysql연결 설정 수정

3193beb

fix: url 설계 수정 #25

076990e

feat: 프론트 빌드 스크립트 수정 #27

37b0f5d

fix: isHome이 false일 때 발생했던 에러 해결

af51c8d

fix: pr 보내기 위해 프론트 코드 삭제

15f7e16

refactor: 코드 정렬 및 불필요한 코드 삭제

5047e17

fix: gitignore 설정

5d55700

fix: gitignore 설정

5c59456

fix: .gitignore 파일 업데이트

f8a80fd

fix: .gitignore 설정

a783dcb

janeljs added the review-BE label May 14, 2021

wheejuni self-assigned this May 14, 2021

wheejuni self-requested a review May 14, 2021 04:58

ehgud0670 pushed a commit that referenced this pull request May 14, 2021

Merge pull request #34 from pbg0205/feature/scoreView

06e8987

[iOS] Merge

wheejuni reviewed May 14, 2021

View reviewed changes

janeljs mentioned this pull request May 15, 2021

[BE] 코드 리뷰 반영하기 janeljs/baseball#35

Open

Comments

Conversation

janeljs commented May 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

데이터 통신 주기

데이터베이스 설계

배포

Uh oh!

wheejuni left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rla36 May 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

janeljs commented May 14, 2021 •

edited

Loading

wheejuni left a comment •

edited

Loading

rla36 May 15, 2021 •

edited

Loading