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

test: InputValidator, PositionIndicatorTest 테스트코드 추가 #1

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

chs1234
Copy link
Contributor

@chs1234 chs1234 commented Apr 29, 2023

No description provided.

Copy link

@dongKos dongKos left a comment

Choose a reason for hiding this comment

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

고생하셨습니당. 좋았던점 :

  • 의미 있게 커밋 단위를 나눈점
  • 테스트 코드 잘 짠점
  • 풍부한 readme가 있어서 코드 이해가 쉬웠음

README.md Outdated
- 도메인 모델
Copy link

Choose a reason for hiding this comment

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

문서화 굳!

private static final String DISPLAY_CHARACTER = "-";

public static String indicateCharacterByPosition(int position) {
return DISPLAY_CHARACTER.repeat(Math.max(0, position));
Copy link

Choose a reason for hiding this comment

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

ㅋㅋㅋㅋㅋ 나도 이거 intellij가 추천해줌. 직접 한거라면 지송!

Copy link

Choose a reason for hiding this comment

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

그나저나 Position Indicator에서 하는 일은 Position 객체의 책임 같은데 어떻게 생각하시는지요??

Copy link
Contributor Author

Choose a reason for hiding this comment

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

저도 인텔리제이의 추천을 받아서 바꿨습니다ㅋㅋㅋ
PositionIndicator 생각해보니 Position 객체의 책임이 더 맞는 거 같아요.!
이 부분은 수정해서 다시 올리겠습니다!

String display = "-".repeat(Math.max(0, position.getPosition()));
ResultView.printText(name.getName() + ": " + display);
// ResultView.printText(name.getName() + ": " + position.getPosition());
RacingGameView.printText(
Copy link

Choose a reason for hiding this comment

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

개인적으로 요렇게 변수 안만들고 바로 파람으로 넘기는거 좋아함 ❤️

Copy link
Contributor Author

Choose a reason for hiding this comment

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

저도 동며들었달까요?ㅎㅎ

@@ -0,0 +1,19 @@
package properties;

public class ErrorMessage {
Copy link

Choose a reason for hiding this comment

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

👍


public static void validCarNames(String carNames) {
isEmptyOrContainWhiteSpace(carNames);
isNotStartsWithComma(carNames);
isNotEndsWithComma(carNames);
canSplitByComma(carNames);
isExistMoreThanTwoCars(carNames);
isNotExceedFiveCharacters(carNames);
isNotDuplicateCarName(carNames);
}

public static int getTryCount(String tryCount) {
Copy link

Choose a reason for hiding this comment

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

이런 Validator에 대한 고찰인데 뭔가 getTryCount라고 명명을 하면 이 친구는 Validation만 하는애인데 tryCount 라는 비즈니스 로직에 묶여 버리는 느낌? 이 든달까나;; 그래서 나는 validator 클래스 삭제하고 각 도메인 객체들한테 체크하는 로직을 전부 위임했음. 요 부분은 님 의견 궁금함

Copy link
Contributor Author

Choose a reason for hiding this comment

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

getTryCount라고 명명을 하면 이 친구는 Validation
이 부분은 저도 계속 꺼림칙했네요..
지난번 과제도 validator 유틸클래스를 만들었었는데, 도메인 객체들에게 위임하는 게 조금 더 객체지향적인 설계겠죠?
객체스러운 사고가 뭔지 좀 더 사고하는 훈련이 필요할 거 같습니다..!

Copy link

Choose a reason for hiding this comment

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

Util 클래스는 과연 필요한가? 필요하다면 무엇을 해야하는가?

}
}

private static void isEmptyOrContainWhiteSpace(String carNames) {
if (StringUtils.isBlank(carNames))
throw new IllegalArgumentException("[ERROR] 공백 혹은 빈 값이 포함되어있습니다.");
throw new IllegalArgumentException(ErrorMessage.CarName.NOT_ALLOW_EMPTY_OR_CONTAINS_WHITE_SPACE);
Copy link

Choose a reason for hiding this comment

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

내 PR에도 코멘트 남겼던 부분인데, 도메인 익셉션 정의하고 각 세부 상황별 Exception class들을 만들어 놓으면 ErrorMessage의 역할을 각 exception class들로 위임할 수 있다고 생각함


class InputValidatorTest {

@Test
Copy link

Choose a reason for hiding this comment

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

굳굳 요거 Parameterized Test 사용하면 메소드 많이 줄일 수 있을듯??

Copy link

Choose a reason for hiding this comment

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

근데 테스트 코드를 짰다는 것이 아주 굳굳인 부분

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Parameterized 요걸로 한번 바꿔봐야겠어요!

class PositionIndicatorTest {

@Test
@DisplayName("숫자 n에 대하여 '-'가 n개 표시되어야 한다.")
Copy link

Choose a reason for hiding this comment

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

굳 테스트, 지만 한 테스트에 검증문은 한개만 들어 가는게 best practice임!

@wapago wapago requested a review from dongKos May 11, 2023 04:12
Copy link

@wapago wapago left a comment

Choose a reason for hiding this comment

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

LGTM

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