Skip to content
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

[숫자 야구 게임] 변해빈 미션 제출합니다. #1613

Open
wants to merge 65 commits into
base: main
Choose a base branch
from

Conversation

h-beeen
Copy link

@h-beeen h-beeen commented Oct 25, 2023

header


📦  패키지 구조

Package Class Description
🕹  controller Game 게임 로직을 메인으로 동작하는 컨트롤러 클래스
💻  domain Number 사용자에게 입력받는 숫자와, 컴퓨터가 생성하는 숫자 클래스
Result Ball Count와 Strike Count에 대한 결과 클래스
    ↘️  / constants ResultType 각 결과에 따라 다른 출력 방법에 대해 정의된 Enum
📃  global GameConfig 전역으로 작용하는 설정과 제약조건에 대한 Enum
    ↘️  / exception BaseballException 전역으로 처리하는 예외상황에 대한 Exception 클래스
ErrorMessage 각 예외 상황에서 전역으로 던져질 예외의 메세지 Enum
✅  validator InputValidator 사용자가 입력하는 숫자에 대한 제약조건 클래스
💬  view InputView 사용자 요청을 처리하는 클래스
OutputView 사용자에게 응답을 출력하는 클래스
    ↘️  / constants StaticNotice 사용자에게 응답할 정적 메세지를 담은 열거형 클래스
Package Structure Overview

✨  기능 구현 목록

a ~ b 사이의 서로 값이 다른 n자리의 정수를 랜덤으로 생성한다.

▪️  Default Setting : `1 ~ 9`사이의 서로 값이 다른 `3자리`의 정수

✅  게임 시작 문구 출력

✅  사용자에게 a ~ b 사이의 서로 값이 다른 n자리의 정수를 입력 받는다.

▪️  입력받은 input이 비어있을 경우 예외처리<br/>
▪️  입력받은 input이 숫자가 아닌 문자가 포함될 경우 예외처리
▪️  입력받은 input에 중복된 숫자가 있을 경우 예외처리

✅  사용자 input 숫자와 랜덤 생성 정수의 자리수를 비교해 출력할 힌트를 계산한다.

▪️  숫자의 값은 같지만 자리수가 다른 경우의 수 n개 : `n볼`
▪️  숫자의 값과 자리수가 모두 같은 경우의 수 m개 : `n스트라이크`

✅  계산된 힌트를 아래 양식으로 출력한다

▪️ 볼 n개, 스트라이크 0개가 존재할 때 : `n볼`
▪️ 볼 0개, 스트라이크 n개가 존재할 때 : `n스트라이크`
▪️ 볼 n개, 스트라이크 m개가 존재할 때 : `n볼 m스트라이크`
▪️ 볼 0개, 스트라이크 0개가 존재할 때 : `낫싱`

✅ 게임 클리어 여부 판단

▪️ n스트라이크가 아니라면`, 다시 사용자에게 입력을 숫자를 받고, 힌트를 출력한다.
▪️ n스트라이크를 맞추었다면`, 아래 메세지를 출력하고 사용자에게 플래그를 입력받는다.
▪️ n개의 숫자를 모두 맞히셨습니다! 게임 종료`
▪️ 게임을 새로 시작하려면 1, 종료하려면 2를 입력하세요.`
  ▪️  입력받은 input이 1과 2가 아닌 숫자일 경우 예외처리 
  ▪️  입력값에 따라 게임을 재시작하거나 종료한다.

🎨  구현 간 고민했던 내용들

1️⃣   확장에는 열려있고, 변경에는 닫혀있는 OCP 설계

  • input 숫자의 범위가 변하더라도(1~9), 자리수가 변하더라도 대응이 손쉽게 가능해야 한다.

    GameConfig 파일에서 NUMBER_LENGTH 변수의 value를 변경해 손쉽게 변경이 가능하다.

    개발 요구사항에서 자릿수 요청까지 처리하는 문제였다면, 더욱 OCP를 준수하는 코드 작성이 가능했을 것 같다.

숫자 3자리 숫자 4자리 숫자 5자리

2️⃣ 4번의 대규모 리팩토링, 그리고 얻어낸 값진 Number

  • 영감을 얻게 해줬던 한 마디

    객체는 '자율적인 존재'라는 점을 명심하라.
    < 중략 >
    객체는 스스로의 행동에 의해서만 상태가 변경되는 것을 보장함으로써 객체의 자율성을 보장한다.
    
    - 객체지향의 사실과 오해 中
  • First-class collection + Static Factory Method 활용

    public class Number {
        private final List<Integer> numbers;
      
        // Player Input Number Constructor
        private Number(String input) {
            validateEmpty(input);
            validateNumberLength(input);   
            validateContainOnlyNumber(input);
            validateContainDuplicatedNumber(input);
    
            this.numbers = convertInputNumber(input);
        }
            
        // Computer Generated Number Constructor
        private Number(List<Integer> computerNumber) {
            this.numbers = computerNumber;
        }
    }
  • 일급 컬렉션과 생성자 검증을 사용해 numbers에 유효하게 검증이 끝난 숫자만 들어오도록 설계

  • playerNumber와, computerNumber의 생성자는 서로 다른 파라미터를 지니기 때문에, 개발자가 사용 간 혼동 가능

    해당 문제를 해결하기 위해, 생성자를 private으로 제한하고, 의미있는 메소드로만 생성자를 호출하도록 설계

      // Computer Generated Number Constructor
      public static Number generateRandomNumbers() {
          List<Integer> randomNumbers = new ArrayList<>();
          while (randomNumbers.size() < NUMBER_LENGTH.getValue()) {
              int number = pickNumberInRange(RANDOM_NUMBER_MINIMUM.getValue(), RANDOM_NUMBER_MAXIMUM.getValue());
              if (!hasDuplicatedNumber(randomNumbers, number)) {
                  randomNumbers.add(number);
              }
          }
          return new Number(randomNumbers);
      }
            
      // Player Number Static Factory Method
      public static Number inputPlayerNumbers() {
          String playerNumbers = InputView.askPlayerNumbers();
          return new Number(playerNumbers);
      }
  • 정적 팩토리 메소드 명에 의미를 부여하고, 개발자가 직관적으로 해석할 수 있도록 했고,

    일급 컬렉션을 활용해 검증이 끝난 유효한 값만 리스트에 담을 수 있게 되었다!

@@ -0,0 +1,209 @@
# ⚾&nbsp;&nbsp;Precourse-Week1 Mission **[숫자 야구]**

## 💌&nbsp;&nbsp;목차
Copy link

Choose a reason for hiding this comment

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

경이롭네요

Copy link
Author

Choose a reason for hiding this comment

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

감사합니다 :)

Choose a reason for hiding this comment

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

깔끔한 기능 구현 목록... 감탄하고 갑니다. 🤗

Copy link
Author

Choose a reason for hiding this comment

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

많은 분들이 리드미에 좋은 평가를 남겨주시는 것 같아요!
가시성 높은 리드미를 만들고자 노력했는데 좋은 평가를 남겨주셔서 감사합니다!

Choose a reason for hiding this comment

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

너무나도 인상 깊은 기능 구현 목록인거 같습니다! 😀

Choose a reason for hiding this comment

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

저 같은 경우에는 리드미 파일을 추상적으로 작성했었는데
해빈님 리드미를 보니까 엄청 비교 되네요..!

2주차 미션에는 저도 본받아야겠어요!

Choose a reason for hiding this comment

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

리드미를 이렇게나 자세하게 쓸 수 있군요... 많이 배웠어요:)

import static baseball.validator.InputValidator.*;
import static camp.nextstep.edu.missionutils.Randoms.pickNumberInRange;

public class Number {

Choose a reason for hiding this comment

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

Number라는 이름은 너무 일반적이고 포괄적인 것 같습니다. 처음에 보고 int나 long, double에 대한 연산을 담은 객체인가 싶었네요. 포함하고 있는 연산들을 보면 숫자야구에서만 사용될 수 있는 것이 대부분이므로 좀 더 이름을 구체적으로 짓는 것이 어떨까요?

Copy link
Author

Choose a reason for hiding this comment

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

위에서 @zxcev , @Gyu-won 님께서 말씀주신 내용과 같은 맥락이라고 생각됩니다.
우리가 숫자야구라는 비즈니스 로직을 아는 상황에서 읽는 코드에서 나름 직관적인 네이밍이라고 생각해서 설계했는데,
최초 설계처럼 두 Number를 분리해서 가져가는게 좋을 것 같아요!

Number 설계 자체를 한 번 바꿔서 다시 설계해봐야겠어요!
추후 설계를 바꾼 뒤에, 다시 한 번 관심 가져주시면, 더 좋은 코드로 보답하겠습니다 :)

성실한 리뷰 감사드려요!!

Copy link

@SuranS2 SuranS2 left a comment

Choose a reason for hiding this comment

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

