-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
base: main
Are you sure you want to change the base?
[숫자 야구 게임] 변해빈 미션 제출합니다. #1613
Changes from all commits
5e5ff7e
285df98
87fa078
403e53a
cd65c41
29a7d86
d5e2f95
d01607d
45e0473
1447d6f
02ff445
3cd75ee
f65861b
641d1d3
dbcdc54
2bd8d97
8306983
f837542
daad19e
e16b264
058853e
c672022
dd8cba7
990b7c7
f5fa1bc
3e84307
6a46e8c
04d9229
5e9b6c2
8962557
b84f780
7acc36b
ba6bd2c
b1dd4e1
fa90c35
3f7bb2e
7f7d7e8
ac8b5bd
4a03375
febd594
1999b2c
99aa219
22f1aef
575e4a5
a1780f0
26561ff
7ac0f58
473e3cd
ab42278
0755554
c56887c
a1307d6
9de442b
fecb933
2b505a0
f9a297c
5fdb512
3fc9cd2
caa0bd3
1fa1b0b
0422d7b
a51764b
5d9eaf2
e33b8a5
6ae2ca1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. 많은 노력을 기울였던 만큼, 좋은 리뷰를 받게되어 기분이 좋습니다 :) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,209 @@ | ||
# ⚾ Precourse-Week1 Mission **[숫자 야구]** | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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 commentThe 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 commentThe 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 commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. README.md 를 이렇게 깔끔하고 예쁘게 작성할 수 있다니.. 너무 놀라워요.. 한 수 배워갑니다. 저는 너무 간결하게만 작성한 것 같아 반성하게 되네요.. ! 😅 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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 commentThe 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 commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. 예쁘게 설계한 만큼 좋은 평가를 받아 너무 뿌듯합니다 :) |
||
## 📦 패키지 구조 | ||
|
||
<div align="center"> | ||
<table> | ||
<tr> | ||
<th align="center">Package</th> | ||
<th align="center">Class</th> | ||
<th align="center">Description</th> | ||
</tr> | ||
<tr> | ||
<td><b>🕹 controller</b></td> | ||
<td><b>Game</b></td> | ||
<td>게임 로직을 메인으로 동작하는 컨트롤러 클래스</td> | ||
</tr> | ||
<tr><td colspan="3"></td></tr> | ||
<tr> | ||
<td rowspan="2"><b>💻 domain</b></td> | ||
<td><b>Number</b></td> | ||
<td>사용자에게 입력받는 숫자와, 컴퓨터가 생성하는 숫자 클래스 | ||
</td> | ||
</tr> | ||
<tr> | ||
<td><b>Result</b></td> | ||
<td>Ball Count와 Strike Count에 대한 결과 클래스</td> | ||
</tr> | ||
<tr> | ||
<td><b> ↘️ / constants</b></td> | ||
<td><b>ResultType</b></td> | ||
<td>각 결과에 따라 다른 출력 방법에 대해 정의된 Enum</td> | ||
</tr> | ||
<tr><td colspan="3"></td></tr> | ||
<tr> | ||
<td><b>📃 global</b></td> | ||
<td><b>GameConfig</b></td> | ||
<td>전역으로 작용하는 설정과 제약조건에 대한 Enum</td> | ||
</tr> | ||
<tr> | ||
<td rowspan="2"><b> ↘️ / exception</b></td> | ||
<td><b>BaseballException</b></td> | ||
<td>전역으로 처리하는 예외상황에 대한 Exception 클래스<br/></td> | ||
</tr> | ||
<tr> | ||
<td><b>ErrorMessage</b></td> | ||
<td>각 예외 상황에서 전역으로 던져질 예외의 메세지 Enum</td> | ||
</tr> | ||
<tr><td colspan="3"></td></tr> | ||
<tr> | ||
<td><b>✅ validator</b></td> | ||
<td><b>InputValidator</b></td> | ||
<td>사용자가 입력하는 숫자에 대한 제약조건 클래스</td> | ||
</tr> | ||
<tr><td colspan="3"></td></tr> | ||
<tr> | ||
<td rowspan="2"><b>💬 view</b></td> | ||
<td><b>InputView</b></td> | ||
<td>사용자 요청을 처리하는 클래스</td> | ||
</tr> | ||
<tr> | ||
<td><b>OutputView</b></td> | ||
<td>사용자에게 응답을 출력하는 클래스</td> | ||
</tr> | ||
<tr> | ||
<td><b> ↘️ / constants</b></td> | ||
<td><b>StaticNotice</b></td> | ||
<td>사용자에게 응답할 정적 메세지를 담은 열거형 클래스</td> | ||
</tr> | ||
<tr><td colspan="3"></td></tr> | ||
<tr> | ||
<td colspan="3" align="center"><b>Package Structure Overview</b></td> | ||
</tr> | ||
<tr> | ||
<td colspan="3"><img src="https://github.com/woowacourse-precourse/java-baseball-6/assets/112257466/f37d479a-d211-4c79-93cf-c0a4be1a7443" width="99%"></td> | ||
</tr> | ||
|
||
</table> | ||
</div> | ||
|
||
--- | ||
|
||
## ✨ 기능 구현 목록 | ||
|
||
✅ `a ~ b` 사이의 서로 값이 다른 `n자리`의 정수를 랜덤으로 생성한다. | ||
|
||
▪️ Default Setting : `1 ~ 9`사이의 서로 값이 다른 `3자리`의 정수 | ||
|
||
✅ 게임 시작 문구 출력<br/> | ||
✅ 사용자에게 `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가 아닌 숫자일 경우 예외처리 | ||
▪️ 입력값에 따라 게임을 재시작하거나 종료한다. | ||
|
||
Comment on lines
+90
to
+123
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 기능 명세서가 자세히 되어 있는건 좋은 것 같습니다. 코드랑 비교하면서 보니 mvc 패턴으로 작성하셨던데, 어떤 객체가 위 기능들을 할당하는지도 같이 명시되어 있으면 좋을것 같아요! 예를 들어 계산된 사용자 input 숫자와 랜덤 생성 정수의 자리수를 비교해 출력할 힌트는 Number domain에서 구현하셨던데 이것을 표시할 수 있다면 좋을 것 같습니다. 기능명세서가 구현 전 어떤 기능이 필요할지 생각하고 구현에 도움을 주는 문서인 만큼 최대한 구현과 가깝게 작성하시면 좋을 것 같아요. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 이 부분은 다음 과제에서 꼭 반영하도록 하겠습니다! 특히나 도메인별로 분류해서 적었으면 더 좋지 않았을까..하는 아쉬움이 드네요! 좋은 리뷰 감사합니다 :) |
||
-------------------------------------------------------- | ||
|
||
## 🎨 구현 간 고민했던 내용들 | ||
|
||
### 1️⃣ 확장에는 열려있고, 변경에는 닫혀있는 OCP 설계 | ||
|
||
- input 숫자의 범위가 변하더라도(1~9), 자리수가 변하더라도 대응이 손쉽게 가능해야 한다.</br> | ||
`GameConfig` 파일에서 `NUMBER_LENGTH` 변수의 value를 변경해 손쉽게 변경이 가능하다.<br/> | ||
개발 요구사항에서 자릿수 요청까지 처리하는 문제였다면, 더욱 OCP를 준수하는 코드 작성이 가능했을 것 같다. | ||
|
||
<div align="center"> | ||
|
||
<table> | ||
<tr> | ||
<th align="center">숫자 3자리</th> | ||
<th align="center">숫자 4자리</th> | ||
<th align="center">숫자 5자리</th> | ||
</tr> | ||
<tr> | ||
<td align="center"><img src="https://github.com/woowacourse-precourse/java-baseball-6/assets/112257466/0b5b10f6-357f-4274-9b42-ea68d18edf85" height="50%"/></td> | ||
<td align="center"><img src="https://github.com/woowacourse-precourse/java-baseball-6/assets/112257466/33d19627-775a-4d56-b2c1-88c186a95336" height="50%"/></td> | ||
<td align="center"><img src="https://github.com/woowacourse-precourse/java-baseball-6/assets/112257466/e41b3fc0-2d6f-4f6c-abe4-f765abcb7aad" height="50%"/></td> | ||
</tr> | ||
</table> | ||
|
||
|
||
-------------------------------------------------------- | ||
|
||
### 2️⃣ 4번의 대규모 리팩토링, 그리고 얻어낸 값진 `Number` | ||
|
||
- 영감을 얻게 해줬던 한 마디 | ||
```bash | ||
객체는 '자율적인 존재'라는 점을 명심하라. | ||
< 중략 > | ||
객체는 스스로의 행동에 의해서만 상태가 변경되는 것을 보장함으로써 객체의 자율성을 보장한다. | ||
|
||
- 객체지향의 사실과 오해 中 | ||
``` | ||
|
||
- First-class collection + Static Factory Method 활용 | ||
```java | ||
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의 생성자는 서로 다른 파라미터를 지니기 때문에, 개발자가 사용 간 혼동 가능<br/> | ||
해당 문제를 해결하기 위해, 생성자를 `private`으로 제한하고, 의미있는 메소드로만 생성자를 호출하도록 설계 | ||
|
||
```java | ||
// 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); | ||
} | ||
``` | ||
- 정적 팩토리 메소드 명에 의미를 부여하고, 개발자가 직관적으로 해석할 수 있도록 했고,<br/> | ||
일급 컬렉션을 활용해 검증이 끝난 유효한 값만 리스트에 담을 수 있게 되었다! | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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 commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. 껄껄 과찬이십니다 :) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,7 +1,10 @@ | ||
package baseball; | ||
|
||
import baseball.controller.Game; | ||
|
||
public class Application { | ||
public static void main(String[] args) { | ||
// TODO: 프로그램 구현 | ||
Game game = new Game(); | ||
game.start(); | ||
Comment on lines
+7
to
+8
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 입력에 사용한 scanner를 close하는 코드가 없습니다! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 앗..Scanner를 사용한다는 점을 망각하고 사용했네요! |
||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,36 @@ | ||
package baseball.controller; | ||
|
||
import baseball.domain.Number; | ||
import baseball.domain.Result; | ||
import baseball.validator.InputValidator; | ||
import baseball.view.InputView; | ||
import baseball.view.OutputView; | ||
|
||
import static baseball.view.OutputView.printStaticNotice; | ||
import static baseball.view.constants.StaticNotice.GAME_START; | ||
|
||
public class Game { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. MVC모델을 잘 활용하신 것 같아 배울 점이 있다고 생각합니다. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. GameController와 Game에 대해서 고민을 많이 했던 것 같습니다! 제가 나타내고자 했던 의도가 분명히 전달된 것 같아 뿌듯합니다 :) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
저도 이부분 많이 고민했었는데 '흐름을 관리하는 역할을 하는 클래스' 를 생각했을 때 Controller 가 직관적으로 떠올라 클래스 이름에 Controller를 포함하여 이름 지었습니다. 이름에 나타내지 않아도 충분히 의미를 전달할 수 있을까? 가 고민이었는데 위에 남겨주신 리뷰가 그 답이 되었습니다! |
||
public void start() { | ||
printStaticNotice(GAME_START); | ||
do { | ||
Number computerNumber = Number.generateRandomNumbers(); | ||
play(computerNumber); | ||
Comment on lines
+15
to
+17
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. Game은 컨트롤러 클래스로 비즈니스 로직을 갖지 않아야 된다는 기조로 설계했습니다. |
||
} while (!askRestartOrExit()); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. 다른 분께서도 언급하신 내용이라서 반복되는 내용인 것 같지만,, 코드 컨벤션 중에서 긍정 조건을 사용하는 것을 권고하는 것으로 저도 알고 있습니다:) 추가로 while()의 인자로 네이밍 측면에서 askRestart 보다는 isRestart를 사용하는 것으로 저는 생각해봤어요! 해빈님의 의견이 궁금합니다 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. true 도 상수화를 통해 true의 의미를 더 잘 나타내면 좋을 것 같습니다. 저 문장만 봤을때는 whie 블록이 어떤 코드인지 알기가 어렵네요. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 이 부분도 많은 분들이 리뷰주셨던 내용인 것 같습니다! 이 부분은 수정해서 더 좋은 코드로 로컬에 저장하고 다시 찾아뵙도록 하겠습니다!! |
||
|
||
private void play(Number computerNumber) { | ||
while (true) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 아래 분기의 result.checkGameOver() 를 잘 활용하여서 while(true) 자리의 조건 자리에 넣으시면 더 좋은 코드가 될 수 있을 거 같아요! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. result라는 객체가 생성되지 않은 시점에서 조건문으로 분기할수 없기에 부득이 이렇게 설계하게 되었습니다 ㅠㅠ 이 부분을 어떻게 더 매끄럽게 고칠 수 있을지 연구해보겠습니다!! |
||
Number playerNumber = Number.inputPlayerNumbers(); | ||
Result result = Result.create(playerNumber, computerNumber); | ||
OutputView.printMessage(result.createResultMessage()); | ||
if (result.checkGameOver()) { | ||
break; | ||
Comment on lines
+24
to
+27
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P4) MVC모델에 무지하고 대학생 지식이 다여서 도움이 될지는 잘 모르겠습니다. MVC모델의 개략적인 구조를 보았을때 입력을 받게되는 V => M 에서 그렇다면 출력은 반대순서인 내부에서 바깥으로 M => V 일 것인데 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. MVC 모델의 연관관계에서 호출 위치에 대한 리뷰를 남겨주신 것으로 이해했습니다! OutputView는 출력할 책임만 가진다! 라는 원칙에 입각해서, 설계했던 내용인 것 같아요. 그런 의미로 OutputView의 printMessage를 Static Method로 정의해 해당 그림에서 UserAction에 해당하는 부분은 Input View -> Controller 이런 방향으로 설계했는데, 역할과 책임의 분리는 적절했다고 생각하지만, 호출 방향은 조금 더 고차원적인 설계의 문제네요! MVC 패턴을 조금 더 깊이 공부해야 할 것 같아요! 저도 MVC는 문외한인 부분이라서....T_T 좋은 질문 감사드립니다 :) |
||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. 위에서 줄줄이 읽어도 딱 이해가는 코드로 만들고자 했던 제 노력을 이해해주셔서 너무 감사드립니다:) |
||
} | ||
} | ||
|
||
private boolean askRestartOrExit() { | ||
String input = InputView.askRestartOrExit(); | ||
return InputValidator.isValidRestartFlag(input); | ||
} | ||
} |
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 저도 일급컬렉션을 사용해서 numbers를 처리하고 싶었는데 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 좋게 봐주셔서 너무 감사합니다! |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,80 @@ | ||
package baseball.domain; | ||
|
||
import baseball.view.InputView; | ||
|
||
import java.util.ArrayList; | ||
import java.util.List; | ||
import java.util.stream.IntStream; | ||
|
||
import static baseball.global.GameConfig.*; | ||
import static baseball.validator.InputValidator.*; | ||
import static camp.nextstep.edu.missionutils.Randoms.pickNumberInRange; | ||
|
||
public class Number { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Number라는 이름은 너무 일반적이고 포괄적인 것 같습니다. 처음에 보고 int나 long, double에 대한 연산을 담은 객체인가 싶었네요. 포함하고 있는 연산들을 보면 숫자야구에서만 사용될 수 있는 것이 대부분이므로 좀 더 이름을 구체적으로 짓는 것이 어떨까요? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. Number라는 클래스명은 이미 추상 클래스로 정의되어 있어서 웬만해선 피하시는게 좋다는 생각이 들어요!! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
이 부분은 지금 제 설계에서 어떤 말씀인지 제대로 이해하지 못했습니다!
이 부분을 Number 추상화를 통해 해결할 수 있을 것 같아요! 공통 책임(숫자를 받아서 검증하고 객체화)을 추상화하고, 나머지를 구현체에서 정의하면 더욱 잘 분리된 설계가 되지 않을까 싶습니다!! 감사합니다 :) |
||
private final List<Integer> numbers; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 저도 일급 컬렉션을 사용했는데요, 생성자를 private으로 설정한게 인상적이었습니다! There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. List를 사용하셨는데, Integer를 포장해서 List 이런 타입으로 받는것이 더 좋았을까? 싶습니다. 포장하게 되면 1~9 사이의 숫자에 대한 검증을 해당 클래스에서 처리할 수 있기도 하지만, 어느 것이 좋을지는 저도 확신이 들지 않네요! 설계하기 나름이라고 생각합니다! There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. @kunsanglee 이렇게 생각할 수도 있군요! 정확히 말씀주신 의도가 Integer 자체를 래핑한 엔티티 자체를 리스트에 넣는 것도 좋을 것 같다는 의견으로 이해했습니다! 저도 설계하기 나름이라고 생각하지만, 검증을 분리하는 것 또한 좋은 설계가 될 수 있을 것 같습니다! 좋은 리뷰 감사합니다 :) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @junest66 하지만 저는 여기서 다시 분기를 고민하고 있습니다! |
||
|
||
// Player Number Constructor | ||
private Number(String input) { | ||
validateEmpty(input); | ||
validateNumberLength(input); | ||
validateContainOnlyNumber(input); | ||
validateContainDuplicatedNumber(input); | ||
|
||
this.numbers = convertInputNumber(input); | ||
} | ||
|
||
// Computer Number Constructor | ||
private Number(List<Integer> computerNumber) { | ||
this.numbers = computerNumber; | ||
} | ||
|
||
// Player Number Static Factory Method | ||
public static Number inputPlayerNumbers() { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more.
가령 Spring 웹 환경에서는 HttpRequest, HttpResponse를 개행으로 처리하지 않고 실제 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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 commentThe 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 commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more.
적정 수준에서 Intergrate 하는 방법은 좋은 방법이라고 생각이 됩니다! 다만 예외처리 가짓수가 늘었을 때의 경우는 명명을 더 직관적으로 설정할 필요가 있을 것 같다는 개인적인 의견입니다! 좋은 질문 감사드립니다 :) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
저도 @sami355-24 님과 같은 점이 궁금했는데 답변 감사합니다👍 예외 케이스에 대한 로직을 모두 보여주는 것도 코드를 이해하기에 더 쉬울 것 같다는 생각이 드네요 |
||
String playerNumbers = InputView.askPlayerNumbers(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. 이런 부분에 대해서 리뷰를 받고 싶었습니다 ㅠ 다른 레이어보다 View 레이어의 입력 책임이 어디까지인가에 대한 부분이 제일 헷갈렸어요. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 저도 @oxdjww 님의 의견에 공감합니다! There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. view에서 입력을 받고 그 정보를 컨트롤러로 넘겨주면 어떨까요? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. controller로 뺴는게 맞다고 생각합니다. 코드를 읽어보니 결과를 출력하는 부분은 outputView를 통해서 출력하셨는데 입력은 Number model에서 받고 있어서 view의 역할이 불분명 한 것 같아요. Controller 코드를 읽을 때도 outputView를 통해 결과를 출력하긴 하는데 inputView를 통해 입력을 받는 부분은 없어서 의아했습니다. 입력 메시지를 출력해주고 사용자의 입력을 받는 부분은 view에서 처리하는게 좋을 것 같습니다. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. InputView 객체의 책임과, 호출 위치에 대해서 가려운 부분을 정확히 긁어주신 것 같아요! 좋은 리뷰 감사합니다 :) |
||
return new Number(playerNumbers); | ||
} | ||
|
||
// Computer Number Static Factory Method | ||
public static Number generateRandomNumbers() { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. RandomNumbers 생성 역할을 별개의 클래스로 분리하면 좋을 것 같습니다. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Number를 ComputerNumber와 PlayerNumber로 초반에는 분리해서 설계했었어요! 현 코드에서 생성의 역할은 정적 팩토리 메소드로 구분할 수 있다고 판단하는데, There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Random 생성 클래스를 분리하게 되면 이후 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 공감합니다. 지금의 코드에서는 사용자의 입력 인스턴스에도 ball과 strike를 계산하는 메소드도 포함되어 있습니다. getter를 쓰지 않기 위해서 구현하셨지만 객체의 의미가 불명확해 진 것 같습니다. 숫자야구라는 과제를 알고 있는 저의 입장에서도 처음 Number라는 객체를 봤을때 어떤걸 의미하는 건지 알기 어려웠어요. 저도 이부분을 고민하고 있었는데 앞에 분이 추상클래스 라는 좋은 답을 주신 것 같습니다. 다형성을 활용하여 코드를 짜보면 좋을 것 같아요! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @zxcev Number 자체를 추상 클래스로 정의하는 부분은 정말 생각하지 못했던 것 같아요..! @Gyu-won 님께서 말씀해주신 내용도, Number라는 객체가 직관적인 네이밍이라기엔 너무 많은 역할과 책임(컴퓨터 숫자 생성, 검증, 유저 숫자 Input, 검증)을 갖고 있다는 의미로 이해했습니다. 두 분 께서 성실히 리뷰해주신 만큼, 로컬 레포지토리에서 꼭 작업해서 반영해보겠습니다!! 감사합니다 :-) |
||
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)) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 해당 부분은 여기서만 사용하는 것이라면 특정 메소드로 분리하지 않고 contains 를 바로 사용해도 되지 않을가요?? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. 역시 개발자에게 꼼꼼한 검토와 의심은 좋은 습관이라고 저도 확신합니다! 같이 좋은 설계에 대해서 고민해주셔서 감사합니다! |
||
randomNumbers.add(number); | ||
} | ||
} | ||
return new Number(randomNumbers); | ||
} | ||
|
||
private static boolean hasDuplicatedNumber(List<Integer> randomNumbers, int number) { | ||
return randomNumbers.contains(number); | ||
} | ||
|
||
private List<Integer> convertInputNumber(String input) { | ||
return input.chars() | ||
.mapToObj(Character::getNumericValue) | ||
.toList(); | ||
} | ||
|
||
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(); | ||
} | ||
Comment on lines
+59
to
+69
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. countBallCount와 countStrikeCount 이 두 부분을 stream을 써서 처리함으로써 일급 컬렉션의 불변이 지켜지는 것 같아 좋은 것 같습니다👍하지만 메서드 명 부분은 count가 중복으로 들어가서 좀 더 명확하면 좋지 않을까 생각해요! There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more.
다만 @noxknow 님께서 말씀 주셨듯
와 같은 이슈가 있는 코드라서
Comment on lines
+59
to
+69
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 오.. 같은 클래스끼리면 private 메소드에 접근이 가능하다는 것 처음 알았네요!!!! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 코드에 DDD를 두 방울 정도 떨어뜨린 느낌으로 설계했습니다. |
||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 질문이 있습니다. 저는 스트라이크를 세면 자연스럽게 볼을 셀 필요가 없어진다고 생각해서 스트라이크와 볼을 함께 세도록 코드를 작성했었는데, 해빈님은 그것을 countBallCount 메서드와 countStrikeCount 메서드로 나누어 두셨습니다. 이것에 대한 이유가 '메서드 하나에 기능을 하나만 넣기 위해서' 이외에 다른 이유가 있을까요? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 스트라이크를 세면 볼을 셀 필요가 없다라고 생각하는 부분이 어떤 말씀인지 잘 이해가 가지 않습니다. 지금 현재 생성자를 통해서 strikeCount와, ballCount를 계산해 Result 객체를 생성하는 로직이기에,
말씀 듣고 머릿속으로 재구상해보았을때, There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 저도 @U-Keun 님처럼 스트라이크를 판단하면 동시에 볼도 같이 판단할 수 있지 않을까라는 생각을 했습니다. 개인적으로 언급하신 "메서드의 세분화"에 더 초점을 둔 것 같아요! 스트라이크와 볼을 각각 구하고 결과값을 출력하는 과정에서 변환하는 방향으로 코드를 작성했던 것 같아요 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. 이 부분은 아직 확신이 잘 서지 않네요! @cosyflower 님, @kunsanglee 께서 말씀주신대로, 저는 메소드 단일책임에 조금 더 입각해서 설계했던 것 같고, 모두 좋은 의견을 들을 수 있는 기회였던 것 같습니다! |
||
// Ball : Contain their number at other position | ||
private boolean isBall(int number, int digit) { | ||
return !isStrike(number, digit) && numbers.contains(number); | ||
} | ||
|
||
// Strike : Contain their number at same digit | ||
private boolean isStrike(int number, int digit) { | ||
return number == numbers.get(digit); | ||
} | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Number 라는 도메인이라는 이름에 좀 많은 메소드가 들어 있어서 Number 의 역할이 2개 이상이라고 느낄거 같아요 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 맞습니다..! 사실 Number <-> Result 관계에 대해서 너무 고민을 많이 했던 것 같아요. 일급 컬렉션에 담은 값들을, 외부에서 값 타입으로 꺼내서 사용하는 순간 그 의미가 퇴색된다고 보았기 때문에 많은 분들이 리뷰해주셨던 내용 중 가장 기억에 남는 내용은, 좋은 리뷰 감사드립니다 :) There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. 저는 number, numbers 표기보다 구체적으로 네이밍을 하는게 로직을 파악하기 쉬울 것 같다는 생각이 들어요! |
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.
많은 분들이 리드미에 좋은 평가를 남겨주시는 것 같아요!
가시성 높은 리드미를 만들고자 노력했는데 좋은 평가를 남겨주셔서 감사합니다!
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.
저 같은 경우에는 리드미 파일을 추상적으로 작성했었는데
해빈님 리드미를 보니까 엄청 비교 되네요..!
2주차 미션에는 저도 본받아야겠어요!
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.
리드미를 이렇게나 자세하게 쓸 수 있군요... 많이 배웠어요:)