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

박순용 숫자야구 - 진행중 #5

Open
wants to merge 37 commits into
base: gzcxadfzc
Choose a base branch
from

Conversation

gzcxadfzc
Copy link

  • 중점적으로 생각한 것

기능을 가능한 쪼개서 분리해보려고 하였음
자바 처음 공부 중이라 기본 제공 클래스, 메서드 이해
intellij 설정 및 intellij를 이용하여 git 사용
코드 컨벤션과 커밋 컨벤션을 활용한 클린코드 지향하기

  • 궁금한 것

최대한 함수를 짧게 짧게 해봤는데 오히려 가독성이 떨어지지는 않는지

사용자 입력 값이 타당한지 확인할 때 좀 더 간단하게 하는 방법이 있는지
현재 userinput 클래스에서 입력값을 확인하는 함수를 너무 중구난방식으로 함수를 많이 만들어놓은 것 같다는 생각이 듭니다,,,

난수생성 시 중복값을 방지하기 위해 arraylist.contains()를 사용했는데 set<>을 사용하는게 더 좋을지

UnitTest는 조금 공부 후에 최대한 빠르게 해보겠습니다,,,,

난수생성 기능 추가
입력 기능 추가
볼/스트라이크 출력 기능 추가
입력 기능 추가
- 예외 발생시 다시 입력
userinput에서 중복값 입력 제한
Copy link
Contributor

@Byeongju-Kong Byeongju-Kong left a comment

Choose a reason for hiding this comment

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

중점적으로 생각한 부분은 잘하신 것 같아요!

궁금한 점에 대한 답변을 좀 해드리자면, 함수는 짧을 수록 좋습니다!

입력 값 검증에 대한 건, 다 같이 이야기 해보면 좋을 것 같아요! 혼자서 조금 더 고민해보시고, 다음주에 같이 이야기 나눠봐요~

Set 활용 잘하셨습니다!

Comment on lines 15 to 16
inputNumberList.add(null);
randomNumberList.add(null);
Copy link
Contributor

Choose a reason for hiding this comment

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

null을 add하는 행위가 꼭 필요하나요?

Copy link
Author

Choose a reason for hiding this comment

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

이게 list를 쓰면서 난수와 사용자 입력을 받을 때 list.set()을 사용해서 구현해놔서 일단 널값으로 초기화를 시켜놨습니다
더 나은 초기화 방법이 잇을까요?

Copy link
Contributor

Choose a reason for hiding this comment

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

add를 하는 방식으로 변경하는게 좋을 것 같아요~

Comment on lines 18 to 19
resetBalls();
resetStrikes();
Copy link
Contributor

Choose a reason for hiding this comment

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

int 값은 아무 것도 할당하지 않으면 기본적으로 0이 할당되는 것으로 알고 있는데, 한번 확인해보시면 좋을 것 같아요~

Copy link
Author

Choose a reason for hiding this comment

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

오 감사합니다
불필요한 코드인것 같습니다