result와 outputview 관계 질문

}
```
- 정적 팩토리 메소드 명에 의미를 부여하고, 개발자가 직관적으로 해석할 수 있도록 했고,<br/>
일급 컬렉션을 활용해 검증이 끝난 유효한 값만 리스트에 담을 수 있게 되었다!
Copy link

Choose a reason for hiding this comment

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

너무 이쁜 표지에 감탄하고갑니다! 정리가 잘 되어있다는 느낌 :)

Copy link
Author

Choose a reason for hiding this comment

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

가시성 높은 리드미를 만들고자 노력했는데 좋은 평가를 남겨주셔서 감사합니다!

Comment on lines +24 to +27
Result result = Result.create(playerNumber, computerNumber);
OutputView.printMessage(result.createResultMessage());
if (result.checkGameOver()) {
break;
Copy link

@SuranS2 SuranS2 Oct 25, 2023

Choose a reason for hiding this comment

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

P4) MVC모델에 무지하고 대학생 지식이 다여서 도움이 될지는 잘 모르겠습니다.
초심자 레벨에서 본 리뷰라고 생각해주시고 너그럽게 봐주십사 합니다. :)

MVC모델의 개략적인 구조를 보았을때
데이터처리 M 전체 작동 C 외부 커뮤니케이션 V 이렇게 작동하는 것으로 생각됩니다.

입력을 받게되는 V => M 에서
Input => Number 로 처리되기에Number내에서 선언된 inputview로 입력 받고 있네요.

그렇다면 출력은 반대순서인 내부에서 바깥으로 M => V 일 것인데
result.printMessage 형태로 result에 넣어서 같은 사용법으로 만들면 안됐을까? 라는 생각을 해봅니다
소중한 리뷰 및 좋은 코드 감사합니다. 많이 배워갑니다! 🙏

Copy link
Author

Choose a reason for hiding this comment

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

MVC 모델의 연관관계에서 호출 위치에 대한 리뷰를 남겨주신 것으로 이해했습니다!
입력은 View -> Model 방향으로 전달되고,
출력은 Model -> View 방향으로 전달되어 출력해야 정석적인 설계라고 판단됩니다!

OutputView는 출력할 책임만 가진다! 라는 원칙에 입각해서, 설계했던 내용인 것 같아요.
만약 Result가 아니라 출력해야할 객체가 여러개 존재해야 했다면, 모든 객체가 print 메소드를 들고 있어야 하니까요!

그런 의미로 OutputView의 printMessage를 Static Method로 정의해
View의 Print 호출 -> Model의 출력 내용 -> View는 출력 내용 출력
이런 Flow를 그리게 되었는데요.

스크린샷 2023-10-26 오후 11 50 26

해당 그림에서 UserAction에 해당하는 부분은

Input View -> Controller
Controller -> OutputView

이런 방향으로 설계했는데, 역할과 책임의 분리는 적절했다고 생각하지만, 호출 방향은 조금 더 고차원적인 설계의 문제네요!
어떤게 정답인지는 아직 잘 감이 서지 않아요!!

MVC 패턴을 조금 더 깊이 공부해야 할 것 같아요! 저도 MVC는 문외한인 부분이라서....T_T
이 부분에 도움을 주실 수 있는 리뷰어 분이 계신다면 부연 설명 남겨주시면 도움이 많이 될 것 같습니다!

좋은 질문 감사드립니다 :)

@@ -0,0 +1,11 @@
package baseball.global.exception;

Choose a reason for hiding this comment

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

제 코드에서도 예외처리부분을 따로 빼고싶었는데 구상이 안갔었는데 많은 도움 되었습니다

Copy link
Author

Choose a reason for hiding this comment

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

도움이 되었다고 하니 뿌듯합니다 :)
Enum과 커스텀 익셉션을 조합해서 사용했는데, 다음 설계에 도움이 되시길 바랍니다!

Copy link

@IMWoo94 IMWoo94 left a comment

Choose a reason for hiding this comment

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

들어오자마다 압도 한번 당하고 리뷰를 해보았습니다.
이미 많은 분들이 리뷰를 해주어서 궁금한점 위주로 질문 드린거 같습니다.
보면 정말 짜임새 있고 기능도 확실하게 무엇을 목적을 가지고 진행하려 하는지 뚜렷하게 느껴지는 코드 였습니다.
많은 것 을 배워 갑니다.

}
```
- 정적 팩토리 메소드 명에 의미를 부여하고, 개발자가 직관적으로 해석할 수 있도록 했고,<br/>
일급 컬렉션을 활용해 검증이 끝난 유효한 값만 리스트에 담을 수 있게 되었다!
Copy link

Choose a reason for hiding this comment

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

보자마자 압도 당했습니다...

Copy link
Author

Choose a reason for hiding this comment

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

껄껄 과찬이십니다 :)
좋은 리뷰 감사합니다!

List<Integer> randomNumbers = new ArrayList<>();
while (randomNumbers.size() < NUMBER_LENGTH.getValue()) {
int number = pickNumberInRange(RANDOM_NUMBER_MINIMUM.getValue(), RANDOM_NUMBER_MAXIMUM.getValue());
if (!hasDuplicatedNumber(randomNumbers, number)) {
Copy link

Choose a reason for hiding this comment

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

해당 부분은 여기서만 사용하는 것이라면 특정 메소드로 분리하지 않고 contains 를 바로 사용해도 되지 않을가요??

Copy link
Author

Choose a reason for hiding this comment

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

메소드를 단일 호출로 사용함에도 분리했던 이유는, 명명을 통한 직관적인 해석이 가능하도록 만들기 위해서였습니다.
불필요한 코드 낭비라고 생각할 수도 있지만, 저는 개인적으로 위에서 부터 줄줄히 읽었을 때 제대로 읽히는 코드를 선호합니다 :)
그런 의미로, contain()의 의미를 해석하는 것 보다, hasDuplicatedNumber 메소드를 읽었을 때
개발자가 이런 목적으로 작성한 코드이구나! 라는 점을 명백하게 이해할 수 있을 거라고 생각했습니다.

물론 무분별한 분리와 분기는 레거시 코드 양을 늘리고, 자칫 더 복잡한 버그를 낳을 수도 있을거라 생각해요!
@IMWoo94 님의 생각이 더 궁금합니다!

Copy link

Choose a reason for hiding this comment

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

직관적인 해석 한눈에 보았을 때, 파악이 쉽게 되도록 구현하는 것 좋은 코드 입니다.
다만 저의 생각은 hasDuplicatedNumber 라는 메소드를 보았을때, "중복 숫자가 있는지 체크" 라는 의미로 해석이 되지만
메소드로 생성되어 있다보니 혹시나 중복체크 말고 또 다른 기능이 있지 않을까 라는 의심을 가지게 됩니다.
직관적인 메소드명을 가지고 있어도 개발자 입장에서는 곧이곧대로 믿지 않고 한번쯤은 의심을 해보고 직접 찾아가 보아야 한다고 생각합니다.
혹시나 hasDuplicatedNumber 리스트 값을 변경한다면? 값을 날려버린다면? 이러한 생각을 하게 되는 거 같습니다.
따라서 단순 contain 만 사용하는 거라면 if 문에 직접 사용하는게 어떠한가 했습니다.

좋은 코드 잘 보고 많이 배우고 갑니다!

Copy link
Author

Choose a reason for hiding this comment

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

역시 개발자에게 꼼꼼한 검토와 의심은 좋은 습관이라고 저도 확신합니다!
좋은 설계에 정답은 없지만, 정답에 가까운 답을 찾아가는게 개발자에게는 늘 숙제라고 생각됩니다.

같이 좋은 설계에 대해서 고민해주셔서 감사합니다!
저도 많이 배우고 갑니다 :)

private boolean isStrike(int number, int digit) {
return number == numbers.get(digit);
}
}
Copy link

Choose a reason for hiding this comment

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

Number 라는 도메인이라는 이름에 좀 많은 메소드가 들어 있어서 Number 의 역할이 2개 이상이라고 느낄거 같아요
야구게임 관련 체크 유틸을 만들어서 ball, strike 를 판별 할 수 있는 네이밍을 가진 클래스가 있다면 Number는 Number 역할만 할 수 있지 않을가요?

Copy link
Author

Choose a reason for hiding this comment

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

맞습니다..! 사실 Number <-> Result 관계에 대해서 너무 고민을 많이 했던 것 같아요.
나름 DDD 구조를 모방한 패턴으로 적용해서, getter() 없는 설계를 하는데 너무 초점을 맞췄던 것 같기도 하구요!

일급 컬렉션에 담은 값들을, 외부에서 값 타입으로 꺼내서 사용하는 순간 그 의미가 퇴색된다고 보았기 때문에
해당 내용을 전부 담은 Number라는 객체가 생각보다 많은 의미와 책임을 담고 있었다고 생각이 드네요.

많은 분들이 리뷰해주셨던 내용 중 가장 기억에 남는 내용은,
Number를 추상클래스로 바꾸고, Number를 상속하는 ComputerNumber, PlayerNumber와 같은 방식으로 분기해서
작동하도록 설계한다면, 객체지향적 설계 + 일급 컬렉션의 값타입 참조하지 않기(getter 사용 안하기) + 책임 분리
모든 토끼를 잘 잡을 수 있을 것 같아요!

좋은 리뷰 감사드립니다 :)

Copy link

Choose a reason for hiding this comment

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

상속도 좋은 방식인거 같습니다.
각각의 역할을 확실하게 나누어서 자식의 기능은 별도로 구성한다.
상속도 좋지만 이러한 경우에는 인터페이스도 생각해 볼만도 한거같습니다.
클래스와 클래스간의 상속은 하나만 상속할 수 있다는 불편한 제약이 있어서 인터페이스도 고려할만 한거 같습니다.

Choose a reason for hiding this comment

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

저는 number, numbers 표기보다 구체적으로 네이밍을 하는게 로직을 파악하기 쉬울 것 같다는 생각이 들어요!
쭉 읽다보면 numbers가 뭐지? 하면서 다시 올려다보고 하는 경우가 발생할 수도 있다는 생각이 들어서 남겨봅니다:)

