3주차 - Step2 사다리(생성) 리뷰 요청드립니다. #198
Conversation
There was a problem hiding this comment.
전체적인 설계와 깔끔한 코드 구현이 좋았습니다. 👍
간단한 생각거리를 남겼으니 코드에 반영해 주셔도 좋습니다.
원활한 3단계 진행을 위해 2단계는 빠르게 Merge 할게요.
아래의 글이 도움 되면 좋겠어요.
https://www.baeldung.com/parameterized-tests-junit-5
| @@ -0,0 +1,6 @@ | |||
| package ladder.domain; | |||
|
|
|||
| public interface BarGenerator { | |||
There was a problem hiding this comment.
BarGenerator 인터페이스 👍 이런 형태의 인터페이스를 함수형 인터페이스라고 부른답니다. @FunctionalInterface가 무엇인지 찾아보고 공부해 보세요.
There was a problem hiding this comment.
@FunctionalInterface 어노테이션을 붙이면 개발자들이 함수형 인터페이스라는 것을 알 수 있고, 이 인터페이스가 오직 하나의 추상메서드만 가지는지 컴파일러가 체크할 수 있게 한다는 점을 알게 되었습니다-! 함수형 인터페이스 자체에 대해서도 더 깊게 공부해보겠습니다. 감사합니다! 👍
|
|
||
| import java.util.Random; | ||
|
|
||
| public class BarGeneratorImpl implements BarGenerator{ |
There was a problem hiding this comment.
클래스 이름의 'Impl'은 의미가 없습니다. 아래의 글을 읽고 코드에 반영하면 어떨까요?
https://octoperf.com/blog/2016/10/27/impl-classes-are-evil
There was a problem hiding this comment.
유익한 글 공유 감사합니다! 무의식적으로 Impl을 붙여왔었는데 잘못된 습관이었네요!
질문이 있습니다. 만약 비즈니스 로직이 BarGeneratorImpl 처럼 랜덤으로 Bars를 생성하는 경우 외에 다른 방법으로 Bars를 생성할 일이 없는 경우에도 인터페이스로 구현하는 것이 맞는지 궁금합니다.. 인터페이스로 빼야 될 것 같은 느낌(?) 때문에 인터페이스로 구현했는데 공유주신 글 내용중에
there is no point of creating an interface for a service which has only a single known implementation
이런 내용이 있어서 BarGenerator를 인터페이스로 만들 필요가 있었는지 의문이 생겼습니다. 제가 인터페이스에 익숙하지 않아서 더더욱 헷갈리네요..ㅠ
|
|
||
| public class Line { | ||
|
|
||
| private List<Boolean> bars; |
| private List<Boolean> bars; | ||
|
|
||
| private Line(List<Boolean> bars) { | ||
| this.bars = new ArrayList<>(bars); |
There was a problem hiding this comment.
새로운 컬렉션에 담아 외부에 의한 변조를 막는 것은 습관처럼 사용하는 것이 좋습니다. 👍
| private List<Boolean> randomBars = new ArrayList<>(); | ||
| private BarGeneratorImpl barGenerator = new BarGeneratorImpl(); | ||
|
|
||
| List<Boolean> generateBars(int numberOfPlayers) { |
There was a problem hiding this comment.
미처 생각하지 못한 부분인데 피드백주셔서 감사합니다!!
제가 이해한 바는, 이 메서드를 한 번 사용하고 나면 LineMaker 클래스의 인스턴스 변수 randomBars에 데이터가 박혀있기 때문에 다시 사용하면, 이전에 사용했던 randomBars에 bars를 추가하게 되므로 재사용할 수 없다고 이해했는데 맞을까요..?
그래서
class LineMaker {
private List<Bar> randomBars;
List<Bar> generateBars(int numberOfPlayers) {
this.randomBars = new ArrayList<>(); // 이 부분 추가
generateFirstBar();
generateMiddleBars(numberOfPlayers);
generateLastBar();
return randomBars;
}
...
}위와 같이 generateBars메서드가 호출되면 randomBars가 초기화되도록 바꾸었는데 올바른 방법인지 궁금합니다..!!
|
|
||
| List<Boolean> generateBars(int numberOfPlayers) { | ||
| randomBars.add(barGenerator.generateBar()); | ||
| for (int i = 1; i < numberOfPlayers - 1; i++) { |
There was a problem hiding this comment.
numberOfPlayers - 1이 왜 필요한지 메서드 분리를 통해 설명하면 어떨까요?
There was a problem hiding this comment.
for문은 첫번째 bar와 마지막 bar를 제외한 가운데 bars를 생성하는 부분이라서 generateMiddleBars()라는 메서드로 분리했습니다. 그리고 나머지 부분도 generateFirstBar() 메서드와 generateLastBar()메서드로 분리해서 generateBars() 메서드가 가지고 있던 역할을 위임했습니다.
혹시 피드백주신 부분에 대해 제가 이해한 바가 맞을까요?
| import java.util.stream.Collectors; | ||
| import java.util.stream.IntStream; | ||
|
|
||
| import static ladder.view.InputView.printEmptyLine; |
There was a problem hiding this comment.
InputView와 OutputView는 서로 다른 View라고 생각하고 분리하면 어떨까요?
|
|
||
| private static void printPlayers(Players players) { | ||
| players.getPlayers().stream() | ||
| .map(OutputView::adjustNameLength) |
There was a problem hiding this comment.
String#format(String, Object...)을 사용해 보면 어떨까요?
안녕하세요-!!
지난 토일월화 가족휴가를 다녀와서 이제서야 2단계 리뷰요청을 드립니다.
문제가 많을 것으로 예상됩니다..
많은 조언과 피드백 부탁드립니다!!