Skip to content

Conversation

Hae-Riri
Copy link

@Hae-Riri Hae-Riri commented Feb 17, 2022

안녕하세요 리뷰어님!
이번 미션은 김윤경 님과 진행했습니다.

이번엔 구현에서부터 어려움이 많았어서 정말 많은 리팩토링이 필요할 것 같아요.
리뷰 잘 부탁드리고, 저도 반영하면서 많이 배우겠습니다. 😄

@Hae-Riri
Copy link
Author

Hae-Riri commented Feb 19, 2022

안녕하세요 리뷰어님~ 시간을 좀 더 주셔서 감사합니다!! 😄

이번 수정사항은 아래와 같습니다.

연료주입

MotorVehicle 인터페이스의 기능을 아래 두 개로 기존의 것을 유지하고,

  • getDistancePerLiter
  • getChargeQuantity

Car 추상클래스는 MotorVehicle의 기능들을 구현하고, 추가 사항으로는 getName만 추상메소드로 갖도록 수정했습니다.
이유는 getName을 차마다 가진 차종의 이름을 바로 반환하는 기능으로 생각할 때 Car를 상속한 Avante, K5가 모두 다르게 구현되기 때문입니다.

블랙잭

Cards 라는 인터페이스를 만들었고 기능은 아래와 같습니다.

  • addCard : 카드를 1장 추가
  • getSumOfCards : 카드 합을 반환

Cards를 구현하는 추상클래스인 PersonCards를 만들었습니다. 이는 Cards의 기능을 구현하도록 했고, Player와 Dealer가 서로 다르게 처리해야 하는 기능만 추상메소드로 두었습니다.

  • addCard 구현
  • getSumOfCards 구현
  • initCard, initSum 구현
  • (abstract) canGetMoreCard : 카드를 한장 더 받을 수 있는 지 확인
  • (abstract) updateSumByDenomination() : 카드 숫자 합을 업데이트

마지막으로 PersonCards를 상속받는 PlayerCards와 DeaaerCards가 있고 이를 각각 Player와 Dealer가 인스턴스 변수로 가집니다.

  • canReceiveMoreCard : Player는 21을, Dealer는 16을 기준으로 서로 다르게 구현됩니다.
  • updateSumByDenomination : Player는 ACE에 대한 로직을 추가로 처리합니다.

Copy link

@hyunniiii hyunniiii left a comment

Choose a reason for hiding this comment

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

안녕하세요 해림님!
몇가지 코멘트 남겼습니다.
확인 부탁드립니다 ″̮

Comment on lines +65 to +67
- [ ] 카드의 합이 21 이하인지 카드를 추가로 뽑을 것인지 질문한다.
- [ ] 딜러에게 필요한 장수만큼 카드를 요구한다.
- [ ] 반환 받은 카드를 통해 딜러와 플레이어들을 초기화한다.

Choose a reason for hiding this comment

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

구현이 완료되었으면 완료 표시를 해주시면 어떨까요?

Comment on lines +14 to +15
private final Dealer dealer;
private final List<Player> players;

Choose a reason for hiding this comment

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

Controller가 상태를 가지고 있네요!
이 부분에 대해서 한번 고민해보면 좋을 것 같습니다 ″̮

또한 List<Player> players를 일급컬렉션으로 만들어보면 어떨까요?


private void printGameResult() {
ResultView.printDealerAndPlayerCardResult(dealer.getDealerCards(), players);
ResultView.printGameResult(new GameResult(dealer.getDealerCards().getSumOfCards(), players));

Choose a reason for hiding this comment

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

디미터 법칙을 위반하고 있습니다.
디미터 법칙을 위반하지 않도록 수정해보면 어떨까요?
https://mangkyu.tistory.com/147

return cards;
}

private List<Card> DenominationByShape(Shape shape) {

Choose a reason for hiding this comment

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

메서드 이름은 소문자로 시작해야합니다. ″̮

Comment on lines +7 to +12
/**
* 카드를 카드 목록에 추가한다.
*
* @return void
* @Param Card
*/

Choose a reason for hiding this comment

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

이름으로 표현이 되니 주석은 삭제해주는건 어떨까요?

Comment on lines +31 to +37
private void updateSumByDenomination(Denomination denomination) {
if (canAddBiggerValueByAce(denomination)) {
sum += 11;
return;
}
sum += denomination.getValue();
}

Choose a reason for hiding this comment

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

반환값이 없어서 이 부분에 대한 테스트가 불가능할 것 같아요.
조건에 맞춰서 11이나 1을 반환하도록 해주면 어떨까요?

import java.util.List;
import java.util.stream.Collectors;

public class ResultView {

Choose a reason for hiding this comment

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

디미터 법칙을 위반하고 있는 부분이 있네요.
수정해주세요 ″̮

}
}

public static void printGameResult(GameResult gameResult) {

Choose a reason for hiding this comment

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

image

a,b 모두 21을 넘었는데 딜러가 지는게 맞는걸까요 ″̮


@Test
void 플레이어를_생성하면_카드를_두장_가져야_한다() {
//given, when

Choose a reason for hiding this comment

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

given, when, then은 이름으로 표시해주고 지워주면 어떨까요?

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.

2 participants