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

2주차 미션 다시 구현 #11

Open
wants to merge 25 commits into
base: oliviarla
Choose a base branch
from

Conversation

oliviarla
Copy link
Member

숫자야구게임 다시 구현

  • 객체지향적으로 구조 수정해서 설계 완료
  • 테스트 주도 개발 진행
  • 클래스에서 값을 가져오는 것이 아닌 is 구문으로 물어보는 방식 적용

@oliviarla oliviarla changed the title Oliviarla feedback 2주차 미션 다시 구현 May 27, 2022
Copy link

@this-is-spear this-is-spear left a comment

Choose a reason for hiding this comment

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

전체적인 설계가 너무 깔끔하게 다듬어져서 놀랐습니다! 그 외 수정이 필요한 부분은 코멘틀를 남겼습니다.
수정 하더라도 Merge는 진행하지 말아주세요!
미션 진행하느라 수고 많으셨습니다.

Comment on lines 17 to 29
while(true){
Baseball randomBaseball = new Baseball(randomNumGenerator.makeNum());
BaseballStatus baseballStatus = new BaseballStatus();

while(!outputView.exitGame(baseballStatus)){
Baseball userBaseball = inputView.inputBall();
baseballStatus = baseballService.compare(userBaseball, randomBaseball);
outputView.printBaseballStatus(baseballStatus);
}
if(!inputView.resumeGame()){
break;
}
}

Choose a reason for hiding this comment

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

이런 방법은 어떨까요? ☺️

Suggested change
while(true){
Baseball randomBaseball = new Baseball(randomNumGenerator.makeNum());
BaseballStatus baseballStatus = new BaseballStatus();
while(!outputView.exitGame(baseballStatus)){
Baseball userBaseball = inputView.inputBall();
baseballStatus = baseballService.compare(userBaseball, randomBaseball);
outputView.printBaseballStatus(baseballStatus);
}
if(!inputView.resumeGame()){
break;
}
}
do{
Baseball randomBaseball = new Baseball(randomNumGenerator.makeNum());
BaseballStatus baseballStatus = new BaseballStatus();
while(!outputView.exitGame(baseballStatus)){
Baseball userBaseball = inputView.inputBall();
baseballStatus = baseballService.compare(userBaseball, randomBaseball);
outputView.printBaseballStatus(baseballStatus);
}
}while(inputView.resumeGame())

}

public BaseballStatus compare(Baseball userBall, Baseball randomBall) {
int strike = 0, ball = 0;

Choose a reason for hiding this comment

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

하나의 선언문에는 하나의 변수만 생성하는 것이 좋습니다. 아래 코딩 컨벤션 을 참고하시면 좋을 것 같아요!
스크린샷 2022-05-28 오후 12 28 29

Suggested change
int strike = 0, ball = 0;
int strike = 0;
int ball = 0;

}

