[서브웨이] 블랙잭 1단계 리뷰요청입니다 #56

joseph415 · 2020-03-12T11:06:15Z

안녕하세요 리뷰어님 아직 리팩토링이 안된 동작만하는코드만 만들어서
리뷰해주시기전까지 refactoring끝내겠습니다!

feat: 딜러와 유저의 승패를 판단하는 기능 구현 및 테스트 코드 작성

young891221

안녕하세요, 서브웨이!
열심히 작성해 주신 만큼 열심히 리뷰해 보았습니다ㅎㅎ
확인 부탁드립니다:)

young891221 · 2020-03-13T11:55:15Z

src/main/java/domain/player/PlayerFactory.java

+        return Singleton.instance;
+    }
+
+    public Players createPlayers(Cards cards, List<String> playerNames) {


이 메서드를 static으로 만들어 사용하면 어떨까요?

young891221 · 2020-03-13T11:55:36Z

src/main/java/domain/player/PlayerFactory.java

+    private static class Singleton {
+        private static final PlayerFactory instance = new PlayerFactory();
+    }
+
+    public static PlayerFactory getInstance() {
+        return Singleton.instance;
+    }


아래 메서드가 static이 된다면 이 부분은 제거해도 괜찮아 보입니다.

young891221 · 2020-03-13T11:56:30Z

src/main/java/domain/card/Cards.java

+    public Cards() {
+        this.cardsDeck = Card.getCards();
+    }
+
+    public Card pop() {
+        validateCardsDeck();
+        Collections.shuffle(cardsDeck);


뺄때마다 섞어야 될 필요가 있을까요?
처음 생성하고 한번만 섞이면 될 것 같습니다.

네! 생성할때 한번만 섞도록 하는게 맞는것 같습니다!

young891221 · 2020-03-13T11:57:20Z

src/main/java/domain/card/Card.java

+        }
+    }
+
+    private Card(CardNumber cardNumber, CardSuitSymbol cardSuitSymbol) {


생성자 라인이 위로 올라오는게 컨벤션에 맞아 보입니다.

사소한 부분을 캐치하지 못했네! 변경했습니다.

young891221 · 2020-03-13T11:58:53Z

src/main/java/domain/player/Dealer.java

+import domain.card.Card;
+import domain.card.Cards;
+
+public class Dealer extends Player {


딜러가 Player 를 상속받는건 어감이 이상합니다. User or Man 과 같은 추상적 용어는 어떨까요?

딜러도 플레이 하는 사람이라 생각해서 player 라고 했는데 Man이 어감이 더 맞는 것 같습니다.

young891221 · 2020-03-13T12:00:43Z

src/main/java/domain/player/User.java

+import domain.card.Card;
+import domain.card.Cards;
+
+public class User extends Player {


User가 Player를 상속하는게 객체지향적으로 더 적절해 보이는데 어떻게 생각하시나요?

young891221 · 2020-03-13T12:01:08Z

src/main/java/dto/RequestAnswerDTO.java

@@ -0,0 +1,13 @@
+package dto;
+
+public class RequestAnswerDTO {


RequestAnswerDTO => Answer로 바꾸는건 어떨까요?

young891221 · 2020-03-13T12:01:19Z

src/main/java/dto/RequestPlayerNameDTO.java

+import java.util.Arrays;
+import java.util.List;
+
+public class RequestPlayerNameDTO {


RequestPlayerNameDTO => PlayerName

young891221 · 2020-03-13T12:01:49Z

src/main/java/dto/ResponseWinningResultDTO.java

+import java.util.*;
+import java.util.stream.Collectors;
+
+public class ResponseWinningResultDTO {


WinningPlayerResult로 명명을 바꾸는건 어떨까요?

young891221 · 2020-03-13T12:02:50Z

src/main/java/dto/ResponseWinningResultDTO.java

+    public List<String> getWinningResult() {
+        int allUserWinCount = (int) winningPlayer.values().stream().filter(win -> win).count();
+        int allUserLoseCount = winningPlayer.values().size() - allUserWinCount;
+        List<String> result = new ArrayList<>(Collections.singletonList("딜러: " + allUserLoseCount + "승 "


"딜러: " + allUserLoseCount + "승 " + allUserWinCount + "패")과 같은 플러스 처리 보다는 String.fomat() 함수를 이용해 보는게 좋아보입니다.

수정했습니다!

1.객체의 상속 구조를 고려해 객체의 nameing 변경 2.DTO naming 변경 3.Factory의 creat 메소드 static 변경으로 불필요한 코드 제거

dto 제거후 책임을 부여해 해당 필드를 담당하는 객체로 변경 하였습니다

young891221

서브웨이~ 리뷰 반영하시느라 고생하셨습니다. 👍
몇 가지 추가 코멘트 남겼습니다. 확인 부탁드려요~!

young891221 · 2020-03-15T10:45:53Z

src/main/java/controller/BlackjackController.java

+        OutputView.printInitialResult(playersName.getPlayerName());
+
+        Users users = new Users(cards, playersName.getPlayerName());
+        UsersInformation usersInformation = new UsersInformation(users);


UsersInformation => UsersInformations가 맞아보입니다.

young891221 · 2020-03-15T10:47:23Z

src/main/java/domain/player/Users.java

+        users.add(new Dealer(cards.giveCard(), cards.giveCard()));
+    }
+
+    public Dealer getDealer() {


딜러를 따로 저장해 두는게 좋지 않을까요?
딜러를 찾기위해 매번 반복문이 들어가는군요.

young891221 · 2020-03-15T10:47:57Z

src/main/java/domain/player/UsersInformation.java

+        return usersInformation.get(DEALER_INDEX);
+    }
+
+    public List<UserInformation> getPlayerInformation() {


여기도 딜러가 아닌것을 위한 불필요한 반복문 연산이 들어갑니다.

young891221 · 2020-03-15T10:48:49Z

src/main/java/controller/BlackjackController.java

+
+public class BlackjackController {
+
+    public static void run() {


컨트롤러가 많은 코드를 들고 복잡해 보입니다.
run()만 남겨두고 다른 메서드들은 각자의 도메인에 위임시켜주세요:)

young891221

고생하셨습니다 서브웨이 👍
많이 개선되었지만 네이밍, 컨벤션이 아쉬운 부분이 있어서
한번 더 확인 부탁드립니다:)

young891221 · 2020-03-17T01:06:34Z

src/main/java/controller/BlackjackController.java

+        Players players = new Players(cards, playersName.getPlayerName());
+
+        OutputView.printUserCard(dealer.getName(),dealer.cardToString());
+        for(Player player:players.getPlayers()){


여기 컨벤션을 34번줄과 같이 일치시켜주도록 해주세요:)
다른 부분들도 모두 부탁드립니다.

young891221 · 2020-03-17T01:16:22Z

src/main/java/controller/BlackjackController.java

+        result(players, dealer);
+    }
+
+    private void userService(Cards cards, Player player) {


메서드명이 명확하지 않습니다. startGame()과 같이 변경해 보는건 어떨까요?

run() { readyGame() startGame() resultGame() }

위와 같은 식으로 메서드를 정리해도 좋을 것 같습니다. 아니면 본인이 더 옳다고 생각하는 방향으로 정리해도 좋습니다.

young891221 · 2020-03-17T01:18:24Z

src/main/java/controller/BlackjackController.java

+        }
+    }
+
+    private void DealerService(Cards cards, Dealer dealer) {


DealerService도 DealerTurn or dealByDealer 와 같이 뭔가 행위를 묘사해 주세요.

young891221 · 2020-03-17T01:19:32Z

src/main/java/domain/answer/Answer.java

+        }
+    }
+
+


young891221 · 2020-03-17T01:20:16Z

src/main/java/domain/answer/Answer.java

+    }
+
+    private static void validateAnswer(String answer) {
+        if (!answer.equals("y") && !answer.equals("n")) {


상수가 조건 앞에 명시되어야 합니다.
!"y".equals(answer)와 같이요. NPE를 예방하는 습관으로 이펙티브 자바와 같은 책에 명시되어 있는 내용입니다ㅎㅎ

감사합니다! 수정했습니다!

young891221

리뷰 반영하시느라 고생하셨습니다. 머지할게요~!

giantim and others added 22 commits March 10, 2020 15:30

doc: README 작성

bc61983

doc: 각 클래스별 메서드 추가

13f7bce

[feat] 카드 넘버 구현 및 테스트 추가

cc7e529

feat: Card 구현 및 테스트 코드 작성

c62a8a8

[feat] 프로그램에서 사용할 전체 카드 묶음 구현

6dbf35d

feat: Dealer 의 카드를 추가로 받는 기능 구현 및 테스트 코드 작성

cfe9c4a

[feat] User 의 카드를 추가로 받는 기능 구현 및 테스트 코드 작성

fcfa087

feat: Dealer 의 카드의 합이 16 이하인지 판단하는 기능 구현 및 테스트 코드 작성

6664869

feat: 블랙젝을 판단하는 기능 구현

b4432eb

refactor: 딜러와 유저의 카드 승패를 판단하는 테스트 코드를 세분화

6aa4b5d

feat: 딜러와 유저의 승패를 판단하는 기능 구현 및 테스트 코드 작성

refactor: convention 수정

694e731

doc: 설계 수정

9d8d468

[refactor] 전체적인 리팩토링

d288f43

[feat] 승/패를 담는 기능 구현

6c85238

feat: Player 를 담는 일급 컬렉션 구현 / Players 에서 Dealer 와 User 를 구분하는 메서드 구현

71404d5

[feat] DTO생성

427e539

feat: controller 에 카드 받기까지 구현

e0460da

[feat] OutputView 구현

0447cae

feat: 모듈 결합

93677ab

[refacotor] 요구사항 refactor & 중복 코드 제거

baec7f9

[refactor] 불필요한 코드 제거 리팩토링

e5c9af6

[refactor] 불필요한 코드 제거 리팩토링

e332925

young891221 suggested changes Mar 13, 2020

View reviewed changes

joseph415 added 6 commits March 14, 2020 21:26

[refactor] 리뷰받은 부분 리팩토링

33053ab

1.객체의 상속 구조를 고려해 객체의 nameing 변경 2.DTO naming 변경 3.Factory의 creat 메소드 static 변경으로 불필요한 코드 제거

[refactor] dto 제거 후 domain객체로 변경

89e1318

dto 제거후 책임을 부여해 해당 필드를 담당하는 객체로 변경 하였습니다

[fix] 결과가 플레이어 입력 순으로 되지 않는 버그 수정

4d71c18

[refactor] view와 domain 의존성 제거

d80f1c4

[refactor] 객체가 의도한 책임을 하도록 controller 로직 변경

19ac920

[refactor] 불필요한 코드 제거 리팩토링

d4952a1

young891221 suggested changes Mar 15, 2020

View reviewed changes

joseph415 added 2 commits March 16, 2020 13:57

[refactor] 불필요한 코드 제거

0415f98

[refactor] 코드 중복을 없애기 위해 클래스 변경 및 불필요한 코드 제거

40a901a

young891221 suggested changes Mar 17, 2020

View reviewed changes

[refactor] 메서드 이름 명확하게 변경, controller run 메서드 정리

31e3d77

young891221 approved these changes Mar 17, 2020

View reviewed changes

young891221 merged commit 9508de9 into woowacourse:joseph415 Mar 17, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[서브웨이] 블랙잭 1단계 리뷰요청입니다 #56

[서브웨이] 블랙잭 1단계 리뷰요청입니다 #56

joseph415 commented Mar 12, 2020

young891221 left a comment

young891221 Mar 13, 2020

young891221 Mar 13, 2020

young891221 Mar 13, 2020

joseph415 Mar 14, 2020

young891221 Mar 13, 2020

joseph415 Mar 14, 2020

young891221 Mar 13, 2020

joseph415 Mar 14, 2020 •

edited

Loading

young891221 Mar 13, 2020

young891221 Mar 13, 2020

young891221 Mar 13, 2020

young891221 Mar 13, 2020

young891221 Mar 13, 2020

joseph415 Mar 14, 2020

young891221 left a comment

young891221 Mar 15, 2020

young891221 Mar 15, 2020

young891221 Mar 15, 2020

young891221 Mar 15, 2020

young891221 left a comment

young891221 Mar 17, 2020

young891221 Mar 17, 2020

young891221 Mar 17, 2020

young891221 Mar 17, 2020

young891221 Mar 17, 2020

young891221 Mar 17, 2020

joseph415 Mar 17, 2020

young891221 left a comment


		public class BlackjackController {

		public static void run() {

[서브웨이] 블랙잭 1단계 리뷰요청입니다 #56

[서브웨이] 블랙잭 1단계 리뷰요청입니다 #56

Conversation

joseph415 commented Mar 12, 2020

young891221 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joseph415 Mar 14, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

young891221 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

young891221 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

young891221 left a comment

Choose a reason for hiding this comment

joseph415 Mar 14, 2020 •

edited

Loading