김정교 #6
김정교 #6
Conversation
print results, ladder, people
now can play ladder game
wbluke
left a comment
wbluke
left a comment
There was a problem hiding this comment.
안녕하세요 챠피탁님! 코드 잘 읽었습니다 :)
리뷰가 조금 많을수도 있는데요. 다 못하셔도 괜찮으니 차근차근 리팩토링 해보시구요
특히 일급컬렉션에 대해서 좀더 집중적으로 공부해보시면 좋을 것 같아요. 화이팅 :)
| public static final String BAR_EXIST = "-----"; | ||
| public static final String BAR_NOT_EXIST = " "; |
There was a problem hiding this comment.
Bar를 어떻게 콘솔화면에 그려줄 것인지를 Bar에서 정의하고 있는데요.
MVC(Model-View-Controller)의 개념으로 생각해 본다면 이 책임은 Model의 책임이라기 보다는 View의 책임이지 않을까요?
View에서 어떻게 그려줄 것인지에 대한 정책이 바뀌면 View에 가서 찾는 것이 아니라 Model에 가서 해당 로직을 수정해야만 하겠네요.
또 콘솔 뷰가 아니라 웹으로 변경된다면? Model을 유지한 채 View만 웹으로 변경할 수 있을까요?
이런 질문들을 던져보면서 설계 시 고민하면 좋을 것 같아요 :)
| if (isExists()) { | ||
| return BAR_EXIST; | ||
| } | ||
| return BAR_NOT_EXIST; |
| } | ||
|
|
||
| public static Bar ofRandom() { | ||
| return new Bar(Math.random() > BAR_EXIST_PERCENTAGE); |
There was a problem hiding this comment.
50%의 확률로 boolean을 결정하셨는데,
new Random().nextBoolean(); 이런 방법도 있습니다ㅎㅎ
| public static Bar ofRandom() { | ||
| return new Bar(Math.random() > BAR_EXIST_PERCENTAGE); | ||
| } | ||
|
|
||
| public static Bar of(boolean exists) { | ||
| return new Bar(exists); | ||
| } |
There was a problem hiding this comment.
사다리의 가장 근간(밑바닥)이 되는 Bar를 랜덤으로 생성하고 있기도 하고, 바깥에서 생성하도록 넣어주고 있기도 한데요.
Ctrl + B 로 확인해보면 밖에서 boolean값을 넣어주는 of(boolean exists) 메소드는 테스트를 위해 어쩔 수 없이 뚫어놓은 생성자로 보여요.
실제 프로덕션 코드에서 사용하는 ofRandom()은 랜덤값 때문에 테스트를 할 수가 없으니 아마 그런것이겠죠.
자동차 게임 미션에서 배웠던 전략 패턴을 사용해서, 테스트 가능한 구조로 만들어보는 것은 어떨까요? :)
| @@ -0,0 +1,44 @@ | |||
| import java.util.List; | |||
|
|
|||
| public class ConsoleOutput { | |||
There was a problem hiding this comment.
staitc 클래스를 만들 때는, private한 기본 생성자를 만들어 놓아 불필요한 객체 생성에 방어적으로 대비하는 습관을 가지고 있으면 좋습니다 :)
| int targetUserPosition = peoplePlayed | ||
| .stream() | ||
| .filter(person -> person.getName() | ||
| .equals(targetUserName)) | ||
| .collect(Collectors.toList()) | ||
| .get(0) | ||
| .getPosition(); |
There was a problem hiding this comment.
List를 가공해서 무엇인가 내가 원하는 데이터를 가져오는 작업, 이전에도 지금도 앞으로도 굉장히 많이 해야할 텐데요.
가공하는 로직이 있을 곳이 없다보니 현재 메소드가 꽤나 길어지고 복잡해집니다. 사실 가공하는 작업과 가공이 끝난 데이터를 기반으로 비즈니스 로직을 수행하는 부분은 엄연히 다른 부분인데 말이죠.
일급컬렉션에 대해 공부하고 적용해보면 어떨까요?
| LadderInput ladderInput = new LadderInput(people, ladderHeight); | ||
| Ladder ladder = Ladder.of(ladderInput); |
There was a problem hiding this comment.
LadderInput이라는 객체의 역할은 무엇일까요?
이름에서 유추하기도 어렵고, 실제로 하는 일도 없어보여요. people과 높이를 받았는데, 정작 ladder만들 때 가로세로 길이만 뽑아주고 끝이네용
LadderGame이라는 객체가 있고, 그 객체가 people, ladder, results를 가지고 있으면 어떨까요? 이건 하나의 제안일 뿐이고, 더 나은 이름이나 구조가 생각나신다면 그걸로 리팩토링 해보셔도 무방합니다.
| @Override | ||
| public int compareTo(Person person) { | ||
| return this.position - person.position; | ||
| } |
There was a problem hiding this comment.
person의 위치 비교를 위해 Comparable을 시도하신 점, 아주 칭찬합니다 👍👍👍 좋은 시도입니다.
하지만 권장하지 않습니다ㅎㅎ
compareTo라는 메소드를 오버라이딩해서 비교를 한다는 것은, 정확히 A객체가 B객체에 비해 크다/작다/같다 라고 말할 수 있어야 한다고 생각합니다. Person이라는 객체의 관념을 떠올렸을 때, 사람의 위치에 따라 사람의 크기를 규정한다는 것이 자연스러울까요?
position이 바뀌면 사람의 상호비교 결과(?)도 여러 번 뒤바뀔 수 있겠네요.
비교할 수 있는 필드가 추가되면? 비교 조건이 상황에 따라 달라진다면? compareTo 라는 아주 추상적인 메소드로는 표현하기 어려운 지점이 생길 수 있습니다.
차라리 position 값만 비교한다고 명시한 별도의 메소드를 만들어보는 것이 좋을 듯 싶습니다 :)
| this.position = position; | ||
| } | ||
|
|
||
| private void validateName(String name) throws IllegalArgumentException { |
There was a problem hiding this comment.
IllegalArgumentException은 언체크 예외라 명시하지 않아도 됩니다.
자바의 체크 예외와 언체크 예외에 대해서 한번 살펴보세요 :)
| assertThatThrownBy(() -> { | ||
| Person person = new Person("abcdef"); | ||
| Person person = new Person("abcdef", 1); | ||
| }); |
There was a problem hiding this comment.
저는 주로 예외 상황을 테스트할때는 다음과 같이 많이 작성합니다. 어떤 예외가 발생하는지도 코드에서 드러나기 때문에요.
assertThrows(IllegalArgumentException.class, () -> new Person("abcdef", 1));2 participants
커밋 로그가 너무 지저분해요. 실수긴 한데 다음부터 정말 잘 해야겠습니다. 느낀 바가 많습니다.
그 외에 음.. 저는 범죄자입니다