[또링] 블랙잭 1차 미션 코드리뷰 요청합니다.

[jnsorn]

안녕하세요! 리뷰를 받게 된 또링이라고 합니다. 아직 모르는 부분이 많은데, 많이 지적해주시면 열심히 고쳐보겠습니다!😁 잘 부탁드립니다.

[young891221]

안녕하세요 또링! 리뷰를 맡게된 하비입니다:)
전체적인 구조가 심플하고 가독성이 좋았습니다. 💯
몇 가지 개선 리뷰 남겼습니다. 확인 부탁드립니다.

[young891221]

생성과 동시에 초기화해 주는것이 더 좋은 형태로 보입니다.

[jnsorn]

초기에 Deck을 매개변수로 갖는 생성자만 존재할 때에는 생성자에서 생성과 동시에 초기화 하도록 구현하였는데요,

    public Hands(Deck deck) {
        this.hands = deck.initialDraw();
        this.score = 0;
        this.hasAce = false;
    }

테스트를 위해 직접 조작한 hands를 가지고 생성해야 하는 경우 때문에 List를 받는 생성자를 추가하게 되었습니다.

    public Hands(List<Card> hands) {
        this.hands = hands;
        this.score = 0;
        this.hasAce = false;
    }

그러다 보니 두개의 생성자에서 중복된 코드가 발생하였고, 중복을 최소화 하기 위해 인스턴스가 생성될 때 실행되는 인스턴스 블록에 중복 코드를 옮겨주었습니다.

즉, 제가 인스턴스 블록을 사용한 이유는 어떤 생성자가 호출되든 그 전에 공통으로 초기화 시키는 작업을 처리해주기 위함입니다! 이런 경우에도 인스턴스 블록을 사용하는 것 보다 두 가지 생성자에 (직접 코드를 넣든, 메소드로 분리하여 호출하든)공통된 코드를 넣어주는 것이 좋은 형태인지 궁금합니다! 제가 잘 몰라서.. 답변 주시면 열심히 고민해보겠습니다! 🙂

[young891221]

아 공통 코드를 구성하는 것은 좋습니다. 다만 생성자로만 처리하는것이 아닌 정적 팩터리 메서드(?)로 만들어서 명확한 네이밍을 만들어 주면 어떨까요? 예를 들어 아래와 같이요ㅎㅎ

public static Hands HandsByDeck() {
   return new Hands()
}

[jnsorn]

아, 정적 팩터리 메서드를 이용하면 함수명을 통해 더욱 명시적으로 함수의 역할을 파악할 수 있겠네요! 감사합니다 해당 부분 수정해보겠습니다! :)

[young891221]

리뷰 반영하시느라 고생하셨습니다. 💯
몇 가지 리뷰 남겼지만 큰 영향이 없는 코멘트라 머지하겠습니다:)

[young891221]

가능성은 없지만 Yes.equals() 로 쓰는 습관을 길러야 합니다.
answer의 경우 NPE 이벤트가 발생할 수 있는 구조니까요ㅎ

[jnsorn]

사실 이 부분은 이 전에도 지적을 받았었는데 위에서 null check를 해주기 때문에 크게 신경을 안쓴 것 같아요😂 의식적인 연습을 통해 항상 NPE에 대비해보도록 하겠습니다. 좋은 지적 감사합니다~!

[young891221]

Hands라는 표현이 어색하네요...ㅎㅎ
card의 집합인데 Cards와 같은 ㅍ현도 좋아 보입니다.(개인적인 의견이에요)

[jnsorn]

좋은 의견 감사합니다! 사실 저도 처음엔 Cards라고 썼다가 Deck과 비슷한 의미라고 느껴져서 변경을 고민하게 되었습니다. 카드게임 용어를 찾아보니 hand/hands라는 용어가 들고있는 패의 의미를 지닌 것을 알게되었고, Hands로 이름을 변경하였습니다. 이름을 변경하면서도 낯선 이름이라 클래스 주석을 통해 설명을 덧붙어야겠다 생각했는데 그 부분을 놓친 것 같습니다! 클래스 주석을 통해 클래스 명의 의미를 확실히 나타내는 방향으로 수정해보겠습니다! 😊