Skip to content
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

Merged
merged 145 commits into from
Mar 5, 2024

Conversation

jminkkk
Copy link

@jminkkk jminkkk commented Mar 2, 2024

안녕하세요 찰리!

이번 단계에서는 여러모로 아쉬운 점이 많아 계속 수정을 하게 되었던 것 같아요..!
아래에 이번 단계를 진행하면서 생각난 궁금한 점을 작성해놓았습니다 😀


TDD cycle을 잘 지키기 위한 방법

  • 아무래도 혼자하다보니 페어와 함께 했을 때보다 TDD cycle을 벗어나도 인지가 조금 늦게 되더라구요ㅠㅠ 그래서 1단계보다 cycle을 많이 어긴 것 같아 아쉽습니다..
    • 졸업식 이슈로 시간이 촉박하게 느껴져 구현에 집중했던 것도 있구요..😭
  • 또 처음 작성한 요구사항 명세서에 많은 부분들을 놓친 것 같아서 테스트할 부분들을 초기에 많이 놓쳤었다는 점도 아쉽습니다..

찰리는 TDD를 활용할 때 cycle을 잘 지킬 수 있는 방법 같은 게 있나요...?

모든 원시값과 문자열을 포장한다의 범위

  • 요구사항 중 "모든 원시 값과 문자열을 포장한다"에 대한 부분을 뒤늦게 인지하여, 1차 구현이 끝난 후 해당 요구사항을 만족하도록 리팩토링했습니다.
    • 리팩토링을 하면서 들었던 의문이 이런 것까지 포장을 해야 하나? 라는 생각이었습니다.
    • 예시로 PersonCount나 ItemCount과 같은 객체들을 도출하면서, Items나 People처럼 기존 일급 컬렉션 안에서 처리할 수도 있는 역할들을 추출한 것이 너무 과할 수도 있지 않을까 하는 생각이 들었구요...

이에 대한 찰리에 생각이 궁금합니다..!

패키지 구조

  • 현재는 model 패키지 아래 items, ladder, people, line의 하위 패키지가 존재하는데요.
  • 제가 생각하기엔 이번 미션에서 핵심 객체는 items, ladder, people라고 생각합니다..!
    • 따라서 line 역시 model/ladder/line 아래에 위치해야 한다고 생각합니다. 하지만 패키지의 너무 depth가 깊어질 것 같아 실천(?)에 옮기지는 않았습니다.

저만의 일관성을 지키는 것이 좋을까요? 아니면 적당한 선에서 타협하는 게 좋을까요?


2단계도 잘 부탁드립니다 :)

jminkkk and others added 30 commits February 20, 2024 14:41
- InputView 에 입력받은 텍스트를 파싱해 전달하는 메서드 작성

Co-authored-by: HaiSeong <[email protected]>
Copy link
Author

@jminkkk jminkkk left a 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 {
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

깃 충돌을 해결하다가 패키지 수정 전의 기존 파일들이 생겨나버렸네요😭
한번 더 확인했어야 하는데 죄송합니다🥲

@@ -1,19 +1,47 @@
### 기능 요구사항 정리

Copy link
Author

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);
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Result의 Dto로 ResultInfo를 사용했습니다..!
의미 전달이 잘 되지 않는 네이밍이였던 것 같네요 수정하겠습니다!🥲

import model.ladder.Ladder;
import model.people.People;

public record LadderInfo(List<String> peopleNames, List<LineInfo> lines, List<String> itemNames) {
Copy link
Author

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 {
Copy link
Author

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";
Copy link
Author

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);
Copy link
Author

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);
Copy link
Author

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의 값을 출력할 수 있도록 도와주는 값 전달 객체로 사용했습니다..! (실수를 범했지만요😭)

import model.people.Person;

public class Result {
private final Map<Person, Item> matchedResult;
Copy link
Author

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 {
Copy link
Author

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의 특성에 맞는 필요한, 공통된 행동이 없다고 판단했어요. 그래서 검증 코드의 재사용을 위해 추상 클래스로 구현해도 괜찮을 것이라 생각했는데, 혹시 사용이 어색한지 찰리에게 묻고 싶어요!

Copy link

@Gomding Gomding left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

안녕하세요 몰리!
고민과 함께 코멘트 잘 반영해주셨네요 😄
댓글에 대한 의견과 함께 코멘트 남겼으니 확인 부탁드려요!

의견 남긴 댓글 링크입니다 😃

궁금한 점 있으면 언제든 DM 주세요~! 🙏


final Results results = findResults(people, ladder, items);
searchResult(results);
Copy link

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();
Copy link

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;
}

Copy link
Author

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;
Copy link

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) {
Copy link

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);
Copy link

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);

Copy link
Author

@jminkkk jminkkk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

찰리 안녕하세요!😀

찰리의 리뷰를 보고 전반적으로 제가 미션에서 의도한 것보다 과한 래핑을 했다는 것을 깨달았어요..!
따라서 Name, Count처럼 이미 래핑되거나 역할이 분명하지 않은 객체들을 제거하였습니다!
좋은 리뷰 감사합니다 👍😁

Copy link

@Gomding Gomding left a 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();
Copy link

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) {
Copy link

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) {
Copy link

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;
Copy link

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 {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

사실.. 출력에 대한 테스트는 꼼꼼하게 하지 않으셔도 괜찮습니다 😄

만약 미션 진행 중 시간이 촉박하면 도메인에 대한 테스트에 집중하셔도 좋아요!
(현재에 대한 얘기는 아닙니다 ㅎㅎ 앞으로 미션에서요!)

도메인~ 요구사항~ 이런것들이 중요한 만큼 해당 테스트 방법을 더 고민해보는것도 좋습니다 😄

물론 꼼꼼하게 테스트해주셔서 코드를 읽는 입장에서 좋았어요 💯 💯 💯

class IndexTest {

@Test
@DisplayName("인덱스는 음수일 수 없다.")
Copy link

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);
Copy link

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 {
Copy link

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;
Copy link

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

어느곳이 더 어울리는지는 몰리의 고민거리로 남겨두겠습니다 😄

@Gomding Gomding merged commit 865ab47 into woowacourse:jminkkk Mar 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants