-
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
[4기][3주차] Wordle 과제 제출 - 와제 #20
base: main
Are you sure you want to change the base?
Conversation
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.
RCA 룰을 사용하여 리뷰를 진행하였습니다 😊
리뷰에 이상한 점이 있다면 말씀해주세요 🙇🏻♂️
(평소에는 Github에서 직접 코멘트를 다는데, 이번엔 IntelliJ에서 달아보았더니 약간 이상하네요😢)
src/main/java/domain/Words.java
Outdated
public Words(String... words){ | ||
this(Arrays.stream(words) | ||
.map(Word::new) | ||
.collect(Collectors.toList())); | ||
} |
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.
a: 한가지 궁금한 점이 있습니다!
IOUtils#readFromResource
에서 List<String>
를 String[]
로 변환하신 이유가 혹시 무엇인지 궁금합니다!
IOUtils#readFromResource
에서 리스트를 그대로 반환받아도 해당 생성자에서 List<String>
을 인자로 받는다면 동일하게 처리할 수 있을 것 같은데, 제가 모르는 무언가의 꿀팁이 있는건가요!? 😮😮
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.
기본 생성자 하나만 두고 쓰기 위해서 매개변수 타입을 다르게 하려고 배열로 받았었습니다:) 말씀 주신 피드백대로 한 번 반영해보려고 정적 팩터리 메서드로 수정해봤습니다!!! 흠 어떤게 더 좋은 방법인지에 대해 고민해보고 싶은데 알렉스님 생각도 궁금합니다🤔
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.
아 저는 기존 기본 생성자의 인자를 String []
이 아닌 List<String>
로 받아도 되지 않나? 라는 생각이었습니다!!
또한 개인적인 생각으로는 필요하다면 생성자는 여러개가 있어도 괜찮지 않을까라는 의견입니다 :)
알렉스님! 리뷰 정성스럽게 적어주셔서 감사합니다🙇🏻♂️ |
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.
몇가지 코멘트를 남겨보았습니다 :)
전체적으로 잘 반영해주신 것 같네요 😊
고생하셨습니다 👍🏻👍🏻
src/main/java/domain/Words.java
Outdated
return words.contains(word); | ||
} | ||
|
||
public Word answer(LocalDate from){ |
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.
혹시 해당 내용에 관련된 레퍼런스를 공유해주실 수 있으신가요?
저는 처음 보는 방법이라 매우 궁금하네요 😊😊
try { | ||
List<String> words = Files.readAllLines(Paths.get(txtUrl.toURI())); | ||
return words; | ||
} catch (IOException e) { | ||
System.out.println("파일을 읽는도중 문제가 발생했습니다."); | ||
throw new RuntimeException(); | ||
} catch (URISyntaxException e) { | ||
System.out.println("존재하지 않는 경로 입니다."); | ||
throw new RuntimeException(); |
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.
a: 따로 말씀 안드려도 평소에는 잘 하실거 같아서 a 레벨로 코멘트 남깁니다 😊
catch문에서 유의미한 Custom Exception으로 변경하여 throw 하는 것도 좋은 방법인 것 같습니다!!
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.
Color들의 집합(게임 1라운드의 결과)을 일급 컬렉션으로 관리해주셨네요 👍🏻👍🏻 💯
@Test | ||
void getArrayLocation(){ | ||
//given | ||
Words words = Words.of(Arrays.asList("aaaaa", "bbbbb")); | ||
|
||
//when | ||
Word answers = words.answer(LocalDate.of(2021, 6, 20)); | ||
|
||
//then | ||
Assertions.assertEquals(answers, new Word("bbbbb")); | ||
} | ||
|
||
@Test | ||
void validation() { | ||
Words words = Words.of(Arrays.asList("aaaaa")); | ||
Assertions.assertThrows(IllegalArgumentException.class, () -> { | ||
words.matchingAnswer(new Word("bbbbb")); | ||
}); | ||
} |
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.
여기도 다른 테스트들과 일관되게 테스트에 대한 설명을 @DisplayName
으로 달아줘도 좋을 것 같습니다 :)
추가로 테스트 계층구조에 대해서도 참고해보시면 좋을 것 같네요 😊
@Test | |
void getArrayLocation(){ | |
//given | |
Words words = Words.of(Arrays.asList("aaaaa", "bbbbb")); | |
//when | |
Word answers = words.answer(LocalDate.of(2021, 6, 20)); | |
//then | |
Assertions.assertEquals(answers, new Word("bbbbb")); | |
} | |
@Test | |
void validation() { | |
Words words = Words.of(Arrays.asList("aaaaa")); | |
Assertions.assertThrows(IllegalArgumentException.class, () -> { | |
words.matchingAnswer(new Word("bbbbb")); | |
}); | |
} | |
@Test | |
@DisplayName("....") | |
void getArrayLocation(){ | |
//given | |
Words words = Words.of(Arrays.asList("aaaaa", "bbbbb")); | |
//when | |
Word answers = words.answer(LocalDate.of(2021, 6, 20)); | |
//then | |
Assertions.assertEquals(answers, new Word("bbbbb")); | |
} | |
@Test | |
@DisplayName("....") | |
void validation() { | |
Words words = Words.of(Arrays.asList("aaaaa")); | |
Assertions.assertThrows(IllegalArgumentException.class, () -> { | |
words.matchingAnswer(new Word("bbbbb")); | |
}); | |
} |
페어프로그래밍