public static BaseballException of(ErrorMessage errorMessage) {
return new BaseballException(errorMessage);
}
}
Copy link

Choose a reason for hiding this comment

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

Exception 을 of 로 별도로 생성해주시는 건
에러 발생 시 추후 log 등의 기능을 추가하기 위함 일가요?

Copy link
Author

Choose a reason for hiding this comment

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

해당 개발과제의 요구사항은 아니기에, log를 고려하고 진행했던 설계는 아니였습니다.
다만, of라는 정적 팩토리메소드를 정의해, new 연산자를 사용하지 않고 의미있는 명명(of가 의미가 있다고 판단될 지는 미지수지만..?)
을 통해 Exception을 설계하였습니다!

Copy link

Choose a reason for hiding this comment

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

자바에서 예외처리는 예외 복구, 예외 회피, 예외전환을 방식이 각각 있습니다.
예외 복구는 try catch -> 예외를 처리하고 정상 로직 처리
예외 회피는 throw 로 상위 컨텐츠에게 위임
에외 전환은 checkedExcpetion 을 unckeckedException 으로 변경하여 리턴 한다는 둥 이러한 방식이 제공 되기도 합니다.
예외는 정적 팩토리메소드 보다는 있는 그대로를 보여주는 것이 더 정확하다고 생각합니다.
Exception 에 대한 처리는 정확하게 처리해야 서버의 죽음을 막을 수 있고, 에러에 대한 정보가 정확 해야지 추후 원인 판단에 빠른 해결책을 제공할 수 있을거라 봅니다.

Copy link
Author

Choose a reason for hiding this comment

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

먼저, 예외 복구, 회피, 전환에 대한 내용은 스프링을 공부했었지만 해당 내용을 몰랐던 채로 throw를 마구자비로 던졌던 제 자신을 조금 반성하게 되네요..말씀 주셨던 내용은 Velog에 따로 공부하면서 포스팅을 추가해봐야겠어요!

단순히 모든 에러가 IllegalArgumentException으로 리턴되기 때문에, 에러 메세지까지 포함하는 커스텀 메시지를 포함해서 팩토리 메소드를 구상하게 되었는데, 생각보다 깊이있는 질문을 주신 것 같아서, 공부가 더욱 필요하다고 많은 자극을 얻고 갑니다!

다중 에러를 처리해야하는 상황(IleegalArgumentException 외에 다른 오류도 기능 요구사항에 있다면)에서 Exception에 대해 어떻게 설계해야 할 지 더욱 고민해봐야겠어요!

깊이있는 답변 감사드립니다!
정말 많이 배우고 가는 리뷰였습니다 :)

Choose a reason for hiding this comment

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

저도 처음엔 try-catch 문을 사용헸다가 예외가 발생되지 않는 걸 확인하고 throw문을 사용하게 되었는데, 왜 그런지는 생각 안 해보고 그냥 테스트 통과에 좋아했던 것 같네요...!

평소엔 생각치도 못 했는데 예외 처리에 대해 좀 더 깊게 생각해볼 수 있었던 것 같습니다! 좋은 말씀 감사드려요!

Copy link

@kunsanglee kunsanglee left a comment

Choose a reason for hiding this comment

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

전반적으로 코드를 엄청 깔끔하게 작성하셨네요! 잘 보고갑니다!

import static camp.nextstep.edu.missionutils.Randoms.pickNumberInRange;

public class Number {
private final List<Integer> numbers;

Choose a reason for hiding this comment

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

List를 사용하셨는데, Integer를 포장해서 List 이런 타입으로 받는것이 더 좋았을까? 싶습니다. 포장하게 되면 1~9 사이의 숫자에 대한 검증을 해당 클래스에서 처리할 수 있기도 하지만, 어느 것이 좋을지는 저도 확신이 들지 않네요! 설계하기 나름이라고 생각합니다!

.filter(i -> comparableNumber.isStrike(numbers.get(i), i))
.count();
}

Choose a reason for hiding this comment

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

저도 볼과 스트라이크를 한 번에 카운트하는 방식으로 메서드를 작성했었는데, 볼과 스트라이크를 세는 기준이 달라지면,
하나의 메서드로 작성했던 메서드를 통째로 수정해야되기 때문에 볼, 스트라이크를 카운트하는 메서드 두개로 분리했습니다. 저는 해빈님이 작성하신 로직이 향후 유지보수에 더 좋을 것 같다고 생각합니다~

return !hasBall() && !hasStrike();
}

private String generateResultMessage(ResultType resultType) {

Choose a reason for hiding this comment

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

각 케이스에 맞게 단순히 메서드로 반환해도 좋을거라는 생각이 듭니다!

Comment on lines +33 to +37
public static void validateNumberLength(final String number) {
if (!isValidLength(number)) {
throw BaseballException.of(INVALID_LENGTH);
}
}

Choose a reason for hiding this comment

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

validateNumberLength() 메서드 안에서 isValidLength() 메서드를 호출해서 boolean을 받아서 에러를 처리하고 계신데,
그냥 validateNumberLength() 메서드 안에서 처리해도 메서드 네이밍으로 의도를 알기 쉬울 것 같습니다.
혹시 isValidLength() 메서드를 다른 곳에서 사용하는 것을 염두에 두고 이렇게 작성하신걸까요?

Copy link
Author

Choose a reason for hiding this comment

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

검증메소드 -> 검증 결과에 따른 포스트컨디션 리턴 -> 포스트컨디션에 맞게 사후 조립

검증만을 담당하는 메소드 (boolean)
포스트컨디션(검증 결과)에 따른 예외처리 조립 (void)

이런 논리 구조를 생각하고 작성하게 되었습니다!

Choose a reason for hiding this comment

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

단순 검증만을 담당하는 메소드 <-> 검증 결과에 따라 예외 처리

역할에 따라 리턴 타입과 네이밍을 차별화한 것이 사소하지만 가독성에 도움이 되는 부분인 것 같습니다!

- [🎨&nbsp;&nbsp;구현 간 고민했던 내용들](#구현-간-고민했던-내용들)

---

Copy link

Choose a reason for hiding this comment

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

리드미 너무 예뻐요 ㅎㅎ

Copy link
Author

Choose a reason for hiding this comment

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

예쁘게 설계한 만큼 좋은 평가를 받아 너무 뿌듯합니다 :)
감사합니다!

}

private void play(Number computerNumber) {
while (true) {
Copy link

Choose a reason for hiding this comment

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

아래 분기의 result.checkGameOver() 를 잘 활용하여서 while(true) 자리의 조건 자리에 넣으시면 더 좋은 코드가 될 수 있을 거 같아요!
true 일 경우 무한 루프라는 것을 인식하지만 그렇게 될 경우 분명히 아래 코드까지 전부다 지켜봐야 어떤 조건문에 따라 종료되는지 알 수 있을 거 같아요!

Copy link
Author

Choose a reason for hiding this comment

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

result라는 객체가 생성되지 않은 시점에서 조건문으로 분기할수 없기에 부득이 이렇게 설계하게 되었습니다 ㅠㅠ
result라는 객체가 생성된 이후, 그 객체가 직접 3스트라이크 여부를 판단해야하는데,
do-while 설계도 결국 Result를 생성하지 않은 상태에서는 동작하지 않더라구요...
그렇다고 지역변수로 선언해서 재참조하면서 사용하는 설계는 좋지 못하다구 생각했구요...!!

이 부분을 어떻게 더 매끄럽게 고칠 수 있을지 연구해보겠습니다!!
정말정말 좋은 리뷰 감사드려요! :)

Copy link

@Gyu-won Gyu-won left a comment

Choose a reason for hiding this comment

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

mvc 패턴으로 너무 복잡하지 않게 잘 구현하신 것 같습니다. 배울점이 많았습니다.
다른 분들의 리뷰를 읽으면서도 많은 도움이 되었습니다.

팔로우 해서 앞으로 이어지는 주차에서도 h-beeen님 코드는 꼭 읽어봐야 할 것 같네요. 좋은 코드 공유 감사합니다!

Comment on lines +90 to +123
## ✨&nbsp;&nbsp;기능 구현 목록

✅ `a ~ b` 사이의 서로 값이 다른 `n자리`의 정수를 랜덤으로 생성한다.

▪️ Default Setting : `1 ~ 9`사이의 서로 값이 다른 `3자리`의 정수

✅&nbsp;&nbsp;게임 시작 문구 출력<br/>
✅&nbsp;&nbsp;사용자에게 `a ~ b 사이의 서로 값이 다른 n자리의 정수`를 입력 받는다.

▪️ 입력받은 input이 비어있을 경우 예외처리<br/>
▪️ 입력받은 input이 숫자가 아닌 문자가 포함될 경우 예외처리
▪️ 입력받은 input에 중복된 숫자가 있을 경우 예외처리

✅&nbsp;&nbsp;사용자 input 숫자와 랜덤 생성 정수의 자리수를 비교해 출력할 힌트를 계산한다.

▪️ 숫자의 값은 같지만 자리수가 다른 경우의 수 n개 : `n볼`
▪️ 숫자의 값과 자리수가 모두 같은 경우의 수 m개 : `n스트라이크`

✅&nbsp;&nbsp;계산된 힌트를 아래 양식으로 출력한다

▪️ 볼 n개, 스트라이크 0개가 존재할 때 : `n볼`
▪️ 볼 0개, 스트라이크 n개가 존재할 때 : `n스트라이크`
▪️ 볼 n개, 스트라이크 m개가 존재할 때 : `n볼 m스트라이크`
▪️ 볼 0개, 스트라이크 0개가 존재할 때 : `낫싱`

✅ 게임 클리어 여부 판단

▪️ n스트라이크가 아니라면`, 다시 사용자에게 입력을 숫자를 받고, 힌트를 출력한다.
▪️ n스트라이크를 맞추었다면`, 아래 메세지를 출력하고 사용자에게 플래그를 입력받는다.
▪️ n개의 숫자를 모두 맞히셨습니다! 게임 종료`
▪️ 게임을 새로 시작하려면 1, 종료하려면 2를 입력하세요.`
▪️ 입력받은 input이 1과 2가 아닌 숫자일 경우 예외처리
▪️ 입력값에 따라 게임을 재시작하거나 종료한다.

Copy link

Choose a reason for hiding this comment

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

기능 명세서가 자세히 되어 있는건 좋은 것 같습니다. 코드랑 비교하면서 보니 mvc 패턴으로 작성하셨던데, 어떤 객체가 위 기능들을 할당하는지도 같이 명시되어 있으면 좋을것 같아요!

예를 들어 계산된 사용자 input 숫자와 랜덤 생성 정수의 자리수를 비교해 출력할 힌트는 Number domain에서 구현하셨던데 이것을 표시할 수 있다면 좋을 것 같습니다.

기능명세서가 구현 전 어떤 기능이 필요할지 생각하고 구현에 도움을 주는 문서인 만큼 최대한 구현과 가깝게 작성하시면 좋을 것 같아요.

Copy link
Author

Choose a reason for hiding this comment

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

이 부분은 다음 과제에서 꼭 반영하도록 하겠습니다!
기능구현서 라고 정말 기능만 단순하게 나열했는데
이 부분을 링크해서 같이 표현한다면 더욱 직관적일 것 같아요!

특히나 도메인별로 분류해서 적었으면 더 좋지 않았을까..하는 아쉬움이 드네요!
다음에는 플로우 차트도 만들어보려고 해요.
더 많이 관심 가져주시면 더 좋은 코드로 뵙겠습니다!!

좋은 리뷰 감사합니다 :)

Number computerNumber = Number.generateRandomNumbers();
play(computerNumber);
} while (!askRestartOrExit());
}
Copy link

Choose a reason for hiding this comment

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

true 도 상수화를 통해 true의 의미를 더 잘 나타내면 좋을 것 같습니다. 저 문장만 봤을때는 whie 블록이 어떤 코드인지 알기가 어렵네요.

Copy link
Author

Choose a reason for hiding this comment

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

이 부분도 많은 분들이 리뷰주셨던 내용인 것 같습니다!
개인적으로 !askRestartOrExit() 함수 부분의 설계가 조금 아쉬웠다고 생각합니다.

이 부분은 수정해서 더 좋은 코드로 로컬에 저장하고 다시 찾아뵙도록 하겠습니다!!
좋은 리뷰 감사합니다 :)


// Player Number Static Factory Method
public static Number inputPlayerNumbers() {
String playerNumbers = InputView.askPlayerNumbers();
Copy link

Choose a reason for hiding this comment

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

controller로 뺴는게 맞다고 생각합니다. 코드를 읽어보니 결과를 출력하는 부분은 outputView를 통해서 출력하셨는데 입력은 Number model에서 받고 있어서 view의 역할이 불분명 한 것 같아요.

Controller 코드를 읽을 때도 outputView를 통해 결과를 출력하긴 하는데 inputView를 통해 입력을 받는 부분은 없어서 의아했습니다.

입력 메시지를 출력해주고 사용자의 입력을 받는 부분은 view에서 처리하는게 좋을 것 같습니다.

}

// Computer Number Static Factory Method
public static Number generateRandomNumbers() {
Copy link

Choose a reason for hiding this comment

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

공감합니다. 지금의 코드에서는 사용자의 입력 인스턴스에도 ball과 strike를 계산하는 메소드도 포함되어 있습니다.
반대로 결과 인스턴스에도 랜덤넘버를 생성할 수 있는 기능이 포함되어 있고요.

getter를 쓰지 않기 위해서 구현하셨지만 객체의 의미가 불명확해 진 것 같습니다. 숫자야구라는 과제를 알고 있는 저의 입장에서도 처음 Number라는 객체를 봤을때 어떤걸 의미하는 건지 알기 어려웠어요.

저도 이부분을 고민하고 있었는데 앞에 분이 추상클래스 라는 좋은 답을 주신 것 같습니다. 다형성을 활용하여 코드를 짜보면 좋을 것 같아요!

RANDOM_NUMBER_MINIMUM(1),
RANDOM_NUMBER_MAXIMUM(9),

EXIT_FLAG(1),
Copy link

Choose a reason for hiding this comment

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

Flag는 true, false 를 나타내는 boolean 타입에 적절한 변수명이라고 생각합니다. 위와 같이 int형으로 지정하셨으면 flag 대신 value를 사용하는 것이 더 좋을 것 같아요.

Copy link

@jschoi-96 jschoi-96 left a comment

Choose a reason for hiding this comment

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

바보같이 리뷰쓰고 서밋을 안해버린..

@@ -0,0 +1,209 @@
# ⚾&nbsp;&nbsp;Precourse-Week1 Mission **[숫자 야구]**

Choose a reason for hiding this comment

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

리드미가 너무 보기 좋네요! 나중에 협업할 때도 되게 좋을 것 같습니다


// Player Number Static Factory Method
public static Number inputPlayerNumbers() {
String playerNumbers = InputView.askPlayerNumbers();

Choose a reason for hiding this comment

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

view에서 입력을 받고 그 정보를 컨트롤러로 넘겨주면 어떨까요?

public static BaseballException of(ErrorMessage errorMessage) {
return new BaseballException(errorMessage);
}
}

Choose a reason for hiding this comment

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

에러 메세지를 한번에 처리하시게 구현하셨군요! 이부분 많이 배워갑니다

Copy link
Author

Choose a reason for hiding this comment

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

view에서 입력을 받고 그 정보를 컨트롤러로 넘겨주면 어떨까요?

맞아요! 이 부분도 많은 분들이 리뷰해주신 사항입니다!
View에서 입력을 받고, 그 내용을 컨트롤러로 넘기는 방향이 슬기로운 설계인 것 같습니다!
이 부분은 단순히 InputView, OutputView로 단순히 분기했다는 사항만을 생각하고
무분별하게 static 메소드를 호출하면서 혼란이 있었던 것 같아요!

좋은 리뷰 감사드립니다!

에러 메세지를 한번에 처리하시게 구현하셨군요! 이부분 많이 배워갑니다

이 부분도 정적 팩토리 메소드를 활용해 IlleagalArgumentException을 커스텀해서 던지도록 설계했습니다!
에러 메세지까지 Enum 값까지 get으로 건드리지 않고 사용할 수 있도록 설계해서 아주 예쁘게 설계된 것 같습니다!
감사합니다 :)

}
if (hasStrike()) {
return ONLY_STRIKE;
}

Choose a reason for hiding this comment

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

Result를 출력하는 부분이 가독성이 좋아보여서 많은 공부가 되네요

Copy link
Author

Choose a reason for hiding this comment

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

감사합니다 :)

Choose a reason for hiding this comment

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

확률로 비교를 했을 때 ONLY_BALL > NOTHING > ONLY_STRIKE > BALL_AND_STRIKE로 추측되는데 확률이 높은 순서로 작성하는 것에 대해 어떻게 생각하시는지 궁금합니다!(제가 틀렸을 수도 있어요 ㅎㅎ)
다른 분들이 이미 많은 리뷰를 남겨주셔서 이런거라도 적어봅니다...😀

Copy link
Author

Choose a reason for hiding this comment

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

호출 확률보다는 확실한 팀 내 컨벤션을 정해서 Enum 내부 상수를 정의하는게 더욱 읽기 편할 것 같다는 생각이 들어요!
가령 예를 들자면 오름차순, 내림차순의 예시를 말씀드릴 수 있을 것 같아요!

그리고 그렇게 차순별로 분류된 상수들을 개행을 활용해 구분하는 방법도 있을 수 있겠네요!
다만, 말씀주신 확률에 대한 내용은, 프로젝트 볼륨에 따라서 예측하기 어려운 부분이거나 지나치게 복잡한 생각을 하게 만들 수도 있을 것 같다는 의견을 조심스럽게 전달 드리고 싶습니다!

좋은 질문 감사합니다 :)

OutputView.printMessage(result.createResultMessage());
if (result.checkGameOver()) {
break;
}

Choose a reason for hiding this comment

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

메서드 네이밍에 대해서 고민을 많이 하신게 보입니다! 이름만 봐도 어떤 역할을 하고 있는지 한눈에 들어오네요

Copy link
Author

Choose a reason for hiding this comment

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

위에서 줄줄이 읽어도 딱 이해가는 코드로 만들고자 했던 제 노력을 이해해주셔서 너무 감사드립니다:)
좋은 칭찬 감사드립니다!

Copy link

@noxknow noxknow 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 +59 to +69
public int countBallCount(final Number comparableNumber) {
return (int) IntStream.range(0, numbers.size())
.filter(i -> comparableNumber.isBall(numbers.get(i), i))
.count();
}

