[1단계 - 사다리 생성] 트레(이충렬) 미션 제출합니다.

[takoyakimchi]

안녕하세요 오리! 페어 에버와 함께 TDD 미션으로 사다리 타기를 진행한 트레입니다.
TDD가 처음이라 익숙하지 않았지만 최대한 요구사항 맞추어 구현하도록 노력하였습니다!
잘 부탁드립니다! 😊

구현하면서 페어와 의견이 맞지 않았던 점

• 이번 미션에서는 객체를 화면에 출력할 문자열로 변환하는 로직을 모두 MessageResolver에 구현하였고, OutputView가 이를 참조하여 사용하도록 구현했습니다. 하지만 저는 이러한 작업이 over engineering이라고 생각하고, OutputView에서 일일이 System.out.print()를 해주는 것이 더 직관적이라고 생각하였습니다.
• 랜덤하게 사다리의 연결 여부를 결정하는 로직을 PointStrategy라는 인터페이스로 작성하였고, RandomPointStrategy가 이를 구현하도록 했습니다. 하지만 이 로직을 사용하는 부분은 Line 뿐이라서, "지금은 인자도 너무 많고 랜덤을 사용하지 않는 곳에서도 PointStrategy를 참조하고 있다. 그냥 Line에서 RandomPointStrategy를 직접적으로 참조하는 것이 낫지 않을까?" 하는 페어의 의견이 있었지만, 저는 이것이 DIP 위반이라고 생각했습니다. 인터페이스화 한 이유는 다음과 같습니다.
  (1) 추후에 새로운 로직이 추가되었을 때 변경을 용이하게 하기 위함 (2) Override하여 테스트하기가 편해짐
  그냥 인터페이스를 포기하고 Line에 해당 로직을 구현할지, 지금과 같은 방식으로 유지할지 고민이 됩니다.
• 검증 로직은 도메인에서 진행하였으며, 변환과 검증을 분리하는 방식으로 구현하였습니다. Height를 변환 및 검증하는 부분에서, 저는 Integer.parseInt(rawValue)를 하고 NumberFormatException을 처리하는 것까지가 "변환"이라고 생각하였고, 지금의 방식대로라면 변환과 검증이 잘 분리되어 있다고 생각합니다. 하지만 페어에게는 NumberFormatException을 처리하는 과정이 검증이므로 현재의 코드는 변환과 검증이 분리되어 있지 않는 방식이라는 의견을 받았습니다.

구현하면서 고민했던 점

• 사다리의 가로줄이 나란히 연결되어 있는 경우를 테스트해야 할지에 대한 고민이 있었습니다. 제 생각은, "사다리의 바로 왼쪽 가로줄이 연결되어 있는 상태인 경우, 오른쪽 가로줄은 연결되지 않도록 한다"를 구현하고 이것을 테스트하면 이 부분에 대한 테스트는 충분하다고 생각하지만, 다른 크루들의 경우 "사다리가 양 옆으로 나란히 연결된 경우, 에러를 발생시킨다"를 구현하고 테스트를 한 크루도 있는 것으로 압니다.

이에 대한 오리의 의견이 궁금합니다.
읽어주셔서 감사합니다!

[jinyoungchoi95]

안녕하세요 트레 😄

사다리 미션의 리뷰어를 맡은 오리입니다.

사다리 미션 잘 구현해주셨네요. 다음 단계 넘어가기 전 반영하면 좋을만한 부분 코멘트 남겨두었으니 확인부탁드려요.

궁금하거나 고민이 되는 부분이 있으시다면 언제든 pr 코멘트 또는 dm으로 요청 부탁드립니다.
감사합니다 🙇‍♂️

[jinyoungchoi95]

이번 미션에서는 객체를 화면에 출력할 문자열로 변환하는 로직을 모두 MessageResolver에 구현하였고, OutputView가 이를 참조하여 사용하도록 구현했습니다. 하지만 저는 이러한 작업이 over engineering이라고 생각하고, OutputView에서 일일이 System.out.print()를 해주는 것이 더 직관적이라고 생각하였습니다.

네 저도 굳이 MessageResolver를 만들 필요는 없어보여요. 클래스로 추출하면서 생기는 장점이 따로 없고 OutputView가 print만 있는 껍데기라면 OutputView의 존재 이유도 사실 어중간해서 개인적인 생각은 트레 말씀대로 합치는 것이 낫다입니다.

[jinyoungchoi95]

