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

[2단계 - 사다리 게임 실행] 히이로(문제웅) 미션 제출합니다. #210

Merged
merged 97 commits into from
Mar 10, 2023

Conversation

MoonJeWoong
Copy link

안녕하세요 코다! 히이로입니다.
이번 2차 미션을 진행하면서 집중했던 부분은 다음과 같습니다.

  • VO 객체들간 필요한 연산은 내부 원시값을 외부로 반환하여 처리하는 것이 아니라 VO 객체 간 협력을 통해 수행할 수 있도록 한다.
  • VO 객체들은 도메인 / 비즈니스 로직 측면에서 사용될 때 내부 Wrapping 값을 반환하지 않는다.
  • VO 객체의 Wrapping 값은 View에서 출력을 위한 목적으로만 unWrap 할 수 있다.
    • 즉 VO 객체들의 unWrap 책임은 view에서 담당한다.

이외에 미션을 진행하며 궁금증이 들었던 부분들은 코드에 직접 명시하였습니다.



별개로 코드에 직접 달기 애매한 질문이 있어 따로 남깁니다.

  • TDD 방식으로 이번 미션을 진행하다보니 기능이 구현되지 않은 상태에서 해당 기능을 어떻게 검증해야 할지가 굉장히 궁금했습니다. 특히 객체 내부 상태 값을 조작하는 기능을 테스트하고자 할 때 많이 고민되었던 것 같습니다.
  • 단순하게만 생각한다면 getter 메서드를 사용하여 변화한 내부 상태값을 반환받아 assertThat 구문을 사용하면 간단합니다. 그런데 만약 해당 객체가 출력을 위한 getter 구현 의무가 없다고 한다면 해당 getter 메서드는 오로지 테스트만을 위해 존재하게 된다고 생각이 들었습니다.
  • 하지만 TDD는 최대한 빠르게 실패하는 테스트를 만들고 이를 통과시키기 위한 코드를 작성하는 것이 핵심인데 이런 부분은 어떻게 생각하며 진행하는 것이 좋을지 의견을 구하고 싶습니다!

세부사항
- 비즈니스 로직 측면 / TDD 측면에서 오버 엔지니어링이라는 판단이 들어 삭제
세부사항
- 비즈니스 로직 측면 / TDD 측면에서 오버 엔지니어링이라는 판단이 들어 삭제
세부사항
- 사다리 게임 전체 결과 목록 반환 기능을 상위 기능과 병합하여 명시
- 사다리 게임 결과 값의 수는 전체 사람 수와 동일해야 한다는 기능 추가
세부사항
- 사다리 게임 결과는 쉽표를 기준으로 입력받는 기능 추가
Copy link
Author

@MoonJeWoong MoonJeWoong left a comment

Choose a reason for hiding this comment

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

step2를 진행하며 궁금했던 사항들입니다!

import java.util.List;
import java.util.stream.Collectors;

public class NameFactory {
Copy link
Author

Choose a reason for hiding this comment

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

  • Factory 패키지에 존재하는 클래스들은 본래 Names, Results 클래스들로 일급 컬렉션으로 구현했었습니다. 하지만 입력 값이 형식에 맞게 들어왔는지 확인하고 Wrapper 객체들을 만들어서 반환해주는 역할을 하는 클래스가 일급 컬렉션일까? 라는 의문이 들었습니다. 그리고 단지 클래스 속성 값으로 컬렉션 하나만 존재한다고 일급컬렉션이라고 생각하지 말자고 결론을 내렸습니다

  • 그래서 해당 클래스들을 VO 객체들을 생성하여 반환하는 Factory 클래스로 리팩토링하였습니다. 두 클래스의 공통점이 입력값을 받아 검증을 거친 후 Wrapper 객체를 만들어준다는 것이라 판단하고 리팩토링한 것인데 정확한 개념으로 사용한 것인지 확신이 들지 않았습니다.

Copy link

Choose a reason for hiding this comment

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

히이로 말씀대로 입력 형식을 확인하고 파싱하는 역할만 일급컬렉션에서 가지고 있으면 좀 아쉬울 것 같아요~
고민하신 부분은 매우 공감합니다. 하지만 팩토리 클래스 구현에 대해 몇가지 질문이 있어요.

  1. 팩토리 클래스는 어떤 역할을 담당하며 왜 사용하는 건가요 ?
  2. 지금 구현하신 팩토리 클래스의 역할은 model과 view 중 무엇에 더 가까운가요?

Copy link
Author

Choose a reason for hiding this comment

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

팩토리 클래스에 대해 던져주신 질문들을 고민하면서 사고한 과정입니다.

  • 팩토리 클래스는 어떤 역할을 담당하며 왜 사용하는 건가요?