public int countStrikeCount(final Number comparableNumber) {
return (int) IntStream.range(0, numbers.size())
.filter(i -> comparableNumber.isStrike(numbers.get(i), i))
.count();
}
Copy link

Choose a reason for hiding this comment

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

countBallCount와 countStrikeCount 이 두 부분을 stream을 써서 처리함으로써 일급 컬렉션의 불변이 지켜지는 것 같아 좋은 것 같습니다👍하지만 메서드 명 부분은 count가 중복으로 들어가서 좀 더 명확하면 좋지 않을까 생각해요!

Copy link
Author

Choose a reason for hiding this comment

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

오..제가 몇번을 코드를 복기하며 읽어봤지만 해당 함수가 읽었을 때 의미가 모호하다는 내용은 지금에서야 깨달았어요!
단순히 finalCount, finalStrike로 정리해도 훨씬 깔끔하겠군요!!

좋은 리뷰 감사드립니다!
즐거운 하루 되세요!

Choose a reason for hiding this comment

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

@noxknow 헉, 엄청 예리하십니다..!!!!
@h-beeen getBallCount, getStrikeCount 요런 이름은 어떨까요~?

Copy link
Author

Choose a reason for hiding this comment

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

getBallCount() 와 같은 getXXX 메소드 명을 의도적으로 회피해서 설계하긴 했습니다.
getter의 의미를 많이 담고 있기 때문에 자칫 Number라는 객체가 멤버변수로 ballCount와 strikeCount를 들고 있다고 오해할 소지가 있기 때문이었습니다.

다만 @noxknow 님께서 말씀 주셨듯

하지만 메서드 명 부분은 count가 중복으로 들어가서 좀 더 명확하면 좋지 않을까 생각해요!

와 같은 이슈가 있는 코드라서

countStrike(), countBall() 정도로 getter의 영역을 벗어나고 조금 더 계산의 의미를 부여한 메소드도 적절할 수 있다고 생각해요!
어떻게 생각하시는지 @wooteco-daram 님의 생각도 궁금합니다!!

Comment on lines +4 to +7
INVALID_LENGTH("The input length cannot be different from length configured by the game."),
DUPLICATED_NUMBER("The input cannot contain duplicated numbers."),
CONTAIN_LETTER("The input cannot contain letters."),
EMPTY_NUMBER("The input cannot be empty."),
Copy link

Choose a reason for hiding this comment

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

EMPTY_NUMBER 경우와 INVALID_LENGTH의 부분이 비슷한 역할을 하는 에러가 아닌가 생각이 듭니다!

Copy link
Author

@h-beeen h-beeen Oct 26, 2023

Choose a reason for hiding this comment

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

복기해보니 EMPTY NUMBER와 INVALID_LENGTH 역시 같은 코드플로우를 타고 흐르는 것 같아요.
Empty Number가 들어왔을 때 Length를 재려고 할 때 NullPointerException이 발생하지 않을까라는 염려에
보험삼아 설계했던 부분입니다!

이 부분도 다시 한 번 면밀히 체크해서, 불필요하다면 제거하도록 하겠습니다!
감사합니다 :)

Copy link

Choose a reason for hiding this comment

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

해빈님 말씀 듣고 보니 NullPointerException이 발생할 것 같네요! validate 순서도 validateEmpty를 먼저 사용하신 이유가 있었군요..! 생각하지 못했던 부분이라 제 코드도 다시 한번 체크해야 할 것 같네요 😲

Copy link

Choose a reason for hiding this comment

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

유효성검사에 진심인 부분이 인상적입니다..!

Copy link
Author

Choose a reason for hiding this comment

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

감사합니다! 다행히 제가 생각 못한 엣지 케이스가 없던 것 같아서 가슴을 쓸어내리게되네요!

Choose a reason for hiding this comment

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

안녕하세요! 코드 엄청 잘 봤습니다!

예외를 던지는 부분이 엄청 인상 깊네요! 저도 2주차 미션때 적용해봐야겠어요!
다름이 아니라 예외처리 관련해서 궁금한 점이 있습니다.

해빈님이 작성하신 코드에는 예외를 던지고 이를 catch 하는 코드가 안보이는데
일부러 그렇게 하신건가요?

만약 catch를 하시는 코드를 작성하셔야 한다면 어디서 할지 여쭤봐도 될까요?

Copy link
Author

Choose a reason for hiding this comment

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

@sami355-24 비즈니스 요구사항이 어떻냐에 따라 다르겠지만, try-catch 문을 사용할 필요를 아직 느끼지 못했습니다.

@IMWoo94 님의 리뷰 내용을 잠시 인용하자면

자바에서 예외처리는 예외 복구, 예외 회피, 예외전환을 방식이 각각 있습니다.
예외 복구는 try catch -> 예외를 처리하고 정상 로직 처리
예외 회피는 throw 로 상위 컨텐츠에게 위임
에외 전환은 checkedExcpetion 을 unckeckedException 으로 변경하여 리턴 한다는 둥 이러한 방식이 제공 되기도 합니다.
<후략>

저는 예외 회피의 방식을 사용해서

  • boolean 메소드로 이상값 검증
  • 위 boolean 메소드 결과를 포스트컨디션으로 void 함수를 호출해 throw BaseballException.of(ERROR_MESSAGE);

이렇게 설계했습니다.
try-catch문은 결국 검증-예외처리-위임까지 다양한 책임을 한 try-catch문에서 수행하므로,
제 기준에서는 가급적 지양하는 설계였습니다!

그럼에도 불구하고 try-catch를 사용해야만 하는 상황이라면...도메인 레벨에서 생성자를 만드는 단계에서 사용하지 않을 까 싶습니다!

Copy link
Author

Choose a reason for hiding this comment

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

특히나 일급컬렉션의 생성자에서 웬만한 검증을 모두 수행하므로, 검증 실패로 생성자를 통해 객체를 생성하지 못한 상황에서 try-catch로 후방 로직을 추가로 실행할 상황이 없었기에 더더욱 try-catch의 필요성을 느끼지 못했던 것 같습니다.

Comment on lines +8 to +17
public static String askPlayerNumbers() {
printStaticNotice(ASK_PLAYER_NUMBER);
return readLine();
}

public static String askRestartOrExit() {
printStaticNotice(GAME_OVER);
printStaticNotice(ASK_RESTART_OR_EXIT);
return readLine();
}
Copy link

Choose a reason for hiding this comment

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

view 부분에 다른 로직이나 책임 없이 입력만 할 수 있도록 한 부분을 잘 작성하신 것 같아요!

Choose a reason for hiding this comment

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

저는 살짝 다른 의견인데요.
저는 개인적으로 간단한 검증(입력이 공백인지?? 입력이 숫자인지 정도??)은 입력을 할 때 검사하는게 옳다고 느껴져요.
잘못된 입력 값을 가지고도 domain에서 모든 검증을 하게 된다면 오히려 domain 영역에 책임이 늘어나 더 안 좋다 생각합니다!

물론 이런 부분은 정답이 없다고 느껴지지만, 부족한 저의 의견이였습니다!ㅎㅎ

Copy link

@seondays seondays left a comment

Choose a reason for hiding this comment

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

안녕하세요 해빈님! 작성하신 코드 둘러보면서 공부해야 할 것들에 대해 방향성을 잡는 데 도움 많이 받았습니다. number를 일급컬렉션으로 만드셔서 활용하신 점과, enum으로 깔끔하게 상수들을 가져다 쓸 수 있도록 하신 부분이 좋았습니다.
아직 부족해서 길게 리뷰를 달아드리진 못해 죄송스럽네요, 고생 많으셨습니다!

Choose a reason for hiding this comment

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

enum으로 에러 메시지까지 묶어서 구현하신 부분에서 배움 얻고 갑니다!

Copy link
Author

Choose a reason for hiding this comment

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

도움이 되었다니 다행입니다 :) 감사합니다!

private static boolean isValidNumber(final String number) {
return number
.chars()
.allMatch(c -> Character.isDigit(c) && c >= '1' && c <= '9');

Choose a reason for hiding this comment

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

1과 9에도 의미를 부여할 수 있도록 상수로 선언해 주시는 것은 어떠실까요?!

Copy link
Author

Choose a reason for hiding this comment

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

맞아요! 이부분도 순간 char형 문자라서 망각하고 사용하게 되었는데!
이 부분도, 포장해서 사용하거나, 상수로 사용하는게 적절했던 것 같습니다 :)

좋은 리뷰 감사드립니다!!

Copy link

@leemiyeonn leemiyeonn left a comment

Choose a reason for hiding this comment

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

저와 비슷한 고민을 많이 하신 것 같아 도움이 많이 되었습니다! 특히 enum 활용 부분과 예외 처리 부분이 인상 깊었습니다! 깔끔한 readme 까지 많이 배워갑니다!

import static baseball.view.OutputView.printStaticNotice;
import static baseball.view.constants.StaticNotice.GAME_START;

public class Game {

Choose a reason for hiding this comment

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

GameController와 Game에 대해서 고민을 많이 했던 것 같습니다! GameController.start / Game.start 이 상호 클래스 명을 고민했을 때 Game이 더 적절하게 클래스의 이름을 나타냈다고 생각해서 설계하게 되었습니다.

제가 나타내고자 했던 의도가 분명히 전달된 것 같아 뿌듯합니다 :) 좋은 리뷰 감사드립니다!!

