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단계 - 사다리 게임 실행] 도이(유도영) 미션 제출합니다. #217

Merged
merged 61 commits into from
Feb 27, 2023

Conversation

yoondgu
Copy link

@yoondgu yoondgu commented Feb 23, 2023

안녕하세요 제이온 !! 도이에요.
지난 1단계 피드백을 바탕으로 진행한 2단계 리뷰 요청드립니다.

기존의 구현 내용을 기반으로 새로운 기능을 추가하는 일이 유독 쉽지 않았고,
사다리 결과를 확인하는 로직을 구현하는 데 있어 알고리즘 사고능력의 부족함도 느낄 수 있었어요. 🥲
클래스 설계 또한 전체적으로 괜찮은지 판단하기가 전보다 어려웠어요.

사다리 결과 확인 기능 구현 방식

  • 1단계에서 구현했던 StepPoint 객체를 그대로 사용하되, 이 좌표값들의 정보를 이용해 다음 라인에서 이동할 위치를 알려주는 Direction 객체를 중간에 추가했습니다.
  • 클래스 설계의 변화는 README.md의 다이어그램으로 함께 정리했습니다.
  • 사다리를 Direction(LEFT/DOWN/RIGHT) 객체로 구성되도록 미리 모두 만들어둔 뒤, 출발지 인덱스를 입력하면 이 객체들을 이용해 원하는 결과를 바로 찾아가도록 구현했습니다. StepPoint(EXIST/NONE)로 이루어져있다면 결과를 확인할 때마다 매번 다시 이동할 방향을 판단해야 하기 때문에, 여러 번 조회가 가능한 프로그램에서는 Direction을 미리 다 만들어놓는 게 생성 로직은 복잡하더라도 조회 측면에서는 더 효율적인 방법이 될 수 있지 않을까? 생각했습니다.
  • 하지만 StepPoint를 전략패턴으로 생성하고, 이를 이용해 또 다시 Direction 객체를 생성하도록 구현되어있는데, 불필요하게 두 번의 단계를 거치고 있는 것 같습니다. 따라서 리뷰 반영 과정에서 가능하면 두 클래스를 하나로 정리해보려고 합니다..!
  • Direction 객체에서 이동할 방향, 즉 다음 라인의 인덱스를 판단할 때에는 외부에서 조건문을 통해 +1, +0, -1 을 계산해주어도 되지만 이러한 연산을 보다 명확히 하기 위해 Enum에서 람다식을 가지도록 해봤습니다.

질문드리고 싶은 점

  • 코드와 질문을 같이 보는 게 더 편하실 것 같아서 해당하는 코드마다 질문을 코멘트로 남겼습니다.
  • 코드 외 질문을 드려도 된다면.. 저는 이번 2단계를 진행하면서 "분명히 더 효율적이고 깔끔한 방법이 있을텐데" 생각하면서도 아직 그 답답함을 해소하지 못한 것 같아요. 그래서 앞서 말씀드린 것처럼 알고리즘 사고능력이 부족한 것일지, 아니면 도메인에 대한 고민의 시간이 더 필요한 것일지 여러 생각이 드는데요. 이런 문제 해결 능력을 기르기 위해 추천하시는 방법이나 조언이 궁금합니다.
  • 그 외에도 여러 부족한 점들에 대하여 피드백 잘 부탁드립니다!!!

이번에도 미리 정말 감사합니다!!!

directions.add(Direction.findDirection(left, generated));
left = generated;
}
directions.add(Direction.findDirection(left, StepPoint.NONE));
Copy link
Author

Choose a reason for hiding this comment

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

리뷰요청 글에 쓴, StepPoint를 만들고 또 Direction을 만들어야 하는 부분(고치고 싶은 부분)입니다.
(질문은 아니고 참고차 함께 코멘트 남깁니다 🥹)

Copy link

Choose a reason for hiding this comment

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

흠 저는 지금 코드도 괜찮다고 생각합니다. StepPoint는 존재 여부고, Direction은 이를 적절히 잘 이용하고 있는 것 같아요! 만약 합친다고 하면 StepPoint 대신 boolean을 Direction에 넣어볼 수도 있겠네요.

Copy link
Author

@yoondgu yoondgu Feb 26, 2023

Choose a reason for hiding this comment

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

사실 저도 바꾸어보려고 고민하다가, 전체 구현을 처음부터 다시 하지 않는 이상 지금으로서는 각 클래스가 맡은 역할을 잘 하고 있지 않나? 싶어서 생각이 바뀌었습니다..ㅎㅎ

만약 합친다고 하면 StepPoint 대신 boolean을 Direction에 넣어볼 수도 있겠네요.

이 부분에 대해서는 저는 생각이 조금 다른데요..!! boolean값을 StepPoint가 포장하면서 true, false 대신 EXIST, NONE으로 더 구체적인 의미를 표현해주는 것 같아서 유지하고 싶습니다. boolean을 사용하면 코드에서 true, false가 무엇을 의미하는지 알기 어려운 경우가 많았던 것 같아요. 제이온 생각은 어떠신가요?

Copy link

Choose a reason for hiding this comment

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

ㅎㅎ 도이 말씀도 맞는 것 같네요.
이번 우테코 미션 취지상 StepPoint로 포장하는 것이 더 적합해 보입니다.
다만, 나중에 좀 더 미션을 진행하거나 도이께서 개인 프로젝트, 팀 프로젝트 넘어서 현업에 가시게 된다면 객체 포장을 함으로써 복잡도가 올라가는 점을 인지하시면 도움이 됩니다! 예를 들어 이번 미션 요구 사항은 모든 값을 포장하라고 하였지만, getter 밖에 없는 등 하는 일이 별로 없는 객체도 포장해야되는지와 같은 의문입니다~
뭐든지 극한으로 작업해봐야 중간을 찾을 수 있으니 좋은 자세같습니다 👍

Copy link
Author

Choose a reason for hiding this comment

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

앗 그러네요..! 제이온 말씀을 들어보니 앞으로는 다양하게 시도해보되 실제 프로젝트, 현업 관점에서도 많이 고려해봐야겠다는 생각이 들어요. 코멘트 감사합니다 ㅎㅎ

import laddergame.domain.generator.StepPointGenerator;
import laddergame.domain.ladder.line.StepPoint;

public class MockedPointGenerator implements StepPointGenerator {
Copy link
Author

@yoondgu yoondgu Feb 23, 2023

Choose a reason for hiding this comment

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

원래는 LineTest의 내부클래스로 정의해두었던 전략패턴 구현클래스를
LadderLinesTest에서도 사용하게 되면서 하나의 클래스로 정의했습니다.
그런데 이런 경우에 이 클래스는 test 패키지에 있는 게 맞을까요?
아니면 이 역시 이런 상황이 생기게 된 클래스 설계를 검토해봐야 하는 문제일까요?

Copy link

Choose a reason for hiding this comment

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

모킹용으로 쓰이는데 공통으로 쓰인다면 공통으로 쓰이는 테스트 패키지에 두어도 좋고, testfixture 패키지를 따로 분리하여 거기에 위치시켜도 됩니다! testfixture는 시간 여유가 있다면 적용해보아도 좋으나, 이번 미션에서는 패스하셔도 괜찮습니다 ㅎㅎ
https://thalals.tistory.com/375

Copy link
Author

Choose a reason for hiding this comment

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

test fixture 방식에 대해 처음 알게 됐어요 !! 다음에 적용해보고 싶네요 감사합니다 😆

Copy link

@pjy1368 pjy1368 left a comment

Choose a reason for hiding this comment

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

안녕하세요 도이!
우선 리뷰가 늦어진 점 죄송합니다 😢
전반적으로 객체 지향적인 코드를 노력해서 작성하는 모습이 인상적이었습니다!
몇 가지 도움이 될 만한 피드백을 남겨뒀으니 한 번 반영해보세요~

PR 본문에 남겨 주신 질문 답변드립니다.
답답함을 느끼실수록 더 성장할 수 있다고 생각해요 ㅎㅎ
알고리즘이나 문제 해결력의 부재라기보다는 지금처럼 계속 더 나은 코드를 작성하려고 노력하시다 보면 자연스럽게 더욱 객체 지향적인 코드를 작성하실 수 있지 않을까해요!
조급하게 생각하지 않으셔도 되고, 스스로 답답함의 원인을 구체적으로 찾아보고 해소하려는 노력을 계속 해 보시길 바랍니다~

src/main/java/laddergame/view/constant/FindCommand.java Outdated Show resolved Hide resolved

private void showGameResult(GameResult gameResult, String keyword) {
validateGameStatus(gameStatus);
if (Objects.equals(FindCommand.QUIT, keyword)) {
Copy link

Choose a reason for hiding this comment

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

keyword가 소문자로 들어와도 처리가 되면 좋겠습니다 ㅎㅎ.. 처음에 왜 계속 프로그램이 안꺼지나 했네요 ㅋㅋㅋㅋㅋ
그리고 이왕 FindCommand를 enum으로 쓰신거 enum 내부에 함수를 둬서 keyword에 해당하는 열거형 상수가 있는지 확인해보면 어떨까요?

Copy link
Author

@yoondgu yoondgu Feb 26, 2023

Choose a reason for hiding this comment

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

앗 대소문자 관련해서는 전혀 생각해보지 못했네요..!! 반영하겠습니다

그리고 이왕 FindCommand를 enum으로 쓰신거 enum 내부에 함수를 둬서 keyword에 해당하는 열거형 상수가 있는지 확인해보면 어떨까요?

이 부분은 어떻게 하는 게 좋을지 감이 살짝 안오는데 수정해보고 더 질문 드리겠습니다..!!

Comment on lines +36 to +38
if (ladder == null) {
throw new IllegalStateException("아직 사다리가 생성되지 않은 게임입니다.");
}
Copy link

Choose a reason for hiding this comment

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

Optional을 사용하여 null-check를 하면 어떨까 합니다.
https://www.daleseo.com/java8-optional-after/

Copy link

Choose a reason for hiding this comment

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

요 부분은 Optional.ofNullable(ladder) 방식으로 처리해도 좋고, LadderGame의 Ladder 자체를 Optional로 둬서 생성자 단계에 Optional.empty()로 초기화하는 방식을 생각했습니다!
가능한 전통적인 null 처리보다는 Optional을 다양하게 사용하시길 권장해서 피드백 드렸었습니다 👍

Copy link
Author

@yoondgu yoondgu Feb 28, 2023

Choose a reason for hiding this comment

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

넵 제이온 덕분에 Optional 사용에 대해 고민해볼 수 있었던 것 같아요. 필드에서 Optional을 쓰는 것을 지양하라는 글을 접해서 고민이었는데 앞으로 더 공부해봐야겠습니당 !!

src/main/java/laddergame/domain/LadderGame.java Outdated Show resolved Hide resolved
src/main/java/laddergame/domain/players/Players.java Outdated Show resolved Hide resolved
src/main/java/laddergame/domain/ladder/GameResult.java Outdated Show resolved Hide resolved
src/main/java/laddergame/domain/ladder/Result.java Outdated Show resolved Hide resolved
directions.add(Direction.findDirection(left, generated));
left = generated;
}
directions.add(Direction.findDirection(left, StepPoint.NONE));
Copy link

Choose a reason for hiding this comment

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

흠 저는 지금 코드도 괜찮다고 생각합니다. StepPoint는 존재 여부고, Direction은 이를 적절히 잘 이용하고 있는 것 같아요! 만약 합친다고 하면 StepPoint 대신 boolean을 Direction에 넣어볼 수도 있겠네요.

- GameResult와 Result 네이밍 중복으로 인한 부정확성 발생
- Result 클래스명을 Item으로 변경, 관련 변수 및 메서드명 일괄 수정
- Destination에 속하는 Item이므로 destination 패키지 추가 후 이동
- 다른 변수명, 메서드명과 통일하여 FindCommand 대신 SearchKeyword로 변경
- 컨트롤러에서만 사용되므로 패키지 이도
- keyword 대소문자 구분 없이 모두 처리 가능하도록 변경
- enum 클래스에서 검색유형에 해당하는 상수 확인하도록 변경
- Line 클래스에서 라인의 내부에 있는 StepPoint만 반환하는 기능 제공
- 이를 통해 뷰에서는 단순히 포맷 로직만 책임지도록 변경
@yoondgu
Copy link
Author

yoondgu commented Feb 26, 2023

안녕하세요 제이온! 주말에도 시간내어 꼼꼼한 리뷰 남겨주셔서 감사합니다.
추가로 남겨주신 조언도 정말 감사합니다. 말씀해주신 것처럼 조급함보다는 스스로 해결해나가는 노력에 더 힘을 써보겠습니다. 🥹

남겨주신 리뷰를 반영하며 리팩터링을 진행해보았고, 이번에도 README.md에 리팩터링 목록을 함께 업데이트하였습니다.

  • 검색 키워드 클래스 FindCommand는 아예 검색 유형을 확인하고 비교할 수 있는 SearchType이라는 이름의 Enum 클래스로 바꾸어보았습니다. c86edf4
  • 사다리 결과를 포맷팅하는 LadderResultFormatter는 단순히 포맷팅만 할 수 있도록, Line 클래스에서 List를 반환하도록 변경했습니다. 501bae1
  • 커스텀 예외에 대해서 저는 이전까지는 표준 예외를 쓰는 것이 좋다 파 였는데요, 이번에 Index 검증이라는 공통 로직에 대해서는 커스텀 예외를 경험해보고 싶어서 적용해보았습니다! b7a99bd

반면 반영해보려고 고민하다가 진행하지 못한 부분도 있는데요.

LadderGame에서 사다리를 아직 만들지 않은 상태(ladder == null)이면 예외를 던지는 코드에 대해서
Optional을 사용해서 null check를 해보라고 피드백 남겨주신 부분입니다.

  • Optional은 반환값이 null일 가능성이 있을 때 활용할 수 있는 것으로 이해가 되는데,
    위 상황은 반환값이 아닌 내부 필드 ladder가 null일 경우에 대한 검증이어서 이를 어떻게 활용하면 좋을지 모르겠습니다.
  • 위 코드는 결국 computeResult() 메서드의 NPE를 피하기 위해 예외를 던지는 것이고, 반환값의 null 여부를 확인하는 것이 아니기 때문입니다.
  • 해당 클래스의 ladder() 메서드에 적용해볼까 했지만 이 메서드는 단순히 값만 꺼내기 때문에 직접 null 체크가 아니라 Optional을 쓸 필요가 없어 보였습니다.

위와 같은 이유로 Optional을 따로 적용하지 않았는데
혹시 제가 피드백을 잘못 이해한 부분이 있는지, 아니면 어떤 의미로 주신 피드백인지 여쭤볼 수 있을까요?

코드를 바꿨다가 돌렸다가 고민을 많이 하다가 리뷰 반영이 늦어졌습니다. ㅠㅠ
이번에도 잘 부탁드리겠습니다. 감사합니다!!

Comment on lines +45 to +49
return players.players()
.stream()
.map(Player::getName)
.collect(Collectors.toMap(Function.identity(), this::findItemByPlayerName, (a, b) -> a,
LinkedHashMap::new));
Copy link

Choose a reason for hiding this comment

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

LinkedHashMap을 만드는 방법을 잘 찾으셨네요 ㅎㅎ 잘하셨습니다 👍

Copy link

@pjy1368 pjy1368 left a comment

Choose a reason for hiding this comment

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

안녕하세요 도이!
피드백을 전반적으로 잘 반영해주셔서 훨씬 깔끔한 코드가 된 것 같습니다 👍
나머지는 선택적으로 반영하거나 고민할 포인트 정도만 남겨둬서 한 번 학습해 보시고 이번 미션 혹은 다음 블랙잭 미션 때 도전해보시면 좋아 보입니다!
이번 미션은 여기서 마치지만, 언제든지 질문 사항있으면 DM 주세요 🙇‍♂️

import laddergame.domain.generator.StepPointGenerator;
import laddergame.domain.ladder.line.StepPoint;

public class MockedPointGenerator implements StepPointGenerator {
Copy link

Choose a reason for hiding this comment

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

모킹용으로 쓰이는데 공통으로 쓰인다면 공통으로 쓰이는 테스트 패키지에 두어도 좋고, testfixture 패키지를 따로 분리하여 거기에 위치시켜도 됩니다! testfixture는 시간 여유가 있다면 적용해보아도 좋으나, 이번 미션에서는 패스하셔도 괜찮습니다 ㅎㅎ
https://thalals.tistory.com/375


public class InputView {

private static final List<String> RESERVED_WORDS = List.of("all", "q");
Copy link

Choose a reason for hiding this comment

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

제가 생각한 방식입니다! 잘하셨어요 ㅎㅎ
하지만 예약어가 SearchType 객체도 관리하고 있는데 여기도 이중으로 관리가되니, 예약어가 바뀌면 2번 바뀌어야한다는 문제점이 있습니다. InputView가 Controller로부터 예약어 목록을 받아오면 어떨까요?

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 +69 to +76
public int indexOf(final Name name) {
final String nameValue = name.getValue();
return IntStream.range(0, players.size())
.filter(index -> Objects.equals(getNameByPlayer(players.get(index)), nameValue))
.findFirst()
.orElseThrow(() -> new IllegalArgumentException(
ExceptionMessageFormatter.format("참가자 이름이 존재하지 않습니다.", nameValue)));
}
Copy link

Choose a reason for hiding this comment

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

요거는 제가 인덱스 반환하지 않는걸로 잘못봤네요 🤔
이대로 두어도 괜찮습니다 ㅎㅎ
다만 여유가 되신다면 index를 raw하게 다루는 것은 outOfBounds의 위험성이 있어서 되도록 한 단계 포장해서 관리하면 어떨까 합니다. index 정수를 한 번 반환하게되면 사용하는 쪽들에서 잠재적으로 outOfBounds 처리를 해주어야하다보니 예외가 퍼질 가능성이 있습니다.
이론적으로 그렇습니다만, 저 또한 이번 미션에서 index를 사용하지 않고 해결하기는 쉽지 않아보이네요 😥

Copy link
Author

Choose a reason for hiding this comment

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

맞아요 그래서 index를 포장해주려다 각 상황마다 다른 범위를 가지는 index에 대한 outOfBounds 처리를 하기에 어려움을 느껴서 보류하게 되었습니다 🥲

Comment on lines +52 to +56
final Set<Player> checker = new HashSet<>();
return players.stream()
.filter(player -> !(checker.add(player)))
.map(Players::getNameByPlayer)
.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.

스트림 내부에서 외부 객체의 상태를 변하게 하지 않고 해결할 수는 없을까요?


private static void validateDuplicated(final List<Player> players) {
final List<String> duplicatedNames = findDuplicatedNames(players);
if (duplicatedNames.size() != 0) {
Copy link

Choose a reason for hiding this comment

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

isEmpty() 메서드를 사용하면 어떨까요?

Copy link
Author

Choose a reason for hiding this comment

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

헉 그러네요 .. 짚어주셔서 감사합니다 !!

directions.add(Direction.findDirection(left, generated));
left = generated;
}
directions.add(Direction.findDirection(left, StepPoint.NONE));
Copy link

Choose a reason for hiding this comment

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

ㅎㅎ 도이 말씀도 맞는 것 같네요.
이번 우테코 미션 취지상 StepPoint로 포장하는 것이 더 적합해 보입니다.
다만, 나중에 좀 더 미션을 진행하거나 도이께서 개인 프로젝트, 팀 프로젝트 넘어서 현업에 가시게 된다면 객체 포장을 함으로써 복잡도가 올라가는 점을 인지하시면 도움이 됩니다! 예를 들어 이번 미션 요구 사항은 모든 값을 포장하라고 하였지만, getter 밖에 없는 등 하는 일이 별로 없는 객체도 포장해야되는지와 같은 의문입니다~
뭐든지 극한으로 작업해봐야 중간을 찾을 수 있으니 좋은 자세같습니다 👍

Comment on lines +59 to +66
public List<StepPoint> toStepPointsInLine() {
List<StepPoint> stepPoints = new ArrayList<>();
for (int i = 0; i < (directions.size() - 1); i++) {
Direction direction = directions.get(i);
stepPoints.add(direction.getRightStepPoint());
}
return stepPoints;
}
Copy link

Choose a reason for hiding this comment

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

이 메서드는 오직 View를 위해서만 사용하고 있네요!
View에서 도메인 객체를 파라미터를 받는다면 getter만 사용하고, 나머지는 적절히 컨트롤러가 뷰에 출력할 객체를 파라미터로 넘겨주도록 해보는 것이 어떨까요? Domain이 View를 알고 있는 듯한 로직은 View가 변하면 Domain이 변하는 일을 낳게 됩니다~

Comment on lines +5 to +12
public class IllegalIndexException extends IndexOutOfBoundsException {

private static final String message = "범위를 벗어났습니다.";

public IllegalIndexException(final List<?> target, final int index) {
super(message + " (size: " + target.size() + ", index: " + index + ")");
}
}
Copy link

Choose a reason for hiding this comment

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

커스텀 예외를 잘 정의해주셨네요!
예외의 범위도 적절하게 저수준으로 선택해주신 것 같습니다. 저수준으로 선택하되, 추후 고수준으로 높일 필요가 생기면 IllegalArgumentException이나 RuntimeException도 고민해보시면 됩니다~

Comment on lines +36 to +38
if (ladder == null) {
throw new IllegalStateException("아직 사다리가 생성되지 않은 게임입니다.");
}
Copy link

Choose a reason for hiding this comment

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

요 부분은 Optional.ofNullable(ladder) 방식으로 처리해도 좋고, LadderGame의 Ladder 자체를 Optional로 둬서 생성자 단계에 Optional.empty()로 초기화하는 방식을 생각했습니다!
가능한 전통적인 null 처리보다는 Optional을 다양하게 사용하시길 권장해서 피드백 드렸었습니다 👍

@pjy1368 pjy1368 merged commit 3cf136f into woowacourse:yoondgu Feb 27, 2023
@yoondgu
Copy link
Author

yoondgu commented Feb 28, 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