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

[LBP] 김태우 사다리 1단계 제출합니다 #42

Open
wants to merge 3 commits into
base: twootwoo
Choose a base branch
from

Conversation

TwooTwoo
Copy link

@TwooTwoo TwooTwoo commented Mar 7, 2025

스터디 정보

리뷰어 : 김우진
리뷰이 : 김태우
페어 : 김준


코드 설명

Line 안에는 여러 개의 Link 가 포함됩니다.
여러 개의 Line이 모여 Ladder를 구성합니다.

Controller

  • LadderController : 사다리 게임 진행 로직을 담당

dto

  • LineDto : Line 내부의 Link 존재 여부를 Boolean 형태로 전달

model

  • Ladder : 사다리 정보 저장 및 사다리 생성
  • Line : 각 행(Line)의 정보 저장 및 각 행 생성
  • Link : 각 기둥 간의 연결 상태 정보 저장 및 연결 상태 생성
  • LinkStatus : 기둥 간 연결 상태의 3가지 종류
    • UNDEFINED : 값이 아직 정해지지 않음
    • ABSENT : 존재하지 않음
    • PRESENT : 존재함

view

  • LadderOutputView : 사다리 게임 출력 담당

질문

  • Link 생성 후 초기화 과정에서 상태 검증 시 NULL을 사용하지 않도록 ENUMLinkStatus 를 도입했는데, 이 방법이 적절했는지 궁금합니다

  • 제가 작성한 Line 객체의 SRP 위배 여부에 대해 리뷰어님의 생각을 여쭙고 싶습니다.

    • Line 객체의 책임을 정의하자면, "각 행의 정보를 관리한다" 라고 할 수 있습니다. 그러나 그 내부를 살펴보면 아래와 같이 구성되어 있습니다.
      1. 링크 초기화(initializeLinks)
      2. 링크 상태 설정(setupLinks)
        b-1. 무작위 인덱스 선정
        b-2. 무작위 난수로 링크 생성 여부 결정
        b-3. 링크 생성 가능 여부 확인
        b-4. 위 정보를 기반으로 링크 상태 설정
    • 하나하나 따져보면 여러 개의 역할을 가지고 있다고 볼 수 있습니다. 하지만 제 생각에는 이것을 "각 행의 정보를 관리한다"라는 하나의 역할을 가지고 있다고 볼 수도 있다고 생각합니다. 따라서 무작위 난수 제공의 책임을 Line 객체가 지도록 만들었습니다. 이것이 SRP를 위배하는지에 대해 리뷰어님의 의견이 궁금합니다.

Copy link

@woogym woogym left a comment

Choose a reason for hiding this comment

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

사다리 1단계 미션 수고하셨어요 태우! 😁😁

사다리 미션이 많이 어려웠죠? 저도 처음에 굉장히 많은 고민을 하게 되었던 미션으로 기억해요

Line : 각 행(Line)의 정보 저장 및 각 행 생성
Link : 각 기둥 간의 연결 상태 정보 저장 및 연결 상태 생성

개인적으로 이 부분이 잘 이해가 되지 않았어요 Line이 행을 관리? -> 아마 이랑 헷갈리지 않았나 싶어요 (Link? Line? 뭐가 관리하는거지?)

Link 생성 후 초기화 과정에서 상태 검증 시 NULL을 사용하지 않도록 ENUM인 LinkStatus 를 도입했는데, 이 방법이 적절했는지 궁금합니다

enum 인스턴스 도입은 아주 훌륭했다고 생각해요, 원시값 포장도 준수하여서 좋은 설계였다고 생각합니다

제가 작성한 Line 객체의 SRP 위배 여부에 대해 리뷰어님의 생각을 여쭙고 싶습니다.

어색한 부분도 있어보였어요 -> 난수를 다루는 것, 링크 생성 가능 여부를 line에서 확인한다는것(link는 판별할 수 없었나?)
이렇듯 어색한 부분들이 있었어요 정답은 없지만 자기만의 이유를 가지고 설계를 한 것 같아서 좋았어요
고민하는 힘을 잘 길러봅시다!

개선해보고 고민해봅시다!

public List<Line> getLines() {
return List.copyOf(lines);
}

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

@TwooTwoo TwooTwoo Mar 15, 2025

Choose a reason for hiding this comment

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

남겨주신 코맨트를 보고 다시 확인해 보았으나 어떤 부분이 불필요한 개행인지 잘 모르겠습니다..!
혹시 몰라 Google Java Style Guide의 Whitespace 단락을 읽어 보았고, 아래 문구에 의해 제 개행이 문제되지 않는다고 판단했습니다.

A single blank line may also appear anywhere it improves readability, for example between statements to organize the code into logical subsections. A blank line before the first member or initializer, or after the last member or initializer of the class, is neither encouraged nor discouraged.

혹시 제가 놓쳤거나 잘못 알고 있는 부분이 있다면 편하게 지적 부탁드립니다. 감사합니다!

Comment on lines +16 to +21
this.lines = Collections.unmodifiableList(lines);
}

public List<Line> getLines() {
return List.copyOf(lines);
}
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 +15 to +22
public Line(int width) {
int size = width - 1;
List<Link> links = initializeLinks(size);

setupLinks(links);

this.links = List.copyOf(links);
}
Copy link

Choose a reason for hiding this comment

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

Link, Line이 비대칭적이에요
LineLink의 상태를 직접적으로 변경하고 있어요 -> 객체의 자율성도 깨지고, 무엇보다 캡슐화가 가장 지켜지지 못한 설계라고 볼 수 있죠
LineLink의 내부 동작을 너무 많이 알고 있어요, 결합도가 매우 높아요 (결국 디미터의 법칙 위배)

결합도를 낮추기 위해서 개선해봅시다!

Comment on lines +14 to +23
public void setLinkStatus(boolean connectDecider, boolean connectable) {
validateUpdateStatus();

if (connectDecider && connectable) {
this.linkstatus = LinkStatus.PRESENT;
return;
}

this.linkstatus = LinkStatus.ABSENT;
}
Copy link

Choose a reason for hiding this comment

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

Link 객체는 스스로 자신의 상태를 관리하는 게 아니라 외부(Line)에서의 상태에 의존하여 내부 상태를 변경하고 있어요
Side Effect의 발생 가능성을 가지고 있어요! 어떻게 개선할 수 있을까요? 🤔🤔

Comment on lines +51 to +53
private int getRandomStartIndex(List<Link> links) {
return random.nextInt(links.size());
}
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 +12 to +19
private LadderOutputView() {
}

private static final LadderOutputView ladderOutputView = new LadderOutputView();

public static LadderOutputView getInstance() {
return ladderOutputView;
}
Copy link

Choose a reason for hiding this comment

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

싱글톤 패턴을 적용하셨군요

  • 싱글톤 패턴을 도입한 태우님의 생각이 궁금해요!
  • 지연 초기화라는 학습 키워드를 통해서 구현된 싱글톤 패턴을 더 개선해봅시다! (개선하고 저한테 설명도 해주시면 좋을 것 같아요)


import java.util.List;

public class LineDto {
Copy link

Choose a reason for hiding this comment

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

model에서 line에 대해서 알 수 없게 하도록 하기 위해서 해당 dto를 도입한 것으로 예상되요
그치만 제 생각에는 dto를 사용하지 않아도 해결할 수 있는 문제라고 생각하는데 태우님은 어떻게 생각하시나요?
혹시 dto에게 책임을 전가하고 있진 않았나 돌아볼까요? 🤔🤔

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