1단계 미션 제출합니다 #8
1단계 미션 제출합니다 #8
Conversation
This reverts commit ed9dc3e.
There was a problem hiding this comment.
안녕하세요 세희님! 스캠에서 서로 한 번쯤은 리뷰어로 매칭되지 않을까요~ 하고 웃었는데, 이렇게 1단계부터 만나뵈어 너무 신기하네요 🤩
전반적으로 깔끔하고 역할 분리가 잘 되어 있어서 읽기 좋은 코드라고 느꼈어요! 예외처리가 꼼꼼하게 되어있는 것도 인상깊었구요~~ 소소하게 제 의견을 조금씩 찹찹 남겨두었으니, 확인하신 후 세희님 의견도 남겨주시면 좋을 것 같아요 👍 고생하셨습니다! 🥳
아래는 PR에 남겨주신 고민 포인트에 대한 답변입니다!
확장성을 고려한 인터페이스 정의 => 단위테스트, 각종 기능 교체를 고려하다보니 인터페이스를 많이 사용하게 되었는데 너무 남용하는 것 같다는 생각도 들더라구요!
저는 남용했다는 느낌은 받지 못했어요! 말씀하신 것처럼 인터페이스를 사용했기에 테스트가 유리한 부분도 있었고, 기능의 수정에도 열려있는 코드가 되었다고 생각해요.
만약 인터페이스 분리로 인한 코드 복잡도가 고민되신다면, Reader이나 Printer를 인터페이스 없이 바로 구현 클래스로 사용하시는 것도 방법일 것 같아요. Read나 Print 방식이 바뀐다면 아마 콘솔 프로그램에서 웹 어플리케이션으로 바뀌는 등의 상황일텐데, 그땐 입출력 뿐만 아니라 시스템 전반적으로 대대적인 변경이 필요할 것이 추상화하는 것이 큰 의미가 없을 수도..! 😇
클래스 상속 vs 인터페이스 구현 => 은지님은 주로 어느 상황에서 각각 방식을 적용하는지 궁금합니다!
개인적으로
- 상속은 클래스 사이의 계층 구조(관계)를 나타낼 때
- 인터페이스 구현은 구현체가 여러개일 때 & 구현을 외부에 노출하고 싶지 않을 때
사용하고 있어요! 목적과 카테고리가 다르다고 생각해요 🤔 상속은 기능의 확장이고, 인터페이스 구현은 기능의 구현이라는 느낌..?!
while문이 들어간 클래스(GamePlayer, Game)도 테스트를 해보고 싶었는데 우선 종료 테스트만 만들어보았습니다! 게임 실행 테스트(while문이 잘 돌아가는지)를 하게 되면 더 복잡해질 것 같아 생략했습니다!
동의해요! while문은 자바의 키워드이니 저희가 테스트하지 않아도 된다고 생각해요. 루프가 특정 조건을 만족할 때 정상적으로 종료되는지 (= 무한 루프 방지) 종료 조건을 테스트하는 것으로 충분할 것 같아요 👍
| this.numbers = numbers; | ||
| } | ||
|
|
||
| public Hint match(Answer otherAnswer) { |
There was a problem hiding this comment.
사용자 입력값과 정답을 비교하는 과정을 Answer의 멤버 메서드로 만들어주셨군요!
지금도 충분히 가독성이 좋지만, Hint가 어떻게 생성되는지 알기 위해서(= 채점 기준을 알기 위해서) Answer 클래스를 열어봐야 한다는 점이 조금 어색하게 느껴지는 것 같아요. Hint의 생성자나 정적 팩토리 메서드를 활용해 책임을 응집시켜보면 어떨까요?
또 지금은 사용자 값과 컴퓨터 정답이 모두 Answer 클래스 인스턴스라, 위 코드에서 otherAnswer이 어떤 아이인지 조금 헷갈리는 것 같아요! Hint의 생성자 (또는 정적 팩토리 메서드)를 이용하면 매개변수명으로 각각의 Answer 인스턴스가 어떤 값인지 좀 더 명시적으로 드러낼 수 있을 것 같아요 👍
There was a problem hiding this comment.
맞습니다! 원래 answer라는 주체가 직접 hint를 만들어내도록 할까 따로 hint를 생성하는 객체를 만들어낼까 고민을 많이 했습니다!
코드를 보니 Answer가 hint를 생성한다는 게 Answer의 뜻과는 조금 거리가 있는 것 같다는 생각이 드네요! 더 나은 구조로 변경해보겠습니당!🥰
| if (this.numbers[index] == otherNumber) { | ||
| if (index == otherIndex) { | ||
| hint.increaseStrikeCount(); | ||
| return; | ||
| } | ||
|
|
||
| hint.increaseBallCount(); | ||
| } |
There was a problem hiding this comment.
여기가 two-depth 이네요! 객체지향 생활체조 요구사항에 맞춰 depth를 줄일 순 없을까요? 🤔
There was a problem hiding this comment.
오오오 매의 눈이십니다! 2중 for문으로 구현하다보니 depth가 잘 안지켜지드라구요! stream 사용해서 구조 개선 해보겟습니다!🚀
| public Answer make() { | ||
| int[] numbers = numberMaker.makeAllUnique(); | ||
|
|
||
| return new Answer(numbers); | ||
| } | ||
|
|
||
| public Answer make(int[] numbers) { | ||
| AnswerNumberValidator.check(numbers); | ||
|
|
||
| return new Answer(numbers); | ||
| } |
There was a problem hiding this comment.
컴퓨터 정답 생성과정과 사용자 입력값 생성 과정을 오버로딩을 이용해 구현해주셨군요!
각각의 메서드의 반환형이 Answer로 동일하지만 생성되는 인스턴스의 유형은 다른 것 같아요. 각 메서드의 목적이 분명하다고 느껴지는데, 메서드명을 통해 무엇을 만드는 메서드인지 명시해주면 어떨까요?
(e.g., makeComputerAnswer, 등)
There was a problem hiding this comment.
저도 살짝 고민이 되었던 부분이었습니다! 더 개선해보도록 하겠습니다! :)
|
|
||
| import sehee.util.numbermaker.NumberMaker; | ||
|
|
||
| public class AnswerFactory { |
There was a problem hiding this comment.
네이밍은 주관적인 의견이 많이 들어가는 부분이라 조금 조심스럽지만..!
Answer이라는 이름만 들었을 때는 이 게임의 정답을 나타내는 값이라는 생각이 들어요. 사용자가 입력하는 값도 여기 클래스의 make메서드를 통해 생성되는 것 같아서 조금 의아했어요. 사용자가 입력하는 값은 엄밀히 말하면 정답은 아니니까요!
(운이 좋다면 한 번에 정답을 맞출 수도 있겠지만요 🤩)
Answer의 이름을 Number 등으로 바꾸거나, 정답 클래스와 사용자 입력값 클래스를 분리하면 좀 더 의미가 분명해질 것 같아요~
| // package-private | ||
| public class RandomNumberMaker implements NumberMaker { | ||
|
|
||
| private final Random random; |
There was a problem hiding this comment.
RandomNumberMaker마다 Random이 별도로 생성될 필요가 없으니, static으로 선언해도 좋을 것 같아요!
또 Random 클래스는 유사 난수를 생성한다는 보안 이슈가 있으니, 보다 안전한 숫자 야구 게임을 위해 SecureRandom을 사용해보면 어떨까요?
There was a problem hiding this comment.
랜덤은 잘 안써봤던 모듈이라 유서 난수에 대해 생각해본 적이 없었네요! 추천해주신 securerandom과 슬랙에 올라온 shuffle 등 더 찾아보면서 반영해보겠습니다!
| assertThat(autoMadeAnswer).extracting("numbers") | ||
| .isEqualTo(answerNumbers); |
There was a problem hiding this comment.
numbers가 private이라서 extracting을 사용해주신 걸까요?.? (extracting을 처음 보아서..! 😅)
There was a problem hiding this comment.
넵! 따로 필드를 추출해주는 메서드로 알고 있습니다! 나름 보안을 신경쓴다고 private로 설정해놨는데 getter 같은 함수를 정의해도 좋을 것 같다는 생각이 드네요! (private 필드를 일부러 추출하는 것이 좋지는 않아보인다고 생각이 들었습니다)
안쓰는 필드 private vs 안쓰더라도 의미가 있는 값이먼 getter 메서드 적용
위 방식 둥에 고민이었습니다!
| Hint matchResult = computerAnswer.match(userAnswer); | ||
|
|
||
| // then | ||
| assertThat(matchResult.isThreeStrike()).isFalse(); |
|
@EunjiShin 늦은 시간에 올렸는데 꼼꼼히 리뷰햐주셔서 감사합니다! 오늘 일정 끝나고 다시 꼼꼼히 확인한 후에 답변드리겠습니다! 감사합니다!! |
🗨️ 코멘트
은지님 안녕하세요! 은지님이 리뷰어셨다니...!🥰
우선.... 저번 주 면접, 코딩테스트로 정신 없이 보내다가 뒤늦게 올리게 되어 먼저 정말 죄송합니다는 말씀드립니다ㅠㅠ
오랜만에 자바로만 작성하다 보니 부족한 점이 많이 보이네요...🥲
불필요한 부분이 보이신다면 과감히 말씀해주시면 감사하겠습니다!🙇♀️(ex. 코드가 너무 복잡해요! 너무 길어요! => 환영입니다!!!!)
고민 포인트
GamePlayer,Game)도 테스트를 해보고 싶었는데 우선 종료 테스트만 만들어보았습니다!추가 업데이트
AnswerFactory,GamePlayer)📁 구조 설명
📷 시연 이미지