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

[1단계 - 사다리 생성] 제이미(임정수) 미션 제출합니다 #91

Merged
merged 50 commits into from
Feb 20, 2023

Conversation

JJ503
Copy link
Member

@JJ503 JJ503 commented Feb 16, 2023

안녕하세요 빙봉!
1단계 미션 사다리 생성 PR 요청드립니다.
또한, 아래는 제가 미션을 진행하며 고민하게 된 의문들입니다.
리뷰 잘 부탁드립니다.
감사합니다!

좀 더 생각해봐야 할 문제

1. TDD 방식에서의 commit 단위

이번 미션에서 TDD로 진행하며, 기존과 다르게 commit을 진행했습니다.
이 과정에서 저희는 기능단위 별로 커밋을 진행했습니다.
그런데 TDD인 만큼 test에 대한 커밋을 남기고 기능 구현을 진행해야 하는지 궁금합니다.
또한, 현재 저희의 commit 단위가 괜찮은지도 궁금합니다.

2. 메서드의 줄 바꿈 컨벤션

메서드를 작성함에 있어 저는 줄 바꿈도 하나의 가독성을 위한 부분이라고 생각합니다.
이 과정에서 페어와 토론을 통해 이번 미션에서는 메서드 내 줄 바꿈을 모두 없애고 진행하기로 결정했습니다.
실제 현업에서는 줄 바꿈도 하나의 가독성으로 생각하시는지 궁금합니다.
만약, 가독성으로 생각한다면 줄 바꿈을 어떤 단위로 진행하는지 궁금합니다.

3. Exception Message 출력에 대한 처리

Exception Message에 대한 출력은 InputView의 역할이라고 판단해, InputView에서 예외 메시지 출력 처리를 진행했습니다.
그런데 이 과정에서 Exception을 판단하기 위한 if문이 너무 많이 발생한다는 문제가 발생했습니다.
메서드 라인도 10을 넘겼으며, 만약 예외가 더 추가된다면 if문을 추가로 생성해야 한다는 문제점이 있습니다.
이를 처리할 수 있는 다른 방법이 있는지 궁금합니다.

4. for문 vs IntStream.range.foreach()

이번 미션에서는 Stream을 공부하기 위해 IntStream.range.foreach()을 사용했습니다.
실제 현업에서도 IntStream.range.foreach()를 사용하시는 궁금합니다.
추가적으로 현재 저희가 사용한 IntStream.range.foreach()에서 반복에 대한 수행만 진행했는데 이 경우도 사용하는지 궁금합니다.

용어, 기능 목록, 요구 사항, 컨벤션(커밋, 코드) 작성
생성자 호출만으로 테스트 가능
중복 체크를 위해 중복 식별 가능한 식별자 부여
참가자 이름 입력 과정, 최대 사다리 높이 입력 과정, 오류 메시지 출력
Copy link

@aegis1920 aegis1920 left a comment

Choose a reason for hiding this comment

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

안녕하세요 제이미! 빙봉입니다 🙂

요구사항을 잘 구현해주셨네요! 👍 몇 가지 코멘트 남겼으니 확인해주세요!
중간에 nit이라는 게 있을텐데 중요하지 않은 부분이니 넘겨도 된다는 의미로 남겨두었어요!

궁금한 점은 언제나 환영이니 DM 주세요!

Comment on lines 19 to 20
String participantsName = inputView.enterParticipantsName();
return new Participants(participantsName);

Choose a reason for hiding this comment

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

participantsName가 아닌 participantNames가 의도하신 바겠죠?

Copy link
Member Author

Choose a reason for hiding this comment

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

네 맞습니다..! 변수명을 짓다 실수가 있었네요.
수정해 두었습니다!

}

private boolean isExist(String participantNames) {
return participantNames != null && !participantNames.isBlank();

Choose a reason for hiding this comment

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

!= null 조건과 !isBlank() 조건이 함께 쓰이는 부분이 있는데요. StringUtils 라는 클래스를 따로 만들어서 활용해보는 건 어떨까요?. 코틀린의 isNullOrBlank 같은 느낌으로요. (링크)

Copy link
Member Author

@JJ503 JJ503 Feb 20, 2023

Choose a reason for hiding this comment

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

항상 validator와 같은 클래스를 만들 생각만 했지, util 클래스와 같이 전역적으로 사용할 생각은 해보지 못했네요..!
항상 공백에 대한 처리가 반복되는 것이 고민이었는데 덕분에 해결할 수 있게 되었습니다.
해당 방법으로 수정해 두었습니다.
혹시 해당 방법이 제가 제대로 이해한 것인지 확인해 주시면 감사하겠습니다!

Choose a reason for hiding this comment

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

넵 맞습니다! 👍 유틸 클래스이므로 인스턴스를 생성하지 못하도록 private 생성자를 추가해도 좋겠네요!

}
}

