-
Notifications
You must be signed in to change notification settings - Fork 252
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단계 - 사다리 게임 실행] 몰리(김지민) 미션 제출합니다. #412
Conversation
Co-authored-by: HaiSeong <[email protected]>
Co-authored-by: HaiSeong <[email protected]>
Co-authored-by: HaiSeong <[email protected]>
Co-authored-by: HaiSeong <[email protected]>
Co-authored-by: HaiSeong <[email protected]>
Co-authored-by: HaiSeong <[email protected]>
Co-authored-by: HaiSeong <[email protected]>
- InputView 에 입력받은 텍스트를 파싱해 전달하는 메서드 작성 Co-authored-by: HaiSeong <[email protected]>
Co-authored-by: HaiSeong <[email protected]>
Co-authored-by: HaiSeong <[email protected]>
Co-authored-by: HaiSeong <[email protected]>
Co-authored-by: HaiSeong <[email protected]>
Co-authored-by: HaiSeong <[email protected]>
Co-authored-by: HaiSeong <[email protected]>
Co-authored-by: HaiSeong <[email protected]>
Co-authored-by: HaiSeong <[email protected]>
Co-authored-by: HaiSeong <[email protected]>
Co-authored-by: HaiSeong <[email protected]>
Co-authored-by: HaiSeong <[email protected]>
Co-authored-by: HaiSeong <[email protected]>
Co-authored-by: HaiSeong <[email protected]>
Co-authored-by: HaiSeong <[email protected]>
Co-authored-by: HaiSeong <[email protected]>
Co-authored-by: HaiSeong <[email protected]>
Co-authored-by: HaiSeong <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
찰리 덕분에 생각을 놓친 부분들에 대해 더 고민해볼 수 있었어요!
다시 한번 컴파일 오류를 체크하지 못한 점 죄송해요 🥲
이번 리뷰도 잘 부탁합니다 찰리🙏
import model.people.People; | ||
import model.people.PersonCount; | ||
import model.Result; | ||
import model.line.RandomLinesGenerator; | ||
import view.InputView; | ||
import view.OutputView; | ||
|
||
public class LadderGame { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
깃 충돌을 해결하다가 패키지 수정 전의 기존 파일들이 생겨나버렸네요😭
한번 더 확인했어야 하는데 죄송합니다🥲
@@ -1,19 +1,47 @@ | |||
### 기능 요구사항 정리 | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
역시 의식적인 연습이 중요하군요! 다음 TDD 때는 더 의식적으로 TDD를 인지하면서 구현을 해봐야겠어요!
감사합니다 찰리:)
|
||
final Result result = findResult(people, ladder, items); | ||
final ResultInfo resultInfo = ResultInfo.from(result); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Result의 Dto로 ResultInfo를 사용했습니다..!
의미 전달이 잘 되지 않는 네이밍이였던 것 같네요 수정하겠습니다!🥲
src/main/java/dto/LadderInfo.java
Outdated
import model.ladder.Ladder; | ||
import model.people.People; | ||
|
||
public record LadderInfo(List<String> peopleNames, List<LineInfo> lines, List<String> itemNames) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dto 패키지 아래에 존재하기 때문에 Dto라는 이름을 중복으로 작성하고 싶지 않았던 것도 있었습니다!
하지만 더 오해를 일으키는 네이밍이 되버렸네요...🥲
찰리 의견에 동의합니다!
|
||
import java.util.Objects; | ||
|
||
public class Index { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
장점으로는 해당 값에 대한 검증을 포장한 객체 안에서 수행할 수 있다는 것 같아요..!
단점은 그만큼 관리해야 하는 객체가 늘어나고, 객체간의 관계를 파악하는데 추가적인 리소스가 발생할 수 있다..라고 생각합니다ㅎㅎ
import view.InputView; | ||
import view.OutputView; | ||
|
||
public class LadderGame { | ||
private static final String ALL = "all"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
앗 전체 입력에 대한 키워드가 변경되었을 때 controller보다는 view를 확인하게 될 것 같네요..!
수정하겠습니다 🫡
private void searchResult(final ResultInfo resultInfo) { | ||
String personName = inputView.inputPersonName(); | ||
while (!personName.equals(ALL)) { | ||
String itemName = resultInfo.getItemNameByPersonName(personName); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
저도 이 부분이 걸렸었는데 resultInfo에게 묻는 것이 더 간단해서 냅다 실수를 범한 것 같아요 🥶
Result에게 묻는 것이 더 적절한 것 같아요! 수정하겠습니다..!
private void searchResult(final ResultInfo resultInfo) { | ||
String personName = inputView.inputPersonName(); | ||
while (!personName.equals(ALL)) { | ||
String itemName = resultInfo.getItemNameByPersonName(personName); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[제가 생각하는 DTO는 어떤 객체?!]
우선 저는 Dto는 계층 간의 데이터 전송을 위해 사용되는 객체라고 이해하고 있어요!
따라서 View에서 Model을 알지 않고도 Model의 값을 출력할 수 있도록 도와주는 값 전달 객체로 사용했습니다..! (실수를 범했지만요😭)
src/main/java/model/Result.java
Outdated
import model.people.Person; | ||
|
||
public class Result { | ||
private final Map<Person, Item> matchedResult; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
오호 이 방식이 관리하기에 더 편할 것 같네요! 👍
한번 시도해보겠습니다!
@@ -0,0 +1,25 @@ | |||
package model; | |||
|
|||
public abstract class Count { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
카운트는 음수일 수 없다는 제약조건을 강제하고 각 Count에 맞는 검증을 추가할 수 있도록 하기 위해 추상 클래스로 구현했습니다!
Count 를 구현한 PersonCount와 ItemCount가 Count 라는 개념으로 묶였을 때 코드상에서 어떤 관계가 표현됐을까요 🤔
코드상에서는 PersonCount와 ItemCount가 Count의 한 종류이다! 라는 관계가 표현될 것 같아요
추가로 Countable이라는 인터페이스로 구현하려면, (검증 코드를 제외하고) Countable이라는 특성과 관련하여 사용될 행동이 있어야 한다고 생각해요.
그런데 제 코드에는 Countable의 특성에 맞는 필요한, 공통된 행동이 없다고 판단했어요. 그래서 검증 코드의 재사용을 위해 추상 클래스로 구현해도 괜찮을 것이라 생각했는데, 혹시 사용이 어색한지 찰리에게 묻고 싶어요!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
안녕하세요 몰리!
고민과 함께 코멘트 잘 반영해주셨네요 😄
댓글에 대한 의견과 함께 코멘트 남겼으니 확인 부탁드려요!
의견 남긴 댓글 링크입니다 😃
- [2단계 - 사다리 게임 실행] 몰리(김지민) 미션 제출합니다. #412 (comment)
- [2단계 - 사다리 게임 실행] 몰리(김지민) 미션 제출합니다. #412 (comment)
궁금한 점 있으면 언제든 DM 주세요~! 🙏
|
||
final Results results = findResults(people, ladder, items); | ||
searchResult(results); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
play() 를 전체적으로 읽을 때 어떤 일이 일어나는지 깔끔하게 표현됐네요 👍
} | ||
|
||
private Results findResults(final People people, final Ladder ladder, final Items items) { | ||
List<Index> resultIndexes = ladder.climbAll(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
현재는 List의 Index에 의존하여 결과의 Index가 연결된 것으로 보여요.
- resultIndexes.get(0) -> 0번째 참가자의 결과 Index
- resultIndexes.get(1) -> 1번째 참가자의 결과 Index
이걸 객체로 표현해보면 어떨까요!
class Something {
private final Index startIndex;
private final Index resultIndex;
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
저도 이 부분이 조금 걸렸었는데 좋은 의견 감사합니다 찰리!
|
||
private int findPersonCount() { | ||
Line line = lines.get(0); | ||
return line.getPathsSize() + 1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Line line = lines.get(0);
이 부분은 변수 할당이 꼭 필요할지 의심해보면 좋겠어요!
극한의 이득을 챙기기위해 변수 할당도 제어해보면 어떨까요 😄
return line.get(0)
.getPathsSize() + 1;
전체적으로 비슷한 부분은 없는지도 살펴보면 좋겠어요!
} | ||
|
||
|
||
public Index climb(final Index startIndex) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
climb 메서드는 Ladder 내부에서만 사용하는것으로 보여요!
private 으로 접근제어자를 높여보죠 😎
outputView.printResult(result); | ||
} | ||
final LadderDto ladderDto = LadderDto.from(people, ladder, items); | ||
outputView.printLadderInfo(ladderDto); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ladderDto 는 밑에서 재사용하는 부분이 없으니
다른 private 메서드들 처럼 추출하여 사용해보면 가독성이 올라가겠어요
// AS-IS
final LadderDto ladderDto = LadderDto.from(people, ladder, items);
outputView.printLadderInfo(ladderDto);
// TO-BE
printLadder(people, ladder, items);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
찰리 안녕하세요!😀
찰리의 리뷰를 보고 전반적으로 제가 미션에서 의도한 것보다 과한 래핑을 했다는 것을 깨달았어요..!
따라서 Name, Count처럼 이미 래핑되거나 역할이 분명하지 않은 객체들을 제거하였습니다!
좋은 리뷰 감사합니다 👍😁
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
안녕하세요 몰리!
사다리 미션 진행하느라 고생하셨어요 🙏
2단계 꼼꼼하게 진행해주셔서 좋았어요!
다음 미션도 파이팅입니다 🎉
@@ -58,7 +58,8 @@ void climbLadderPersonResult() { | |||
); | |||
Ladder ladder = Ladder.from(height, personCount, pathGenerator); | |||
Index startIndex = new Index(0); | |||
assertThat(ladder.climb(startIndex)).isEqualTo(startIndex); | |||
List<Index> climbedResult = ladder.climbAll(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
접근 제어자를 변경하고 테스트도 적절하게 변경해주셨군요 👍
} | ||
return new Line(paths); | ||
} | ||
|
||
private Path getNextPath(final List<Path> paths) { | ||
private Path getNextRandomPath(final List<Path> paths) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이름을 더 명확하게 변경해주신점 💯
package model; | ||
|
||
|
||
public record MatchedIndex(Index startIndex, Index finalIndex) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
MatchedIndex 명확한 이름이군요! 😃
@@ -23,7 +22,7 @@ void createLadder() { | |||
List<Line> expectedLines = List.of(line); | |||
FixedLinesGenerator pathGenerator = new FixedLinesGenerator(expectedLines); | |||
Height height = new Height(1); | |||
PersonCount personCount = new PersonCount(2); | |||
int personCount = 2; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
만약 포장하지 않았더라면 현재처럼 테스트 작성할 때도 더 편했을 거에요!
극한까지 포장하는것은 꼼꼼하고 좋지만
코드 작성이 힘들어진다는 단점이 있어요 😢
몰리의 손가락, 손목 건강을 위해서라도 포장이 필요한지 고민해보죠! 👍
그래도 꼼꼼하게 원시값을 포장했던 점이 좋았습니다 💯
import java.util.List; | ||
import org.junit.jupiter.api.Test; | ||
|
||
class ResultsDtoFormatterTest { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
사실.. 출력에 대한 테스트는 꼼꼼하게 하지 않으셔도 괜찮습니다 😄
만약 미션 진행 중 시간이 촉박하면 도메인에 대한 테스트에 집중하셔도 좋아요!
(현재에 대한 얘기는 아닙니다 ㅎㅎ 앞으로 미션에서요!)
도메인~ 요구사항~ 이런것들이 중요한 만큼 해당 테스트 방법을 더 고민해보는것도 좋습니다 😄
물론 꼼꼼하게 테스트해주셔서 코드를 읽는 입장에서 좋았어요 💯 💯 💯
class IndexTest { | ||
|
||
@Test | ||
@DisplayName("인덱스는 음수일 수 없다.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
아주 사소한 미세 의견으로는 😄
인덱스는 개발자스러운 뉘앙스가 풍기는군요 😌
사다리 게임에서 어떤 위치라는 것은 인덱스 말고 표현할 수 있는 단어가 더 있을것 같아요..!
좋은 이름을 지어준다는 것은 결국 가독성을 위한 것인데
그런 점에서 코드를 읽는 사람은 개발자이니 인덱스도 큰 문제는 없다고 생각해요 ㅎㅎ...
하지만..! 현업에서 일하면 비개발자에게 설명을 해야할 때가 있어요!
그럴 때 인덱스라고 하면 소통이 힘들 수 있겠죠..!
이름 지을 때 목표는 개발을 모르는 사람도 이해할 수 있는 정도로 도메인에 대한 개념을 녹여보는것이 좋다고 생각해요!
|
||
public class ItemFormatter { | ||
public static String format(final String itemName) { | ||
return String.format("%5s", itemName); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
여기서 5는 어떤 값을 의미하는지 고민과 추측을 통해서만 알 수 있군요 😃
상수로 표현하면 더 명확하게 의도를 전달할 수 있겠어요
@@ -0,0 +1,25 @@ | |||
package model; | |||
|
|||
public abstract class Count { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
더 이상 사용하지 않는 Count 클래스는 삭제해도 되겠군요 😉
@@ -0,0 +1,11 @@ | |||
package dto; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dto 는 사실 view나 controller 에서 사용하고 있으니
적절한 곳에 하위 패키지도 놓아도 괜찮겠어요~
- view.dto
- controller.dto
어느곳이 더 어울리는지는 몰리의 고민거리로 남겨두겠습니다 😄
안녕하세요 찰리!
이번 단계에서는 여러모로 아쉬운 점이 많아 계속 수정을 하게 되었던 것 같아요..!
아래에 이번 단계를 진행하면서 생각난 궁금한 점을 작성해놓았습니다 😀
TDD cycle을 잘 지키기 위한 방법
졸업식 이슈로 시간이 촉박하게 느껴져 구현에 집중했던 것도 있구요..😭찰리는 TDD를 활용할 때 cycle을 잘 지킬 수 있는 방법 같은 게 있나요...?
모든 원시값과 문자열을 포장한다의 범위
이에 대한 찰리에 생각이 궁금합니다..!
패키지 구조
저만의 일관성을 지키는 것이 좋을까요? 아니면 적당한 선에서 타협하는 게 좋을까요?
2단계도 잘 부탁드립니다 :)