랜덤하게 사다리의 연결 여부를 결정하는 로직을 PointStrategy라는 인터페이스로 작성하였고, RandomPointStrategy가 이를 구현하도록 했습니다. 하지만 이 로직을 사용하는 부분은 Line 뿐이라서, "지금은 인자도 너무 많고 랜덤을 사용하지 않는 곳에서도 PointStrategy를 참조하고 있다. 그냥 Line에서 RandomPointStrategy를 직접적으로 참조하는 것이 낫지 않을까?" 하는 페어의 의견이 있었지만, 저는 이것이 DIP 위반이라고 생각했습니다. 인터페이스화 한 이유는 다음과 같습니다.
(1) 추후에 새로운 로직이 추가되었을 때 변경을 용이하게 하기 위함 (2) Override하여 테스트하기가 편해짐
그냥 인터페이스를 포기하고 Line에 해당 로직을 구현할지, 지금과 같은 방식으로 유지할지 고민이 됩니다.

자동차 미션에서 학습하셨겠지만 Random으로 인한 도메인 로직의 테스트 불가로 인해서 추상화를 진행(2)하셨을거에요. 도메인인 Line의 테스트가 가능해졌는데 인터페이스를 포기하고 다시 Random구현체를 넣어야하는 이유가 있을까요?

Line에서 RandomPointStrategy를 직접적으로 참조는 도메인에 대한 테스트가 다시 불가능하게 만드는데 넘어갈 이유가 없어보여요. 처음부터 추상화를 넣었다는 생각보다는 추상화를 왜넣었었던건지를 고민해보는건 어떨까요?

[jinyoungchoi95]

검증 로직은 도메인에서 진행하였으며, 변환과 검증을 분리하는 방식으로 구현하였습니다. Height를 변환 및 검증하는 부분에서, 저는 Integer.parseInt(rawValue)를 하고 NumberFormatException을 처리하는 것까지가 "변환"이라고 생각하였고, 지금의 방식대로라면 변환과 검증이 잘 분리되어 있다고 생각합니다. 하지만 페어에게는 NumberFormatException을 처리하는 과정이 검증이므로 현재의 코드는 변환과 검증이 분리되어 있지 않는 방식이라는 의견을 받았습니다.

변환이라기보다는 입력을 어디까지 받고자하는가일 것 같아요. Height에서 생성자로 int만 받으면 NumberFormat에 대한 고민을 하지 않아도 되고, String까지 받는 것이 Height의 비즈니스로직이라면 NumberFormat을 처리하는 생성자를 추가하면 되겠죠.

검증은 안정적인 객체를 만드는데 필요하기 때문에 항상 추가되어야해요. 문자를 입력받고, 그 문자로 숫자를 변환해서 생성하는 생성자에서 안정적인 객체를 생성하려면 숫자 변환에 대한 검증은 꼭 필요하지 않을까요? 분리되어야하는 이유가 조금 어색한 것 같아요.

[jinyoungchoi95]

사다리의 가로줄이 나란히 연결되어 있는 경우를 테스트해야 할지에 대한 고민이 있었습니다. 제 생각은, "사다리의 바로 왼쪽 가로줄이 연결되어 있는 상태인 경우, 오른쪽 가로줄은 연결되지 않도록 한다"를 구현하고 이것을 테스트하면 이 부분에 대한 테스트는 충분하다고 생각하지만, 다른 크루들의 경우 "사다리가 양 옆으로 나란히 연결된 경우, 에러를 발생시킨다"를 구현하고 테스트를 한 크루도 있는 것으로 압니다.

왼쪽에서 오른쪽 가로줄을 연결하는 로직이기 때문에 저도 충분하다고 생각해요. 사다리를 왼쪽 혹은 오른쪽 둘 다에 생성되게끔하면 말씀하신 크루의 방법대로 테스트해야겠죠. 구현을 어떻게하느냐에 따라서 다르기 때문에 실패케이스에 대해서 정확하게 테스트만 작성한다면 자유롭게 구현하셔도 좋다고 생각합니다 :)

[jinyoungchoi95]

요구사항 작성 잘해주셨네요.

TDD로 구현하시면서 실패 테스트 커밋 -> 구현 커밋으로 진행하셨는데요. 그럼 특정 커밋에 컴파일 에러가 나거나 테스트가 실패하여 동작하지 않는 커밋이 올라가게되어요. 불안정한 커밋이 들어가는 것에 대해서 어떻게 생각하시나요?

[takoyakimchi]

저도 에러가 나지 않는 커밋 단위를 좋아하는데
TDD의 각 사이클을 연습해 보라는 차원에서 네오가 이 방식을 시도해 보라고 말씀해 주셨습니다!
이 부분을 미리 리드미에 작성하였어야 하는데 잊고 있었네요 😅

[jinyoungchoi95]

여기도 Scanner를 계속 생성하고 있네요

[takoyakimchi]

불필요한 객체 생성 줄이겠습니다! 습관화가 안 되어 있네요...

[jinyoungchoi95]

unmodifiableList 👍

[takoyakimchi]

첫 번째 미션 진행하면서 배웠습니다! 감사합니다 👍👍

[jinyoungchoi95]