    • 팩토리 클래스는 객체를 생성하는 책임을 담당하는 클래스를 의미합니다.
    • 팩토리 패턴을 사용하는 방식에는 팩토리 메서드 패턴과 추상 팩토리 패턴이 존재합니다.
      • 팩토리 메서드 패턴은 어떤 객체를 생성하는 책임을 수행하는 팩토리 클래스를 추상화하고 해당 팩토리 클래스(슈퍼클래스)에서 어떤 타입의 객체를 생성하는지는 하위 팩토리 클래스(서브클래스)가 결정하도록 위임하는 방식을 말합니다.
      • 추상팩토리 패턴은 생성하는 객체들 중 연관되는 객체들을 모아서 생성할 수 있도록 추상화된 팩토리 클래스(슈퍼클래스)에 이를 명시하고 연관된 객체들의 타입을 결정하여 실제 생성하는 책임은 하위 팩토리 클래스(서브클래스)가 결정하도록 위임하는 방식을 말합니다.
      • 즉 추상팩토리 패턴은 팩토리 메서드 패턴을 사용하는 방식입니다.
    • 단순하게 팩토리 클래스가 어떤 객체를 생성하는 책임을 가지는 클래스라는 관점에서 봤을 떄는 지금 구현한 방식이 맞습니다.
    • 하지만 팩토리 패턴이라는 것을 학습한 결과 생성하고자 하는 객체의 생성부를 캡슐화하여 결합도를 느슨하게 하고 구체적인 객체 타입에 의존하지 않도록 하는 것임을 알았습니다.
    • 이런 관점에서 봤을 때 클래스 명이 Factory임에도 불구하고 단순 객체 생성의 책임을 수행할 뿐 위와 같은 책임을 수행하는 것은 아니라는 생각이 들었습니다.
  • 지금 구현하신 팩토리 클래스의 역할은 model과 view 중 무엇에 더 가까운가요?

    • 현재 구현한 팩토리 클래스의 역할은 사용자의 입력값을 받아서 파싱 및 검증과정을 거친 후 VO 객체들을 생성하는 책임을 갖고 있습니다.
    • 여기에서 사용자의 입력값을 받아서 파싱 및 검증과정을 거친 후 VO 객체들을 생성하는 책임은 충분히 view에서 수행할 수 있다고 생각이 들었습니다.
    • 오히려 팩토리 패턴을 사용하는 이유에 대해서 학습하고 다시 생각해보니 특별한 이유 없이 view의 책임을 도메인이 과도하게 지게 된 것 같다는 생각이 듭니다.

해당 부분은 view에서 처리하여 도메인으로 넘겨주는 것으로 수정하겠습니다!

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 27 to 31
private static void validateDuplicatedNames(List<String> names) {
if (names.size() != getDistinctCount(names)) {
throw new IllegalArgumentException(DUPLICATED_NAME_ERROR);
}
}
Copy link
Author

Choose a reason for hiding this comment

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

또한 Factory 클래스에서 몇 가지 검증 작업을 수행하는데 이 부분도 괜찮은 것인지 궁금했습니다.

Copy link

Choose a reason for hiding this comment

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

이 질문은 factory 클래스에 대한 정의가 무엇인지 먼저 생각해본 이후에 고민해보면 좋을 것 같아요. (그래야 어떤 역할을 가지는게 좋을지 자연스럽게 연결이 될 테니까요😉)

Comment on lines 20 to 22
public String getValue() {
return name;
}
Copy link
Author

Choose a reason for hiding this comment

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

원시타입을 포장하기 위해 VO를 구현하면서 들었던 의문입니다.

