-
Notifications
You must be signed in to change notification settings - Fork 43
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기]3주차 Wordle 과제 제출 - 치즈 #30
base: main
Are you sure you want to change the base?
Conversation
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.
안녕하세요 치즈님! 리뷰를 맡게된 싸무엘입니다.
제가 과제를 진행하면서 고민했던 부분과 궁금한 점, 개선해보면 좋을 것 같은 부분을 코멘트로 남겨드렸어요.
코멘트에 대한 의견, 궁금한 점이 있다면 말씀해주시고, 제가 잘못 알고 전달드린 부분있다면 알려주세요!
바쁘신 와중에 과제하시느라 고생 많으셨습니다! 👍🏻👍🏻 계속 같이 파이팅해요💪🏻
|
||
public class AnswerProvider { | ||
|
||
private static final LocalDate REFERENCE_DATE = LocalDate.of(2021, 6, 19); |
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.
WordleGameConfig 클래스가 게임 규칙 관련한 상수를 모아놓은 역할이라고 이해했어요.
그래서 이 상수도 게임 규칙 관련한거라 WordleGameConfig 에 포함되면 좋을 것 같다고 생각하는데, 혹시 WordleGameConfig에서 관리하지 않고 따로 분리하신 이유가 있을까요?? 👀
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.
맞습니다 ! 👍 정확히 표현하자면 WorldeGameConfig는 클라이언트 단에 즉각적으로 변화가 생길 수 있는 게임 규칙의 역할로 설계했어요.
- 단어 파일(정답) 교체
- 라운드(기회) 수 조정 등
이 상수도 고민이 많았지만, 오늘 날짜와 차이를 비교해서 정답을 도출해내는 도구에 국한된다고 생각해서 AnswerProvider에 있는게 맞다고 생각했습니다 !
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.
오호 저는 이런 부분들이 모두 게임의 규칙이라고만 생각했었는데,, 구체적으로 답변 주셔서 감사합니다 😄
round = new Round(input); | ||
roundResults.append(round.roundResult(answer)).append("\n"); |
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.
input, answer 둘 다 Round 생성 시점에 넣어주도록 할 수도 있고, 둘 다 메서드 호출 시 넘겨주도록 구현할 수도 있을 것 같아요.
Round 객체 생성 시 input만 넣어주고, 메서드 호출 시 answer를 넘겨주도록 설계하신 이유가 궁금합니다!
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.
Round가 answer를 갖는 형태가 어색하다고 생각했어요.
반대로 Round 하나 당 사용자 입력을 한 번 받으니까 input은 라운드에 속해있다는 관점으로 설계했습니다.
이렇게 정리하면서 보니까 어색한 느낌이 들기도 하네요 !
리팩토링 포인트로 붙잡아두겠습니다 ... 🤔
char[] inputChars = input.toCharArray(); | ||
int[] countPerCharacter = Arrays.copyOf(answer.getCountPerCharacter(), 26); | ||
for (int i = 0; i< WORD_LENGTH; i++) { | ||
Tile key = getTile(countPerCharacter, answer.charAt(i), inputChars[i]); |
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.
딱구님과 제가 이쪽 로직에서 countPerCharacter가 Answer의 멤버로 포함되어 있고, 단순히 getter만 제공하고 있는 구조라서 counterPerCharacter가 Answer 내부에 있는게 옳은 방향인가 고민하다가 클래스를 분리했었는데요.
이 부분을 어떤 논의 과정으로 설계, 구현하셨는지 궁급합니다!
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.
이 부분은 깊게 고민하지 않았는데, 말씀을 들어보니 Answer 내부에 countPerCharacter가 있는 게 아주 매끄럽지는 못하군요 🤔
저도 어떻게 풀어나가야 할 지 고민해봤는데 명쾌한 답을 아직 못 찾았습니다 ..
싸무엘님과 딱구님은 어떤 과정으로 클래스를 분리하셨을까요 ~?
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.
저희도 동일한 부분에서 어색하다고 느껴서 분리의 목적으로 countPerCharacter 에 관련된 책임을 갖는 Counter라는 클래스를 만들었어요.
단어의 문자를 비교할 때 count 관련한 기능은 해당 클래스가 담당하도록 했고, 비즈니스 로직에서 Answer와 Counter를 조합해서 문제를 풀어나가면 되지 않을까 생각했던 것 같아요.
100% 명쾌하게 해결한 것인지는 모르겠어서 저도 고민이 더 필요한 부분이긴 해요 ㅎㅎ 다른 분들의 문제 해결 과정도 궁금해지네요!
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.
제 의견도 생각해서 반영해주시고, 궁금한 점이 많았는데 모두 친절하게 구체적으로 답변해주셔서 감사합니다! 치즈님의 생각을 알아가며 많이 배울 수 있었어요 🥰
제 부족한 리뷰를 받아주셔서 감사합니다. 앞으로도 파이팅 하시죠! 🚀
public Round next(String input) { | ||
return new Round(value + 1, input); | ||
} |
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.
불변 객체 좋네요👍🏻
char[] inputChars = input.toCharArray(); | ||
int[] countPerCharacter = Arrays.copyOf(answer.getCountPerCharacter(), 26); | ||
for (int i = 0; i< WORD_LENGTH; i++) { | ||
Tile key = getTile(countPerCharacter, answer.charAt(i), inputChars[i]); |
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.
저희도 동일한 부분에서 어색하다고 느껴서 분리의 목적으로 countPerCharacter 에 관련된 책임을 갖는 Counter라는 클래스를 만들었어요.
단어의 문자를 비교할 때 count 관련한 기능은 해당 클래스가 담당하도록 했고, 비즈니스 로직에서 Answer와 Counter를 조합해서 문제를 풀어나가면 되지 않을까 생각했던 것 같아요.
100% 명쾌하게 해결한 것인지는 모르겠어서 저도 고민이 더 필요한 부분이긴 해요 ㅎㅎ 다른 분들의 문제 해결 과정도 궁금해지네요!
|
||
public class AnswerProvider { | ||
|
||
private static final LocalDate REFERENCE_DATE = LocalDate.of(2021, 6, 19); |
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.
오호 저는 이런 부분들이 모두 게임의 규칙이라고만 생각했었는데,, 구체적으로 답변 주셔서 감사합니다 😄
진행 방식
잘한점
아쉬운점
어려웠던점