주석은 지워주셔도 되어요.

[jinyoungchoi95]

(반영하지 않으셔도 됩니다.) Game의 역할이 무엇인가요? 다음 단계의 요구사항보다 현재의 요구사항에 집중한다면 추가되어야할 클래스일까요?

[takoyakimchi]

페어 프로그래밍 과정에서도 이 부분에 대해 논의를 했었는데,
현재의 요구사항에 집중한다면 필요하지 않은 클래스라고 생각합니다.
하지만 추후 게임 로직이 추가될 것이라고 예상하였고
Game에 로직을 넣으려는 생각으로 추가하였습니다.

[jinyoungchoi95]

(반영하지 않으셔도 됩니다.) Controller가 적절한 네이밍인지 고려해보면 좋을 것 같아요.

MVC나 Spring에서 관습적으로 사용하는 Controller라는 네이밍은 뒤로하고 저희가 1레벨에서 개발하는건 "순수 자바만으로 돌아가는" 콘솔게임이에요.

그렇다면 실제로 게임을 "실행"시키는 객체의 이름이 컨트롤러가 적당할까요? 컨트롤러는 예를들면 조이스틱처럼 제어하는데 쓰이는 입력장치인데 실제로 게임을 실행시키는건 컴퓨터나 머신이 더 적절하지 않을까 싶어요.

[takoyakimchi]

컨트롤러의 역할은 유저의 요청을 받아 모델에 있는 데이터를 처리하고, 뷰를 통해 그 결과를 보여주는 것으로 이해했습니다.
기존 컨트롤러에 있던 메서드에서 run()이라는 이름은 '유저의 요청'과는 거리가 멀다고 생각하였고,
이를 start()로 바꾸었습니다. 또한 페어의 리뷰 #316 (comment) 를 참고하여 과도한 메서드 분리를 줄여보았습니다.

하지만 현재 코드에서 여전히 컨트롤러의 역할은 모호한 것 같습니다.
그냥 메인 메서드에 작성해도 잘 돌아가는데, 과도한 클래스 분리를 하고 있는 것이 아닌가 싶습니다.
그렇지만 또다른 유저의 요청이 추가되면, 다양한 유저 요청에 대응하기 위해 컨트롤러 클래스가 필요하다고 생각합니다.

예를 들면, 현재는 게임에 대한 로직이 크게 없지만,
"새로운 사다리를 만들어줘", "이름 순서를 섞어줘" 등의 클라이언트 요청이 추가되면,
이것을 컨트롤러에서 처리하도록 코드를 작성하면 좋을 것 같습니다.

제가 이해한 내용이 맞는지 궁금합니다!

[jinyoungchoi95]

예를 들면, 현재는 게임에 대한 로직이 크게 없지만,
"새로운 사다리를 만들어줘", "이름 순서를 섞어줘" 등의 클라이언트 요청이 추가되면,
이것을 컨트롤러에서 처리하도록 코드를 작성하면 좋을 것 같습니다.

네 말씀하신 내용이 일반적인 MVC 패턴에서 요구하는 컨트롤러가 맞습니다.

제가 이야기드리고싶은건 자바 콘솔 프로그램에서 MVC 패턴에 종속적으로 controller를 구현해야하냐는 것이에요. MVC패턴이란게 존재한다고 무조건 그 패턴에 맞추어서 구현하는 것이 요구사항일까요?

하지만 현재 코드에서 여전히 컨트롤러의 역할은 모호한 것 같습니다.
그냥 메인 메서드에 작성해도 잘 돌아가는데, 과도한 클래스 분리를 하고 있는 것이 아닌가 싶습니다.

에서 말씀하신대로 현재 요구사항에 더 잘 맞는 구현 방법은 메인 메소드에 작성하는게 더 적절한 것이 맞아보여요. 구현해주신 controller도 위에서 이야기한대로 현재 구현체는 MVC라는 패턴 네이밍을 제거해놓고나면 컨트롤러라는 영단어 네이밍 그자체가 게임을 실행시키는게 말이 안되기도 하고요.

[takoyakimchi]

안녕하세요! 정성스럽게 리뷰 남겨 주셔서 감사합니다.
리뷰 주신 부분 최대한 반영해 보았습니다. 잘 부탁드립니다 :)

수정하면서 궁금했던 부분

정적 팩토리 메서드의 static 사용

생성자에 있는 로직을 줄이기 위해 정적 팩토리 메서드를 사용하도록 리팩토링 진행하였습니다! 7b47504
하지만 static 메서드가 불필요하게 늘어난 것이 아닌가 하는 의문이 드는데,
정적 팩토리 메서드가 적절하게 잘 사용되고 있는지 궁금합니다!

생성자의 책임