저도 이부분 많이 고민했었는데 '흐름을 관리하는 역할을 하는 클래스' 를 생각했을 때 Controller 가 직관적으로 떠올라 클래스 이름에 Controller를 포함하여 이름 지었습니다. 이름에 나타내지 않아도 충분히 의미를 전달할 수 있을까? 가 고민이었는데 위에 남겨주신 리뷰가 그 답이 되었습니다!

Comment on lines +5 to +8
NOTHING("낫싱"),
ONLY_BALL("%d볼"),
ONLY_STRIKE("%d스트라이크"),
BALL_AND_STRIKE("%d볼 %d스트라이크");

Choose a reason for hiding this comment

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

저도 enum 으로 메세지 타입을 관리했는데 결과 출력 부분에서 막혔었습니다!ㅜ
포맷을 정해주는 방식은 생각하지 못했었는데!! 많이 배워갑니다!

@h-beeen
Copy link
Author

h-beeen commented Oct 26, 2023

@leemiyeonn 아래 enum 부분도 미연님 코드에 리뷰 남겨드렸어요! 참고하시면 도움 되실거라 믿어요 :) 좋은 리뷰 감사합니다!

@@ -0,0 +1,209 @@
# ⚾&nbsp;&nbsp;Precourse-Week1 Mission **[숫자 야구]**
Copy link

Choose a reason for hiding this comment

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

README.md 를 이렇게 깔끔하고 예쁘게 작성할 수 있다니.. 너무 놀라워요.. 한 수 배워갑니다. 저는 너무 간결하게만 작성한 것 같아 반성하게 되네요.. ! 😅

Copy link
Author

Choose a reason for hiding this comment

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

좋게 봐주셔서 감사드립니다 :)

package baseball.global.exception;

public enum ErrorMessage {
INVALID_LENGTH("The input length cannot be different from length configured by the game."),
Copy link

Choose a reason for hiding this comment

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

주어진 문제 구현 이외에 +@에 대해서 정말 열심히 고민하시고 코드를 작성하셨다는게 느껴져요.. 👍

Copy link
Author

Choose a reason for hiding this comment

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

이 부분도 많은 고민 끝에 탄생한 코드인데, 좋은 설계라고 칭찬을 받아 뿌듯합니다.
감사합니다 !!

Copy link

@jcoding-play jcoding-play left a comment

Choose a reason for hiding this comment

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

인상깊게 본 부분도, 배울 점도 너무나도 많은 코드였던 거 같아요!! 시간되실 때 맞리뷰 부탁드립니다!!

Choose a reason for hiding this comment

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

너무나도 인상 깊은 기능 구현 목록인거 같습니다! 😀

import static baseball.validator.InputValidator.*;
import static camp.nextstep.edu.missionutils.Randoms.pickNumberInRange;

public class Number {

Choose a reason for hiding this comment

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

Number라는 클래스명은 이미 추상 클래스로 정의되어 있어서 웬만해선 피하시는게 좋다는 생각이 들어요!!
또한 이것을 제외하고도 Number 객체가 현재 너무 많은 책임을 가지고 있다 느껴져서 조금씩 분리해보는 것도 좋다고 생각들어요!
부족한 저의 의견입니다.

Copy link
Author

@h-beeen h-beeen Oct 27, 2023

Choose a reason for hiding this comment

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

Number라는 클래스명은 이미 추상 클래스로 정의되어 있어서 웬만해선 피하시는게 좋다는 생각이 들어요!!

이 부분은 지금 제 설계에서 어떤 말씀인지 제대로 이해하지 못했습니다!
추가 설명 남겨주시면 많은 도움 구할 수 있을 것 같아요.

또한 이것을 제외하고도 Number 객체가 현재 너무 많은 책임을 가지고 있다 느껴져서 조금씩 분리해보는 것도 좋다고 생각들어요!

이 부분을 Number 추상화를 통해 해결할 수 있을 것 같아요! 공통 책임(숫자를 받아서 검증하고 객체화)을 추상화하고, 나머지를 구현체에서 정의하면 더욱 잘 분리된 설계가 되지 않을까 싶습니다!!

감사합니다 :)

}

private boolean isNothing() {
return !hasBall() && !hasStrike();

Choose a reason for hiding this comment

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

개인적으로 저는 예상치 못한 상황까지 신경써서 명확하게 구현하는 것이 좋다 느끼는 편입니다.
Nothing이 될 수 있는 경우는 한가지인데 이 부분은 Nothing이 될 수 있는 범위가 너무 넓어 side effect를 발생시킬 수 있을 거 같아요!!
side effect를 줄이기 위해 다른 방법은 무엇이 있을까요??

Copy link
Author

Choose a reason for hiding this comment

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

Not Ball And Not Strike -> Contain Any Ball Or Contain Any Strike

말씀주신 리뷰를 읽고, 로직을 곰곰히 고민해봤는데, 아직 어떤 사이드이펙트를 고려해야할지 감을 잡지 못했습니다.
낫싱의 범위가 넓게 잡혔을 때, 예상되는 사이드 이펙트가 있는지 여쭙고 싶어요!

Comment on lines +17 to +20
@Override
public String toString() {
return String.valueOf(value);
}

Choose a reason for hiding this comment

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

equals, hashcode, toString 같이 오버라이드된 메서드의 위치는 일반 메서드 또는 getter 메서드 아래에 존재하는게 관례??라고 본적이 있어요!!

Copy link
Author

Choose a reason for hiding this comment

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

함수의 선언 위치도 컨벤션이다! 라고 어디선가 들은 기억이 있어요!
이 부분은 체크하지 못했던 부분이군요!
제 코드를 정말 열심히 봐주신 것 같아 감사한 마음이 큽니다!

이 부분도 다시 설계하는 과정에서 꼭 진행해보겠습니다! 감사합니다 :)

Comment on lines +8 to +17
public static String askPlayerNumbers() {
printStaticNotice(ASK_PLAYER_NUMBER);
return readLine();
}

public static String askRestartOrExit() {
printStaticNotice(GAME_OVER);
printStaticNotice(ASK_RESTART_OR_EXIT);
return readLine();
}

Choose a reason for hiding this comment

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

저는 살짝 다른 의견인데요.
저는 개인적으로 간단한 검증(입력이 공백인지?? 입력이 숫자인지 정도??)은 입력을 할 때 검사하는게 옳다고 느껴져요.
잘못된 입력 값을 가지고도 domain에서 모든 검증을 하게 된다면 오히려 domain 영역에 책임이 늘어나 더 안 좋다 생각합니다!

물론 이런 부분은 정답이 없다고 느껴지지만, 부족한 저의 의견이였습니다!ㅎㅎ

Copy link

@wooteco-daram wooteco-daram left a comment

Choose a reason for hiding this comment

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

안녕하세요! 예외 관리와 상수 관리 모두 인상적으로 봤습니다!!!

나중에 제 PR 에도 놀러오세요!
링크 : #858

Comment on lines +59 to +69
public int countBallCount(final Number comparableNumber) {
return (int) IntStream.range(0, numbers.size())
.filter(i -> comparableNumber.isBall(numbers.get(i), i))
.count();
}

public int countStrikeCount(final Number comparableNumber) {
return (int) IntStream.range(0, numbers.size())
.filter(i -> comparableNumber.isStrike(numbers.get(i), i))
.count();
}

Choose a reason for hiding this comment

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

오.. 같은 클래스끼리면 private 메소드에 접근이 가능하다는 것 처음 알았네요!!!!
어떻게 접근해야하지 고민이었는데, 의외로 간단했네요.
좋은 인사이트를 주셔서 감사합니다!!
나중에 리팩터링할 때 참고하곘습니다. :)

Copy link
Author

Choose a reason for hiding this comment

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

코드에 DDD를 두 방울 정도 떨어뜨린 느낌으로 설계했습니다.
좋게 봐주셔서 감사합니다 :)
도움이 되었다니 뿌듯합니다!

Comment on lines +59 to +69
public int countBallCount(final Number comparableNumber) {
return (int) IntStream.range(0, numbers.size())
.filter(i -> comparableNumber.isBall(numbers.get(i), i))
.count();
}

public int countStrikeCount(final Number comparableNumber) {
return (int) IntStream.range(0, numbers.size())
.filter(i -> comparableNumber.isStrike(numbers.get(i), i))
.count();
}

Choose a reason for hiding this comment

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

@noxknow 헉, 엄청 예리하십니다..!!!!
@h-beeen getBallCount, getStrikeCount 요런 이름은 어떨까요~?

Copy link

@sami355-24 sami355-24 left a comment

Choose a reason for hiding this comment

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

진짜 코드 대박이네요! 많이 얻어갑니다!
해빈님 코드 덕분에 2주차에 응용해보고 싶은 것들이 많아졌어요!
감사합니다! :)

Choose a reason for hiding this comment

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

저 같은 경우에는 리드미 파일을 추상적으로 작성했었는데
해빈님 리드미를 보니까 엄청 비교 되네요..!

2주차 미션에는 저도 본받아야겠어요!

Choose a reason for hiding this comment

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

안녕하세요! 코드 엄청 잘 봤습니다!

예외를 던지는 부분이 엄청 인상 깊네요! 저도 2주차 미션때 적용해봐야겠어요!
다름이 아니라 예외처리 관련해서 궁금한 점이 있습니다.

해빈님이 작성하신 코드에는 예외를 던지고 이를 catch 하는 코드가 안보이는데
일부러 그렇게 하신건가요?

만약 catch를 하시는 코드를 작성하셔야 한다면 어디서 할지 여쭤봐도 될까요?

Comment on lines +6 to +11
public static void printStaticNotice(StaticNotice notice) {
System.out.print(notice.getMessage());
}

public static void printMessage(String message) {
System.out.println(message);

Choose a reason for hiding this comment

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

printMessage 메서드에
출력하는 메서드가 뿐인데 왜 따로 메서드로 작성하신 이유가 있을까요?
확장성을 고려하셔서 일까요?

GAME_START("숫자 야구 게임을 시작합니다.\n"),
ASK_PLAYER_NUMBER("숫자를 입력해주세요 : "),
ASK_RESTART_OR_EXIT("게임을 새로 시작하려면 1, 종료하려면 2를 입력하세요.\n"),
GAME_OVER(format("%d개의 숫자를 모두 맞히셨습니다! 게임 종료%n", NUMBER_LENGTH.getValue()));

Choose a reason for hiding this comment

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

enum에 포맷 사용하는건 상상도 못했네요!
배우고 갑니다!

}

// Player Number Static Factory Method
public static Number inputPlayerNumbers() {

Choose a reason for hiding this comment

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

오버로드 사용하신건 엄청 인상 깊네요!

컴퓨터가 랜덤으로 생성하는 수에도 로직이 담겨 있는 점에 대해 이미 다른 분들이 말씀하셨네요..!
DDD이야기만 들었지 적용한 코드를 실제로 보는건 처음인데 많은 생각을 던지게 만들어주시네요!

추가로 저는 필요한 검증 메서드들을 모두 호출해주는 메서드를 생성해서 호출하니까
ex) integrateValidation()
도메인에서 검증로직에 관해 추상화가 가능하더라구요
그래서 그런데 혹시 검증하는 메서드들을 하나씩 직접 호출하신 이유가 있을까요?
의견이 궁금합니다!

@wsk221e
Copy link

wsk221e commented Oct 27, 2023

안녕하세요. 코드를 보며 개인적으로 느낀 점 남겨보겠습니다.

이미 너무 많은 분들이 리뷰를 남기셔서, 제 리뷰가 도움이 될까 싶지만

이미 많이 올라와 있는 코드 개선사항 말고 초보자 입장에서 보면서 전체적으로 느낀 점을 남겨보겠습니다.

  • 처음 페이지를 열었을 때 펼쳐지는 방대한 README 파일

    • 패키지 설명, 구조도, 예외처리 내용에 대한 깔끔한 명세, 출력될 수 있는 경우의 수 정리 등 미처 생각하지 못했던 부분들이 많아 많이 배울 수 있었습니다.
  • 본인이 고민하고, 경험을 통해 얻어낸 결과 정리

    • 해빈님께서 하신 것처럼 한 번 곰곰히 고민한 내용을 다시 정리하는 것은 뇌리에 강하게 박히게 해준다고 생각합니다.
      이를 통해 나중에 다시 고민하는 수고를 줄여줄 수 있습니다. 고민한 내용을 README 파일에 정리한다는 것은 생각해보지 못했는데, 굉장히 좋은 방법 같습니다.
  • 코드의 전체적인 흐름

    • 처음 보는 메소드들도 많아 한 눈에 읽히지 않는 부분들도 있지만, 전체적으로 보았을 때 흐름이 한 눈에 들어오도록 잘 작성하신 것 같습니다. 제3자 입장에서 보았을 때 잘 읽히는 코드가 좋은 코드라고 생각하는데, 이 코드를 읽으며 그런 생각이 들었습니다.
  • Number 클래스

    • 리드미에 적으신 것처럼, Number 클래스를 통해 숫자 값들을 관리한다는 것에서 신선한 충격을 받았습니다. 게임 내에서 사용되는 숫자 값들을 각자의 역할 단위에서 저장하는 것이 아닌, Number 클래스를 통해 한꺼번에 관리하되, 직관적으로 사용하게 할 수 있다는 점을 배웠습니다.
  • 기타 에러 처리 및 잘 적용된 MVC 패턴

    • 다른 분들이 너무 잘 작성해주셔서 따로 적지는 않았습니다.

다른 곳에서 찾아보며 따로따로 본 개념들(enum 클래스, 개선된 switch문, Stream API 등)이 한 곳에 모여있는 것을 보니 하나의 작품을 보는 것만 같았습니다. 실례가 되지 않으신다면 가끔 찾아와 교과서같이 활용하고 싶습니다.

코드를 읽는 내내, 한시도 눈을 떼지 못하고 봤던 것 같습니다. 이상으로, 코드리뷰 마치겠습니다. 감사합니다.

Comment on lines 5 to 14
public class PlayerNumber {
import baseball.view.InputView;
import java.util.List;

public String inputNumber(final int numberLength) {
System.out.print("숫자를 입력해주세요 : ");
String number = Console.readLine();
public class Number {

private String number;

private Number(String number) {
Copy link

Choose a reason for hiding this comment

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

저는 클래스의 역할을 유저와 컴퓨터로 나눠서 생각해서 이렇게 다시 합쳐지게 된 이유가 있는지 궁금했는데 앞에 분들이 이미 얘기를 해주셨군요....!

Copy link
Author

Choose a reason for hiding this comment

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

가장 아쉬웠던 부분이에요!
도메인 계층에서 검증 + 생성까지 처리하면서 추상화를 활용했다면!!!
개인적으로 해당 내용이 이번 리뷰를 통해 가장 많은 아이디어를 얻었던 리뷰였던 것 같아요!

감사합니다 :)

docs/README.md Outdated
Comment on lines 1 to 3
# 📝&nbsp;&nbsp;Precourse-Week1 Mission **[숫자 야구]**

## ✨&nbsp;&nbsp;클래스 및 기능 구현 목록
Copy link

Choose a reason for hiding this comment

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

지금까지 본 구현 목록 문서중에 가장 깔끔하게 작성하신거 같에요 볼때마다 감탄이 나오네요.

Copy link
Author

Choose a reason for hiding this comment

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

많은 분들이 리드미를 좋게 봐주신 것 같아요 :)
진심으로 감사드립니다!

Copy link

@dev-prao dev-prao left a comment

Choose a reason for hiding this comment

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

코드를 작성하면서 고민하신 흔적부터 꼼꼼한 README와 다양한 개념을 적용하신 코드까지... 저에게 아직 갈길이 많다는 걸 느끼고 배워갑니다..ㅎㅎ

Choose a reason for hiding this comment

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

리드미를 이렇게나 자세하게 쓸 수 있군요... 많이 배웠어요:)

private boolean isStrike(int number, int digit) {
return number == numbers.get(digit);
}
}

Choose a reason for hiding this comment

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

저는 number, numbers 표기보다 구체적으로 네이밍을 하는게 로직을 파악하기 쉬울 것 같다는 생각이 들어요!
쭉 읽다보면 numbers가 뭐지? 하면서 다시 올려다보고 하는 경우가 발생할 수도 있다는 생각이 들어서 남겨봅니다:)

private final int ballCount;
private final int strikeCount;

private Result(final Number playerNumber, final Number computerNumber) {

Choose a reason for hiding this comment

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

저도 ballCount와 strikeCount는 private로 은닉하고 getter 없이 반환하는 걸 이렇게 할 수 있구나...하면서 배워갑니다😃

}
if (hasStrike()) {
return ONLY_STRIKE;
}

Choose a reason for hiding this comment

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

확률로 비교를 했을 때 ONLY_BALL > NOTHING > ONLY_STRIKE > BALL_AND_STRIKE로 추측되는데 확률이 높은 순서로 작성하는 것에 대해 어떻게 생각하시는지 궁금합니다!(제가 틀렸을 수도 있어요 ㅎㅎ)
다른 분들이 이미 많은 리뷰를 남겨주셔서 이런거라도 적어봅니다...😀

Comment on lines +49 to +55
return switch (resultType) {
case NOTHING -> resultType.getValue();
case BALL_AND_STRIKE -> format(BALL_AND_STRIKE.getValue(), ballCount, strikeCount);
case ONLY_BALL -> format(ONLY_BALL.getValue(), ballCount);
case ONLY_STRIKE -> format(ONLY_STRIKE.getValue(), strikeCount);
};
}

Choose a reason for hiding this comment

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

17의 새로운 문법을 보면서 알게 된 내용인데 바로 적용하신 모습이 멋지십니다👍👍
확실히 가독성도 좋네요!!

Copy link
Author

Choose a reason for hiding this comment

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

IntelliJ가 해당 기능은 오토 포맷팅으로도 제공한답니다!
맥 유저시라면 Opt + Enter 버튼을 누르면 기존 스위치문을 새로운 확장된 switch 문으로 변경해준답니다!

public boolean checkGameOver() {
return strikeCount == NUMBER_LENGTH.getValue();
}
}

Choose a reason for hiding this comment

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

제가 궁금한 부분은 가독성 측면에서 hasBall()hasStrike()를 만드신 것 같습니다!
복잡하지 않은 로직의 경우 저는 바로 조건문에 기입하곤 하는데,
조건문에 바로 기입했을 때와 hasBall()과 같이 새로운 메서드를 생성했을 때 어떤 것이 나은지 해빈님의 생각이 궁금해요😀

RANDOM_NUMBER_MINIMUM(1),
RANDOM_NUMBER_MAXIMUM(9),

EXIT_FLAG(1),

Choose a reason for hiding this comment

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

물론 저는 enum을 사용하지 않고(어떻게 사용할 지 감이 안잡혔었습니다) 구현을 했는데, 만약 ENUM을 사용하고 변수명을 지정한다면 COMMAND나 다른 분이 말씀해주신 VALUE 또는 INPUT? 과 같은 변수명이 어울릴 거라 생각이 들어 의견 남겨봅니다:)


public String getMessage() {
return message;
}

Choose a reason for hiding this comment

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

와 예외처리가 이렇게나 깔끔하다니... 저도 배워갑니다!!

@h-beeen
Copy link
Author

h-beeen commented Oct 27, 2023

다른 곳에서 찾아보며 따로따로 본 개념들(enum 클래스, 개선된 switch문, Stream API 등)이 한 곳에 모여있는 것을 보니 하나의 작품을 보는 것만 같았습니다. 실례가 되지 않으신다면 가끔 찾아와 교과서같이 활용하고 싶습니다.

@wsk221e 아직까지 부족하고 부끄러운 코드이지만, 좋게 봐주시고 본인의 학습에 도움이 되신다면 얼마든지 활용하셔도 무방합니다! 추가적으로 질문 있으시면 디스코드 DM [BE]변해빈 이나, [email protected] 으로 연락 주시면 성심껏 답변 드리겠습니다!

좋게 봐주셔서 너무 감사합니다 :)

Copy link

@kkonii kkonii left a comment

Choose a reason for hiding this comment

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

리뷰보다는 제가 배워가는 것이 더 컸던 거 같아요..!
제가 고민했던 것들에 대한 방향을 잡을 수 있었습니다🙏

코드를 읽으면서 궁금한 점들을 남겨봤어요!

Copy link

Choose a reason for hiding this comment

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

리드미를 이런 식으로 작성할 수도 있군요..!
개발의 전체적인 과정을 볼 수 있는 것 같아 좋아요👀

Copy link
Author

Choose a reason for hiding this comment

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

많은 노력을 기울였던 만큼, 좋은 리뷰를 받게되어 기분이 좋습니다 :)

Comment on lines +15 to +17
do {
Number computerNumber = Number.generateRandomNumbers();
play(computerNumber);
Copy link

Choose a reason for hiding this comment

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

저는 computerNumberGame 클래스에서 미리 private 객체로 생성한 후 이후 따로 값을 할당하는 로직을 실행했는데
이 경우 NPE가 발생한다는 피드백을 받았었습니다!
해빈 님도 이 경우를 고려하셔서 이렇게 짜신 걸까요?

Copy link
Author

Choose a reason for hiding this comment

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

Game은 컨트롤러 클래스로 비즈니스 로직을 갖지 않아야 된다는 기조로 설계했습니다.
지역변수를 갖고, 재할당하면서 while문을 돌게되면, 이는 컨트롤러 자체가 비즈니스 로직을 갖는 설계로 이어지지 않았을까 싶습니다!
그 경우까지 고려하지는 않았지만, NPE에서는 자유로운 설계로 작성된 것 같습니다!

}

// Player Number Static Factory Method
public static Number inputPlayerNumbers() {
Copy link

Choose a reason for hiding this comment

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

integrateValidation() 와 같이 명명하게 된다면, 한 가지 고민해 볼 사항이 있을 것 같아요! 만약 숫자 외에 다른 예외처리가 존재한다면? 협업 과정이라고 가정했을 때, 다양한 케이스에 적절한 예외케이스를 조립하는데 어려움을 겪을 것 같아요.

저도 @sami355-24 님과 같은 점이 궁금했는데 답변 감사합니다👍 예외 케이스에 대한 로직을 모두 보여주는 것도 코드를 이해하기에 더 쉬울 것 같다는 생각이 드네요

Copy link

Choose a reason for hiding this comment

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

저도 일급컬렉션을 사용해서 numbers를 처리하고 싶었는데
ball과 strike를 계산하는 로직이 떠오르지 않아서 실패했어요..!
해빈 님 코드를 보고 많은 것을 배워갑니다🙏

Copy link
Author

Choose a reason for hiding this comment

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

좋게 봐주셔서 너무 감사합니다!
이번에 작성중인 레이싱카도 일급 컬렉션을 극한으로 활용하기 위해 노력중입니다!
다음 코드도 @kkonii 지켜봐주신다면, 많은 도움이 될 것 같아요!
정성스러운 리뷰 감사드립니다!!

Comment on lines +6 to +9
import static baseball.domain.constants.ResultType.*;
import static baseball.global.GameConfig.NUMBER_LENGTH;
import static baseball.global.exception.ErrorMessage.SYSTEM_ERROR;
import static java.lang.String.format;
Copy link

Choose a reason for hiding this comment

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

저도 import static을 사용했다면 코드가 조금은 더 깔끔해지지 않았을까란 생각이 드네요
배워갑니다...!

Copy link
Author

Choose a reason for hiding this comment

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

import static에 대해서 좋게 평가해주셨지만, 이 부분을 무분별하게 남발하는 것은 오히려 독이 될 것 같아요!
개발자가 명백한 기준을 정하고, 명백하게 줄여도 되는 부분에서 사용할 때 빛을 내는 기능인 것 같아요!!

Comment on lines +4 to +9
NUMBER_LENGTH(3),
RANDOM_NUMBER_MINIMUM(1),
RANDOM_NUMBER_MAXIMUM(9),

EXIT_FLAG(1),
RESTART_FLAG(2);
Copy link

@kkonii kkonii Oct 27, 2023

Choose a reason for hiding this comment

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

GameConfig는 Integer값의 상수만 관리하는 용도로 만드신 건가요?
궁금해서 여쭤봅니다..!

더불어 global 패키지의 용도도 궁금합니다

Copy link
Author

Choose a reason for hiding this comment

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

NUMBER_LENGTH와 같이, 전역(Global Layer)에서 작용하는 Configuration Setting을 담은 Enum 클래스입니다!
특히나 예외처리도 지금은 전역에서 BaseballException으로 처리하기 때문에 Global 패키지에 담게 되었습니다!

Copy link

Choose a reason for hiding this comment

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

BaseballException 클래스를 따로 만드신 것도 생성자를 내부에서만 생성하려는 목적으로 하신 건지 궁금합니다..!

Copy link
Author

Choose a reason for hiding this comment

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

음.. 일단 new 연산자로 throw하지 않기 위해서의 목적도 있었구요!

  • 추후 Console.close() 명령어를 예외처리에 추가해서, 예외를 던지기 전에, Scanner를 close할 수 있도록 리팩토링 해볼 예정이구요!
  • 예외처리간 커스텀 + 전역에서 단일 예외를 처리할 수 있도록 설계했습니다.

GAME_START("숫자 야구 게임을 시작합니다.\n"),
ASK_PLAYER_NUMBER("숫자를 입력해주세요 : "),
ASK_RESTART_OR_EXIT("게임을 새로 시작하려면 1, 종료하려면 2를 입력하세요.\n"),
GAME_OVER(format("%d개의 숫자를 모두 맞히셨습니다! 게임 종료%n", NUMBER_LENGTH.getValue()));
Copy link

Choose a reason for hiding this comment

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

저는 @oxdjww 님의 의견과 달리
미션에만 국한되지 않은 코드라고 생각되어 확장을 고려한 설계의 필요성을 느꼈습니다..!

Comment on lines +33 to +37
public static void validateNumberLength(final String number) {
if (!isValidLength(number)) {
throw BaseballException.of(INVALID_LENGTH);
}
}

Choose a reason for hiding this comment

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

단순 검증만을 담당하는 메소드 <-> 검증 결과에 따라 예외 처리

역할에 따라 리턴 타입과 네이밍을 차별화한 것이 사소하지만 가독성에 도움이 되는 부분인 것 같습니다!

public static BaseballException of(ErrorMessage errorMessage) {
return new BaseballException(errorMessage);
}
}

Choose a reason for hiding this comment

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

저도 처음엔 try-catch 문을 사용헸다가 예외가 발생되지 않는 걸 확인하고 throw문을 사용하게 되었는데, 왜 그런지는 생각 안 해보고 그냥 테스트 통과에 좋아했던 것 같네요...!

평소엔 생각치도 못 했는데 예외 처리에 대해 좀 더 깊게 생각해볼 수 있었던 것 같습니다! 좋은 말씀 감사드려요!

@@ -0,0 +1,25 @@
package baseball.global;

public enum GameConfig {

Choose a reason for hiding this comment

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

클래스 이름에 Config라는 접미사를 붙인 것에 대해 리뷰 드립니다!
처음에 파일을 보고 설정 파일인 줄 알아서, 설정 파일 이외의 클래스에는 해당 단어를 지양하는 것이 좋을 것 같다는 의견 드립니다 : )

Copy link
Author

Choose a reason for hiding this comment

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

맞아요! Configuration (게임 설정) 정도의 의미로 지었는데, 그렇게 생각이 들 만 하네요! 특히 Spring에서 Enum Config는 어노테이션과 함께 그런 구조로 많이 설계한 것 같아요! 조금 더 직관적인 네이밍을 고민해보겠습니다!

예컨데, GameOption 정도의 네이밍도 직관적일 것 같아요!
좋은 리뷰 감사드립니다 :)

Copy link

@Mingyum-Kim Mingyum-Kim left a comment

Choose a reason for hiding this comment

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

안녕하세요! 리뷰 창에 딜레이가 있을 정도로 많은 리뷰와 그에 대한 친절하고 통찰력 있는 답변들에 깊은 반성을 하고 갑니다.
앞으로의 행보가 기대되는 분입니다. 2주차도 화이팅하시기 바랍니다. 😊

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.