-
Notifications
You must be signed in to change notification settings - Fork 46
[BE][Team02] TDD 로 Baseball 서버 구현 #32
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
base: ghojeong
Are you sure you want to change the base?
Conversation
[BE] 스프링 초기화를 함
[BE] OAuth 를 연습해보기 위한 웹 프론트 구현
- GET /api/match 구현(참가할 수 있는 게임 목록 조회)
- GET /api/progress/{id} 구현
- GET /api/gameInfo/{id} 구현
- dto는 불변 객체이여야 하기 때문에 dto의 멤버 final로 수정 및 setter 메서드 삭제
- List 를 사용하면 java Collections 패키지의 풍부한 기능을 활용할 수 있음
[BE] GET에 대한 요청 구현
[BE] 로컬 개발환경을 도커화
[BE] Swagger 세팅
|
커밋되면 안되는 파일이 포함된 것 같은데 확인부탁드릴게요~ |
|
혹시 React 자바스크립트 코드를 말씀하신건가요? IOS 분들이 모바일로 OAuth 관련 프론트 개발을 어려워하셔서 |
이런 파일은 굳이 버전관리 되지 않아도 되지 않나요? ㅎㅎ |
|
그리고 OAuth는 웹과 관련된 기술이지만, 별도의 웹 구현 없이도 애플리케이션 환경에서 OAuth 인증을 구현할 수 있습니다. |
생각해보니, 굳이 React 앱을 만들지 않더라도, html 의 앵커 링크 만으로도, OAuth 관련 동작을 하기에 충분하다는 생각이 드는 군요. |
yarn.lock 를 말씀하신 것이군요. 실제로 자바스크립트 개발을 할 때 (React 프론트 개발이던, NodeJS 를 통한 백엔드 개발이던) |
피드백 정말 감사합니다. |
리뷰를 남기는 와중에 코멘트를 남기려고 했더니 안되는군요. 제가 FE에는 무지해서, 찾아보니 lock 파일을 버전관리 해야한다는 글이 나오네요. 역시 자바가 짱이네요. |
|
자바 코드 외에 불필요한 부분이 너무 많은 것 같아서 현재 PR 을 Close 하고 새로 올리겠습니다. |
|
제가 알아서 보겠습니다 ㅋㅋ |
감사합니다. ㅎㅎ 👍 |
생각하지 못했던 것 뿐이라고 생각합니다. 화이팅하세요! |
자바가 저런 부분에서는 짱이죠 ㅎㅎ |
| ### Gradle Patch ### | ||
| **/build/ | ||
|
|
||
| # End of https://www.toptal.com/developers/gitignore/api/intellij+all,intellij+iml,intellij,java-web,java,gradle,maven |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
너무 길지 않나? 하는 생각이 드네요. 한 번 중복되는 줄이 있는지, 불필요한 주석은 그냥 제거하는게 어떤지도 생각해봐주세요.
ksundong
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
안녕하세요! Pyro & Robin! 리뷰어 Dion입니다.
일단 TDD로 개발하셨다는 것에 대단하다는 말씀 드리고 싶습니다.
쉽지 않은 도전이었을 텐데, 잘 해내고 계신 것 같네요.
총평
아직 중요한 부분에 대한 리뷰가 완료되지 않았지만, 좋은 경험이었으리라 생각됩니다.
저도 아직 TDD로는 사이드 프로젝트를 구현해본 적이 없네요. 여러분 코드를 리뷰하면서 TDD 과정을 수강했으니 조만간 도전을 해봐야겠네요.
도메인 로직이 구현된 상태에서 DB를 붙이는 것은 어려운 경험일겁니다.
특히 Spring Data JDBC는 일부 객체지향적인 부분을 버려야 했던 것 같습니다.
어쩌면 제가 실력이 부족해서 그래야 했을 수도 있지만, Spring PetClinic마저 그런 형태로 구현된 부분이 있어서, 아마 아쉬운 부분이 분명히 있으셨을 것 같습니다.
제 어렴풋한 기억으로는 Spring Data JDBC에서 구현하면 JPA로 구현한 것에 비해서 좀 더 RDBMS에 가까운 형태로 구현되었던 것 같아요.
다음 3주차 작업을 할 때는 괜히 TDD 에 욕심 내지 말고, DB 와 인프라에 더 집중해서 작업을 하도록 하겠습니다.
한정된 시간 내에 TDD로 구현하는건 대단히 힘든 경험이라고 생각합니다. 중요한건 TDD로 도메인 로직만 구현하는 것이 아니라, 살을 붙여가면서 구현하는게 중요한 것 같아요. 가령 DB와 연동하는 것이 굉장히 어려웠던 기억이 있네요.
좀 더 애자일 했다면 좋았을 뻔 했지만, 개인적으로는 잘하셨다고 칭찬드리고 싶네요.
모든것을 경험하기는 힘듭니다. 프로젝트 완성도 중요하긴 하지만, 코드스쿼드에 프로젝트 완성하러 오신게 아니라, 학습하러 오신거니까요.
이번 프로젝트에는 'TDD도 해보고 그랬더니 기간내에 개발을 못하겠더라' 하는 실패경험을 만드는 것도 좋지 않나 싶어요.
의외로, 이런 경험들이 면접볼 때도 중요하게 여겨집니다. '남들은 하지 않는 TDD를 해봤고, 실패했고, 다음엔 어떻게 하면 좋겠다.'라는 생각을 갖게 되니까요.
다음 프로젝트에는 DB와 인프라를 학습하신다고 하셨으니 JdbcTemplate을 사용해서 Query 작성 경험을 늘린다던지, AWS에 대해서 좀 더 깊이 알아가는 식으로 학습거리를 늘려나가는 것을 추천드려요.
특히 코드스쿼드 백엔드는 중요하다고 여겨지는 트래픽에 대한 경험을 할 수 없기 때문에 다른 부분에서 강점을 늘려나가야 합니다.
예를 들자면, 웹 보안은 어떻게 구현하는지, 웹의 통신 과정을 잘 이해하고 백엔드를 작성한다던지, 쿼리를 어떻게 하면 잘 작성하는 것인지, 좀더 객체지향적으로 애플리케이션을 개발한다던지 등등.. 많은 것들이 있겠죠.
쓰다보니 리뷰에 대한 내용보다는 이것 저것 생각하는 내용을 작성하게 되었네요.
이번 리뷰의 중요 포인트는, 도메인과 테스트를 제외한 코드들에 대한 리뷰입니다. 조금 순서를 달리할 걸 싶기도 하네요.
코멘트 참고해주시구, 궁금하시거나 도움이 필요하거나, 이해가 안간다거나 토론하고 싶으신 부분이 있으면 코멘트 남겨주세요.
고생 많으셨습니다!
나머지 리뷰는 오후중으로 해드리겠습니다!
| TOMCAT_PROCESS=$(ssh -i $KEY_PATH $AWS_PATH "lsof -t -i tcp:8080") | ||
| ssh -i $KEY_PATH $AWS_PATH "kill -9 $TOMCAT_PROCESS" | ||
| ssh -i $KEY_PATH $AWS_PATH "java -jar ~/$JAR_FILE & >> log.txt 2>&1" | ||
| # ssh -i $KEY_PATH $AWS_PATH "nohup java -jar ~/$JAR_FILE & >> log.txt 2>&1" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cronjob을 돌려서 주기적으로 체크하고 배포하는것도 해보면 좋겠네요.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
넵. S3 에 올려서 CI/CD 환경을 구축해보는데도 시간을 더 투자해보겠습니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
S3에 올리지 않고도 cronjob을 로컬에서 돌려도 되지 않을까요?
| @@ -0,0 +1,52 @@ | |||
| # Docker 안내서 | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Docker compose까지 알차게 사용하시는군요. 대단합니다. 👍
| character-set-server = utf8 | ||
| collation-server = utf8_unicode_ci |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mysql의 charset구조는 독특합니다.
한 번 charset과 collation에 대해서도 찾아보세요.
꽤 재밌습니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
오오오, 그렇군요.
혹시 재밌게 읽으신 자료가 있으시면 공유 부탁드려도 될까요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| image: mysql:5.7 | ||
| container_name: mysql-5.7 | ||
| environment: | ||
| MYSQL_ROOT_PASSWORD: root1234 | ||
| MYSQL_DATABASE: pyrodb | ||
| MYSQL_USER: pyro | ||
| MYSQL_PASSWORD: pyro1234 | ||
| ports: | ||
| - "3306:3306" | ||
| volumes: | ||
| - ./db/initdb:/docker-entrypoint-initdb.d | ||
| - ./db/data:/var/lib/mysql | ||
| - ./db/conf:/etc/mysql/conf.d | ||
| restart: always |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
두 분 다 docker에 대한 기본적인 지식이 있는 상태에서 compose를 사용하신건가요?
되도록이면 기본적인 docker 명령어를 아는 상태에서 compose를 사용하는게 좋을 것 같다는 의견입니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
MySQL 에 대해서 위와 동일한 컨테이너를 만들어주는 Dockerfile 을 먼저 작성했었습니다.
다만 여러 컨테이너에 대한 Dockerfile 을 개별적으로 굳이 작성할 필요없이, docker-compose 만으로 충분하다는 생각이 들어서 위처럼 작성하게 되었네요.
다음 미션 때는 작성한 Dockerfile 을 남겨서 활용할 수 있도록 해보겠습니다.
말씀하신대로, Docker 기본 명령어에 제가 소홀히 할 수도 있는데,
기본기에 충실해야한다는 점 일깨워주셔서 감사합니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
아뇨 잘 하셨어요 ㅎㅎ
저같은 경우엔 기본 명령어로 사용을 했던 케이스여서, docker 명령어에 익숙해질 필요가 있지 않나 하는 생각이 있었습니다.
| public MockedBaseballRepository() { | ||
| matches.put("MATCH_ID", new Match(new Teams( | ||
| createTeam("AWAY"), | ||
| createTeam("HOME") | ||
| ))); | ||
| matches.put("U924AX", new Match(new Teams( | ||
| createTeam("Marble"), | ||
| createTeam("Captain") | ||
| ))); | ||
| matches.put("H132UY", new Match(new Teams( | ||
| createTeam("Crong"), | ||
| createTeam("Honux") | ||
| ))); | ||
| matches.put("M887UW", new Match(new Teams( | ||
| createTeam("Apple"), | ||
| createTeam("Android") | ||
| ))); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
자바 컨벤션을 지켜주세요.
필드 - 생성자 - 메서드의 순서로 작성해주시면 되고
저 같은 경우에는 Override된 메서드는 유동적으로 위치를 잡아주는 편입니다. (가독성에 좋은 쪽으로)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
필드 - 생성자 - 메서드의 순서
유의하도록 하겠습니다. 읽는 순서에 따라 배치도 신경써야겠군요.
| createTeam("AWAY"), | ||
| createTeam("HOME") | ||
| ))); | ||
| matches.put("U924AX", new Match(new Teams( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Match Id를 이렇게 지정해주신 이유가 있고, 규칙이 있나요?
대충 영어 대문자 + 숫자 3개 + 영어 대문자 2개로 보이는데 어떤건 MATCH_ID이고 해서 헷갈리네요.
10 ^ 3 * 26 ^ 3 = 17576000은 일반적으로는 충돌 가능성이 없어보이긴 하는데, 그렇다고 아예 없다고 하기에도 조금 고민되네요.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Match Id를 이렇게 지정해주신 이유가 있고, 규칙이 있나요?
Match_ID 같은 경우는, 저희가 UUID 혹은 AUTO_INCREMENT 숫자값을 통해 유니크하게 생성하려고 하다가,
프론트 분들이 개발하기 쉽도록, 자신들이 원하는 예시대로 해달라고 부탁을 해와서,
그 요구사항을 저렇게 반영했습니다.
저런식으로 ID 를 생성하면 안된다는 점은 충분히 인지했습니다.
프론트가 개발이 어느정도 완료되어서, 익숙해지시면 프론트 팀과 다시 협의해서 변경을 요청해보려고 합니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
아예 안된다는 아니고 애매하다? 정도로 받아들여주시면 좋을 것 같아요.
협의 잘 하셔서 좋은 방향으로 나아가면 좋을 것 같네요~
| private List<Pitcher> createPitchers(String teamName, int size) { | ||
| return IntStream.rangeClosed(1, size) | ||
| .mapToObj(i -> new Pitcher(String.format("%s%d투수", teamName, i))) | ||
| .collect(Collectors.toList()); | ||
| } | ||
|
|
||
| private List<Batter> createBatters(String teamName, int size) { | ||
| return IntStream.rangeClosed(1, size) | ||
| .mapToObj(i -> new Batter(String.format("%s%d타자", teamName, i))) | ||
| .collect(Collectors.toList()); | ||
| } | ||
|
|
||
| private Team createTeam(String teamName) { | ||
| return new Team(teamName, new Players( | ||
| createPitchers(teamName, 5), | ||
| createBatters(teamName, 9) | ||
| )); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
private 메서드는 사용되는 지점 아래에 위치시켜주는 편이 가독성에 좋습니다.
가령 저라면 생성자 밑에 createTeam을 위치시키고, 그 아래에 pitchers, batters 순으로 위치시킬 것 같네요.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
오호... 배치를 통해서 로직의 응집도를 높일 수도 있군요.
코드 혹은 메서드의 배치 순서에 대해서 주의를 기울여 봐야 한다는 배려심은 처음 깨닫게 되었네요.
좋은 관점 같습니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
네 개인적으로 읽기 좋은 코드가 좋은 코드라는 얘기를 좋아합니다.
그런면에서 private은 최초 사용지점의 아래에 위치하는 것이 좋고,
private 메서드가 과도해지면 리팩토링의 시점이라고 보는게 좋겠죠. 읽기 힘들어지니까요.
여러 메서드에서 해당 private 메서드를 사용한다면, 객체로 끄집어 내는 시도를 하거나
private 메서드의 depth가 깊어진다면, 중간 객체를 둔다거나 하는식으로 리팩토링을 하는 편입니다.
| return IntStream.rangeClosed(1, size) | ||
| .mapToObj(i -> new Pitcher(String.format("%s%d투수", teamName, i))) | ||
| .collect(Collectors.toList()); | ||
| } | ||
|
|
||
| private List<Batter> createBatters(String teamName, int size) { | ||
| return IntStream.rangeClosed(1, size) | ||
| .mapToObj(i -> new Batter(String.format("%s%d타자", teamName, i))) | ||
| .collect(Collectors.toList()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
굳이 stream을 쓸 필요는 없습니다. 작성해주신 코드는 어쩌면 for문이 성능도 좋고, 이해하기도 쉬울것 같네요.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
객체 스트림이 아닌, 숫자 루프에 굳이 stream 을 쓰는 것은 낭비가 맞는 것 같습니다.
다음부터는 불필요한 IntStream 을 쓸 때는, for 루프로 대체할 수 있지는 않을까 항상 고민하겠습니다.
좋은 피드백 감사합니다! ❤️
| @@ -0,0 +1,7 @@ | |||
| package com.baseball.exception; | |||
|
|
|||
| public class MatchNotFoundException extends NullPointerException { | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NPE를 상속하기보다는 RumtimeException을 상속해주세요.
NPE는 NotFound와는 다른 개념입니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
앗 찾아보니 그렇군요. 동의합니다.
커스텀 예외는 모두 RuntimeException 이 될 수 있도록 하고,
NPE 는 NotFound 와는 별도의 에러 핸들링을 할 수 있도록 하겠습니다.
총평 감사합니다! 그에 대한 고심끝 결론은, JPA 의 Derived Query 기능을 포기하고, 다음 3주짜리 미션때는, 테스트에 시간을 너무 많이 들이지 않더라도, |
|
엄밀히 말하자면 Spring JdbcTemplate은 역사가 긴 API입니다. |
ksundong
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2번째 리뷰입니다. 도메인과, 테스트 코드를 중점적으로 리뷰했습니다.
대부분은 정말 잘 짜주신 것 같고, 생각해 볼 거리들을 남겨보았습니다.
특히 SoftAssertion은 저는 그렇게 정이 가지 않는 기능이네요. 테스트 코드가 조금 지저분해지는 느낌이 들었습니다. 벌써 꼰대가 된건가..
이제는 코드 프리징이 가까워졌네요. 고생많으셨습니다!
| if (selectedTeam != NONE) { | ||
| throw new MatchOccupiedException(); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이부분을 별도의 메서드로 빼는건 어떻게 생각하시나요?
저는 이번에 이렇게 validate하는 부분을 메서드로 빼니까 가독성이 좋아지고, 테스트 포인트를 잡기 수월해지더라고요~
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
확실히 그렇네요. validate 하는 부분들은 별도의 private 메서드로 분리해야겠습니다.
| if (selectedTeam != NONE) { | ||
| throw new MatchOccupiedException(); | ||
| } | ||
| String homeTeamName = teams.getHomeTeam().getName(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
디미터 법칙을 준수한 코드를 사용한다면
teams.getHomeTeamName() 을 호출하는 것도 시도해볼만 할 것 같아요.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
. 이 연속되어서 2개 이상 찍히지 않도록 조심하겠습니다.
| throw new MatchOccupiedException(); | ||
| } | ||
| String homeTeamName = teams.getHomeTeam().getName(); | ||
| selectedTeam = homeTeamName.equals(teamName) ? HOME : AWAY; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
homeTeamName.equals(teamName) ? HOME : AWAY를 별도의 메서드로 추출하는건 어떨까요?
그러면 삼항연산자가 빠지고 if문으로만 구성할 수 있어보입니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
오우 그렇군요!
조건의 분기가 일어날 때에는 항상 별도 메서드로 분리하는 것을 고려해보겠습니다.
참고로 저는 함수형 프로그래밍을 좋아해서 그런지,
만약 expression 으로 표현 가능하다 싶을 때에는, statement 보다는 expression 을 더 우선해서 사용하는 편입니다.
Rx 시리즈의 라이브러리를 사용하다보면 (RxJava, RxJS 등등),
statement 의 사용이 불가능한 구간들이 나와서, 가능한 expression 을 쓰도록 훈련을 받은 적이 있어서요
(취향 확고하네요 ㅋㅋ)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
맞아요. 취향이 확고한 것도 좋은 것 같습니다.
저는 갈대같아서 한 달이 지나면 또 다른 생각을 하게 되더라고요~
| selectedTeam = homeTeamName.equals(teamName) ? HOME : AWAY; | ||
| } | ||
|
|
||
| public Boolean getUserOffense() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wrapper Type을 리턴해주시는 이유가 있나요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
저 부분은 굳이 Wrapper Type 으로 할 필요가 없는게 맞습니다.
다만, 코드 통일성을 위해 전부 Wrapper Type 으로 작성해보는 것은 어떨까 팀 내부적으로 합의를 했었습니다.
다음 번에는 불필요한 Wrapper 타입은 자제를 해보려고 하겠습니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
네 가급적이면 Wrapper Type을 필요할 때만 사용해주는 것이 좋다고 생각해요.
| if (selectedTeam == AWAY) { | ||
| return matchInfo.getUserTop(); | ||
| } | ||
| return !matchInfo.getUserTop(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
뭔가 좀 더 개선할 수 있을 것 같은데...
어떻게 하면 if문을 제거하고, matchInfo.getUserTop()만 사용할 수 있을지 고민해보시면 좋을 것 같아요.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if 문의 제거...
늘 어렵지요. 구조적으로 저도 Match 와 MatchInfo 가 제일 아쉬웠습니다.
좀더 분리해서 쪼개고 싶었는데, 시연이 얼마 남지 않은 상황이라,
시연 가능하도록 코드기 아닌 인프라 세팅에 시간을 다 쏟아붇게 되었네요.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
저도 딱 바로 해결책이 안떠오르는 걸로 봐서는 어려운 문제죠.
이럴 때에는 저는 TODO로 남겨두곤 합니다.
| @Test | ||
| @DisplayName("String 이 PlayResult 로 잘 변환되는지 테스트 (HIT, STRIKE, BALL, 랜덤)") | ||
| void of() { | ||
| softly.assertThat(PlayResult.of("HIT")).isEqualTo(HIT); | ||
| softly.assertThat(PlayResult.of("STRIKE")).isEqualTo(STRIKE); | ||
| softly.assertThat(PlayResult.of("BALL")).isEqualTo(BALL); | ||
| softly.assertThat(PlayResult.of("랜덤")).isIn(HIT, STRIKE, BALL); | ||
| softly.assertAll(); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Parameterized Test를 작성해보시고
랜덤은 좀 더 생각해봐주세요!
|
|
||
| @Test | ||
| @DisplayName("PlayResult 가 Boolean 으로 잘 변환되는지 테스트 (STRIKE 만 true 이다)") | ||
| void toBoolean() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이 부분도 isStrike가 나을지 고민해봐주시면 좋을 것 같습니다.
| import java.util.stream.IntStream; | ||
|
|
||
| public class TeamFactory { | ||
| private static List<Pitcher> createPitchers(String teamName, int size) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mock이랑 똑같은 친구같네요 ~.. ~
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
맞습니다. 원래는 기존의 Mock 도 전부 테스트로 옮겨서 의존성을 주입받게 하려고 했습니다.
계획이 꼬여서 깔끔하게 되지 못해서 중복된 코드가 발생했습니다.
| } | ||
|
|
||
| @Test | ||
| @DisplayName("playDefense 를 하면, 투수가 play 를 한 것과 동일한 효과가 나타나야 한다.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DisplayName은 정말 잘 작성해주시는 것 같아요.
DisplayName 대로 테스트를 작성하면 쉽게 이해할 수 있는 테스트가 될 것 같습니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
칭찬 기분 좋네요. 😄
약간 자랑을 해보자면 위와 같은 DisplayName 은 제가 작성한 것이고,
"~테스트" 로 끝나는 DisplayName 은 로빈이 작성한 것입니다.
로빈이 많이 힘들어해서 Parameterized 테스트라던가, DisplayName 과 같은 부분을 매우 엄격하게 검수하지는 않았습니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
저도 습관적으로 테스트를 붙여주다가 NextStep TDD 과정듣고 좀 바뀐 것 같아요.
로빈 고생이 많았겠네요...
| softly.assertThat(teams.getAwayTeam().getScores()) | ||
| .isEqualTo(Arrays.asList(0)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
굳이 검증할 필요가 없는건 검증해주지 않아도 됩니다.
필요한 부분만 검증해줍시다.
[iOS] 최종 결과물 dev/iOS 브랜치에 반영
|
리뷰 감사합니다. 많이 아쉬움이 남는 프로젝트네요. |
|
부족한 리뷰를 좋아해주셔서 감사합니다. 공부 열심히 해야겠네요. 개인적으로 리뷰를 자주 올리는편이 훨씬 학습 효과가 좋다고 생각합니다. 다음 프로젝트도 화이팅입니다!! |



TDD 로 구현을 하다보니, 기능 구현을 하는데 시간을 엄청 잡아 먹게 되었습니다.
이제 이렇게 설계된 domain 객체를 JDBC Template 을 통해 테이블로 만들거나,
테이블의 정보를 객체로 변환하는 작업을 추가로 해야합니다.
작업을 하면 할 수록 규모가 자꾸 커지게 되어서,
일단 현재까지 작업한 내용을 먼저 PR 로 만들게 되었습니다.
다음 3주차 작업을 할 때는 괜히 TDD 에 욕심 내지 말고, DB 와 인프라에 더 집중해서 작업을 하도록 하겠습니다.
스케쥴 관리에 실패해서 학습 커리큘럼에 충실치 못한 작업을 한 점 죄송합니다.
테스트를 더 많이 작성했는데, 맥북 화면이 조그만해서 전부 캡처를 하지 못했습니다.