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 과제 제출 - 윤백 #23

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

Conversation

yunbaek
Copy link

@yunbaek yunbaek commented Apr 2, 2023

3주차 페어프로그래밍 과제 제출입니다

페어 프로그래밍이 처음이라 어렵기도 하고 재밌기도 했습니다.
페어프로그래밍을 하면 논의하는데에 시간을 많이 쓰게 되어 코딩 시간이 길어질 수 있을 것 같았지만,
혼자서 쉽게 결정하지 못했을 것 같은 내용을 공유하고 함께 고민할 수 있어
결과물은 혼자 하는 것 보다 더 좋았다고 생각됩니다.

yunbaek and others added 30 commits March 24, 2023 23:42
- 디렉토리 경로에서 파일을 읽어와 문자열 list로 반환하는 기능 구현.
- 파일에서 요구사항 로직에 해당하는 단어를 가져오는 기능 구현.
- 테스트 wordPath 전역변수처리
- 이름 직관적으로 변경
- 생성자 필요없는 값 노출 제거
Copy link

@mrlee7 mrlee7 left a comment

Choose a reason for hiding this comment

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

PR 리뷰가 늦었습니다.. 🙇
3차 과제 리뷰 시작일에 리뷰 드리고 싶은 마음이 컸다는 점..
윤백님의 코드로 많은 공부가 되었습니다 ✍️ 📖 📚
코멘트 남겨주시면, 꾸준히 확인하겠습니다. 고생 많으셨습니다 !! bbb


### 페어 프로그래밍 진행 사항
#### 1회차
- 사용 기술
Copy link

Choose a reason for hiding this comment

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

사용 기술에 대해 디테일하게 명시해주신 점 너무 좋습니다 👍


public void start(LocalDate currentDate) {
inputView.printStartMessage();
for (int i = 0; i < 6; i++) {
Copy link

Choose a reason for hiding this comment

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

이 부분은 Round 클래스로 분리하여 사용할 수도 있을 것 같아요 ! 😄

Round 클래스로 분리하여

private static final int MAX_ROUND = 6;

과 같이 명시해준다면 더욱 명확하게 읽을 수 있고 책임도 명확해질 것 같습니다 ! 😄

}

private void validateInput(String inputString) {
inputValidator.validateEnglish(inputString);
Copy link

Choose a reason for hiding this comment

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

영문과 길이에 대한 검증 로직을 분리해두고, 추상화하여 관리하는 점 너무 좋은 것 같아요 👍 👍
많은 공부가 되었습니다 📖

private List<String> getWordList(String filePath) {
Path path = Path.of(filePath);
try {
words = Files.readAllLines(path);
Copy link

Choose a reason for hiding this comment

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

파일 변환 부분에 대해서는 별도 메소드로 분리하는것은 어떨까요 ??

메소드를 잘게 나누는 것에는 여러가지 이유가 있겠지만,,
저는 코드의 depth를 의도적으로 깊게 가져가지 않고자 하는데요, 이는 후속 작업자가 코드를 읽을 때 인지 부하를 줄여주는데 있어서 장점을 가진다고 생각합니다 😀

이 부분에 대해 윤백님의 의견도 궁금합니다 ✍️

return words;
}

public String getAnswer(LocalDate currentDate) {
Copy link

Choose a reason for hiding this comment

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

생성자를 포함하여 메서드들을 private은 private끼리, public은 public 끼리 군집화 시켜주는 것을 좋아합니다. 😃
해서, 윤백님께도 살포시 의견 드려봅니다,, (총총)

자바 표준 관례로 클래스는 다음 순서로 정의한다고 하는데, 메서드도 해당되는지는 잘 모르겠지만, 제 개인의 취향에 의해 접근 제어자별로 군집화 시켜주면 코드의 가독성이 높아진다고 생각하여 조심스레 제안드려봅니당 🙇

정적 공개 상수 → 변수 목록 → private 변수 → 비공개 인스턴스 변수 → 공개 함수 → 비공개 함수는 자신을 호출하는 공개 함수 직후에

Copy link
Author

Choose a reason for hiding this comment

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

감사합니다!
말씀하신대로 군집화하는게 훨씬 가독성이 높아질 것 같네요 😄
바로 수정하겠습니다.

if (o == null || getClass() != o.getClass())
return false;

Letter letter1 = (Letter) o;
Copy link

Choose a reason for hiding this comment

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

엇 letter1이라는 표현도 좋지만, 무언가 더 직관적인 변수명이 되면 좋을 것 같아요 !! ㅎㅎ 🤔
마땅히 떠오르는게 없을때, 저는 sourceLetter/targetLetter라는 표현을 사용하기도 한답니다 ! 🙏

Copy link
Author

Choose a reason for hiding this comment

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

헉...IDE가 자동 생성해주는데로 하고 확인도 안했더니..
꼼꼼하게 체크해주셔서 감사합니다! 😂

}

public boolean hasCorrect() {
List<MatchStatus> greenStatuses = List.of(
Copy link

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