public boolean isBall(int userNum, int userIdx, Baseball randomBall) {
if (!isStrike(userNum, randomBall.getBaseballs().get(userIdx))) {

Choose a reason for hiding this comment

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

isStrike 조건문을 지나 isBall 메서드가 호출이 될 때, isStrike를 재호출할 필요가 있을까 생각이 듭니다.
중복으로 확인하는 방법 말고 한 번에 전부 확인하는 방법이 있으면 좋을 것 같아요!

Scanner scanner = new Scanner(System.in);
System.out.print("숫자를 입력해 주세요 : ");
String input = scanner.next();
List<Integer> list = Arrays.stream(input.split("")).mapToInt(Integer::parseInt).boxed().collect(Collectors.toList());

Choose a reason for hiding this comment

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

입력되는 숫자를 중복확인 하는 방법이 있을까요? ☺️

public class OutputView {
public String outputBaseballStatus(BaseballStatus baseballStatus) throws Exception {
String result = "";
if (!baseballStatus.existsBall() && !baseballStatus.existsStrike() && !baseballStatus.nothing())

Choose a reason for hiding this comment

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

외부에서 유효성 검사를 진행해도 좋지만, baseballStatus를 생성할 때, 유효성 검사를 진행할 수 있으리라 생각이 들어요!

Random random = new Random();
Set<Integer> set = new HashSet<>();

while(set.size()<3){

Choose a reason for hiding this comment

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

3이라는 숫자를 상수로 변환하면 코드의 가독성도 좋아지고 유지보수에도 좋아보입니다. ☺️

Set<Integer> set = new HashSet<>();

while(set.size()<3){
int num = random.nextInt(9)+1;

Choose a reason for hiding this comment

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

9도 마찬가지로 의미있는 숫자이니 상수로 만들어도 좋을 것 같아요!

Suggested change
int num = random.nextInt(9)+1;
int num = random.nextInt(9)+1;

baseballStatus.setBall(0);
baseballStatus.setStrike(3);
while(true){
if(outputView.exitGame(baseballStatus)) break;

Choose a reason for hiding this comment

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

중괄호가 필요해 보여요!
스크린샷 2022-05-28 오후 12 42 45

BaseballStatus baseballStatus = new BaseballStatus();
baseballStatus.setBall(0);
baseballStatus.setStrike(3);
while(true){

Choose a reason for hiding this comment

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

이렇게 테스트를 진행하는 방법이 아닌 Assertions 라이브러리를 사용해서 테스트를 진행하는건 어떨까요? ☺️

import java.util.List;

public class Baseball {
List<Integer> baseballs;

Choose a reason for hiding this comment

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

그리고 Integer 객체를 담는 리스트 보다 새로운 객체를 만들어 숫자 야구 게임에서의 숫자를 더 의미있게 사용해보는건 어떨까요? ☺️

Copy link

@this-is-spear this-is-spear left a comment

Choose a reason for hiding this comment

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

수정하시느라 수고 많으셨습니다. ☺️
추가적으로 수정해야할 부분을 코멘트로 남겼으니 나중에 확인해주세요!

public List<Integer> getBaseballs() {
return baseballs;
public List<Ball> getBaseballs() {
return Collections.unmodifiableList(baseballs);
Copy link

@this-is-spear this-is-spear Jun 4, 2022

Choose a reason for hiding this comment

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

리스트를 조회할 때, unmodifiableList 메서드를 사용해 수정할 수 없도록 구현하셨군요!
좋은 방법입니다. 👍
하지만 unmodifiableList 메서드만 사용해서 리스트를 호출하게 되면 정말 수정이 불가능할까요? ☺️
이 부분에 대해서는 수정이 필요하진 않지만, 생각해보는걸 추천드려요! 👍

@@ -0,0 +1,13 @@
package baseball.domain;

public class Ball {

Choose a reason for hiding this comment

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

Ball 객체를 구현하셨군요!! 👍
해당 Ball 객체를 구현하면서 들어가게 되는 숫자의 유효성 검사를 해당 Ball에서 구현하는건 어떨까요?
즉, InputView에서 Balseball에 들어가는 숫자들의 유효성 검사를 Ball 구현체에서 진행하게 된다면 InputView와 그 외 객체들의 결합력을 낮추고 Ball이라는 구현체의 응집도를 높일 수 있을 것 같아요!

@@ -1,15 +1,28 @@
package baseball.domain;

import java.util.ArrayList;
import java.util.Collections;
import java.util.List;

public class Baseball {

Choose a reason for hiding this comment

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

InputView에서 Balseball의 리스르 크기 검사를 Baseball 구현체에서 진행하는건 어떨까요? ☺️


public void isDistinct(){

Choose a reason for hiding this comment

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

해당 메서드의 depth가 4칸 정도 되는 것 같아요! depth를 낮추는 방법이 있을까요? ☺️

@@ -0,0 +1,13 @@
package baseball.domain;

public class Ball {

Choose a reason for hiding this comment

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

추가적으로 Ball의 생애주기와 Baseball의 생애주기가 같은지에 대해서도 고민해보시는 걸 추천 드립니다! 힌트를 드리자면, 엔티티와 값 객체의 차이가 되겠군요! 👍

public BaseballStatus(int ball, int strike) throws Exception {
this.ball = ball;
this.strike = strike;
this.isValid();

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 +9
static int MAX_SIZE = 3;
static int MAX_NUM = 9;

Choose a reason for hiding this comment

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

static을 사용해서 변수를 공유할 수 있게 됐군요!
하지만 값이 변경될 수 있기 때문에 이슈가 발생할 수 있는 코드입니다. 😱
final 변수를 생성할 때에는 static은 선택적이지만, static일 때에는 final은 필수일 것 같아요! ☺️

그리고 추가적으로 접근 제어자를 선언하지 않는다면 기본적으로 설정되는 접근 제어자가 무엇일지 고민해봐도 좋을 것 같습니다. ☺️ 물론 생략은 좋지 않아요 👍

static int MAX_SIZE = 3;

private void isValidInput(List<Ball> ballList) throws RuntimeException{
if(ballList.size()!=MAX_SIZE){
Copy link

@this-is-spear this-is-spear Jun 4, 2022

Choose a reason for hiding this comment

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

줄 정렬이 필요해보이는 코드이군요 ☺️
그 외적인 코드들도 줄 정렬이 필요한 부분들이 많았습니다. 프로젝트를 전체 줄 정렬을 해주는 기능을 인텔리제이에서 제공해주고 있으니 한 번 사용해보시는 걸 추천드려요!

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