현재는 생성자를 모두 private으로 바꾸고 정적 팩토리 메서드를 적용시킨 상태이지만,
예를 들면 Name의 경우, 기존 코드처럼 생성자에서 값의 검증과 생성을 같이 진행해도 괜찮지 않을까 하는 의문이 있습니다.
생성자에 검증 로직이 들어간다면 원치 않는 곳에서 예외가 터지는 것을 막을 수 있다고 생각합니다.

오리는 어떠한 방식으로 생성자를 작성하시는 지 궁금합니다!

stream 적용에 따른 가독성 문제

피드백 주신 대로 Line에 반복문 대신 stream을 적용해 보았습니다. a669758
하지만 기존 for문을 사용하던 코드에 비해 가독성이 다소 떨어졌다고 느낍니다.

제가 stream 사용이 익숙하지 않아서 그럴 수도 있겠지만,
이번 예시가 아니더라도 분명히 반복문을 stream으로 바꾸는 작업에서 가독성이 떨어지는 일은 발생할 수 있다고 생각합니다.
가독성 vs stream(개발자의 편의성?): 어떤 것을 중시하는 것이 좋다고 생각하시나요?

읽어 주셔서 감사합니다!

[jinyoungchoi95]

안녕하세요 트레 😄

피드백 잘 적용해주셨네요. 다음 단계 진행하시면서 작성해둔 코멘트도 함께 확인부탁드려요.

궁금하거나 고민이 되는 부분이 있으시다면 언제든 pr 코멘트 또는 dm으로 요청 부탁드립니다.
감사합니다 🙇‍♂️

[jinyoungchoi95]

이건 Height를 생성할때 제약조건이기때문에 생성자에 들어가야하지 않을까요?

[jinyoungchoi95]

Suggested change

	List<Line> lines = new ArrayList<>();
	for (int i = 0; i < height.getValue(); i++) {
	lines.add(Line.of(pointStrategy, playerCount));
	}
	return new Lines(lines);
	return new Lines(IntStream.range(0, height.getValue())
	.mapToObj(i -> Line.of(pointStrategy, playerCount))
	.toList());

로 만들수도 있겠네요 :)

[jinyoungchoi95]

정적 팩토리 메서드의 static 사용
생성자에 있는 로직을 줄이기 위해 정적 팩토리 메서드를 사용하도록 리팩토링 진행하였습니다! > 7b47504
하지만 static 메서드가 불필요하게 늘어난 것이 아닌가 하는 의문이 드는데,
정적 팩토리 메서드가 적절하게 잘 사용되고 있는지 궁금합니다!

정적 팩토리에 기능을 위한 static 메소드 추가는 괜찮은 것 같아요.

생성자의 책임
현재는 생성자를 모두 private으로 바꾸고 정적 팩토리 메서드를 적용시킨 상태이지만,
예를 들면 Name의 경우, 기존 코드처럼 생성자에서 값의 검증과 생성을 같이 진행해도 괜찮지 않을까 하는 의문이 있습니다.
생성자에 검증 로직이 들어간다면 원치 않는 곳에서 예외가 터지는 것을 막을 수 있다고 생각합니다

생성자에 많은 책임이 가지게 되면 안좋다는 것은 공감하지만 올바른 생성에 대한 책임은 생성자가 가져야하지 않을까요? from만 있는 지금은 문제없지만 다른 정적 팩토리 메소드가 추가되면 거기에도 validation이 추가되어야하고 실수로 누락되는 경우가 생기지 않을까요?

[takoyakimchi]

맞네요! validation이 누락되는 경우 원하는 동작이 일어나지 않겠네요.
감사합니다!

[jinyoungchoi95]

피드백 주신 대로 Line에 반복문 대신 stream을 적용해 보았습니다. a669758
하지만 기존 for문을 사용하던 코드에 비해 가독성이 다소 떨어졌다고 느낍니다.
제가 stream 사용이 익숙하지 않아서 그럴 수도 있겠지만,
이번 예시가 아니더라도 분명히 반복문을 stream으로 바꾸는 작업에서 가독성이 떨어지는 일은 발생할 수 있다고 생각합니다.
가독성 vs stream(개발자의 편의성?): 어떤 것을 중시하는 것이 좋다고 생각하시나요?

for문이 가독성이 올라갈때도 있고, stream이 가독성이 올라갈때가 있어서 가독성 vs stream이라기보단 그 상황에 맞게 적절하게 사용하면 좋을 것 같아요. 물론 답이 없는 "적절하게"라는 말이기 때문에 여러가지 상황에서 계속 써보면서 트레만의 기준을 만들고, 코드를 보는 사람이 쉽게 이해할 수 있는 정도라고 생각하면 될 것 같아요.

현재 코드들에서 사용해주신 정도라면 저는 충분한 것 같습니다 :)

[takoyakimchi]

더 경험 쌓아 보겠습니다. 답변 감사합니다!