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기] 3주차 Wordle 과제 제출 - 딱구 #35

Open
wants to merge 58 commits into
base: main
Choose a base branch
from

Conversation

SeolYoungKim
Copy link

느낀 점

  • 생각보다 구현이 매우 어려웠습니다. 알고리즘을 짜는것 자체가 첫 난관이었는데, 생각보다 잘 되지 않아서 돌고 돌았습니다. 그러다 싸무엘님이 오프라인으로 만나서 하자고 제안을 해주셨고, 화이트보드에 그려가면서 함께 설계 해나가니까 문제가 더 잘 해결되는 느낌이었습니다. 온라인으로 할 때 보다 오프라인으로 만나서 하는 것이 더 소통하기 쉽다는 것을 깨달았습니다.
  • 역할에 맞는 객체를 찾아내는 과정이 어려웠습니다. 이 역할을 수행하는 객체는 무엇이 좋지? 하다가 결국은 XxxService라는 익숙한 이름을 써버리게 되었습니다 😅 좀 더 구체적인 역할을 하는 이름을 떠올려보고 싶었는데, 아쉬웠습니다. 어쩌면 책임 분리를 잘못한 것 같기도 하고, 좀 더 세분화를 못한 것 같기도 하네요. 아님 알고리즘이 어려워서 역할을 나누기가 너무 어려웠다거나... 😓 원인은 다양하겠지만 근본적인 원인은 뭔가 설계에 있는 것 같습니다. DDD 강의를 들었어야 했는데
  • 싸무엘님이 극 T라고 하셔서 약간은 기대했는데(?) 소프트스킬이 매우 좋으셔서 편안하게 대화를 주고받을 수 있었습니다. 사실은 극 T셨어도 별일은 없었을거지만..ㅋㅋㅋ
  • 페어프로그래밍이 과한 매몰을 방지하는 효과도 있는 것 같습니다. 매몰된다 싶을 때 15분이 끝나서 바로 넘겨드려야 하더라고요. 너무 짧은 것 같긴 했지만 길게 했다면 과연 더 좋았을까? 라는 의문이 들기는 합니다

배운 점

  • 무언가에 매몰될 때는 조금 쉬었다가 생각의 전환을 하는 것이 중요하다는 것을 배웠습니다. 싸무엘님은 이 부분을 상당히 잘 해주셨는데, 더 좋은 방향으로 나아가는 것을 정말 잘 도와주셨습니다. 저도 앞으로는 너무 매몰되는 것 같을 때는 한숨 쉬고 큰그림을 보는 연습을 해야할 것 같습니다.
  • 생각을 잘 말하는 방법을 배웠습니다. 물론, 싸무엘님은 제가 이상하게 말해도 잘 알아들어 주셔서 말하는 것이 쉬웠지만, 저는 제가 아는것을 말로 설명하는 것과 원하는 것을 말로 설명하는 것을 잘 못하는 것 같다는 점을 알았습니다. 잘 못하는 부분을 알게 되었으니 앞으로는 어떻게 개선할 수 있을지를 고민해봐야 겠습니다. (잘 안될땐 그려가면서 말하는게 짱이다)

많은 시간을 투자한 부분

  • 아무래도 Wordle 알고리즘을 구현하는 부분이 가장 오래걸렸던 것 같습니다. 그 다음으로는 설계가 좀 오래걸렸네요. 적절히 역할을 나누고 그에 맞는 객체를 찾는 과정도 오랜 시간을 투자했습니다.

samkim-jn and others added 30 commits June 6, 2024 11:06
Copy link

@djawnstj djawnstj 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 11 to 17
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);
}
}

Choose a reason for hiding this comment

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

LettersLetter 제네릭 타입의 1급 컬렉션으로 보여지네요!👍🏻
저는 개인적으로 외부에서 for 문을 사용하는 1급 컬렉션이라면 Iterable 를 구현하는 방법도 좋다고 생각하는데 어떻게 생각하시나요?

Suggested change
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);
}
}

Copy link
Author

@SeolYoungKim SeolYoungKim Jun 25, 2024

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문은 자주 사용하지 않거든요!

