-
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 과제 제출 - 딱구 #35
base: main
Are you sure you want to change the base?
The head ref may contain hidden characters: "\uB531\uAD6C-\uC2F8\uBB34\uC5D8"
Conversation
- 같은 문자가 있는 경우 오동작
- 정답 : lurid, 입력 : hello일 때 노란 타일이 두 번 출력되는 문제 수정
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 LetterCounter(Letters letters) { | ||
this.letterCountMap = new HashMap<>(); | ||
for (Letter answerLetter : letters.getLetters()) { | ||
char value = answerLetter.getValue(); | ||
letterCountMap.put(value, letterCountMap.getOrDefault(value, 0) + 1); | ||
} | ||
} |
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.
Letters
는 Letter
제네릭 타입의 1급 컬렉션으로 보여지네요!👍🏻
저는 개인적으로 외부에서 for
문을 사용하는 1급 컬렉션이라면 Iterable
를 구현하는 방법도 좋다고 생각하는데 어떻게 생각하시나요?
public LetterCounter(Letters letters) { | |
this.letterCountMap = new HashMap<>(); | |
for (Letter answerLetter : letters.getLetters()) { | |
char value = answerLetter.getValue(); | |
letterCountMap.put(value, letterCountMap.getOrDefault(value, 0) + 1); | |
} | |
} | |
public LetterCounter(Letters letters) { | |
this.letterCountMap = new HashMap<>(); | |
letters.forEach(answerLetter -> { | |
char value = answerLetter.getValue(); | |
letterCountMap.put(value, letterCountMap.getOrDefault(value, 0) + 1); | |
}); | |
// 또는 | |
for (Letter answerLetter : letters) { | |
char value = answerLetter.getValue(); | |
letterCountMap.put(value, letterCountMap.getOrDefault(value, 0) + 1); | |
} | |
} |
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.
오 이런방법이...... 👍 감사합니다. 몰랐는데, 적용하니까 훨씬 깔끔해지네요! 회사 코드에도 적용했습니다 ㅋㅋㅋㅋ 😄 꿀팁 너무 감사합니다.
추가로 궁금한게 있는데, 혹시 stream()
도 구현해서 사용하시는 편이실까요? 회사 코드에는 stream()
을 애용하고 있어서 Iterable로 만듦으로써 사용 가능해지는 forEach()나 향상된 for문은 자주 사용하지 않거든요!
이를 여쭙는 이유는, 내가 미처 생각하지 못한 주의사항이 있을까?
라는 노파심에 여쭤봅니다 ㅎㅎ 혹시 없다면 회사 코드에도 적용해보려고 해요 ㅎㅎ
(워들에는 적용 했습니다 😁)
public List<Letter> getLetters() { | ||
return letters; | ||
} |
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.
컬렉션을 그대로 반환하는 이유가 있을까요?
컬렉션을 반환할 땐 완전 직렬화는 못 해도 Collections.unmodifiableList(letters);
처럼 깊은 복사를 한번 해주는 게 좋을 것 같아요!
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.
동의합니다. 불변 객체로 복사해서 반환을 해줬어야 했는데, 당시에 구현에 심취(?)하다보니 대충 넘어간 흔적이 보이네요 😓
하지만 getLetters()
는 Iterable
을 구현하면서 자연스럽게 사용하지 않게 되어 제거하였습니다 👍
|
||
private boolean isOnlySameValue(Letter other) { | ||
return letters.stream() | ||
.anyMatch(letter -> (Objects.equals(letter.getValue(), other.getValue())) && letter.getPosition() != other.getPosition()); |
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.
값 비교 로직은 Letter
클래스의 책임이라고 생각하는데 어떻게 생각하시나요?
다른 검증 로직에서 Letter::equals
, Letter::hashCode
해주는 것과 isOnlySameValue()
경우가 같다고 생각합니다!
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.
부끄럽지만 이때 Letter
에 메서드를 구현하려고 네이밍을 이래저래 생각하다가, "아 일단 구현이 중요하니 이렇게 하고 넘어갑시다"했던 기억이 있네요 😓
저도 yooth님 의견에 동의합니다.
그런데, 네이밍 관련해서 고민인 부분이 있습니다. ㅋㅋ
이는 회사에서도 팀원들과 고민하는 부분인데,
Letter
의equals()
는position
,value
가 모두 동일하다- 그러면
postion
만 같은 메서드는 뭐라고 할까?hasEqualPosition()
equalPosition()
hasSamePosition()
isOnlySamePosition()
등등 다양한 의견이 오고갔지만 여전히 좋은 이름을 찾아 헤매는 중입니다 ㅋㅋㅋ has를 써야할까 is를 써야할까도 고민이네요 (쓸데없는 고민일수 있지만욤😅)
작성하면서 생각해보니 Letter
에 position
이 와버려서 생기는 어색함이 아닌가? 라는 생각도 드네요 ㅋㅋㅋ Letter에 어울리지 않는 필드가 있는게 아닌지, Class 이름이 어색한건지는 고민을 좀 더 해보겠습니다. ㅋㅋㅋ
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.
일단은 고민끝에 답이 안나오는데다 position을 분리해보자니 대공사가 돼서, Letter
에 isOnlySameValue()
를 구현해주는 것으로 커밋했습니다 ~!
public TileService(TileStorage tileStorage) { | ||
this.tileStorage = tileStorage; | ||
} |
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.
TileStorage
를 주입 받은 이유가 있나요??
저는 백엔드 개발 경험은 적고 비교적 앱 개발을 주로 하다 보니 무조건 의존 주입을 받는 것 보다 상황에 따라 내부에서 생성하는 것도 좋다고 생각합니다.
주로 생각하는 기준은 '확장 가능성이 있는가?' 를 고민하는 것 같아요.
당장 추상화를 하진 않아도 충분히 확장 가능성이 있다면 주입을 받도록 하는 것 같습니다.
싸딱팀의 WordsReader
역할이 이 경우라고 생각해요.
딱구님은 어떻게 생각하시나요?
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.
워들 애플리케이션은 그 자체가 변화할 가능성이 매우 적고, 확장성을 고려하지 않아도 되는 상황이라 현재 상황에는 yooth님 말씀대로 내부에서 생성하는 것도 편리하고 좋을 것 같습니다!
하지만, 저는 다음과 같은 생각들 때문이 DI받는 것을 선호합니다.
TileService
에TileStorage
가 필요함을 명시적으로 알릴 수 있다 (숨기지 않는다)- 이건 개인 취향인 것 같은데, 저는 생성자에 해당 클래스는 a,b,c가 필요합니다라고 알려주는 것을 좋아합니다. 이를 통해 역할이 명확해지는 것 같다고 생각하고, 협력관계가 더 명확히 표현되어 좋은 것 같습니다.
- 테스트 하기 용이해진다. (
TileStorage
구현이 복잡하다면 더더욱 장점 증대) TileService
가 과연TileStorage
를 생성할 책임도 갖고 있는지도 생각합니다. 보통은 생성 책임은 다른데에 있는 것 같아요 😄 (아직 많은 경우를 만나보지 못한걸수도 있습니다.)- 그리고, 정말로 확장성이 없을까?도 생각합니다. 변경 가능성이 없다고 생각한 주민등록번호 정책이 변경됨으로써 대참사를 맞이했던 것 처럼... 어느정도의 확장성은 고려해두는게 좋지 않을까? 생각도 하는 것 같습니다. (그렇다고 너무 고려하면 복잡도만 증가하겠죠.. 무한 트레이드오프 😂)
반면에, Collection같은 것은 내부에
private final List<Obj> objs = new ArrayList<>();
와 같이 선언하기도 합니다.
yooth님 덕분에 아무생각 없이 습관처럼 해왔던 패턴을 다시한번 생각해보게 되었네요 👍 쥐어 짜내듯 생각한거라 어색할수도 있는데요! 어색한 부분이 있다면 언제든지 편하게 물어봐주세요!
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
가 역할을 물려받았습니다 ㅋㅋ
private void processNoneMatchLetters(Letters answerLetters, Letters inputLetters, Tiles tiles) { | ||
Letters noneMatchingLetters = inputLetters.findNoneMatchingLetters(answerLetters); | ||
tiles.addGrayTile(noneMatchingLetters); |
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.
findNoneMatchingLetters()
메서드로 포함하지 않는 문자를 가져오고 있는데,
Tiles
에서 기본값을 GRAY_TILE 로 설정하면 이런 불필요한 작업이 없어질 것 같아요!
그렇게 되면 processSameValueLetters()
에서도 굳이 GRAY_TILE 을 추가하는 로직이 필요 없어질 것 같은데 어떻게 생각하시나요?
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.
오 이런생각은 못했네요 ㅋㅋㅋ 너무너무너무 좋은 의견 같습니다. 바로 반영하였습니다 👍 저는 이생각은 전혀못하고 늪에 빠져있었는데, 구해주셔서 감사합니다 😊
Tiles
를 생성할 때 GRAY_TILE을 넣어주도록 변경했습니다
private Letters getInputLetters(Words words) { | ||
Letters inputLetters; | ||
while (true) { | ||
console.printInputRequestMessage(); | ||
String input = console.inputAnswer(); | ||
inputLetters = new Letters(input); | ||
|
||
if (isInputLettersInvalid(words, inputLetters)) { | ||
continue; | ||
} | ||
|
||
break; | ||
} | ||
return inputLetters; | ||
} |
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.
start()
메서드와 함께 2중 while 구조는 생각 못 했는데 좋은 것 같네요!
private boolean isEnd(Tiles tiles, int tryCount) { | ||
if (tileService.isAnswer(tiles)) { | ||
console.printResult(tryCount, TRY_COUNT_LIMIT, tileService.findAll()); | ||
return true; | ||
} |
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.
TileService
한테 Tiles
을 넘겨주고, TileService
는 받은 Tiles
에게 isFilledWith()
를 호출해 전부 ANSWER_TILE 인지 확인하는 것으로 보이네요!
isFilledWith()
는 TileService
의 isAnswer()
에서만 호출되는 것 같은데
Tiles
에게 바로 isAnswer()
를 물어봐도 될 것 같은데 이렇게 한 이유가 궁금합니다.
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.
초기 구현 의도는 Tiles
가 과연 정답 여부를 아는게 맞을까? 에서 시작돼서 이렇게까지 됐던 것 같습니다. ㅎㅎ
지금은 Tiles
를 정답과 입력을 비교한 결과인 Result
로 변경했고, Result
를 List형태로 들고 있는 Results
까지 구현하게 됐으며, 이에 따라 자연스럽게 Result
에 isAnswer()
를 구현하도록 변경됐습니다!
public void printTiles(List<Tiles> tiles) { | ||
for (Tiles tile : tiles) { | ||
System.out.println(tile); | ||
} | ||
} |
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 void printTiles(List<Tiles> tiles) { | |
for (Tiles tile : tiles) { | |
System.out.println(tile); | |
} | |
} | |
public void printTiles(List<Tiles> tiles) { | |
final String tilesString = tiles.stream() | |
.map(Tiles::combine) | |
.collect(Collectors.joining("\n")); | |
System.out.println(tilesString); | |
} |
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.
너무 좋은것 같습니다 ㅋㅋ 특히, 표준 출력 블로킹을 줄일 수 있다는 관점 너무 좋네요 👍
반영 완료하였습니다 :) !!! 많이 배워가네요!!!
@ParameterizedTest | ||
@MethodSource("provideResultData") |
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.
@MethodSource 신기하네요!
하나 배워갑니다.
src/test/java/wordle/WordsTest.java
Outdated
@DisplayName("입력한 단어의 포함 여부를 확인한다.") | ||
@ParameterizedTest | ||
@CsvSource({ | ||
"organ, true", | ||
"apple, false" | ||
}) | ||
void notContains(String input, boolean expected) { | ||
List<String> wordList = List.of("apple", "hello", "lemon"); | ||
Words words = new Words(wordList, LocalDate.now()); | ||
|
||
boolean notContains = words.notContains(input); | ||
|
||
assertThat(notContains).isEqualTo(expected); | ||
} |
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.
worldList
를 주입받음으로써 테스트하기 쉬워진것 같네요!
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.
yooth님 리뷰 하시느라 고생 많으셨습니다!
yooth님 걱정과는 다르게 저는 이번 리뷰에서 많은걸 배웠고, 덕분에 좋은 고민 포인트들과 내용들을 얻어서 다시 리팩토링을 해볼 기회를 얻었다고 생각합니다 ㅎㅎ
또한, 다양한 관점들을 얻을 수 있었어요! 저에게는 하나도 부족하지 않은 실력이었습니다!!
시간 되시면 확인 한번 더 부탁드려요! 👍
|
||
private boolean isOnlySameValue(Letter other) { | ||
return letters.stream() | ||
.anyMatch(letter -> (Objects.equals(letter.getValue(), other.getValue())) && letter.getPosition() != other.getPosition()); |
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.
일단은 고민끝에 답이 안나오는데다 position을 분리해보자니 대공사가 돼서, Letter
에 isOnlySameValue()
를 구현해주는 것으로 커밋했습니다 ~!
public TileService(TileStorage tileStorage) { | ||
this.tileStorage = tileStorage; | ||
} |
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.
워들 애플리케이션은 그 자체가 변화할 가능성이 매우 적고, 확장성을 고려하지 않아도 되는 상황이라 현재 상황에는 yooth님 말씀대로 내부에서 생성하는 것도 편리하고 좋을 것 같습니다!
하지만, 저는 다음과 같은 생각들 때문이 DI받는 것을 선호합니다.
TileService
에TileStorage
가 필요함을 명시적으로 알릴 수 있다 (숨기지 않는다)- 이건 개인 취향인 것 같은데, 저는 생성자에 해당 클래스는 a,b,c가 필요합니다라고 알려주는 것을 좋아합니다. 이를 통해 역할이 명확해지는 것 같다고 생각하고, 협력관계가 더 명확히 표현되어 좋은 것 같습니다.
- 테스트 하기 용이해진다. (
TileStorage
구현이 복잡하다면 더더욱 장점 증대) TileService
가 과연TileStorage
를 생성할 책임도 갖고 있는지도 생각합니다. 보통은 생성 책임은 다른데에 있는 것 같아요 😄 (아직 많은 경우를 만나보지 못한걸수도 있습니다.)- 그리고, 정말로 확장성이 없을까?도 생각합니다. 변경 가능성이 없다고 생각한 주민등록번호 정책이 변경됨으로써 대참사를 맞이했던 것 처럼... 어느정도의 확장성은 고려해두는게 좋지 않을까? 생각도 하는 것 같습니다. (그렇다고 너무 고려하면 복잡도만 증가하겠죠.. 무한 트레이드오프 😂)
반면에, Collection같은 것은 내부에
private final List<Obj> objs = new ArrayList<>();
와 같이 선언하기도 합니다.
yooth님 덕분에 아무생각 없이 습관처럼 해왔던 패턴을 다시한번 생각해보게 되었네요 👍 쥐어 짜내듯 생각한거라 어색할수도 있는데요! 어색한 부분이 있다면 언제든지 편하게 물어봐주세요!
private void processNoneMatchLetters(Letters answerLetters, Letters inputLetters, Tiles tiles) { | ||
Letters noneMatchingLetters = inputLetters.findNoneMatchingLetters(answerLetters); | ||
tiles.addGrayTile(noneMatchingLetters); |
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.
오 이런생각은 못했네요 ㅋㅋㅋ 너무너무너무 좋은 의견 같습니다. 바로 반영하였습니다 👍 저는 이생각은 전혀못하고 늪에 빠져있었는데, 구해주셔서 감사합니다 😊
Tiles
를 생성할 때 GRAY_TILE을 넣어주도록 변경했습니다
public void printTiles(List<Tiles> tiles) { | ||
for (Tiles tile : tiles) { | ||
System.out.println(tile); | ||
} | ||
} |
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.
너무 좋은것 같습니다 ㅋㅋ 특히, 표준 출력 블로킹을 줄일 수 있다는 관점 너무 좋네요 👍
반영 완료하였습니다 :) !!! 많이 배워가네요!!!
private boolean isEnd(Tiles tiles, int tryCount) { | ||
if (tileService.isAnswer(tiles)) { | ||
console.printResult(tryCount, TRY_COUNT_LIMIT, tileService.findAll()); | ||
return true; | ||
} |
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.
초기 구현 의도는 Tiles
가 과연 정답 여부를 아는게 맞을까? 에서 시작돼서 이렇게까지 됐던 것 같습니다. ㅎㅎ
지금은 Tiles
를 정답과 입력을 비교한 결과인 Result
로 변경했고, Result
를 List형태로 들고 있는 Results
까지 구현하게 됐으며, 이에 따라 자연스럽게 Result
에 isAnswer()
를 구현하도록 변경됐습니다!
@Override | ||
public String toString() { | ||
return String.join("", tiles); | ||
} |
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.
오 그런 주의사항이 있었군요! 덕분에 큰거 하나 배워갑니다~! 😄
메서드 이름을 뭘 할까? 하다가 Letters
에는 toWord()
로 변경했고, Result(구 Tiles)
에는 List<String> getTiles()
를 구현해서 Console
에서 직접 변경하도록 바꿔봤습니다. 의견 너무 감사합니다!
public TileService(TileStorage tileStorage) { | ||
this.tileStorage = tileStorage; | ||
} |
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
가 역할을 물려받았습니다 ㅋㅋ
느낀 점
XxxService
라는 익숙한 이름을 써버리게 되었습니다 😅 좀 더 구체적인 역할을 하는 이름을 떠올려보고 싶었는데, 아쉬웠습니다. 어쩌면 책임 분리를 잘못한 것 같기도 하고, 좀 더 세분화를 못한 것 같기도 하네요. 아님 알고리즘이 어려워서 역할을 나누기가 너무 어려웠다거나... 😓 원인은 다양하겠지만 근본적인 원인은 뭔가 설계에 있는 것 같습니다. DDD 강의를 들었어야 했는데배운 점
많은 시간을 투자한 부분