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

[3주차] Wordle 과제 제출 - 홀든 #17

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

Conversation

seung-00
Copy link

@seung-00 seung-00 commented Apr 2, 2023

pair: Alex 님

후기

  • 페어 프로그래밍에 대한 배움
    • 페어 프로그래밍은 어떤 식으로 진행되는가, 안티 패턴에는 어떤 것이 있는가
  • 좋은 코드에 대한 고민을 공유하고 실시간으로 함께 해결하는 경험

@seung-00 seung-00 changed the title [4기] [3번 과제] Wordle 과제 제출 - 홀든 [3주차] Wordle 과제 제출 - 홀든 Apr 2, 2023
Copy link

@rkd5656 rkd5656 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 26 to 28
public boolean isClear() {
return isClear;
}
Copy link

Choose a reason for hiding this comment

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

isClear 보단 위의 isGameOver 처럼 isGameClear로 명명하시는게 어떨까요?
WordleGameUI에서 isClear를 사용하시는데 Storage가 비어 있다는 건가?
라는 착각으로 여기까지 들어와서 무엇인지 찾아봤거든요 ㅎㅎ

Comment on lines 22 to 30
public void printResult(List<String[]> gameResult) {
if (wordleGameStorage.isClear()) {
System.out.println(wordleGameStorage.getRestChance() + "/" + WordleGame.TOTAL_CHANCE);
}

gameResult.stream()
.map(array -> String.join(" ", array))
.forEach(System.out::println);
}
Copy link

Choose a reason for hiding this comment

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

저도 현재 막 넥스트 스텝 TDD 강의를 입장으로서 미개한 생각이지만은...

이 부분으로 인하여 WordleGameStorage를 가지고 있는 거 같은데

제 생각엔 UI는 단 순 UI 책임만 가지고 있고, Storage를 가지고 있을 필요 없이 UI 로직만 풀면 좋을 것 같아요!

그래서 이 문제를 해결하기 위해 List<String[]> gameResultGameResult란 객체로 받는 건 어떨까요?

GameResult 객체는 게임 클리어 여부, 남은 횟수, 그리고 게임 결과 문자열 까지도 넣어
게임 결과에 대한 책임을 다 여기에 넘기면 어떨까 싶어요.

그리고 현재 24 라인을 보면 남은 횟수 / 전체 횟수 로 보이는데 사용 횟수 / 전체 횟수 가 요구 사항인 걸로 알고 있습니다!

@@ -0,0 +1,10 @@
package woowaapplication.pair.game.wordle.exception;

public class InvalidAnswerKeywordException extends RuntimeException {
Copy link

Choose a reason for hiding this comment

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

전반적으로 Exception을 따로 나눈게 깔끔해서 좋았어요! ㅎㅎ


@DisplayName("워들 인수 테스트")
public class WordleGameAcceptanceTest {

Copy link

Choose a reason for hiding this comment

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

제가 테스트 관련해선 이제 막 넥스트 스텝 강의 듣고 있어서 완전 초짜인데...

완전 배워 갑니다. 저도 하면서 John 님한테 정말 많이 배웠는데 이렇게 케이스 별로 묶는 것도 있군요.
한수 배웁니다.

Comment on lines 56 to 61
private Scanner ready() {
Scanner sc = new Scanner(System.in);
WordleGameUI.printReady();

return sc;
}
Copy link

Choose a reason for hiding this comment

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

ScannerWordleGameUI 로 넘겨서 static으로 꺼내쓰게 하고

모든 UI관련 된 책임을 지게 하는게 좋아 보입니다. ㅎㅎ


* [Alex 님의 repository](https://github.com/giibeom/study-java-wordle) 에서 페어 프로그래밍을 진행했습니다.
* 이 repository 는 해당 repository 를 미러링했습니다.
* 진행 방식은 해당 [repository wiki](https://github.com/giibeom/study-java-wordle/wiki) 에 정리되어 있습니다.
Copy link

Choose a reason for hiding this comment

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

와우 ... 이걸 wiki로... 문서 정리가 정말 깔끔하고 트렌디한 느낌이네요..
한수 배웁니당

Comment on lines 12 to 18
if (!isAlphabetic(keyword)) {
throw new InvalidInputKeywordException();
}

if (!isLengthValid(keyword, lengthLimit)) {
throw new InvalidInputKeywordException();
}
Copy link

Choose a reason for hiding this comment

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

정규식으로 하나로 합칠 수 있지 않을까 싶어요!


import woowaapplication.pair.game.wordle.WordleGame;

public class Application {
Copy link

Choose a reason for hiding this comment

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

wordle 패키지 안의 클래스 들이 패키지로 구분이 되었으면 좋겠어요!

}

public List<String[]> playRound(String inputKeyword) {
String answerKeyword = getAnswerKeyword();
Copy link

Choose a reason for hiding this comment

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

이 부분은 run을 할때마다 실행하는 것으로 보이는데
이렇게 되면 WordleGame 에서 while 문이 실행하는 동안
파일을 매번 읽어오게 되는거 같애요

성능적인면은 둘째 치더라도 어짜피 같은 것이기 떄문에 최초에 게임 실행 시에만
읽는 것이 좋아보여요.

또한, 혹시나 그럴일은 정말 작지만... 게임 실행을 23시 59분에 해서 다음날 00시 01분에 끝난다면..
문제를 푸는 도중에 단어가 바뀌지 않을까 싶어요.


public WordleGameStorage(Coin coin) {
this.coin = coin;
this.isClear = false;
Copy link

Choose a reason for hiding this comment

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

boolean default는 어차피 false라서 초기화 안 해주셔도 될 것 같아요!

seung-00 and others added 6 commits April 16, 2023 17:13
* 클래스간 데이터 이동을 위한 GameResultDto 를 추가했습니다.
* wordleGameUI 에서 WordleGameStorage 에 종속적이었던 부분을 제거했습니다.
* 테스트 코드를 수정했습니다.
* input 을 받는 로직을 WordleGameIO 로 분리했습니다.
* answerKeyword 받아오는 로직이 반복 실행되지 않도록 수정헀습니다.
@seung-00
Copy link
Author

@rkd5656
스페셜님~ 디테일한 리뷰 감사합니다! 많은 도움이 됐습니다 ☺️

seung-00#1
수정한 부분을 간단히 설명드리면 아래와 같습니다~

  • WordleGameService 가 스프링 레이어드 아키텍처를 의식하다 보니 생긴 불필요한 클래스고, 뎁스만 증가시킨다고 느껴서요. 정답 단어 뽑아오는 부분을 유틸리티 클래스로 분리하고, WordleGameService 를 제거했습니다.
  • GameResultDto 추가했습니다
  • WordleGameUI -> WordleGameIO 로 네이밍을 변경하고, WordleGameIO 에서 서비스 종속적인 부분을 제거했습니다
  • dto, util 패키지를 추가해 패키지 구조를 조금 더 구체적으로 변경했습니다.

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