  • 출력을 위한 getter를 구현해두고 해당 객체의 속성값이 일치하는지 판단하는 메서드는 따로 만드는 것이 옳은가?
    • 예를 들어 VO인 Name 값이 다른 Name과 동일한 값을 갖는지 확인하는 isSame() 메서드를 구현해 참/거짓 값을 반환한다.
    • VO도 객체이기에 메세지를 던져 책임을 수행하는 방향이 옳다고 생각했습니다.
  • 위 경우처럼 구현하면 Name 객체에 메세지를 던지는 좋은 케이스가 되겠지만... 출력을 위한 getter가 Name 내에 있어서 다른 개발자는 getter를 사용해 내부 값을 반환받아 사용해 버린다면 어떡하나라는 생각이 들었습니다.
  • 처음 의도했던 대로 메서드들이 사용되지 못할 것 같은데 이런 경우는 어떻게 하는 것이 좋은지 궁금했습니다.

Copy link

Choose a reason for hiding this comment

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

중요한 고민인 것 같아요 !
제 의견을 물어보신다면 저도 히이로처럼 isSame()이라는 메서드를 열어놓고 객체 외부에서는 그 의도에 맞게 메서드를 호출할 것 같아요.

아무리 히이로가 이렇게 잘 구현해 놓더라도 말씀하신대로 다른 개발자는 의도와 다르게 사용할 수 있습니다 😭 고것은 현업에서도 여전히 숙제인 부분이구요 ㅎㅎ

그래서 페어로 구현하고, 지속적으로 커뮤니케이션하고, 컨벤션을 논의했으면 그것을 잘 지키는 것이 무엇보다 중요한 것 같아요. 타 개발자와 너무 달라 맞추기 힘들면 클린코드, 리팩토링과 같은 주제로 함께 스터디를 하면서 싱크를 맞출수도 있구요 ㅎㅎ 사소해 보여도 대단한 기술을 쓰는 것보다 훨씬 중요한 부분이라고 생각해요 😊

또한 책임을 굉장히 잘 나누어서 단일 클래스 단위가 작고, 해당 클래스에 있는 메서드가 몇가지 없다면 한눈에 어떤 기능들이 있는지 확인하기 편하겠죠. 그러면 대부분 의도에 맞는 메서드를 쉽게 찾아서 쓸 것이라고 생각해요. 반대로 클래스의 크기가 너무 크고 메서드가 10개 이상 된다면 찾지 못하고 getter를 가져다 쓸 수도 있겠죠.

개발적인 해답을 기대하셨을 수도 있지만 저는 아직 못찾은 것 같네요 ㅎㅎ 다만 위 말했던 비개발적인 자세가 굉장히 중요하다고만 말씀드릴 수 있을 것 같아요.

Copy link
Author

Choose a reason for hiding this comment

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

그렇군요... 아직 2주차 미션 진행중이지만 개발자는 코드 외적인 부분도 정말 중요하게 생각해야 함을 다시 한 번 인지하게 되었습니다. 감사합니다!

Comment on lines 24 to 25
public boolean isSame(Name other) {
return this.name.equals(other.name);
Copy link
Author

Choose a reason for hiding this comment

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

  • step1 리뷰 때 이름에 대한 String 원시 값을 포장한 Name 객체간의 비교에서 equals와 hashCode 메서드를 오버라이딩 했다가 리뷰를 받고 삭제했었습니다.
  • 그런데 레벨2를 진행하다 보니 Name간 객체 비교를 해야하는 경우가 생겼습니다.
  • 일단은 동일한 타입 객체를 인자로 받아 내부 속성값들을 비교하여 참 거짓을 반환하는 메서드를 정의하여 해당 기능을 수행하도록 했습니다.
  • 그렇다면 예전에 한 것처럼 equals와 hashCode를 재정의하는 방법이 아니라 객체 내부에서 직접 값을 비교해주는 메서드를 구현해주는 것이 항상 더 나은 방법인지 궁금했습니다.
  • 또한 equals와 hashCode를 재정의해야 하는 상황이 따로 존재하는 것인지 궁금하다.
    • 현재 알고있는 것으로는 HashMap 등 사용자 정의 객체를 자료구조를 통해 관리하려고 할 때 재정의해줘야 한다는 것 정도로만 알고있는데 이런 경우에만 재정의 해줘야 하는지 궁금합니다.

Copy link

Choose a reason for hiding this comment

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

어떤 상황에는 equals & hashCode를 구현해야하고 어떤 상황에는 메서드를 구현해야하는지 딱 말씀드릴 수 있는 공식이 있지는 않은데요,
히이로가 말씀주신 자료구조를 사용할 때 구현을 할 수도 있구요, 또 해당 도메인의 성격을 잘 고민해보고 필요하다면 구현할 것 같아요.

예를 들어, 위 Name과 같은 경우에는 동명이인인 경우 이름이 같다면 같은 사람인가요 이름만 같은 다른 사람일까요? 이런 여러 상황을 고려하여 필요하다면 얼마든지 equals & hashCode 구현하셔도 좋습니다 😊

Copy link
Author

Choose a reason for hiding this comment

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

제가 구현한 사다리 게임에서는 Name 객체가 저장하고 있는 이름 문자열의 값이 동일하면 같은 사람이라고 판단하고 있습니다. 그 이유는 동일한 이름이 입력되었을 때 결과를 출력하기 불가능해지는 문제가 발생하기 때문입니다. 그리고 Name 객체를 현재 hashMap등과 같은 자료구조로 관리하는 것도 아니기에 equals와 hashCode를 지금 구현할 필요는 없어 보이네요!

Copy link
Author

Choose a reason for hiding this comment

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

지금 다시 리뷰를 검토해보니 동명이인이 존재하지 않기 때문에 논리적 동치성을 보장하기 위해서는 equals를 재정의 해줘야겠다는 생각이 들었습니다. 게임 상 같은 이름은 같은 사람으로 취급되어야 하기에 같은 이름의 다른 객체가 생긴다면 동일시 해줘야 하기 때문입니다. 이 부분은 마지막 PR 제출 때 같이 수정해보도록 하겠습니다!

Comment on lines 71 to 79
private List<String> unwrapResults(List<Result> results) {
return results.stream().map(Result::getValue).collect(Collectors.toList());
}

private List<String> unwrapNames(List<Name> names) {
return names.stream()
.map(Name::getValue)
.collect(Collectors.toList());
}
Copy link
Author

Choose a reason for hiding this comment

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

도메인/모델 쪽에서는 VO 클래스에 메세지를 던져 로직을 수행하도록 하고 모든 원시값 getter는 view에서 도메인에게 의존하는 것으로 변경했습니다. 혹 잘못 생각한 부분이 있을지 궁금합니다.

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.

출력을 위한 원시 값 getter를 어떻게 처리해야 하나 고민을 많이 하다가 결정한 사항이었습니다.
조금 마음에 걸렸던 부분이라고 한다면 이후에 view에서 unwrap과 관련된 메서드를 구현하면서 나중에 디미터의 법칙이 혹시 깨질 여지가 생길 수 있지 않을까 싶었던 것 같습니다. 하지만 그것도 getter를 사용해 unwrap하는 메서드를 세분화해서 구현할 수 있도록 하면 해결할 수 있을 것 같다고 생각을 정리해서 따로 더 말씀드리고 싶은 부분은 없는 것 같습니다!

Copy link

@yjksw yjksw left a comment

Choose a reason for hiding this comment

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

안녕하세요 히이로 ~ 코다입니다 🙇‍♀️

정말 어려운 미션인데 TDD도 지키며 잘 구현해주셨어요 ㅎㅎ
궁금하신 부분도 코드에 달아주셔서 덕분에 수월하게 확인할 수 있었습니다.

몇가지 코멘트 남겼는데 확인 부탁드려요.
궁금한 점 있으시면 언제든지 DM이나 댓글로 남겨주세요 🙌

@@ -1,4 +1,4 @@
package model;
package model.VO;
Copy link

Choose a reason for hiding this comment

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

package 명 컨벤션은 항상 소문자 입니다 ☺️
model > vo 로 배치하셨는데 히이로 생각하기에 vo가 무엇인가요 ? 다른 domain과의 차이점은 무엇인가요 ?

Copy link
Author

Choose a reason for hiding this comment

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

  • vo와 다른 domain과의 차이?
    • 저는 vo가 내부 인스턴스 변수 값으로 단일 원시타입 혹은 문자열을 가지는 일종의 wrapper 클래스라고 생각을 합니다.
    • 그래서 내부에 저장하는 원시타입 및 문자열 값에 대한 검증 및 작업은 vo에서 이뤄져야 한다고 생각했습니다.
    • 또한 domain에서는 vo를 이용해 여러 책임들을 수행할 수 있지만 실질적으로 vo내 원시타입 및 문자열을 외부로 반환받는 경우는 없도록 했습니다.
    • 원시타입 및 문자열을 포장하는 vo의 개수가 많아지다 보니 패키지를 정리할 때 일반적인 비즈니스 로직을 수행하는 domain과 분리시키면 좋을 것 같아 이렇게 진행했습니다.

패키지 명은 vo로 수정하였습니다!

Copy link

Choose a reason for hiding this comment

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

그렇군요 ㅎㅎ 히이로 의도를 잘 적어주셨네요.
제가 해당 리뷰를 남겼던 이유는 vo를 따로 패키지 분리할 필요성을 못 느껴서 추가했었는데요,
패키지 구조에 답이 있는 것은 아니라 히이로가 하신 것처럼 기능적으로 분리를 할 수도 있고, 의미적(도메인 별)로 분리를 할 수도 있을 것 같아요.

개인적인 생각을 추가해보자면 현재 vo에 있는 클래스들이 완전이 비즈니스 로직과 무관한 클래스는 아니라고 생각해요.
Name의 형식이나 길이에 대한 정책, Result 개수에 대한 정책 등등은 비즈니스 정책이라고 생각 했어요.
이 부분은 개인적인 의견으로 참고만 해주세요 😉

validateNameLength(name);
validateNameHasOnlyCharacters(name);
validateLength(name);
validateHasOnlyCharacters(name);
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 20 to 22
public String getValue() {
return name;
}
Copy link

Choose a reason for hiding this comment

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

사람마다 취향이 다를 순 있을텐데요, 저는 단순 getter이더라도 getValue와 같은 네이밍을 지양하는 편이에요 ㅎㅎ
getValue() 라는 것은 너무 범용적으로 사용될 수 있는 표현이라서 Name객체의 값 중 무엇을 리턴하는지 한번 더 들어가서 확인하게 되는 것 같아서요
히이로의 생각은 어떤가요? 요 부분은 꼭 수정하지는 않아셔도 됩니다.

Copy link
Author

Choose a reason for hiding this comment

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

저는 vo 클래스를 설계할 때 인스턴스 속성 값으로 항상 단일 값을 갖도록 설계했습니다. 그래서 일반적인 도메인과 다르게 vo 객체의 getter를 사용할 때는 출력을 위한 용도임을 전달하고자 getValues라는 이름을 짓게 되었던 것 같아요. 하지만 코다가 짚어주신 것처럼 이렇게 작명한다고 getter를 올바르게 사용한다는 보장도 없고 오히려 다른 개발자로 하여금 혼동을 일으킬 수 있는 여지가 생기는 것 같습니다.

해당 부분은 수정하도록 하겠습니다!

Comment on lines 20 to 22
public String getValue() {
return name;
}
Copy link

Choose a reason for hiding this comment

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

중요한 고민인 것 같아요 !
제 의견을 물어보신다면 저도 히이로처럼 isSame()이라는 메서드를 열어놓고 객체 외부에서는 그 의도에 맞게 메서드를 호출할 것 같아요.

아무리 히이로가 이렇게 잘 구현해 놓더라도 말씀하신대로 다른 개발자는 의도와 다르게 사용할 수 있습니다 😭 고것은 현업에서도 여전히 숙제인 부분이구요 ㅎㅎ

그래서 페어로 구현하고, 지속적으로 커뮤니케이션하고, 컨벤션을 논의했으면 그것을 잘 지키는 것이 무엇보다 중요한 것 같아요. 타 개발자와 너무 달라 맞추기 힘들면 클린코드, 리팩토링과 같은 주제로 함께 스터디를 하면서 싱크를 맞출수도 있구요 ㅎㅎ 사소해 보여도 대단한 기술을 쓰는 것보다 훨씬 중요한 부분이라고 생각해요 😊

또한 책임을 굉장히 잘 나누어서 단일 클래스 단위가 작고, 해당 클래스에 있는 메서드가 몇가지 없다면 한눈에 어떤 기능들이 있는지 확인하기 편하겠죠. 그러면 대부분 의도에 맞는 메서드를 쉽게 찾아서 쓸 것이라고 생각해요. 반대로 클래스의 크기가 너무 크고 메서드가 10개 이상 된다면 찾지 못하고 getter를 가져다 쓸 수도 있겠죠.

개발적인 해답을 기대하셨을 수도 있지만 저는 아직 못찾은 것 같네요 ㅎㅎ 다만 위 말했던 비개발적인 자세가 굉장히 중요하다고만 말씀드릴 수 있을 것 같아요.

Comment on lines 10 to 12
public boolean isSamePosition(Position other) {
return this.position == other.position;
}
Copy link

Choose a reason for hiding this comment

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

여기도 위에 Name에서 수정하신 것처럼 Position.isSamePosition() -> Position.isSame() 로 변경해보면 어떨까요 ?
메서드가 짧아져도 충분히 의미가 잘 전달될 것 같아요 ~


private static List<String> splitInputResults(String inputResults) {
return Arrays.stream(inputResults.split(RESULTS_DELIMITER))
.map(String::trim)
Copy link

Choose a reason for hiding this comment

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

NameFactory에서는 strip()하고, ResultFactory에서는 trim()을 했는데 의도하신 걸까요 ?

Copy link
Author

Choose a reason for hiding this comment

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

엇 원래는 trim을 사용했었는데 scanner 관련 개행 문자 디버깅 때 바꿔놓고 지나친 것 같습니다...
해당 기능은 view에 이관되면서 NameFactory와 ResultFactory의 메서드가 통합되었고 해결되었습니다!

import java.util.List;
import java.util.stream.Collectors;

public class NameFactory {
Copy link

Choose a reason for hiding this comment

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

히이로 말씀대로 입력 형식을 확인하고 파싱하는 역할만 일급컬렉션에서 가지고 있으면 좀 아쉬울 것 같아요~
고민하신 부분은 매우 공감합니다. 하지만 팩토리 클래스 구현에 대해 몇가지 질문이 있어요.

  1. 팩토리 클래스는 어떤 역할을 담당하며 왜 사용하는 건가요 ?
  2. 지금 구현하신 팩토리 클래스의 역할은 model과 view 중 무엇에 더 가까운가요?

Comment on lines 27 to 31
private static void validateDuplicatedNames(List<String> names) {
if (names.size() != getDistinctCount(names)) {
throw new IllegalArgumentException(DUPLICATED_NAME_ERROR);
}
}
Copy link

Choose a reason for hiding this comment

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

이 질문은 factory 클래스에 대한 정의가 무엇인지 먼저 생각해본 이후에 고민해보면 좋을 것 같아요. (그래야 어떤 역할을 가지는게 좋을지 자연스럽게 연결이 될 테니까요😉)

@Disabled("참여자 이름을 검색하여 최종결과 반환 기능 테스트 내용과 중복되므로 비활성화한다.")
@DisplayName("사다리 타기가 완료된 참여자의 위치에 맞는 결과 저장 기능 테스트")
void saveResultByPositionTest() {
// //given
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.

아 테스트 코드에 해당해서 나중에 다시 필요해질 가능성도 있지 않을까 싶어 일부러 남겨뒀던 부분이었습니다. 삭제하도록 하겠습니다!

Comment on lines 35 to 49
@ParameterizedTest
@MethodSource("provideNamesInputValues")
@DisplayName("참여자 이름은 쉼표를 기준으로 입력받는 기능 테스트")
void splitNamesByCommasTest(String inputValue, List<String> expectedResult) {
//given
List<Name> createdNames = NameFactory.create(inputValue);

//when
List<String> thenResult = createdNames.stream()
.map(Name::getValue)
.collect(Collectors.toList());

//then
assertThat(thenResult).isEqualTo(expectedResult);
}
Copy link

Choose a reason for hiding this comment

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

given-when-then 구조 👍

@yjksw
Copy link

yjksw commented Feb 24, 2023

질문 주신 부분 답변드려요 🙇‍♀️

TDD 방식으로 이번 미션을 진행하다보니 기능이 구현되지 않은 상태에서 해당 기능을 어떻게 검증해야 할지가 굉장히 궁금했습니다. 특히 객체 내부 상태 값을 조작하는 기능을 테스트하고자 할 때 많이 고민되었던 것 같습니다.
단순하게만 생각한다면 getter 메서드를 사용하여 변화한 내부 상태값을 반환받아 assertThat 구문을 사용하면 간단합니다. 그런데 만약 해당 객체가 출력을 위한 getter 구현 의무가 없다고 한다면 해당 getter 메서드는 오로지 테스트만을 위해 존재하게 된다고 생각이 들었습니다.
하지만 TDD는 최대한 빠르게 실패하는 테스트를 만들고 이를 통과시키기 위한 코드를 작성하는 것이 핵심인데 이런 부분은 어떻게 생각하며 진행하는 것이 좋을지 의견을 구하고 싶습니다!

getter가 테스트만을 위해 존재한다면, 해당 데이터를 외부에 제공할 의무가 없는 객체라는 뜻이군요! 외부에 대한 책임이 없는 부분까지 테스트할 필요가 있을까요 ? 저는 개인적으로 이 객체의 책임은 public 메서드들이 나타낸다고 생각해서 그 책임을 중점적으로 테스트 하는 것을 선호해요.

말씀하신대로 기능이 구현되지 않은 상태에서 검증을 미리 하기는 매우 어려운 일입니다 😢 그래서 미리 기능의 스펙(책임을 정의하는 것)을 고민하는 것이 중요하다고 생각해요. 어떤 객체에 어떤 책임(public 메서드)가 있을 것이며, 이 메서드는 무엇을 반환할 것인가를 미리 고민해보는거죠.

Copy link
Author

@MoonJeWoong MoonJeWoong left a comment

Choose a reason for hiding this comment

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

안녕하세요 코다! 히이로입니다.

Players와 player와 관련된 부분 이외에는 리팩토링을 완료하였습니다.
해당 부분은 코다의 의견을 한 번 더 듣고 생각을 정리한 뒤 리팩토링하고 싶어 일단은 그대로 리뷰요청을 드린 점 양해 부탁드립니다... 😭

import java.util.List;
import java.util.stream.Collectors;

public class NameFactory {
Copy link
Author

Choose a reason for hiding this comment

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

팩토리 클래스에 대해 던져주신 질문들을 고민하면서 사고한 과정입니다.

  • 팩토리 클래스는 어떤 역할을 담당하며 왜 사용하는 건가요?

    • 팩토리 클래스는 객체를 생성하는 책임을 담당하는 클래스를 의미합니다.
    • 팩토리 패턴을 사용하는 방식에는 팩토리 메서드 패턴과 추상 팩토리 패턴이 존재합니다.
      • 팩토리 메서드 패턴은 어떤 객체를 생성하는 책임을 수행하는 팩토리 클래스를 추상화하고 해당 팩토리 클래스(슈퍼클래스)에서 어떤 타입의 객체를 생성하는지는 하위 팩토리 클래스(서브클래스)가 결정하도록 위임하는 방식을 말합니다.
      • 추상팩토리 패턴은 생성하는 객체들 중 연관되는 객체들을 모아서 생성할 수 있도록 추상화된 팩토리 클래스(슈퍼클래스)에 이를 명시하고 연관된 객체들의 타입을 결정하여 실제 생성하는 책임은 하위 팩토리 클래스(서브클래스)가 결정하도록 위임하는 방식을 말합니다.
      • 즉 추상팩토리 패턴은 팩토리 메서드 패턴을 사용하는 방식입니다.
    • 단순하게 팩토리 클래스가 어떤 객체를 생성하는 책임을 가지는 클래스라는 관점에서 봤을 떄는 지금 구현한 방식이 맞습니다.
    • 하지만 팩토리 패턴이라는 것을 학습한 결과 생성하고자 하는 객체의 생성부를 캡슐화하여 결합도를 느슨하게 하고 구체적인 객체 타입에 의존하지 않도록 하는 것임을 알았습니다.
    • 이런 관점에서 봤을 때 클래스 명이 Factory임에도 불구하고 단순 객체 생성의 책임을 수행할 뿐 위와 같은 책임을 수행하는 것은 아니라는 생각이 들었습니다.
  • 지금 구현하신 팩토리 클래스의 역할은 model과 view 중 무엇에 더 가까운가요?

    • 현재 구현한 팩토리 클래스의 역할은 사용자의 입력값을 받아서 파싱 및 검증과정을 거친 후 VO 객체들을 생성하는 책임을 갖고 있습니다.
    • 여기에서 사용자의 입력값을 받아서 파싱 및 검증과정을 거친 후 VO 객체들을 생성하는 책임은 충분히 view에서 수행할 수 있다고 생각이 들었습니다.
    • 오히려 팩토리 패턴을 사용하는 이유에 대해서 학습하고 다시 생각해보니 특별한 이유 없이 view의 책임을 도메인이 과도하게 지게 된 것 같다는 생각이 듭니다.

해당 부분은 view에서 처리하여 도메인으로 넘겨주는 것으로 수정하겠습니다!

@@ -1,4 +1,4 @@
package model;
package model.VO;
Copy link
Author

Choose a reason for hiding this comment

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

  • vo와 다른 domain과의 차이?
    • 저는 vo가 내부 인스턴스 변수 값으로 단일 원시타입 혹은 문자열을 가지는 일종의 wrapper 클래스라고 생각을 합니다.
    • 그래서 내부에 저장하는 원시타입 및 문자열 값에 대한 검증 및 작업은 vo에서 이뤄져야 한다고 생각했습니다.
    • 또한 domain에서는 vo를 이용해 여러 책임들을 수행할 수 있지만 실질적으로 vo내 원시타입 및 문자열을 외부로 반환받는 경우는 없도록 했습니다.
    • 원시타입 및 문자열을 포장하는 vo의 개수가 많아지다 보니 패키지를 정리할 때 일반적인 비즈니스 로직을 수행하는 domain과 분리시키면 좋을 것 같아 이렇게 진행했습니다.

패키지 명은 vo로 수정하였습니다!

Comment on lines 20 to 22
public String getValue() {
return name;
}
Copy link
Author

Choose a reason for hiding this comment

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

그렇군요... 아직 2주차 미션 진행중이지만 개발자는 코드 외적인 부분도 정말 중요하게 생각해야 함을 다시 한 번 인지하게 되었습니다. 감사합니다!

Comment on lines 20 to 22
public String getValue() {
return name;
}
Copy link
Author

Choose a reason for hiding this comment

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

저는 vo 클래스를 설계할 때 인스턴스 속성 값으로 항상 단일 값을 갖도록 설계했습니다. 그래서 일반적인 도메인과 다르게 vo 객체의 getter를 사용할 때는 출력을 위한 용도임을 전달하고자 getValues라는 이름을 짓게 되었던 것 같아요. 하지만 코다가 짚어주신 것처럼 이렇게 작명한다고 getter를 올바르게 사용한다는 보장도 없고 오히려 다른 개발자로 하여금 혼동을 일으킬 수 있는 여지가 생기는 것 같습니다.

해당 부분은 수정하도록 하겠습니다!

Comment on lines 24 to 25
public boolean isSame(Name other) {
return this.name.equals(other.name);
Copy link
Author

Choose a reason for hiding this comment

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

제가 구현한 사다리 게임에서는 Name 객체가 저장하고 있는 이름 문자열의 값이 동일하면 같은 사람이라고 판단하고 있습니다. 그 이유는 동일한 이름이 입력되었을 때 결과를 출력하기 불가능해지는 문제가 발생하기 때문입니다. 그리고 Name 객체를 현재 hashMap등과 같은 자료구조로 관리하는 것도 아니기에 equals와 hashCode를 지금 구현할 필요는 없어 보이네요!

Comment on lines 71 to 79
private List<String> unwrapResults(List<Result> results) {
return results.stream().map(Result::getValue).collect(Collectors.toList());
}

private List<String> unwrapNames(List<Name> names) {
return names.stream()
.map(Name::getValue)
.collect(Collectors.toList());
}
Copy link
Author

Choose a reason for hiding this comment

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

출력을 위한 원시 값 getter를 어떻게 처리해야 하나 고민을 많이 하다가 결정한 사항이었습니다.
조금 마음에 걸렸던 부분이라고 한다면 이후에 view에서 unwrap과 관련된 메서드를 구현하면서 나중에 디미터의 법칙이 혹시 깨질 여지가 생길 수 있지 않을까 싶었던 것 같습니다. 하지만 그것도 getter를 사용해 unwrap하는 메서드를 세분화해서 구현할 수 있도록 하면 해결할 수 있을 것 같다고 생각을 정리해서 따로 더 말씀드리고 싶은 부분은 없는 것 같습니다!

@Disabled("참여자 이름을 검색하여 최종결과 반환 기능 테스트 내용과 중복되므로 비활성화한다.")
@DisplayName("사다리 타기가 완료된 참여자의 위치에 맞는 결과 저장 기능 테스트")
void saveResultByPositionTest() {
// //given
Copy link
Author

Choose a reason for hiding this comment

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

아 테스트 코드에 해당해서 나중에 다시 필요해질 가능성도 있지 않을까 싶어 일부러 남겨뒀던 부분이었습니다. 삭제하도록 하겠습니다!

import java.util.stream.Collectors;
import java.util.stream.IntStream;

public class Players {
Copy link
Author

Choose a reason for hiding this comment

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

지난 step1 코드 리뷰 때 일급컬렉션을 사용하는 이유에 대해 정리해봤던 제 생각입니다.

일급 컬렉션을 사용하는 이유
  - 만약 일급컬렉션 내부 객체 값에 하나하나 접근해서 수행해야 하는 행위가 있다면 해당 행위를 일급 컬렉션 외부가 아닌 내부로 이관시켜야 합니다.
  - 그리고 외부에서 해당 행위를 수행하고자 한다면 일급 컬렉션 내부에 구현된 메서드를 호출하고 그에 해당하는 결과 메세지를 전달 받기만 하면 됩니다.

사실 Players와 Results를 가지고 게임을 진행하는 LadderGame 클래스를 생각해보지 않은 것은 아닙니다. 하지만 Players는 일급 컬렉션이고 Player와 관련된 모든 외부 행위는 Players 내부에서 이뤄지도록 구현하려다 보니 사다리 게임 결과를 계산하는 책임도 Players 가지게 되었습니다. 일급 컬렉션 요소 객체들에 대한 작업은 일급 컬렉션 내부에서 진행해야 한다는 한가지 이유에만 집착해버린 것일까요...?

또한 LadderGame을 구현하게 된다면 Players 가 저장하고 있는 List혹은 Player를 반드시 꺼내줘야 할 것 이라고 생각했습니다. 이것만은 일급 컬렉션을 사용하는 측면에서 1순위로 피해야 할 케이스라고 생각했던 것 같아요.

여기서 코다의 생각은 어떤지 많이 궁금합니다. 저는 여태껏 일급컬렉션을 사용한다면 요소 객체들을 외부에 그대로 반환해서 수행해야 하는 비즈니스 로직은 일급컬렉션 내부로 이관시켜 왔습니다. 하지만 이렇게만 하다보면 일급컬렉션과 일급컬렉션의 요소 객체에게도 너무 많은 책임이 부과되는 지금 같은 문제가 생길 수 밖에 없는 것 같습니다.

과도한 책임 부과를 회피하기 위해 일급컬렉션 내부 요소를 외부로 반환하는 것은 허용을 할 수 밖에 없는 부분인데 제가 아집을 부리는 것은 아닐 지 고민이 됩니다.

Comment on lines +7 to +10
public class Player {
private final Name name;
private Result result;
private Position position;
Copy link
Author

Choose a reason for hiding this comment

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

Players에게 과도한 책임이 부여되는 문제가 발생하면서 같이 생긴 문제라고 생각합니다. 여기도 Player가 Players 외부에 반환되어 게임 진행 이후에 result 객체와 맵핑할 수 있는 객체를 설계한다면 해결할 수 있는 부분이라고 생각됩니다. 해당 부분은 Players에 달아주신 코멘트에 대한 답변을 듣고 수정해보고 싶어 일단은 그대로 두었습니다!

import java.util.stream.Collectors;
import java.util.stream.IntStream;

public class Players {
Copy link
Author

Choose a reason for hiding this comment

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

사실 지금도 일급 컬렉션 외부로 요소 값 반환만 허용한다고 하면 LadderGame클래스를 구현하고 Player가 Result로 인해 데이터 무결성이 깨지는 경우도 피하게 될 수 있을 것 같아요. 또한 게임 진행 자체에 대한 책임도 Players에서 LadderGame으로 어느정도 분산시킬 수도 있을 것 같습니다.

또 다시 생각해보면 이미 Ladder라는 일급 컬렉션에서 요소 값인 Line을 외부로 반환하고 있으면서 이런 고민을 하는 것 자체가 모순인가 싶다는 생각도 듭니다. 생각이 많이 복잡한데 이 부분은 피드백을 한 번 더 받고 정리해보고 싶어 일단은 그대로 놔두고 리뷰 요청드렸습니다...!

Copy link

@yjksw yjksw left a comment

Choose a reason for hiding this comment

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

안녕하세요 히이로 ~ 코다 입니다 🙇‍♀️

히이로가 학습해주신 내용 정말 잘 읽었어요 !
저도 생각하지 못했던 부분인데 읽으면서 고민이 많이 되었습니다 ㅎㅎ

코드에 대한 추가적인 리뷰는 없어서, 히이로가 남겨주신 고민에 대한 추가 댓글만 남겼어요.
아직 더 고민하고 적용하고 싶은 것이 있으실까 싶어 Request changes를 하지만
추가 수정하지 않고 다시 리뷰요청 주시더라도 바로 머지할게요 😊

감사합니다 👍

@MoonJeWoong
Copy link
Author

MoonJeWoong commented Mar 9, 2023

안녕하세요 코다! 히이로입니다. 🙇
우선 리뷰 요청을 너무나 늦게 드리게 되어 죄송합니다...
지난 주 테코톡 발표 준비에 욕심을 부린 데다가 개인적인 사정이 겹쳐 제 시간에 리뷰 요청을 드리지 못했습니다.

코드적으로 수정한 부분은 다음과 같습니다.

  • 예전에 말씀드린 대로 Name 객체가 동일한 이름을 가질 땐 같은 사람으로 판단되어야 하기에 논리적 동치성 보장을 위한 equals & hashCode 만 오버라이딩한 것
  • controller 부분에서 view와 상관 없는 도메인 로직이 드러나는 playGame 메서드 기능을 LadderGame 클래스로 이관

코다가 마지막에 남겨주셨던 리뷰들을 다시 읽어보며 생각을 정리해봤습니다.

  1. 이번 미션에서 position 상태 값을 player가 갖도록 했기 때문에 자연스럽게 위치 관련 게임 진행 로직 또한 players 내부로 이관되었어요.
  • 때문에 players 가 지는 책임이 굉장히 커지는 이슈가 발생했습니다.
  • 저는 예전에 이 문제가 일급 컬렉션을 잘못 사용하고 있기 때문이라는 생각에 매몰되어 있었던 것 같아요.
  • 하지만 이번 블랙잭 미션을 진행하고 다시 사다리 게임 코드를 보니 Players 일급 컬렉션 클래스 사용의 문제가 아니라 Player 객체 자체에 많은 책임이 부과된 문제가 있는게 아닌가 라는 생각이 들었습니다.
  • 여기까지 생각이 미치니 코다가 달아주신 리뷰도 새로운 관점에서 다시 이해가 되었던 것 같아요.
  • 코다의 리뷰대로 position 관련 책임이 Player에 없다고 한다면 Players가 가지는 책임이 플레이어의 이름과 게임 결과를 저장하고 반환하는 기능밖에 수행할 수 없어지기에 Players 자체를 구현하는 이유가 없어져버리게 되요.
    • 물론 전체적인 설계를 다시 생각해서 Player가 지는 책임을 다시 생각한다면 Players가 구현되어야 할 이유를 찾아볼 수는 있을 거라고 생각합니다.
    • Position을 관리하는 책임을 ladder에서 지도록 구현했던 코다가 players라는 일급컬렉션을 사용하지 않았던 이유도 같이 이해가 되었습니다.
  • 현재 블랙잭 미션을 진행하면서도 그렇고 앞으로 단일 객체에 책임이 과중하게 부과된다면 객체 간 설계를 다시 생각해보는 방식으로 다시 접근할 수 있을 것 같습니다. (물론 아직도 설계에 대한 부분은 모르는게 너무 많네요...)
  1. 다른 하나는 controller 에서 view와 무관한 비즈니스 로직이 노출되고 있다는 문제입니다.
  • 이 부분은 전적으로 코다의 리뷰가 옳다고 생각했어요.
  • controller 에서 playGame 메서드 부분을 LadderGame 클래스로 이관 시키면서 다음과 같은 생각이 들었습니다.
  • Players가 가진 과도한 책임을 다른 객체로 분리시켜 줘야 LadderGame이 질 수 있는 책임도 명확해질 수 있겠다고 판단했습니다.
  • 현재 LadderGame은 게임을 진행하고 단순히 Players 객체를 반환해줄 뿐입니다.
  • 이를 해결하기 위해서는 Players의 책임을 게임을 진행하는 객체, 게임 최종 결과를 관리하는 객체 등으로 분리해내는 것이 필요하다고 생각이 들었어요.
  • 다만 해당 고민은 현재 진행중인 블랙잭 미션에서도 계속 생각해야 하는 부분이기에 이 때문에 사다리 게임 미션에 더 시간을 투자하는 시기는 지났다고 스스로 판단했습니다.
  • 이러한 이유로 부족한 부분이 보임에도 리뷰 요청을 드렸습니다.

코다가 정말 리뷰를 성심성의껏 달아주셨던만큼 마무리도 잘 해보고 싶었는데 개인적으로 아쉬움이 남을 수 밖에 없는 미션이었던 것 같습니다. 다시 한 번 늦게 요청드려서 죄송합니다... 😭

코다가 이번 리뷰어였어서 공부하는 과정이 너무나 즐거웠었습니다!
앞으로 학습 진행하며 궁금한 점이 생기면 DM 드려도... 되겠죠...? ㅎㅎ
이번 미션 리뷰해주시느라 고생 많으셨습니다. 감사합니다!

Copy link

@yjksw yjksw left a comment

Choose a reason for hiding this comment

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

안녕하세요 히이로 ~ 코다 입니다 🙇‍♀️

그랬군요 ! 테코톡은 잘 끝내셨나요 ? 😊
마지막 리뷰 때도 이미 머지하기 충분했기 때문에 너무 아쉬워하지 않으셔도 됩니다 ㅎㅎ
히이로가 남겨주신 마지막 정리도 잘 읽어보았어요.
항상 의도한 대로 잘 학습해주셔서 리뷰가 수월했습니다 👍

교육 들으시면서 언제든지 DM 주셔도 되고 말고요 !
앞으로도 화이팅 입니다 🙌

@yjksw yjksw merged commit 050015b into woowacourse:moonjewoong Mar 10, 2023
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