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

[4기][3주차] Wordle 과제 제출 - 분수 #25

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

Conversation

Saerang
Copy link

@Saerang Saerang commented Apr 3, 2023

페어: 하현님

Copy link

@MoonHKLee MoonHKLee 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 54 to 92
/*@DisplayName("그린 1개, 노란색 1개, 회색 3개인 답안을 비교한다.")
@Test
void test01() {
CorrectAnswer correctAnswer = new CorrectAnswer(new Word("spill"));

List<Tile> tiles = correctAnswer.compare(new Word("hello"));

assertThat(tiles).containsExactly(Tile.GRAY, Tile.GRAY, Tile.YELLOW, Tile.GREEN, Tile.GRAY);
}

@DisplayName("그린 1개, 노란색 1개, 회색 3개인 답안을 비교한다.")
@Test
void test02() {
CorrectAnswer correctAnswer = new CorrectAnswer(new Word("spill"));

List<Tile> tiles = correctAnswer.compare(new Word("label"));

assertThat(tiles).containsExactly(Tile.YELLOW, Tile.GRAY, Tile.GRAY, Tile.GRAY, Tile.GREEN);
}

@DisplayName("입력된 답안을 존재하는 Word 인지 비교한다.")
@Test
void test03() {
CorrectAnswer correctAnswer = new CorrectAnswer(new Word("spill"));

List<Tile> tiles = correctAnswer.compare(new Word("spell"));

assertThat(tiles).containsExactly(Tile.GREEN, Tile.GREEN, Tile.GRAY, Tile.GREEN, Tile.GREEN);
}

@DisplayName("입력된 답안을 존재하는 Word 인지 비교한다.")
@Test
void test04() {
CorrectAnswer correctAnswer = new CorrectAnswer(new Word("spill"));

List<Tile> tiles = correctAnswer.compare(new Word("spill"));

assertThat(tiles).containsExactly(Tile.GREEN, Tile.GREEN, Tile.GREEN, Tile.GREEN, Tile.GREEN);
}*/

Choose a reason for hiding this comment

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

사용하지 않는 코드가 남아있는 것 같네요.

Choose a reason for hiding this comment

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

Controller, Service 네이밍을 통해서 MVC 패턴을 적용했다는 의도가 한번에 느껴지네요.👍

Choose a reason for hiding this comment

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

FileConfig 클래스에서 file 관련 설정을 관리하셨네요. 이렇게 하면 추후 파일관련 내용이 추가되더라도 응집도 높은 코드를 작성할 수 있겠네요.😃


public class InputView {

public static final int GAME_TOTAL_ROUND = 6;

Choose a reason for hiding this comment

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

GAME_TOTAL_ROUND 상수는 InputView 보다는 GameService 클래스에서 관리하는게 더 좋아보이는데 어떻게 생각하시나요?

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 +11 to +15
try {
return Files.readAllLines(Paths.get(filePath));
} catch (IOException e) {
throw new RuntimeException("파일을 읽을 수 없습니다.", e);
}

Choose a reason for hiding this comment

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

파일이 없는 경우에 발생하는 NoSuchFileException과 같이 구체적인 예외를 작성해보는건 어떨까요?

Copy link
Author

Choose a reason for hiding this comment

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

음 이번 프로젝트에는 있는 예외를 사용하려고 했습니다.


private static final String INPUT_WORD_MESSAGE = "정답을 입력해주세요.";

private static final Scanner scanner = new Scanner(System.in);

Choose a reason for hiding this comment

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

Scanner가 게임 종료후에도 close 선언을 하지 않은것 같습니다.
scanner 리소스를 닫아주는것은 어떨까요?

Copy link
Author

Choose a reason for hiding this comment

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

오 scanner 닫을 생각을 못했는데 ..

Copy link
Author

Choose a reason for hiding this comment

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

근데 고민이 되는게, static 이라서 닫으려면 쓸때마다 쓰고 닫고 해야할 것 같은데 고민이군요.
다른곳들은 안닫더라구요.

Comment on lines +22 to +30
Long count = letterMap.get(answer.getWord().get(i));

Tile tile = getTile(count, this.answer.getWord().get(i), answer.getWord().get(i));
result.add(tile);

letterMap.put(answer.getWord().get(i), letterMap.getOrDefault(answer.getWord().get(i), 0L) - 1);
}
endGame(result);

Choose a reason for hiding this comment

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

중복되는 값을 변수 선언하여 재사용을 할 수 있을것 같습니다.

Copy link
Author

@Saerang Saerang Apr 11, 2023

Choose a reason for hiding this comment

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

음 너무 길어져서 10줄이 넘어서 ㅋㅋㅋ 재선언 안하긴 했습니다.
다른방법으로 구현을 리팩터링 해야할 것 같네요.

Choose a reason for hiding this comment

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

enum을 활용해서 각각의 종류에 맞는 이모지를 할당하셨세요.
깔끔하고 명확안 코드인것 같습니다.😃

}

private void endGame(List<Tile> result) {
int count = Collections.frequency(result, Tile.GREEN);

Choose a reason for hiding this comment

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

이런 Collections 메서드가 있는줄 처음 알았네요. 덕분에 배워갑니다!❤

Choose a reason for hiding this comment

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

일급컬렉션을 활용하셨네요!😀

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