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

[5기] 3주차 Wordle 과제 제출 - 얄뭉 #37

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

Conversation

yeojiin
Copy link

@yeojiin yeojiin commented Jun 17, 2024

느낀점

페어 코딩을 처음 해 보는 것이라 서툰 부분이 많았고, 생각 했던 것 보다 체력 소모나 정신력 소모가 큰 편이었다. 하지만 시간 가는 줄 모르고 개발할만큼 간만에 개발이 재밌었다. 혼자서 코딩을 하게 되면 갇힌 시야에서 작업을 하게되어 사고 확장이 어려운 편인데 다른 사람의 조언을 실시간으로 들으니까 새로운 시도도 할 수 있고, 도메인을 나누거나 할 때 과하게 쪼개는 경향이 있는데 그 부분을 점프님이 잘 잡아주신 것 같다. 그러면서도 점프님에게 도움이 되기 위해서 기본 개념을 더 공부해서 좀 더 신뢰가 갈만한 가이드 라인을 제시해보고 싶단 생각도 들었다.🤓


잘한점

의견을 조율할 때 무리하게 자신의 의견만을 강조하지 않고, 서로 의사소통을 통해 타협점을 찾으려고 한 점이 잘 된 것 같다. 그러면서도 수정하는게 좋다고 생각한 부분은 근거를 들어 설명하고 설득하는 방식으로 진행한 점은 서로 잘한 것 같다! 서로 최선을 다해서 상호보완한 점도 매우 칭찬해 💯


아쉬운점

처음 페어 코딩을 진행하면서 code with me를 사용하다보니 소스 레파지토리 관리를 잘 못한 것 같다. 그날 작업이 끝나면 점프님 소스를 옮겨오는 방식으로 진행했는데 나중에 점프님 말대로 점프님 브랜치에서 하나 더 브랜치를 따서 pr을 날리면 commit 관리가 똑같이 올라갔을 것 같다.🫤
또한, 객체 설계를 어느 단위까지 나누는게 맞는 지 확신을 하기 어려웠다. 특히, 오브젝트를 읽으면서 진행하다보니 과연 책임을 도메인 단위로 나눈게 맞나 생각이 들었다. 이전과 같은 습관으로 절차지향적으로 가는 건가 싶어서 word 부분에서 String 대신 Letter라는 객체를 만들어서 List로 쓰는게 좋았을까 싶었는데 공수가 더 크게 드는 상황이라 점프님을 설득하지 못했다. 지식이 부족함을 느끼고 더 공부해야겠단 생각이 들었다.


배운점

점프님이 객체를 하나 만들 때마다 테스트 코드를 작성하시면서 검증을 거친 후 진도를 나가는 점이 좋았다. 실무에서 테스트 코드 작성을 많이 하지 않는 편인데 습관을 들이면 좋을 것 같다. 또한, Iterable을 능숙하게 사용하셔서 좀 더 깔끔하고 좋은 코드를 작성하는데 도움이 될 것 같다. 마지막으로 코드를 작성할 때 다른 개발자를 설득하고, 의견을 조율하는 커뮤니케이션 능력이 상승한 것 같아서 뿌듯하다.😁

Copy link

@ycheese ycheese left a comment

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.

리팩토링 포인트를 작성해 주신 부분이 인상깊네요 !
미리 페어와 상의 하에 리스트업을 해두고 리팩토링을 하신 걸까요 ?
취소선이 그어진 항목은 왜 진행하지 않기로 하셨을까요 ?

Copy link
Author

Choose a reason for hiding this comment

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

페어분과 주로 논의한 내용이 나눠진 클래스들을 합치는 것과 String 값 처리였는데 클래스의 분리는 충분하다고 판단했습니다! 😁
String의 경우 배열로 변경하려고 했으나 클래스가 하나 더 생길 수 있다는 점과 IntStream으로 처리해보자는 의견이 나와서 진행하지 않았습니다!


public class WordLoader {

public WordLoader() {
Copy link

Choose a reason for hiding this comment

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

public 기본 생성자를 명시해두신 이유가 있을까요 ?
WordLoader 클래스는 유틸리티 클래스 용도로 만들어진 것 같은데, 외부에서의 객체 생성을 막기 위해 기본 생성자를 private으로 변경하는 건 어떨까요 ?

Copy link
Author

Choose a reason for hiding this comment

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

초반에 사용하게 될 것을 염두하고 public으로 선언했는데 사용되는 부분이 없어 protected로 변경했습니다~
저는 보통 기본생성자를 protected로 선언하는 편인데 이번에 좀 더 찾아보면서 상세한 이유를 알게됐네요~
왜 protected로 선언했을까?

@@ -0,0 +1,5 @@
package config;

public class FileConfig {
Copy link

Choose a reason for hiding this comment

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

FileConfig 클래스도 WordLoader와 동일한 이유로 private 기본 생성자를 만들어두면 어떨까요 ?

Copy link
Author

Choose a reason for hiding this comment

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

반영했습니다~ 👍

private RoundView roundView;
private boolean isWinning = false;

protected GameManager() {
Copy link

Choose a reason for hiding this comment

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

GameManager 클래스에 기본 생성자는 어떤 용도로 쓰이나요 ?

Copy link
Author

Choose a reason for hiding this comment

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

정적 필드도 없으니 필요 없어보이네요! 삭제했습니다!

}

public static boolean isCorrect(Hint hint) {
return Hint.CORRECT.equals(hint);
Copy link

Choose a reason for hiding this comment

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

enum 비교 관련해서 코멘트를 멋지게 달고 싶어서 서치하다가
흥미로운 글을 발견했는데요.
같이 보시면 좋을 것 같아 공유 드립니다 😆

Copy link
Author

Choose a reason for hiding this comment

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

오~ 정말 재밌는 글이네요! 습관적으로 equals를 사용했는데
역시 치이사님 덕분에 좋은 지식 얻어갑니다! 최고! 💯 👍

import java.util.Iterator;
import java.util.List;

public class MatchResult implements Iterable<Hint>{
Copy link

Choose a reason for hiding this comment

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

일급컬렉션 👍🏻
+) MatchResults 까지 👍🏻👍🏻

Copy link
Author

Choose a reason for hiding this comment

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

👍 👍

@Test
@DisplayName("단어목록파일 읽기")
void loadWordsFromFile(){
List<String> words = WordLoader.read("src/test/resources/words.txt");
Copy link

Choose a reason for hiding this comment

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

테스트를 위한 단어 파일 생성 👍🏻
배워갑니다 ~

Copy link
Author

Choose a reason for hiding this comment

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

감사합니다🙇🏻‍♀️

class InputWordTest {
@DisplayName("입력단어 유효성 검증 성공 테스트")
void validateSuccessInputWord() {
List<String> words = List.of("apples", "cherry");
Copy link

Choose a reason for hiding this comment

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

words를 InputWordTest의 멤버변수로 빼는 것도 좋을 것 같아요 !

Copy link
Author

Choose a reason for hiding this comment

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

좋은 의견 같아 수정했습니다!


class InputWordTest {
@DisplayName("입력단어 유효성 검증 성공 테스트")
void validateSuccessInputWord() {
Copy link

Choose a reason for hiding this comment

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

요 메소드는 @Test 어노테이션이 안 달려있네요 ? 🤔

@ParameterizedTest를 통해 조금 더 다양한 성공 케이스를 검증해 볼 수 있을 것 같습니다 ! (사실 제 취향입니다 .. 🙈)

Copy link
Author

Choose a reason for hiding this comment

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

반영했습니다! 👍

@Test
@DisplayName("정답은 매일 바뀌며 ((현재 날짜 - 2021년 6월 19일) % 배열의 크기) 번째의 단어이다")
void answerSelectTest() {
List<String> words = List.of("apple", "banana", "cherry", "date", "elderberry");
Copy link

Choose a reason for hiding this comment

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

여기도 words를 AnswerTest의 필드로 분리하면 조금 더 가독성 높은 테스트 코드가 될 것 같아요 !

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

@ycheese ycheese left a comment

Choose a reason for hiding this comment

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

누추한 코멘트에 답변도 세세히 달아주시고... 반영까지😎 완벽합니다
얄뭉님의 코드로 더 많이 배우고 더 깊게 생각할 수 있어서 리뷰도 재밌게 할 수 있었어요 🫧
너무너무 고생 많으셨습니다 ! 남은 주말도 행복하세요 ~

EXIST("🟨"), //\uD83D\\uDFE8
NOT_EXIST("⬜");

private final String tile;
Copy link

Choose a reason for hiding this comment

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

후기까지 세세히 남겨주신 덕분에 저도 또 한 번 배워갑니다 🫡

import domain.MatchResults;

public class HintView {
public void render(MatchResults matchResults) {
Copy link

@ycheese ycheese Jun 29, 2024

Choose a reason for hiding this comment

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

맞아요 저도 구현하면서 그 지점때문에 많이 고민했습니다 😂
이 프로그램이 콘솔 출력을 이용하다보니 구조상 한계가 있다고도 생각해요.

저라면 MatchResults 클래스에 toString을 오버라이딩해서 쓰거나, MatchResult 클래스의 getHintTiles() 메소드를 래핑하는 메소드를 만들어보지 않았을까 싶어요 !

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