public void setRandomNumberList() {
Random random = new Random();
Copy link
Contributor

Choose a reason for hiding this comment

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

Random 객체를 게임마다 new로 새로 생성할 필요가 있을까요?


public void setRandomNumberList() {
Random random = new Random();
int tempRandomNumber = random.nextInt(9);
Copy link
Contributor

Choose a reason for hiding this comment

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

9는 어떤 값일까요?

Comment on lines 50 to 58
if(i == 0) {
randomNumberList.set(i, random.nextInt(8)+1);
}
else {
while(randomNumberList.contains(tempRandomNumber)) {
tempRandomNumber = random.nextInt(9);
}
randomNumberList.set(i, tempRandomNumber);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

첫번째 숫자에만 0이 할당되지 않도록 하신 것 같은데, 숫자야구 게임은 1 ~ 9까지의 숫자로만 진행됩니다.
요구사항을 꼼꼼히 읽으셔야해요~

Copy link
Contributor

Choose a reason for hiding this comment

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

그리고 for ~ if ~ while 문으로 depth가 3이네요! 2까지가 최대입니다!

Copy link
Author

Choose a reason for hiding this comment

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

다시 수정해야겠습니다

유닛 테스트를 하려면 반환값이 있어야 하는데 그렇다면 setRandomNumber()를 반환값이 있는 getRandomNumber() 식으로 바꾸는게 좋을까요

Copy link
Contributor

Choose a reason for hiding this comment

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

그렇게 하는게 좋겠죠~?

Comment on lines 20 to 22
public boolean isValidLength(String input, int length) {
return input.length()==length;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
public boolean isValidLength(String input, int length) {
return input.length()==length;
}
public boolean isValidLength(String input, int length) {
return input.length() == length;
}

Comment on lines 68 to 73
public void setInputNumberList() {
int dividend;
dividend = userInput.getNumber();
for(int i = Constant.inputLength -1; i >= 0; i--) {
inputNumberList.set(i,dividend%10);
dividend /= 10;
Copy link
Contributor

Choose a reason for hiding this comment

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

UserInput에서 ""으로 잘라서 Set에 넣으셨던데, UserInput에서 자른 값을 getNumber 메서드를 통해서 반환하면 안되나요?

Copy link
Author

Choose a reason for hiding this comment

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

그 부분을 고민 중입니다

UserInput 메서드들이 뭔가 중구난방식으로 만들어진거 같아서 구조를 좀 바꿔야 할 것 같습니다


private void check() {
for(int i = 0; i < Constant.inputLength; i++) {
if(inputNumberList.get(i).equals(randomNumberList.get(i))) {
Copy link
Contributor

Choose a reason for hiding this comment

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

조건을 내부 메서드로 빼는건 어떨까요? 불필요하다고 생각하면 하지 않으셔도 됩니다!
바로 아래줄에 addStrikes()가 따라오기때문에 strike 조건이라고 판단할 수도 있을 것 같네요! 근데 더 명확해질 것 같은데, 고려해보시고 불필요하다고 생각하시면 적용하지 않으시면 좋습니다~

Copy link
Author

Choose a reason for hiding this comment

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

좋은 방법인것 같습니다

@@ -0,0 +1,25 @@
package gmbs;
import gmbs.Constant;
Copy link
Contributor

Choose a reason for hiding this comment

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

불필요한 import가 들어갔네요~

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 18 to 23
if(strikes == 0 && balls == 0) {
System.out.println("낫싱");
}
else {
System.out.println(strikes + " 스트라이크 " + balls + " 볼");
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if(strikes == 0 && balls == 0) {
System.out.println("낫싱");
}
else {
System.out.println(strikes + " 스트라이크 " + balls + " 볼");
}
if (strikes == 0 && balls == 0) {
System.out.println("낫싱");
} else {
System.out.println(strikes + " 스트라이크 " + balls + " 볼");
}

Copy link
Contributor

@Byeongju-Kong Byeongju-Kong left a comment

Choose a reason for hiding this comment

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

순용님~ 테스트 작성하신거 봤어요! @Test그리고 @ParameterizedTest@ValueSource, CsvSource를 잘 활용하셨네요.

다만, 테스트 해야할 기능들이 아직 더 많이 남아있는 것 같아요~ 클래스들을 기능 단위로 분리해서 최대한 많은 기능들을 테스트하려고 해보시면 좋을 것 같아요.

그리고 저번 리뷰들이 아직 반영이 안되어있네요! 테스트를 위한 방향 때문에 빨리 리뷰 요청을 하셨다고 했는데, 다음 리뷰 떄는 이전 모든 리뷰들을 다 반영해서 리뷰 요청해주시길 바랍니다~

그리고 불필요하게 public으로 열려있는 접근 제어자들이 많은 것 같아서, 이부분은 꼭 정리해주세요! 테스트를 하기 위해서 접근제어자를 public으로 열어놓으실 필요 없습니다~

Comment on lines 9 to 11
public int getRandomDigit() {
return random.nextInt(8) + 1;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

public 일 필요가 없어보이네요! 테스트를 위해서 메서드를 public으로 열어놓을 필요가 없습니다~
getRandomDigit라는 메서드를 getRandomNumbers라는 메서드에서 사용하기 때문에, getRandomNumbers를 테스트 하면서 수의 범위에 대한 검증은 충분히 하실 수 있으실 거예요~

그리고 지금 로직으로는 1 ~ 8까지의 숫자만 생성되는 것으로 보여요~
Random의 nextInt 메서드에 대해 찾아보시고 변경하셔야 할 것 같아요!

마지막으로, get 혹은 set과 같은 이름은 지양하는게 좋아요. 단순히 값을 반환하는 메서드에서는 get을 사용할 수 밖에 없지만, 지금은 특정 범위의 수를 생성하는 메서드니까, 그에 맞는 구체적인 메서드 명을 지어주는 것이 좋아보여요~

}

public ArrayList<Integer> getRandomNumbers(int length) {
ArrayList<Integer> tempNumbers = new ArrayList<>();
Copy link
Contributor

Choose a reason for hiding this comment

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

Java는 아래와 같이 구현체를 상위 타입으로 참조 할 수 있는데요.

List<Integer> tempNumbers = new ArrayList<>();

일단 알고만 계시고 이 부분은 다음 세션 때, 공통적으로 피드백해 드리겠습니다~

Copy link
Contributor

Choose a reason for hiding this comment

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

그리고 tmpNumbers라는 변수명이 좋지 않아보여요. temporary numbers 라는 뜻으로 보이는데,
해당 변수에 담기는 값은 최종적으로 반환될 랜덤숫자들로 보여요! 더 구체적인 네이밍을 해주세요~

int tempRandomDigit;
for (int i = 0; i < length; i++) {
do {
tempRandomDigit = getRandomDigit();
Copy link
Contributor

Choose a reason for hiding this comment

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

여기도 마찬가지입니다! 변수명!

}

@ParameterizedTest
@DisplayName("입력값에 중복이 있는지 확인한다")
Copy link
Contributor

Choose a reason for hiding this comment

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

테스트 로직을 보니 입력값에 중복이 없는지 확인한다가 맞겠네요~
중복이 없을 때 true가 반환되니까요!

}


public boolean isValidNumberInput(String input, int length) {
Copy link
Contributor

Choose a reason for hiding this comment

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

꼭 length 값을 파라미터로 전달해야하나요? 사용처를 보니 3이라는 값만 계속 할당 되고 있네요~

Comment on lines 56 to 64
private void check() {
for (int i = 0; i < Constant.inputLength; i++) {
if (inputNumbers.get(i).equals(randomNumbers.get(i))) {
addStrikes();
} else if (randomNumbers.contains(inputNumbers.get(i))) {
addBalls();
}
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

여기가 숫자 야구 게임에 가장 중요한 로직으로 보이는데요. 이 부분의 테스트가 필요해 보입니다~

Copy link
Contributor

Choose a reason for hiding this comment

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

또한, 지금 strikes와 balls라는 인스턴스 변수를 선언해두고 void 메서드로 이를 ++하는 형태인데요.
strikes와 balls라는 변수 없이 메서드의 반환값으로 strikeCount와 ballCount를 반환하는 건 어떨까요?

Comment on lines 6 to 7
private final ArrayList<Integer> inputNumbers = new ArrayList<>();
private final ArrayList<Integer> randomNumbers = new ArrayList<>();
Copy link
Contributor

Choose a reason for hiding this comment

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

inputNumbers와 randomNumbers도 인스턴스 변수로 할당하지않고 충분히 가능할 것으로 보여요~



class RandomNumberGeneratorTest {
RandomNumberGenerator random = new RandomNumberGenerator();
Copy link
Contributor

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.

테스트클래스에서 접근제어자를 사용하면 커밋 시 수정하라고 계속 뜹니다
image

Copy link
Contributor

@Byeongju-Kong Byeongju-Kong 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 7 to 39
public class InputValidator {
public boolean isNeither(Object a, Object b, Object input) {
return !(input.equals(a) || input.equals(b));
}

public boolean isNaturalNumber(String input) {
try {
if (Integer.parseInt(input) > 0) {
return true;
}
} catch (Exception e) {
return false;
}
return false;
}

public boolean isValidLength(String input, int length) {
return input.length() == length;
}

public boolean hasNoOverlap(String input) {
Set<String> inputChecker = new HashSet<>(Arrays.asList(input.split("")));
return inputChecker.size() == input.length();
}

public boolean hasNoZero(String input) {
return !input.contains("0");
}

public boolean isValidNumberInput(String input, int length) {
return isNaturalNumber(input) && isValidLength(input, length) && hasNoOverlap(input) && hasNoZero(input);
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

사용자가 입력을 하는 부분과 입력에 대한 검증을 잘 분리하셨네요! 이렇게 하니까, Input을 통제하지 않아도 테스트를 할 수 있다는 것을 느끼셨나요?ㅎㅎ
다만, 지금 해당 클래스에서 public 접근제어가 필요한 메서드는 isValidNumberInput 밖에 없는데요.
나머지 메서드들을 private으로 변경하고 isValidNumberInput 만으로 모든 입력에 대한 테스트를 진행하는 것이 좋아보여요.
테스트를 위해서 메서드들을 public으로 선언하는건 불필요합니다!
어차피 private으로 변경하여도 isValidNumberInput 메서드에서 private 메서드들을 내부호출 하기 때문에 테스트 하실 수 있어요!

Copy link
Contributor

Choose a reason for hiding this comment

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

그리고, 사용자 입력에 대한 검증에서 자연수인지를 확인하는 부분이 있는데요. 이런 부분들은 parseInt를 하지 않고 정규표현식이라는 것을 통해서 검증할 수 있어요! 정규표현식에 대해서 알아보시고 다음 미션에 적용해보세요~

Comment on lines +15 to +49
class BaseballCheckerTest {
BaseballChecker checker = new BaseballChecker();

@ParameterizedTest
@DisplayName("두 리스트에서 인덱스와 값이 모두 일치하는 요소의 개수를 반환한다")
@MethodSource("generateStrikeData")
void strikeCount(List<Integer> firstList, List<Integer> secondList, int expected) {
assertThat(checker.strikeCount(firstList, secondList)).isEqualTo(expected);
}

static Stream<Arguments> generateStrikeData() {
return Stream.of(
Arguments.of(Arrays.asList(1, 2, 3), Arrays.asList(1, 2, 3), 3),
Arguments.of(Arrays.asList(3, 2, 2), Arrays.asList(5, 1, 2), 1),
Arguments.of(Arrays.asList(1, 2, 1, 5, 6), Arrays.asList(2, 3, 4, 7, 8), 0),
Arguments.of(Arrays.asList(3, 2, 3, 4, 5), Arrays.asList(2, 2, 2), 1)
);
}

@ParameterizedTest
@DisplayName("오른쪽 리스트에서 두 리스트의 교집합 중 인덱스가 다른 요소의 수를 반환한다")
@MethodSource("generateBallData")
void ballCount(List<Integer> firstList, List<Integer> secondList, int expected) {
assertThat(checker.ballCount(firstList, secondList)).isEqualTo(expected);
}

static Stream<Arguments> generateBallData() {
return Stream.of(
Arguments.of(Arrays.asList(1, 2, 3), Arrays.asList(1, 2, 3), 0),
Arguments.of(Arrays.asList(1, 2, 3), Arrays.asList(3, 1, 2), 3),
Arguments.of(Arrays.asList(3, 3, 1, 1, 1), Arrays.asList(1, 2), 1),
Arguments.of(Arrays.asList(1, 2, 3), Arrays.asList(1, 3, 2), 2)
);
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

👍🏻

BaseballChecker checker = new BaseballChecker();

@ParameterizedTest
@DisplayName("두 리스트에서 인덱스와 값이 모두 일치하는 요소의 개수를 반환한다")
Copy link
Contributor

Choose a reason for hiding this comment

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

리스트와 인덱스라는 단어보다 조금 더 통용적으로 사용되는 단어들을 사용하면 좋을 것 같아요~
리스트 -> 수들
인덱스 -> 자리수
와 같이요~

assertThat(checker.ballCount(firstList, secondList)).isEqualTo(expected);
}

static Stream<Arguments> generateBallData() {
Copy link
Contributor

Choose a reason for hiding this comment

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

프로그래밍에서 모든 것들은 값이고 따라서 정보인데, Data라는 단어는 모든 변수에 붙힐 수 있는 것 같아요.
그리고 지금 3개로 이뤄진 수들 2개와 예상되는 ballCount를 생성하고 있는데, 그렇다면 위 의미에 맞게 좀 더 구체적으로 작성하면 어떨까요?

Comment on lines 12 to 49
@ParameterizedTest
@DisplayName("입력값이 자연수인지 확인한다")
@CsvSource(value = {"123, true", "123.4, false", "123f, false", "-123, false", "abc, false", "0, false"})
void isNaturalNumber(String input, boolean expect) {
boolean actual = validator.isNaturalNumber(input);
assertThat(actual).isEqualTo(expect);
}

@ParameterizedTest
@DisplayName("입력값에 중복이 없는지 확인한다")
@CsvSource(value = {"1123, false", "asdf, true", "13a3, false", "31, true"})
void hasNoOverlap(String input, boolean expect) {
boolean actual = validator.hasNoOverlap(input);
assertThat(actual).isEqualTo(expect);
}

@ParameterizedTest
@DisplayName("주어진 길이에 맞는 각 자리수가 서로 다른 자연수인지 확인한다")
@CsvSource(value = {"111, 3, false", "1, 1, true", "123, 4, false", "123, 3, true", "0, 1, false", "120, 3, false"})
void isValidNumberInput(String input, int length, boolean expect) {
boolean actual = validator.isValidNumberInput(input, length);
assertThat(actual).isEqualTo(expect);
}

@ParameterizedTest
@DisplayName("input이 a와 b 둘 다 아닌지 확인한다")
@CsvSource(value = {"a, b, b, false", "1, 2, 3, true", "1, 2, 1, false", "b, b, a, true", "a, b, c, true"})
void isNeither(Object a, Object b, Object input, boolean expect) {
assertThat(validator.isNeither(a, b, input)).isEqualTo(expect);
}

@ParameterizedTest
@DisplayName("0을 포함하지 않는지 확인한다")
@CsvSource(value = {"123, true", "234, true", "0, false", "1230, false"})
void hasNoZero(String input, boolean expect) {
assertThat(validator.hasNoZero(input)).isEqualTo(expect);
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

hasNoOverlap, isNaturalNumber, hasNoZero 테스트에 있는 테스트케이스들을 isValidNumberInput 테스트에 다 포함시켜보아요! 결국 isValidNumberInput 내부에서 해당 메서드들을 사용하기 때문에 테스트가 가능하답니다! 메서드들은 private으로 닫으시고요~

Comment on lines 25 to 35
@Test
@DisplayName("한자리 자연수를 반환한다")
void getRandomDigit() {
int testCount = 1000;
HashSet<Integer> randomNumbers = new HashSet<>();
HashSet<Integer> expected = new HashSet<>(Arrays.asList(1, 2, 3, 4, 5, 6, 7, 8, 9));
for (int i = 0; i < testCount; i++) {
randomNumbers.add(random.getRandomDigit());
}
assertThat(randomNumbers).isEqualTo(expected);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

테스트 메서드를 개행으로 given ~ when ~ then이 나타나게 구분하면 좋을 것 같네요~
다른 테스트들도 마찬가지 입니다~

Comment on lines 13 to 15



Copy link
Contributor

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.

None yet

2 participants