이를 여쭙는 이유는, 내가 미처 생각하지 못한 주의사항이 있을까?라는 노파심에 여쭤봅니다 ㅎㅎ 혹시 없다면 회사 코드에도 적용해보려고 해요 ㅎㅎ

(워들에는 적용 했습니다 😁)

Comment on lines 27 to 29
public List<Letter> getLetters() {
return letters;
}

Choose a reason for hiding this comment

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

컬렉션을 그대로 반환하는 이유가 있을까요?
컬렉션을 반환할 땐 완전 직렬화는 못 해도 Collections.unmodifiableList(letters); 처럼 깊은 복사를 한번 해주는 게 좋을 것 같아요!

Copy link
Author

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());

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() 경우가 같다고 생각합니다!

Copy link
Author

@SeolYoungKim SeolYoungKim Jun 25, 2024

Choose a reason for hiding this comment

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

부끄럽지만 이때 Letter에 메서드를 구현하려고 네이밍을 이래저래 생각하다가, "아 일단 구현이 중요하니 이렇게 하고 넘어갑시다"했던 기억이 있네요 😓

저도 yooth님 의견에 동의합니다.

그런데, 네이밍 관련해서 고민인 부분이 있습니다. ㅋㅋ
이는 회사에서도 팀원들과 고민하는 부분인데,

  • Letterequals()position, value가 모두 동일하다
  • 그러면 postion만 같은 메서드는 뭐라고 할까?
    • hasEqualPosition()
    • equalPosition()
    • hasSamePosition()
    • isOnlySamePosition()

등등 다양한 의견이 오고갔지만 여전히 좋은 이름을 찾아 헤매는 중입니다 ㅋㅋㅋ has를 써야할까 is를 써야할까도 고민이네요 (쓸데없는 고민일수 있지만욤😅)

작성하면서 생각해보니 Letterposition이 와버려서 생기는 어색함이 아닌가? 라는 생각도 드네요 ㅋㅋㅋ Letter에 어울리지 않는 필드가 있는게 아닌지, Class 이름이 어색한건지는 고민을 좀 더 해보겠습니다. ㅋㅋㅋ

Copy link
Author

Choose a reason for hiding this comment

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

일단은 고민끝에 답이 안나오는데다 position을 분리해보자니 대공사가 돼서, LetterisOnlySameValue()를 구현해주는 것으로 커밋했습니다 ~!

Comment on lines 12 to 14
public TileService(TileStorage tileStorage) {
this.tileStorage = tileStorage;
}

Choose a reason for hiding this comment

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

TileStorage 를 주입 받은 이유가 있나요??

저는 백엔드 개발 경험은 적고 비교적 앱 개발을 주로 하다 보니 무조건 의존 주입을 받는 것 보다 상황에 따라 내부에서 생성하는 것도 좋다고 생각합니다.
주로 생각하는 기준은 '확장 가능성이 있는가?' 를 고민하는 것 같아요.

당장 추상화를 하진 않아도 충분히 확장 가능성이 있다면 주입을 받도록 하는 것 같습니다.
싸딱팀의 WordsReader 역할이 이 경우라고 생각해요.

딱구님은 어떻게 생각하시나요?

Copy link
Author

Choose a reason for hiding this comment

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

워들 애플리케이션은 그 자체가 변화할 가능성이 매우 적고, 확장성을 고려하지 않아도 되는 상황이라 현재 상황에는 yooth님 말씀대로 내부에서 생성하는 것도 편리하고 좋을 것 같습니다!

