[1단계 사다리 생성] 다즐(최우창) 미션 제출합니다. #70
Conversation
Co-authored-by: 70825 <hello70825@gmail.com>
Co-authored-by: 70825 <hello70825@gmail.com>
Co-authored-by: 70825 <hello70825@gmail.com>
Co-authored-by: 70825 <hello70825@gmail.com>
Co-authored-by: 70825 <hello70825@gmail.com>
Co-authored-by: 70825 <hello70825@gmail.com>
Co-authored-by: 70825 <hello70825@gmail.com>
Co-authored-by: 70825 <hello70825@gmail.com>
Co-authored-by: 70825 <hello70825@gmail.com>
Co-authored-by: 70825 <hello70825@gmail.com>
Co-authored-by: 70825 <hello70825@gmail.com>
Co-authored-by: 70825 <hello70825@gmail.com>
Co-authored-by: 70825 <hello70825@gmail.com>
Co-authored-by: 70825 <hello70825@gmail.com>
Co-authored-by: 70825 <hello70825@gmail.com>
Co-authored-by: 70825 <hello70825@gmail.com>
Co-authored-by: 70825 <hello70825@gmail.com>
Co-authored-by: 70825 <hello70825@gmail.com>
Co-authored-by: 70825 <hello70825@gmail.com>
Co-authored-by: 70825 <hello70825@gmail.com>
Co-authored-by: 70825 <hello70825@gmail.com>
Co-authored-by: 70825 <hello70825@gmail.com>
Co-authored-by: 70825 <hello70825@gmail.com>
Co-authored-by: 70825 <hello70825@gmail.com>
woo-chang
changed the title
| 사다리 | ||
|
|
||
| - [x] 선들의 집합이다. |
There was a problem hiding this comment.
해당 명세서를 개발자만 읽는 것이 아닌, 모두가 읽을 수 있게 만들기 위해 고민했던 것 같습니다 😀
| ladderController.run(); | ||
| } | ||
|
|
||
| private static InputView inputView() { |
There was a problem hiding this comment.
의미있는 추상화였을까요?
단순 메서드 전달자로서의 메서드는 결합도와 복잡성을 중가시켜요 :)
public static void main(String[] args) {
var inputView = new InputView(new Scanner(System.in));
var outputView = new OutputView();
var lineGenerator = new LineGenerator(new RandomDirectionGenerator());
LadderController ladderController = new LadderController(inputView, outputView , lineGenerator);
ladderController.run();
}로 설정해도 충분할거같아서요 :)
There was a problem hiding this comment.
위의 코드와 제 코드를 비교해보았을 때 확실히 복잡성이 높은 것을 느낄 수 있었습니다 .. 감사합니다 :)
| private <T, R> R retry(final Function<T, R> function, final Supplier<T> supplier) { | ||
| try { | ||
| return function.apply(supplier.get()); | ||
| } catch (IllegalArgumentException e) { | ||
| outputView.printErrorMessage(e.getMessage()); | ||
| return retry(function, supplier); | ||
| } | ||
| } |
There was a problem hiding this comment.
다른 크루와 같은 피드백 남깁니다..!
예외를 뱉어, 다시 예외를 유발하는 코드를 호출하는 것은 위험할 수 있습니다.
아무리 단순한 구현이라도, 작성하지 않는것을 추천드립니다.
이경우, 해당 함수를 이해하기 위해선, 예외 상황과, 정상적인 상황을 여러번 확인해야할 것 같아요.
또한, IllegalArgumentException가 아닌 다른 예외가 발생하면 그에따른 플로우는 어떻게 되는걸까요?
(기대하신 플로우로 진행되지 않을 것)
이부분은 "이펙티브자바 69. 예외는 진짜 예외 상황에만 사용하라"를 참고해보실 수 있을 것 같아요 .
입력을 하는것이 도메인 로직만큼 중요한 로직일까요?
기대한 값이 아니면 "예외"가 아닌 재 입력 호출(while 등.)을 하는것은 어떨까요?
There was a problem hiding this comment.
입력을 받는 과정에서는 예외가 발생하지 않지만 입력값을 받아 플레이어와 높이 도메인을 생성하는 과정에서 유효하지 않은 값이 들어왔을 경우 예외를 발생시키도록 구성해 보았습니다.
유효하지 않은 값에 의한 도메인 생성은 예외 상황이라 판단하였는데 예외 상황이 아닌지 궁금합니다! 또한 예외 상황이 아니라면 예외를 발생시키지 않고 생성을 막을 방법도 궁금합니다!
while을 사용할지, 재귀를 사용할지 프리코스 과정 떄부터 무수히 많은 고민을 해보았습니다. 재귀는 스택 오버플로우의 위험성이 존재하기에 while을 사용하는 것이 옳은 것 같습니다. 감사합니다 :) 하지만 while을 사용하더라도 재입력을 요청하기 위해서는 다시 예외를 유발하는 코드가 호출되어야 하는데 재귀와 어떤 차이점이 있는지 궁금합니다!
There was a problem hiding this comment.
입력값을 받아 플레이어와 높이 도메인을 생성하는 과정에서 유효하지 않은 값이 들어왔을 경우 예외를 발생시키도록 구성해 보았습니다.
다즐의 가정에는, 예외가 어디서 발생하는지 알고 해당 로직이 작성되었다는 느낌이 드는데요.
해당 예외를 개발을 한 다즐은 알고 있겠지만, 그러한 히스토리를 모르는 상황에서는 하나의 사이드 이펙트 지점이에요.
작성하신 메서드는 전체 예외를 찾아보고, 언제 발생하는지, 언제 발생안하는지 등을 알아야 사용할 수 있게 됩니다.
이런 메서드가 많다면, 코드를 파악하기 힘들지 않을까요?!
제가 리뷰한 모든 분들이 해당 코드를 동일하게 작성하시던데, 혹시 수업에서 예시로 나왔을까요?!
private <T, R> R retry(final Function<T, R> function, final Supplier<T> supplier) {
try {
return function.apply(supplier.get());
} catch (IllegalArgumentException e) {
outputView.printErrorMessage(e.getMessage());
return retry(function, supplier);
}
}이 코드만 봤을때, 사실 어떤 행동을 하는지 와닿지 않아요.
제네릭을 쓸만큼 복잡한 로직이었을까요?
해당 제네릭이 controller 내에 있어야하는걸까요?
| final Players players = retry(Players::new, inputView::readPlayerNames); | ||
| final Height height = retry(Height::new, inputView::readHeight); | ||
| final Ladder ladder = new Ladder(lineGenerator, players, height); | ||
| outputView.printLadderResult(players, ladder); | ||
| } |
There was a problem hiding this comment.
함수를 위에서 읽다보면,
"LadderController가 run을 수행 하기 위해선, "retry"가 필수적 이구나"
라는 느낌을 받게돼요. 함수 세부 사항을 보면, 입력을 위해 처리한 것을 알 수 있으나,
메서드를 들어가서 봐야 알 수 있을 것 같아요.
단순히 readInputValue()으로 설정하는건 어떨까요?
retry의 구현 세부사항은, 처음 읽는 당시에는 쉽게 읽히지 않아요.
There was a problem hiding this comment.
다시 읽어보니 저도 왜 반복을하지라는 의문이 드는 메서드명이였습니다 .. 😂
There was a problem hiding this comment.
사실 generate도 너무 추상적인 단어여서요,
하나의 메서드에, stream read하고, 새로운 객체를 만들고 있는게 보이네요 :)
| public static Direction from(final int value) { | ||
| return Arrays.stream(Direction.values()) | ||
| .filter(direction -> direction.getMove() == value) | ||
| .findFirst() | ||
| .orElseThrow(() -> new IllegalArgumentException("유효하지 않은 값입니다.")); | ||
| } |
There was a problem hiding this comment.
매번 stream을 열어줘야 하는걸까요 ？
(너무 깊게 생각하진 말아주세요 :) )
There was a problem hiding this comment.
조건문을 사용하는 방법도 생각해 보았는데, 조건문을 사용하게 되면 Direction에 새로운 값이 추가될 때마다 from 메서드의 수정이 발생하게 되었습니다. 그렇기에 매번 stream을 열어주는 리스크가 있더라도 위의 방법을 사용하게 되었습니다.
메서드를 수정 비용보다 매번 stream을 열어주는 비용이 더 큰걸까요 .. ?
There was a problem hiding this comment.
해당 로직을 static화 해보는게 어떨지 말씀드린거였어요 :)
크게 중요하진 않은 부분이라 깊게 생각하지 않으셔도 됩니다 :)
| private final DirectionGenerator directionGenerator; | ||
|
|
||
| public LineGenerator(final DirectionGenerator directionGenerator) { | ||
| this.directionGenerator = directionGenerator; | ||
| } |
There was a problem hiding this comment.
DirectionGenerator directionGenerator;
가 한군데이서만 쓰이고 있네요, generate() 에서 의존성을 넣어주는건 어떨까요?
service가 다른 service를 의존하고 있는것 같아서요!
There was a problem hiding this comment.
generate에서 의존성을 넣어주게 되면 다른 방향 생성 전략을 취하게 되더라도 새로운 LineGenerator를 생성할 필요가 없군요!
의존성에 대해서도 고민해볼 수 있었습니다. 감사합니다 :)
| if (direction == RIGHT) { | ||
| directions.add(LEFT); | ||
| } |
There was a problem hiding this comment.
Direction이 Right면, LEFT를 추가한다...? 어떤 의미일까요 ?!
There was a problem hiding this comment.
저와 페어와 구상한 사다리는 위 사진과 같습니다! 발판이 있다 없다에 초점을 맞춘 것이 아닌 빨간점에 해당하는 포인트에서 이동하는 방향으로 하나의 라인을 구성하도록 하였습니다.
그렇기에 RIGHT 다음에는 LEFT가 무조건 따라오기에 해당하는 코드를 작성하였습니다.
There was a problem hiding this comment.
해당 그림을 README에도 올리면 이해하기 정말 쉬울 것 같아요 :)
| private void generateDirectionsStrategy(final List<Direction> directions, final int directionCount) { | ||
| if (directions.size() >= directionCount - 1) { | ||
| addStayIfNotEnough(directions, directionCount); | ||
| return; | ||
| } | ||
| addDirections(directions); | ||
| generateDirectionsStrategy(directions, directionCount); | ||
| } |
There was a problem hiding this comment.
저희는 라인을 사다리 가로줄로 선택하여 미션을 진행하였습니다. 가로줄에 선택한 개수만큼 방향을 채우기 위해 재귀함수를 사용하는 것을 선택했습니다. 재귀함수는 스택 오버플로우라는 위험성이 존재하지만, 사다리 높이에 대한 철저한 개수 제한을 걸어두었기에 재귀함수를 선택하였는데, 재귀함수 사용은 지양해야 하는지 궁금합니다!
이부분이네요..!
저는 재귀함수 부분을 절대로 반복문으로 해결할 수 없을 때 사용하곤 합니다.(거의 없어요 ㅎ)
이경우엔 사다리 높이에 개수 제한을 뒀다고 하셨는데, 반복문으로 해결할 수 없었더 부분일까요?
"테스트 동작만 확인
private List<Direction> generateDirections(final int directionCount) {
final List<Direction> directions = new ArrayList<>();
while (directions.size() < directionCount) {
generateDirectionsStrategy(directions, directionCount);
}
return directions;
}
private void generateDirectionsStrategy(final List<Direction> directions, final int directionCount) {
if (directions.size() >= directionCount - 1) {
addStayIfNotEnough(directions, directionCount);
return;
}
addDirections(directions);
}요렇게 진행해도 테스트는 정상 동작돼서요.
요구사항을 충분히 만족한 테스트가 아니었을까요?
There was a problem hiding this comment.
동일한 동작입니다 .. ! 재귀로 하면 코드가 깔끔해지지 않을까라는 생각에 사용했는데 위의 코드가 훨씬 보기 좋은 것 같습니다 👍
| private String getFoothold(final Direction direction) { | ||
| if (direction.equals(Direction.LEFT)) { | ||
| return FOOTHOLD; | ||
| } | ||
|
|
||
| return BLANK_FOOTHOLD; | ||
| } |
There was a problem hiding this comment.
현재 Direction으로 해당 로직을 옮기기는 했지만, 만약 -----에서 +++++로 출력 수정이 발생하게 된다면 view쪽을 먼저 찾아볼 것 같다는 생각이 들었습니다. 혹시 이 점에 대해서는 어떻게 생각하시는지 궁금합니다!
There was a problem hiding this comment.
그렇다면, 아까 Line에서 해당 검증 로직을 넣고 반환하는게 좋겠네요 :) (기존에는 속성만 반환함)
현재는 OutPutView가 비즈니스 로직을 가지고 있는것처럼 보여서요 :)
| void validLadder() { | ||
| final ArrayList<Direction> directions = Lists.newArrayList( | ||
| RIGHT, STAY, STAY, RIGHT, RIGHT, STAY, RIGHT, STAY, RIGHT, RIGHT); | ||
| final DirectionGenerator directionGenerator = new TestDirectionGenerator(directions); | ||
| final LineGenerator lineGenerator = new LineGenerator(directionGenerator); | ||
| final Players players = new Players(List.of("pobi", "crong")); | ||
| final Height height = new Height(5); | ||
|
|
||
| final Ladder ladder = new Ladder(lineGenerator, players, height); | ||
|
|
||
| assertThat(ladder.getLines()).hasSize(5); |
There was a problem hiding this comment.
given when then 패턴을 활용해보셔도 좋을 것 같아요 :)
|
++
다국어 지원이 현재 요구사항에 포함되어 있나요? 그렇지 않다면, 최대한 간결하게 구현하는것 추천드려요.추측에 근거한 설계는 팀원의 혼란을 야기시킬 수 있어요. 또한, 예외메세지를 한곳에서 관리하게 된다면, 예외메세지 구현 객체 변경이 도메인 로직의 장애로 이뤄질 수 있습니다. (결합도 증가로인한) 전체 예외로 관리되어야 한다면, 임의의 body를 가지는 공통 예외를 반환하게 작성되면 좋을 것 같아요. 즉, 예외메세지는 예외가 발생하는 곳에서 관리하는 것이 복잡도를 떨어뜨릴 수 있다 생각합니다. |
|
++
저의 경우에는, 가장 세밀한 부분까지 commit한다음, 기능이 완료될 될 때 git rebase -i를 이용하여 하나의 코드로 합칩니다!! 로컬에서 개발할때는, 마음것 커밋 하고, remote에 올릴때, 조작해서 올리는 습관을 한번 만들어 보시는건 어떨까요? |
테스트는 계속 유지되면서 도메인 개발을 진행하였습니다! 정말 꼼꼼하고 좋은 리뷰를 해주신 덕분에 많이 배워갈 수 있었습니다. 질문하신 내용에 대해 답변하고, 리팩토링이 필요한 부분에 대해서는 수정을 해보았습니다. 다시 한번 리뷰 부탁드립니다! 감사합니다 ⛄️ |
| private boolean isNotEnough(final List<Direction> directions, final int directionCount) { | ||
| return directions.size() == directionCount - 1; | ||
| } |
There was a problem hiding this comment.
충분하지 않다.. 라는 말이 와닿지 않아서, 다른 네이밍을 고려해보셔도 좋을 것 같아요
isNotPreparing 등으로도 괜찮겠네요 :)
안녕하세요 😄 새로운 리뷰어분과 만나게 돼서 떨리네요. 잘 부탁드립니다!
TDD를 활용하여 사다리 생성 미션을 구현해 보았습니다. TDD라는 개념은 들어 보았지만, 직접 사용해본 적은 처음이라 시간도 오래 걸리고 페어와 많은 얘기를 나누면서 개발했던 것 같아요.
미션을 진행하면서 아래의 내용들을 고민할 수 있었습니다. 바쁘신 와중에 답변해 주시면 감사합니다! ⛄️
TDD를 진행하면 실패 테스트 코드 작성 -> 테스트 통과하는 코드 작성 -> 리팩토링의 과정을 거치게 됩니다. TDD로 진행한 만큼 각 과정을 분리하여 커밋을 하려고 하였으나 그러면 커밋 메시지가 무수히 발생하게 되는 문제점을 고려할 수 있었습니다. 페어와 얘기 후 어느 커밋으로 이동하더라도 테스트가 통과한 상태를 유지하는 것이 좋다는 결론이 나와 하나의 기능을 완성 후 커밋하는 방법을 선택하였습니다. 그 결과 TDD를 진행했다는 것은 커밋으로 확인할 수 없는 것 같습니다. 현업에서 TDD를 사용하실지는 모르겠지만, 만약 TDD로 진행하신다면 커밋 단위를 어떻게 나누시는지 궁금합니다!
저희는 라인을 사다리 가로줄로 선택하여 미션을 진행하였습니다. 가로줄에 선택한 개수만큼 방향을 채우기 위해 재귀함수를 사용하는 것을 선택했습니다. 재귀함수는 스택 오버플로우라는 위험성이 존재하지만, 사다리 높이에 대한 철저한 개수 제한을 걸어두었기에 재귀함수를 선택하였는데, 재귀함수 사용은 지양해야 하는지 궁금합니다!
현재 저희 구조에서 도메인에서 발생하는 예외에 대한 메시지를 도메인에서 관리하고 있습니다. 하지만 예외 메시지 다국어 지원과 같은 상황을 고려한다면 예외 메시지를 별도로 관리하는 것이 필요하다고 생각합니다. 하지만 한편으로는 예외 메시지를 한 곳에서 관리하게 되면 해당 도메인에서만 알아야 하는 예외 메시지를 노출하게 되는 것이 아닐까라는 의문이 발생하였습니다. 이런 부분은 어떻게 해결할 수 있을지 궁금합니다!
그 외에도 부족한 점이나 추가 및 확장했으면 하는 부분에 대해 자세히 알려주시면 많이 배워갈 기회가 될 것 같습니다. 리뷰 부탁드립니다! ⛄️