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 과제 제출 - kr #31

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

Conversation

ckr3453
Copy link

@ckr3453 ckr3453 commented Jun 16, 2024

  • 진행 방식

    • 페어 : 치즈, kr
    • 20분 마다 교체
    • 테크 살롱 오프라인, code with me + 게더
  • 느낀점

    • code with me를 처음써봤는데 cpu 온도가 100도가 넘어가고 프로세스 점유율이 100%가 넘어가는 등 꽤나 애먹었습니다.
    • 저의 잦은 야근으로 인해 페어와 시간을 맞추기가 힘들었습니다. 지금에 와선 새벽 시간에라도 진행하는걸 치즈님께 제안 했어야 했나.. 하는 아쉬움이 있습니다.
    • 시간이 없다보니 전체적인 그림을 그리지않고 기능단위로만 계속해서 개발한 것 같아서 아쉬웠습니다.
    • 저는 페어 프로그래밍이 이번에 처음이며, 테스트 코드 작성 경험도 많지 않습니다. 이러한 부분들을 페어이신 치즈님에게 많이 배울 수 있어서 좋았습니다.
    • 기능 개발 외적으로도 치즈님과 많은 얘기를 나눴는데요. 평소에도 개발을 좋아하시는 분이라고 느낄 수 있을 정도로 치즈님의 생각에서 많은 걸 배울 수 있었습니다.
    • 페어 프로그래밍을 할때 마다 서로 쉬는 시간도 가지지 않고 몰두할 정도로 너무 즐거웠습니다. 다음에 기회가 생긴다면 또 해보고싶네요.

ycheese added 30 commits June 9, 2024 14:30
Copy link

@hoa0217 hoa0217 left a comment

Choose a reason for hiding this comment

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

안녕하세요 kr님!! 늦은 리뷰드려서 죄송합니다.

미션 진행하시느라 고생하셨어요~~ 몇가지 저의 생각을 남겨보았는데 보시고 궁금하신 점이나 이야기 나누고싶은 부분은 코멘트 또는 DM주세요!!

감사합니다 😊

this.console = new Console();
this.answer = new Answer();
this.roundResults = new StringBuilder();
this.currentRound = 0;
Copy link

Choose a reason for hiding this comment

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

매직넘버는 상수로 빼면 어떨까요~? 또는 Config로 게임관련된 설정을 관리하고 있는 것 같은데 요런 부분도 한곳에 모아두면 좋을 것같네요!

currentRound++;
String input = console.userInput();
round = new Round(input);
roundResults.append(round.roundResult(answer)).append("\n");
Copy link

Choose a reason for hiding this comment

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

"\n"를 더해주는건 UI적 요소인거같은데 Console이 책임을 갖는게 더 좋지않을까 싶은데 어떻게 생각하시나요?🙂

Comment on lines +3 to +6
public enum Tile {
GREEN("🟩"),
YELLOW("🟨"),
GRAY("⬜️");
Copy link

Choose a reason for hiding this comment

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

게임 결과를 Enum으로 정의하셨네요~~ 다형성을 활용하는 것 너무 좋습니다.
하지만 위에서 말씀드린것처럼 UI와의 결합이 높아보여요!

Comment on lines +14 to +16
if (InputValidator.isNotValid(input)) {
throw new IllegalArgumentException();
}
Copy link

Choose a reason for hiding this comment

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

input에 대한 검증 책임을 InputValidator라는 유틸성 클래스에게 주고있는데, VO(값객체)를 생성하여 스스로 검증할 수 있도록 만들어주는건 어떨까요? 그러면 더욱 응집도가 높아질거같아요~ 그리고 IllegalArgumentException 을 공통적으로 던지는것 같은데 custom Exception을 통해 케이스를 좀 나눠보는건 어떨까요??

Comment on lines +15 to +18
protected Answer(String value) {
this.value = value;
countPerCharacter();
}
Copy link

Choose a reason for hiding this comment

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

테스트코드에서만 사용되는 생성자이기때문에 protected로 정의한걸까용?

private final StringBuilder result;

public RoundResult() {
this.countPerTile = new EnumMap<>(Tile.class);
Copy link

Choose a reason for hiding this comment

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

EnumMpa의 활용 너무 좋습니다~

Comment on lines +26 to +29
@Override
public String toString() {
return result.toString();
}
Copy link

Choose a reason for hiding this comment

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

toString을 Tile과 RoundResult에 정의해주셨는데 이부분도 UI와의 결합이 높아보여요~ 😢

Comment on lines +31 to +33
private static boolean isNotInWords(String input) {
return !words.contains(input);
}
Copy link

Choose a reason for hiding this comment

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

메서드명에 부정어구를 넣는것 보다 긍정어구를 넣는게 가독성이 더 좋습니다!
https://latte1114.tistory.com/519

Comment on lines +18 to +21
while (InputValidator.isNotValid(input)) {
System.out.println("다시 입력해주세요.");
input = sc.nextLine();
}
Copy link

Choose a reason for hiding this comment

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

Console에서 input이 비지니스 규칙을 지키고 있는지 검증하는 것 같은데, 이를 Console에서 할 필요가 있을까요~?
표현계층에서는 데이터 포맷정도만 검증해도 괜찮을것같아요~~

void 입력_null() {
Assertions.assertTrue(InputValidator.isNotValid(null));
}

Copy link

Choose a reason for hiding this comment

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

@NullAndEmptySource의 사용법을 아시나요~~?

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