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 과제 제출 - 와제 #20

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

Conversation

akayj820
Copy link

@akayj820 akayj820 commented Apr 2, 2023

페어프로그래밍

  • 페어 멤버: jimbaemon
  • 페어 기간: 2023/03/25
  • 페어 방식: 기능 별로 네비게이터, 드라이버 체인지
  • 사용 툴: Code with me
  • 회고
    • 페어프로그래밍을 하다보면 혼자할 때보다 집중도가 조금 떨어질 것이라고 생각했었는데 오히려 그 반대였습니다.
      • 5시간 동안(중간에 30분 휴식) 서로 몰입하면서 기능 별로 페어를 주고 받을 수 있었습니다.
    • 페어를 주고 받으며 코딩에 너무 집중한 나머지 기능 단위로 커밋하는 걸 깜빡했습니다.
      • 다음 번에는 기능 요구 사항을 정리하면서 커밋에 대한 언급도 추가해서 네비게이터가 체크해야할 사항으로 추가하여 누락되지 않도록 하려고 합니다.

Copy link

@giibeom giibeom left a 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/controller/WordleController.java Outdated Show resolved Hide resolved
src/test/java/WordleTest.java Outdated Show resolved Hide resolved
src/test/java/WordleTest.java Show resolved Hide resolved
src/test/java/WordleTest.java Outdated Show resolved Hide resolved
src/test/java/WordsTest.java Outdated Show resolved Hide resolved
src/main/java/domain/Words.java Outdated Show resolved Hide resolved
Comment on lines 14 to 18
public Words(String... words){
this(Arrays.stream(words)
.map(Word::new)
.collect(Collectors.toList()));
}
Copy link

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>을 인자로 받는다면 동일하게 처리할 수 있을 것 같은데, 제가 모르는 무언가의 꿀팁이 있는건가요!? 😮😮

Copy link
Author

Choose a reason for hiding this comment

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

기본 생성자 하나만 두고 쓰기 위해서 매개변수 타입을 다르게 하려고 배열로 받았었습니다:) 말씀 주신 피드백대로 한 번 반영해보려고 정적 팩터리 메서드로 수정해봤습니다!!! 흠 어떤게 더 좋은 방법인지에 대해 고민해보고 싶은데 알렉스님 생각도 궁금합니다🤔

Copy link

Choose a reason for hiding this comment

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

아 저는 기존 기본 생성자의 인자를 String []이 아닌 List<String>로 받아도 되지 않나? 라는 생각이었습니다!!
또한 개인적인 생각으로는 필요하다면 생성자는 여러개가 있어도 괜찮지 않을까라는 의견입니다 :)

src/main/java/ui/ResultView.java Show resolved Hide resolved
src/main/java/ui/ResultView.java Outdated Show resolved Hide resolved
@akayj820
Copy link
Author

알렉스님! 리뷰 정성스럽게 적어주셔서 감사합니다🙇🏻‍♂️
피드백 주신 내용들 반영해보았습니다🙏

Copy link

@giibeom giibeom left a comment

Choose a reason for hiding this comment

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

몇가지 코멘트를 남겨보았습니다 :)
전체적으로 잘 반영해주신 것 같네요 😊
고생하셨습니다 👍🏻👍🏻

src/main/java/controller/WordleController.java Outdated Show resolved Hide resolved
return words.contains(word);
}

public Word answer(LocalDate from){
Copy link

Choose a reason for hiding this comment

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

혹시 해당 내용에 관련된 레퍼런스를 공유해주실 수 있으신가요?
저는 처음 보는 방법이라 매우 궁금하네요 😊😊

Comment on lines +18 to +26
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();
Copy link

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 하는 것도 좋은 방법인 것 같습니다!!

Copy link

Choose a reason for hiding this comment

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

Color들의 집합(게임 1라운드의 결과)을 일급 컬렉션으로 관리해주셨네요 👍🏻👍🏻 💯

Comment on lines +12 to +30
@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"));
});
}
Copy link

Choose a reason for hiding this comment

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

여기도 다른 테스트들과 일관되게 테스트에 대한 설명을 @DisplayName으로 달아줘도 좋을 것 같습니다 :)
추가로 테스트 계층구조에 대해서도 참고해보시면 좋을 것 같네요 😊

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

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.

2 participants