Conversation
| import java.util.Map; | ||
| import java.util.stream.IntStream; | ||
|
|
||
| public class Discrimination { |
There was a problem hiding this comment.
class 이름을 discrimination으로 설정하신 이유가 궁금합니다!
There was a problem hiding this comment.
두 숫자를 비교하는 행위를 '판별한다(discriminate)'고 표현하고 싶었습니다.
검증하다(Verify)나 판단하다(Judge)로 표현하는 게 더 자연스러웠을지 모르겠네요..
Discrimianation은 생성되는 순간 두 숫자(Number)를 판별하고 그 정보를 저장해놓았다가 표시하는 역할을 하는 클래스입니다.
src/main/java/Main.java
Outdated
| } | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
proceed와 askPlayEnd를 BaseballGame이나 다른 클래스에서 구현 후 호출하는 방향은 어떤지 생각이 듭니다!
There was a problem hiding this comment.
해당 메소드를 어떤 클래스로 호출하는 게 더 적절할지 생각해보았으나... 역시 게임의 메뉴가 이 역할을 수행하는 게 자연스럽다고 생각하여 메소드의 위치를 옮기지는 않았습니다.
대신 Main이라는 클래스 이름을 BaseballGame이라는 이름으로 변경하였습니다. (원래 BaseballGame이라는 이름을 가진 스테이지 역할의 클래스는 BaseballStage로 변경했습니다)
| private final List<Discrimination> history = new ArrayList<>(); | ||
|
|
||
| public void setNewGame() { | ||
| Set<Integer> bucket = new HashSet<>(Arrays.asList(1, 2, 3, 4, 5, 6, 7, 8, 9)); |
There was a problem hiding this comment.
List.of() 스태틱 매서드를 사용해보면 어떨까요?
| public List<Discrimination> getHistory() { | ||
| return new ArrayList<>(this.history); | ||
| } |
There was a problem hiding this comment.
사용하지 않는 매서드는 없애주는 것이 깔끔할 것 같아요!
|
|
||
| private int pickNumber(Set<Integer> bucket) { | ||
| while (true) { | ||
| int random = (int) (Math.random() * 9) + 1; |
There was a problem hiding this comment.
이 방식이 숫자를 담은 리스트를 셔플하는 것보다 수행시간이 더 빠를 수 있겠다고 얘기를 했었는데.. 다시 생각해보니 치명적인 문제가 있었습니다. 바로 random 함수에서 꺼낸 값이 중복되면 중복될 수록 수행 시간이 더 길어질 수 있다는 것이었습니다.
그래서 셔플하는 방법으로 변경했습니다!
| public void setNewGame() { | ||
| Set<Integer> bucket = new HashSet<>(Arrays.asList(1, 2, 3, 4, 5, 6, 7, 8, 9)); | ||
|
|
||
| int num = 0; | ||
| int digit = 1; | ||
| for (int i = 0; i < 3; i++) { | ||
| num += pickNumber(bucket) * digit; | ||
| digit *= 10; | ||
| } | ||
|
|
||
| this.targetNumber = new Number(num); | ||
| this.history.clear(); | ||
| } | ||
|
|
||
| private int pickNumber(Set<Integer> bucket) { | ||
| while (true) { | ||
| int random = (int) (Math.random() * 9) + 1; | ||
| if (bucket.contains(random)) { | ||
| bucket.remove(random); | ||
| return random; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
랜덤한 세자리수의 생성을 위해 해당 방식을 선택한 이유가 궁금해요! 궁금한점은
- Set을 사용한 이유
- setNewGame() 매서드에서 history를 clear하는 이유 (아마도 history 객체는 사용하지 않는 것 같아요)
더불어 Number 객체에게 더 많은 일을 시키게 된다면 어떤 모습이 될지도 궁금해요!
| protected void setTargetNumber(Number targetNumber) { | ||
| this.targetNumber = targetNumber; | ||
| } |
There was a problem hiding this comment.
이 매서드는 테스트를 위한 매서드같아요!
저도 항상 고민하는 부분인데, 테스트만을 위한 매서드를 만들어내는 것은 항상 찜찜하더라구요.
이 부분에 대해서 어떻게 생각 혹은 고민하는지 궁금해요!
There was a problem hiding this comment.
다른 많은 사람들의 의견 중에서 가장 와닿은 의견은 테스트 만을 위한 메서드가 필요한 순간이 온 것은 잘못된 설계에 의한 구조적인 문제에 의한 것일 수도 있다는 것이었습니다.
랜덤한 숫자를 골라내는 로직을 다른 객체로 이관하는 것으로 구조를 고쳐봤습니다.
| for (int i = 0; i < baseArr.length; i++) { | ||
| if (compareMap.containsKey(baseArr[i])) { | ||
| if (compareMap.get(baseArr[i]) == i) { | ||
| strike++; | ||
| } else { | ||
| ball++; | ||
| } | ||
|
|
||
| compareMap.remove(baseArr[i]); | ||
| } | ||
| } |
There was a problem hiding this comment.
해당 로직에서 else는 필요치 않은 것 같아요!
더불어 새로운 게임이 생성되고 새로운 비교(Descrimination)을 할 때마다 해당 객체가 생성되는 것 같아요! 이 방법 대신 변수들(strike, ball...)을 초기화시켜주는 방법을 사용하는 것은 어떨까요?
| public Number(int number) { | ||
| for (int i = 2; i >= 0; i--) { | ||
| nums[i] = number % 10; | ||
| number = number / 10; | ||
| } | ||
| } |
There was a problem hiding this comment.
해당 매서드는 사용하는 측에서의 활용을 모른다면(사용하는 측의 로직을 모른다면) 이해하기 힘든 로직인 것 같아요!
- 해당 로직의 책임을 다른 곳으로 위임하거나
- 해당 생성자 매서드 안에 있는 로직에 "이름을 부여" 해서 의도를 밝혀보는
것은 어떨까요?
|
|
||
| import static org.assertj.core.api.Assertions.assertThat; | ||
|
|
||
| class BaseBallTest { |
There was a problem hiding this comment.
중복되는 로직을 (주로 초기화 로직을) 줄여보면 어떨까요?
| BaseballGame baseballGame = new BaseballGame(); | ||
| baseballGame.setTargetNumber(new Number(Integer.parseInt(targetNumber))); |
There was a problem hiding this comment.
매서드의 해당 부분만을 보면 baseBallGame 은 오직 세터로직으로만 초기화되는것으로 보여요. (물론 그렇지 않음을 알지만)
해당 게임 객체의 초기화를 위한 다른 방법은 없을까요?
| return true; | ||
| } | ||
|
|
||
| @Override |
There was a problem hiding this comment.
hashCode() 와 equals(Object obj) 를 오버라이드한 이유가 궁금합니다...!
Number클래스는 뭔갈 상속받고있는 클래스도 아닌것 같은데.. 왜 사용하신건가요..?
There was a problem hiding this comment.
Object는 Java의 모든 클래스의 최고 조상 클래스입니다.
equals()와 hashCode()는 모두 이 Object가 가지고 있는 메소드입니다.
equals() 메소드는 두 인스턴스가 같은지를 비교하는 메소드로 재정의 하지 않는다면 두 인스턴스가 저장된 메모리 주소를 비교하게 됩니다. 이를 재정의 하고 있는 이유는 두 Number 인스턴스가 같은지 비교할 때 멤버 변수로 저장하고 있는 정수 배열(int[] nums) 안의 정수 값들과 그 순서로 비교하기 위해서 입니다. 이로써 실제로 같은 숫자를 저장하고 있는 Number의 인스턴스를 여러개 만들어도 메모리에 저장된 위치만 다를 뿐 같은 데이터라고 인식할 수 있습니다.
hashCode()는 보통 equals()를 재정의 할 때 함께 재정의 하는 것이 권장되고 있습니다. 이 메소드는 인스턴스의 해시코드를 구해서 반환하는 메소드로 재정의 하지 않는다면 인스턴스가 저장된 메모리 주소로 해시코드를 만들어 반환합니다. 위의 equals() 메소드에서처럼 이 메소드의 실질적인 데이터의 본질을 말하고 있는 건 int[] nums안의 정수 값들입니다. 따라서 hashCode()도 재정의하고 있습니다. (이 함수는 HashMap이나 HashSet과 같은 자료형에 저장될 때 사용됩니다.)
There was a problem hiding this comment.
equals(), hashCode() 두 메소드는 해시 코드를 이용하는 컬렉션에서 주로 활용됩니다.
해시 알고리즘은 모든 인스턴스에 대해서 완전히 중복되지 않는 고유한 값을 만들어주지는 않습니다. 알고리즘에 따라서 그 확률은 다르겠지만 해시 코드값이 중복되는 경우도 있습니다.
해시 코드를 이용하는 컬렉션들은 값을 저장, 조회할 때 일단 해시코드를 이용해서 저장하고, 만약 중복되는 해시코드 값이 이미 있다면 그 이후에는 equals()를 이용해서 값이 같은지 비교하여 처리하는 방식입니다.
| assertThat(result.getBall()).isEqualTo(1); | ||
| assertThat(result.getStrike()).isEqualTo(0); | ||
| assertThat(result.getOut()).isEqualTo(2); | ||
| } |
There was a problem hiding this comment.
@CsvSource 어노테이션을 처음봐서 검색해봤는데 입력값에 따라 결과값이 다른 경우를 테스트할 때 쓴다고 하더라구요.. 그럼 135 이 부분이 입력값이고 218 이 부분이 결과값을 미리 어노테이션으로 정의해놓은 건가요?
어떻게 적용되는것인지 궁금합니다..!
There was a problem hiding this comment.
Csv는 사실 쉼표(comma ,)로 여러 데이터를 구분하고 있는 텍스트 데이터를 말합니다. 예를 들어서 135, 218 처럼...
@CsvSource는 이런 Csv의 개념을 가져와서, 서로 다른 케이스의 Csv 데이터를 이용해서 테스트를 반복 수행하도록 돕는 어노테이션입니다.
어노테이션의 value={} 속성에는 실제 수행될 여러 케이스의 Csv 데이터들이 들어갑니다.
delimiter= 속성에는 Csv 데이터 안에서 데이터들을 구분할 때 사용합니다.
구분된 데이터는 순서대로 테스트 메소드의 인자로 들어가게 됩니다.
제가 구현한 테스트 메소드에서는 "135:218" Csv 데이터라면 ':'를 기준으로 135와 218이 순서대로 메소드의 targetNumber, input 인자로 들어오게 됩니다.
4 participants
No description provided.