하지만, 저는 다음과 같은 생각들 때문이 DI받는 것을 선호합니다.

  • TileServiceTileStorage가 필요함을 명시적으로 알릴 수 있다 (숨기지 않는다)
    • 이건 개인 취향인 것 같은데, 저는 생성자에 해당 클래스는 a,b,c가 필요합니다라고 알려주는 것을 좋아합니다. 이를 통해 역할이 명확해지는 것 같다고 생각하고, 협력관계가 더 명확히 표현되어 좋은 것 같습니다.
  • 테스트 하기 용이해진다. (TileStorage 구현이 복잡하다면 더더욱 장점 증대)
  • TileService가 과연 TileStorage를 생성할 책임도 갖고 있는지도 생각합니다. 보통은 생성 책임은 다른데에 있는 것 같아요 😄 (아직 많은 경우를 만나보지 못한걸수도 있습니다.)
  • 그리고, 정말로 확장성이 없을까?도 생각합니다. 변경 가능성이 없다고 생각한 주민등록번호 정책이 변경됨으로써 대참사를 맞이했던 것 처럼... 어느정도의 확장성은 고려해두는게 좋지 않을까? 생각도 하는 것 같습니다. (그렇다고 너무 고려하면 복잡도만 증가하겠죠.. 무한 트레이드오프 😂)

반면에, Collection같은 것은 내부에 private final List<Obj> objs = new ArrayList<>();와 같이 선언하기도 합니다.

yooth님 덕분에 아무생각 없이 습관처럼 해왔던 패턴을 다시한번 생각해보게 되었네요 👍 쥐어 짜내듯 생각한거라 어색할수도 있는데요! 어색한 부분이 있다면 언제든지 편하게 물어봐주세요!

Copy link
Author

Choose a reason for hiding this comment

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

참고해 주세요:
🥹 이친구는 역사속으로 사라졌습니다. Answer가 역할을 물려받았습니다 ㅋㅋ

Comment on lines 54 to 56
private void processNoneMatchLetters(Letters answerLetters, Letters inputLetters, Tiles tiles) {
Letters noneMatchingLetters = inputLetters.findNoneMatchingLetters(answerLetters);
tiles.addGrayTile(noneMatchingLetters);

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 을 추가하는 로직이 필요 없어질 것 같은데 어떻게 생각하시나요?

Copy link
Author

Choose a reason for hiding this comment

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

오 이런생각은 못했네요 ㅋㅋㅋ 너무너무너무 좋은 의견 같습니다. 바로 반영하였습니다 👍 저는 이생각은 전혀못하고 늪에 빠져있었는데, 구해주셔서 감사합니다 😊

  • Tiles를 생성할 때 GRAY_TILE을 넣어주도록 변경했습니다

Comment on lines +46 to +60
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;
}

Choose a reason for hiding this comment

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

start() 메서드와 함께 2중 while 구조는 생각 못 했는데 좋은 것 같네요!

Comment on lines 76 to 80
private boolean isEnd(Tiles tiles, int tryCount) {
if (tileService.isAnswer(tiles)) {
console.printResult(tryCount, TRY_COUNT_LIMIT, tileService.findAll());
return true;
}

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()TileServiceisAnswer() 에서만 호출되는 것 같은데
Tiles 에게 바로 isAnswer() 를 물어봐도 될 것 같은데 이렇게 한 이유가 궁금합니다.

Copy link
Author

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까지 구현하게 됐으며, 이에 따라 자연스럽게 ResultisAnswer()를 구현하도록 변경됐습니다!

Comment on lines 27 to 31
public void printTiles(List<Tiles> tiles) {
for (Tiles tile : tiles) {
System.out.println(tile);
}
}
Copy link

@djawnstj djawnstj Jun 25, 2024

Choose a reason for hiding this comment

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

개취인데 코틀린 러버로써 메서드 체이닝을 사용하는걸 선호합니다!
지금 애플리케이션에선 큰 차이는 없지만 표준 출력을 한번만 사용하게 됨으로 표준 출력 블로킹에서 조금의 이점도 있을 것 같구요ㅎㅎ

딱구님은 어떻게 생각하시나요?

Suggested change
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);
}

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 14 to 15
@ParameterizedTest
@MethodSource("provideResultData")

Choose a reason for hiding this comment

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

@MethodSource 신기하네요!
하나 배워갑니다.

Comment on lines 43 to 56
@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);
}

Choose a reason for hiding this comment

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

worldList 를 주입받음으로써 테스트하기 쉬워진것 같네요!

Copy link
Author

@SeolYoungKim SeolYoungKim left a 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());
Copy link
Author

Choose a reason for hiding this comment

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

일단은 고민끝에 답이 안나오는데다 position을 분리해보자니 대공사가 돼서, LetterisOnlySameValue()를 구현해주는 것으로 커밋했습니다 ~!

Comment on lines 12 to 14
public TileService(TileStorage tileStorage) {
this.tileStorage = tileStorage;
}
Copy link
Author

Choose a reason for hiding this comment

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

워들 애플리케이션은 그 자체가 변화할 가능성이 매우 적고, 확장성을 고려하지 않아도 되는 상황이라 현재 상황에는 yooth님 말씀대로 내부에서 생성하는 것도 편리하고 좋을 것 같습니다!

하지만, 저는 다음과 같은 생각들 때문이 DI받는 것을 선호합니다.

  • TileServiceTileStorage가 필요함을 명시적으로 알릴 수 있다 (숨기지 않는다)
    • 이건 개인 취향인 것 같은데, 저는 생성자에 해당 클래스는 a,b,c가 필요합니다라고 알려주는 것을 좋아합니다. 이를 통해 역할이 명확해지는 것 같다고 생각하고, 협력관계가 더 명확히 표현되어 좋은 것 같습니다.
  • 테스트 하기 용이해진다. (TileStorage 구현이 복잡하다면 더더욱 장점 증대)
  • TileService가 과연 TileStorage를 생성할 책임도 갖고 있는지도 생각합니다. 보통은 생성 책임은 다른데에 있는 것 같아요 😄 (아직 많은 경우를 만나보지 못한걸수도 있습니다.)
  • 그리고, 정말로 확장성이 없을까?도 생각합니다. 변경 가능성이 없다고 생각한 주민등록번호 정책이 변경됨으로써 대참사를 맞이했던 것 처럼... 어느정도의 확장성은 고려해두는게 좋지 않을까? 생각도 하는 것 같습니다. (그렇다고 너무 고려하면 복잡도만 증가하겠죠.. 무한 트레이드오프 😂)

반면에, Collection같은 것은 내부에 private final List<Obj> objs = new ArrayList<>();와 같이 선언하기도 합니다.

yooth님 덕분에 아무생각 없이 습관처럼 해왔던 패턴을 다시한번 생각해보게 되었네요 👍 쥐어 짜내듯 생각한거라 어색할수도 있는데요! 어색한 부분이 있다면 언제든지 편하게 물어봐주세요!

Comment on lines 54 to 56
private void processNoneMatchLetters(Letters answerLetters, Letters inputLetters, Tiles tiles) {
Letters noneMatchingLetters = inputLetters.findNoneMatchingLetters(answerLetters);
tiles.addGrayTile(noneMatchingLetters);
Copy link
Author

Choose a reason for hiding this comment

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

오 이런생각은 못했네요 ㅋㅋㅋ 너무너무너무 좋은 의견 같습니다. 바로 반영하였습니다 👍 저는 이생각은 전혀못하고 늪에 빠져있었는데, 구해주셔서 감사합니다 😊

  • Tiles를 생성할 때 GRAY_TILE을 넣어주도록 변경했습니다

Comment on lines 27 to 31
public void printTiles(List<Tiles> tiles) {
for (Tiles tile : tiles) {
System.out.println(tile);
}
}
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 76 to 80
private boolean isEnd(Tiles tiles, int tryCount) {
if (tileService.isAnswer(tiles)) {
console.printResult(tryCount, TRY_COUNT_LIMIT, tileService.findAll());
return true;
}
Copy link
Author

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까지 구현하게 됐으며, 이에 따라 자연스럽게 ResultisAnswer()를 구현하도록 변경됐습니다!

Comment on lines 40 to 43
@Override
public String toString() {
return String.join("", tiles);
}
Copy link
Author

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에서 직접 변경하도록 바꿔봤습니다. 의견 너무 감사합니다!

Comment on lines 12 to 14
public TileService(TileStorage tileStorage) {
this.tileStorage = tileStorage;
}
Copy link
Author

Choose a reason for hiding this comment

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

참고해 주세요:
🥹 이친구는 역사속으로 사라졌습니다. Answer가 역할을 물려받았습니다 ㅋㅋ

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.

3 participants