private boolean isNum(String height) {

Choose a reason for hiding this comment

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

isNumber라고 풀네임을 적어줘도 되지 않을까요?

Copy link
Member Author

Choose a reason for hiding this comment

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

길지 않은 메서드 명이기에 굳이 축약해 작성할 필요는 없겠네요!
이 상황에서는 isNumber라고 작성하는 게 더 좋겠습니다!

축약에 대해 찾아보다 아래와 같은 글을 본 적이 있는데 아래와 같은 상황에선 축약하는지 궁금합니다!

변수 이름의 길이가 평균적으로 10-16일 때 프로그램을 디버깅하기 위해서 들이는 노력을 최소화할 수 있고, 변수의 평균 길이가 8~20인 프로그램은 디버깅하기가 쉽다.
numberOfPeopleOnTheUsOlympicTeam와 같은 변수는 numTeamMembers, teamMemberCount와 같이 수정하는 것이 더 적당하다.
출처 : https://remotty.github.io/blog/2014/03/01/hyogwajeogin-ireumjisgi/

Choose a reason for hiding this comment

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

변수 이름 길이를 기준으로 삼진 않습니다. 상황에 따라 다르겠지만 저는 가급적 길게 작성하는 편입니다. 로직이 짧고 이해하기 쉽다면 축약해도 괜찮겠지만 로직이 복잡하고 이해하기 어려운 결제 혹은 정산 도메인의 단어를 사용해야하거나 법조문이 들어간다면 일단 그 변수를 이해하는 게 먼저라서요.

Comment on lines 38 to 39
final int minLadderCount = 1;
final int maxLadderCount = 10;

Choose a reason for hiding this comment

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

LadderCount보단 LadderHeight가 더 어울릴 것 같은데 어떻게 생각하시나요?

Copy link
Member Author

Choose a reason for hiding this comment

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

고민해 본 결과 Height가 더 어울리다고 생각합니다.
처음에는 ladder을 추가해야 하기 때문에 개수정도로 생각해 Count라고 짓게 되었습니다.
그런데 Map에서는 이미 height라는 용어를 사용하였고 메서드 등도 Height로 지었기 때문에 Height가 더 어울리겠네요!

validate(height, lineWeight);
this.height = Integer.parseInt(height);
this.lineWeight = lineWeight;
ladder = generate(booleanGenerator);

Choose a reason for hiding this comment

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

this.을 쓸 것인지 쓰지 않을 건지 일관성을 맞춰주는 건 어떨까요?

Copy link
Member Author

@JJ503 JJ503 Feb 20, 2023

Choose a reason for hiding this comment

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

코드의 일관성을 위해 맞추는 것이 좋겠네요!
this.ladder = generate(booleanGenerator); 로 수정했습니다!

Participants participants = new Participants("jamie,split,pobi");
Map map = new Map("4", participants.getParticipantCount(), () -> true);
outputView.printMap(participants, map);
Assertions.assertThat(byteArrayOutputStream.toString()).isEqualTo("\n실행결과\n\n"

Choose a reason for hiding this comment

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

nit : isEqualTo가 아닌 hasToString을 써보셔도 좋을 것 같습니다.

Copy link
Member Author

Choose a reason for hiding this comment

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

toStirng()을 하지 않고도 바로 비교가 가능해지겠군요!
감사합니다!

return readLine();
}

public void printErrorMessage(IllegalArgumentException exception) {

Choose a reason for hiding this comment

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

InputView에서 에러 메시지를 출력을 하는 게 맞을까요?

Copy link
Member Author

Choose a reason for hiding this comment

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

페어와 함께 토론한 이야기는 아래와 같습니다.
에러가 발생하게 되는 상황은 inputView에서 값을 받아 처리하는 과정이라고 생각합니다.
이러한 이유로 에러 발생에 대한 처리(예외 출력) 역시 InputView에 대한 로직이라고 생각하게 되어 InputView에서 출력하도록 했습니다.
혹시 Controller의 try-catch문에서 출력해 주는 것이 더 나았을까요?

Choose a reason for hiding this comment

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

printErrorMessage의 역할이 메시지를 출력하는 거라 OutputView에서 하면 좋겠다는 생각이 들었습니다. OutputView가 완전 출력을 담당하도록 하고 InputView는 완전 입력만 담당하도록이요. (InputStream, OutputStream 같은 뉘앙스입니다ㅎㅎ)

Comment on lines 12 to 13
private final int height;
private final int lineWeight;

Choose a reason for hiding this comment

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

해당 원시값도 포장해보는 건 어떨까요?

Copy link
Member Author

Choose a reason for hiding this comment

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

검증에 대한 로직이 존재하기 때문에 원시값으로 포장해 해당 로직을 수행하는 게 더 좋겠네요!

만약 height과 weight에 대한 검증 없이 저장하기만 하는 경우가 존재할 때에도 원시값으로 포장해 주는 것이 더 좋을까요?

Choose a reason for hiding this comment

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

만약 height과 weight에 대한 검증 없이 저장하기만 하는 경우가 존재할 때에도 원시값으로 포장해 주는 것이 더 좋을까요?

그 적정선을 지키는 게 가장 어려운데요. 현재는 검증이 있고, 원시값 포장이라는 걸 배우는 입장이니 말씀드렸습니다. 검증없이 그 자체만 나타낸다면 하지 않을 것 같아요. (그럴 이유가 없으니까요?)


<br>

## 👨‍🍳 기능 목록

Choose a reason for hiding this comment

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

기능목록 및 요구사항 너무 잘 작성해주셨네요 👍


<br>

## 👨‍🍳 기능 목록

Choose a reason for hiding this comment

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

이번 미션에서 TDD로 진행하며, 기존과 다르게 commit을 진행했습니다.
이 과정에서 저희는 기능단위 별로 커밋을 진행했습니다.
그런데 TDD인 만큼 test에 대한 커밋을 남기고 기능 구현을 진행해야 하는지 궁금합니다.
또한, 현재 저희의 commit 단위가 괜찮은지도 궁금합니다.

네, TDD를 진행하면서 테스트와 도메인 함께 구현되기 때문에 지금처럼 commit 단위 그대로 진행해주시면 될 것 같습니다. 👍

메서드를 작성함에 있어 저는 줄 바꿈도 하나의 가독성을 위한 부분이라고 생각합니다.
이 과정에서 페어와 토론을 통해 이번 미션에서는 메서드 내 줄 바꿈을 모두 없애고 진행하기로 결정했습니다.
실제 현업에서는 줄 바꿈도 하나의 가독성으로 생각하시는지 궁금합니다.
만약, 가독성으로 생각한다면 줄 바꿈을 어떤 단위로 진행하는지 궁금합니다.

넵, 현업에서도 줄 바꿈을 하나의 가독성으로 생각합니다. 애초에 가독성이란 글이 읽히는 정도를 말하기 때문에 개개인마다 선호도가 다를 수 있다는 것을 인지하고 팀원들과 공유한 뒤 팀 컨벤션으로 맞추게 됩니다. (저희는 구글 스타일 가이드를 암묵적으로? 따르고 있습니다)

제이미와 페어 크루 모두 줄바꿈을 하지 않기로 했다면 하지 않아도 됩니다. 다만 줄바꿈을 하지 않으려는 이유가 단순히 함수의 메서드 길이를 줄이기 위해서가 아니어야 한다는 걸 잊지마세요.

Exception Message에 대한 출력은 InputView의 역할이라고 판단해, InputView에서 예외 메시지 출력 처리를 진행했습니다.
그런데 이 과정에서 Exception을 판단하기 위한 if문이 너무 많이 발생한다는 문제가 발생했습니다.
메서드 라인도 10을 넘겼으며, 만약 예외가 더 추가된다면 if문을 추가로 생성해야 한다는 문제점이 있습니다.
이를 처리할 수 있는 다른 방법이 있는지 궁금합니다.

예외 메시지는 앞으로도 더 많아질 수 있는데요. 예외 클래스 안에 메시지를 넣고 예외의 메시지를 출력하면 되지 않을까요? 여기서 궁금한 점이 생겼는데 메시지를 왜 Enum으로 만드셨는지도 궁금합니다.

이번 미션에서는 Stream을 공부하기 위해 IntStream.range.foreach()을 사용했습니다.
실제 현업에서도 IntStream.range.foreach()를 사용하시는 궁금합니다.
추가적으로 현재 저희가 사용한 IntStream.range.foreach()에서 반복에 대한 수행만 진행했는데 이 경우도 사용하는지 궁금합니다.

IntStream.range.foreach()도 내부적으로 Int를 위한 Consumer(IntConsumer)를 사용하는 것으로 보아 Stream.foreach()와 같다고 생각되는데요. 해당 내용은 이쪽 링크를 보셔도 좋을 것 같습니다. (현업에서도 당연히 Stream.foreach()를 사용합니다.)

@JJ503
Copy link
Member Author

JJ503 commented Feb 20, 2023

안녕하세요 빙봉!
여러 리뷰들과 질문에 대한 답변 정말 감사합니다!
덕분에 제가 왜 그렇게 했는가에 대해 고민하고 실수들을 바로잡을 수 있는 시간이었습니다.
리뷰에 대한 추가 질문은 답변으로 남겨두었으니 천천히 확인해 주시면 감사하겠습니다☺️

그리고 리뷰를 수정하며 추가적으로 생각하게 된 문제들은 다음과 같습니다.

추가로 생각해봐야 할 문제

매직 넘버 상수화

  1. 매직넘버 지역변수로 사용하는 문제점
    매직넘버와 같은 경우 이번 페어와 토론을 통해 상수가 아닌 지역 변수로 선언하기로 했습니다.
    그런데 일반적으로는 상수로 선언해 사용하는 것으로 알고 있습니다.
    이때, 상수로 빼는 것이 더 효과적이거나 장점이 있을까요?

제가 생각한 바로는 상수로 선언함으로써 매직넘버임이 바로 확인이 가능하고,
가장 상단에 있음으로써 바로 확인하거나 수정에 용이하다고 생각했습니다.

  1. static final vs final
    상수를 선언함에 있어 static final이 아닌 final을 사용하기도 하나요?
    상수를 선언할 때 해당 상수를 공유해 사용하지 않더라도 static으로 선언하는 것이 좋을까요?

컨트롤러와 Application

아직 MVC 패턴에 대해 완벽히 파악하지 못해서인지 Controller와 Application에 대한 역할 구분이 어려운 상태입니다.
현재 Controller에서 view에 대한 멤버를 갖지 않게 하기 위해 Application에서 선언 후 play의 파라미터를 통해 Controller에서 사용할 수 있게 했습니다.
그런데 이것이 의미가 있는지, 맞는 방법인지 궁금합니다.

상수와 Enum 클래스

저는 예외 메시지에 대해 Enum 클래스를 사용해 메시지 상수들을 관리했습니다.
비슷한 카테고리의 상수들을 한 번에 확인할 수 있으며, 이가 수정해 용이하다고 생각했기 때문입니다.
제 생각과 같은 경우에도 enum 클래스를 사용하는 것이 맞을까요?
혹은 enum 클래스를 어느 정도부터 사용하는지 권장하는지 궁금합니다.

Copy link

@aegis1920 aegis1920 left a comment

Choose a reason for hiding this comment

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

안녕하세요 제이미! 빙봉입니다 🙂

1단계 구현하시느라 고생 많으셨습니다! 모든 패키지 테스트 커버리지 100% 대단합니다 💯 기존 피드백 및 추가 코멘트는 2단계에서 함께 반영 부탁드립니다. 질문 주신 것 또한 따로 코멘트로 남겼으니 확인해주세요ㅎㅎ 클래스 간 의존(도메인과 다른 패키지)을 고민해보시면서 2단계 진행해주시면 될 것 같습니다!

Comment on lines +3 to +7
import exception.EmptyInputException;
import exception.InvalidLadderHeightException;
import exception.InvalidLineWeightException;
import exception.InvalidParticipantsCountException;
import exception.InvalidPersonNameException;

Choose a reason for hiding this comment

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

사용하지 않는 패키지는 삭제해주세요!

Comment on lines +3 to +5
import domain.Line;
import domain.GameMap;
import domain.Participants;

Choose a reason for hiding this comment

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

nit : OutputView가 도메인을 모른다면 훨씬 유연해질 수 있을 것 같은데요. 많은 변경이 필요해질 것 같아 도전적인 의미로 받아들이시면 될 것 같습니다ㅎㅎ


<br>

## 👨‍🍳 기능 목록

Choose a reason for hiding this comment

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

매직넘버와 같은 경우 이번 페어와 토론을 통해 상수가 아닌 지역 변수로 선언하기로 했습니다. 그런데 일반적으로는 상수로 선언해 사용하는 것으로 알고 있습니다. 이때, 상수로 빼는 것이 더 효과적이거나 장점이 있을까요? 제가 생각한 바로는 상수로 선언함으로써 매직넘버임이 바로 확인이 가능하고, 가장 상단에 있음으로써 바로 확인하거나 수정에 용이하다고 생각했습니다.

상수화를 한다는 건 장점을 따지기보다 상수의 역할을 살펴봐야될 것 같은데요. (링크) 아시다시피 상수는 변하지 않는 값을 의미합니다. 현재는 로직이 간단해서 상수로 선언해야할 값을 지역 변수로 선언해도 별 차이가 없어보이지만 추후에 로직이 더 복잡해지고 요구사항이 추가된다면 어떻게 될까요? 상수로 선언해야할 값을 임시로 지역 변수로 뒀다가 변경된다면? 에러가 터질 수 있겠죠?

상수를 선언함에 있어 static final이 아닌 final을 사용하기도 하나요? 상수를 선언할 때 해당 상수를 공유해 사용하지 않더라도 static으로 선언하는 것이 좋을까요?

근본적으로 상수의 의미를 생각한다면 static을 붙여도 아무런 이상이 없어야 합니다. 변하지 않는 값이니까요. 공유되더라도 상관이 없고 접근제한지가 제한하지 않는 이상 어디서 사용하든 같은 값이어야 합니다. 해당 상수를 공유해 사용하지 않아도 static으로 두는 게 낫겠죠? 좀 더 깊이 들어가면 static으로 선언하면 메모리 static 영역에 저장돼 GC 대상이 아니게 됩니다. (GC 대상이 아니게 되어도 되는거죠) static으로 선언하지 않을 이유가 없습니다.

아직 MVC 패턴에 대해 완벽히 파악하지 못해서인지 Controller와 Application에 대한 역할 구분이 어려운 상태입니다. 현재 Controller에서 view에 대한 멤버를 갖지 않게 하기 위해 Application에서 선언 후 play의 파라미터를 통해 Controller에서 사용할 수 있게 했습니다. 그런데 이것이 의미가 있는지, 맞는 방법인지 궁금합니다.

네, 충분히 의미있는 시도입니다. Application은 말 그대로 시작 지점(앱의 모든 컴포넌트가 떠있는 공간)이라고 생각하시면 되고 Controller는 그 컴포넌트 중 하나라고 생각하시면 될 것 같습니다. Controller와 View는 별개로 나뉘어져있고 현재는 play의 매개변수로 넣어서 입력과 출력을 하고 있지만 추후 웹에서는 어떻게 입력(요청)과 출력(응답)을 해야할 지 고민하시면 좋을 것 같아요!

저는 예외 메시지에 대해 Enum 클래스를 사용해 메시지 상수들을 관리했습니다.
비슷한 카테고리의 상수들을 한 번에 확인할 수 있으며, 이가 수정해 용이하다고 생각했기 때문입니다. 제 생각과 같은 경우에도 enum 클래스를 사용하는 것이 맞을까요? 혹은 enum 클래스를 어느 정도부터 사용하는지 권장하는지 궁금합니다.

개인적으로 예외 메시지에 대해서만 관리하는 거라면 Enum이 큰 의미가 있을까 싶긴 합니다. 메시지가 변경되면 Enum 값도 변경되어야한다는 점에서 둘 간의 의존이 있다고 생각해요. 이런 이유로 현재 상황에서는 예외 메시지를 굳이 enum으로 분리하지 않아도 될 것 같다라고 생각하긴 합니다. 이와 별개로 특정 예외 코드에 대해서 예외 메시지가 정해진다면 의존이 없을테니 의미가 있을 것 같습니다.

@aegis1920 aegis1920 merged commit 06074df into woowacourse:jj503 Feb 20, 2023
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.

3 participants