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단계 - 자동차 경주 구현] 돌범(조재성) 미션 제출합니다. #381

Merged
merged 108 commits into from
Feb 21, 2022

Conversation

is2js
Copy link

@is2js is2js commented Feb 17, 2022

안녕하세요 코니!
1차에서 고민해보았던 DTO를 도입하고, name과 position을 VO로 도입하면서 MVC구조를 잡아보았습니다!

중복 검증과 생성자에 대한 고민

  1. private생성자를 기본적으로 정의하고

  2. inputView에서 들어온 값(기본입력 검증만 거친)에 대해서는

    1. 정적팩토리메소의 네이밍을 fromInput +
    2. 자체검증만 (도메인 객체내 자체검증)
  3. 테스트 코드 등 프로덕션 코드에서 생성할 경우

    1. 정적팩토리메소드를 from 으로 네이밍 +
    2. 자체검증 +
    3. inputView에서 해주던 유틸화된 기본 검증 추가

이런식으로 제 나름대로 고민하고 법칙을 세웠습니다.
그리고 예외발생 시 재입력하기 위한 재귀+ try/catch하는 부분을 inputViewgetInput만 감싸던 것에서 -> main메서드 getInput + 객체생성까지 확장하여 감싸도록 변형하여

  • inputView의 getInput에서도메인관련 검증까지 모조리 하던 구조 -> 도메인 객체 자체 검증이 필요한 부분은 생성자에서 처리되도록 검증의 종류를 콘솔input용검증ex>null, empty + 도메인자체검증ex> 이름은 1~5자이상 나누어 분리하는 식으로 설계하였습니다.

DTO, VO도입 후 리팩토링에 따른 새로운 질문들

  1. Dto는 도메인보다는 Controller에 가깝다고 공부하였습니다. 그러나, Dto를 사용하는 곳은 RacingGame이며 이놈이 의존성을 가지고 있다면, CarDto도 domain패키지에 넣어야할까요??

    • 전달용이므로 controller에 위치시키는게 맞지만 고민을 해도 답이 잘 안나옵니다!
  2. 같은 도메인인 CarsNames는 전달할 일이 없으므로 서로 의존하여 필요시 getter를 사용해도 될까요?

    • SQL의 join처럼 괜찮을 것 같습니다만.. getter를 지양하라.. 라는 객체지향 관점에서.. 잘 모르겠습니다!
    //Cars
    public static Cars fromNames(Names names) {
        return new Cars(names.getNames().stream()
                        .map(name -> Car.fromName(name))
                        .collect(Collectors.toList()));
    }
    
    
    
    
    //Names 
    public List<Name> getNames() {
        return this.names;
    }
  3. Car는 이제 NamePosition이라는 VO를 가지게 되었는데, Name과 Position도 Dto가 필요한지 궁금합니다.

    • CarDto List만 넘겨주면, OutputView에서는 cardto.getName() -> Name객체 -> .toString() -> string으로 원하는 값을 체인하여 Name, Position에 의존하지 않고 사용할 수 있습니다.
    • 하지만, Name객체속 값을 얻어낼 때도 Car처럼.. dto를 통해 가져와야할지 궁금합니다!
    //OutputView.java
    
    public static void printRacingRecord(List<CarDto> cars) {
        for (CarDto car : cars) {
            System.out.println(car.getName().toString() + NAME_AND_SCORE_DELIMITER + printDash(car.getPosition().toInt()));
        }
        System.out.println();
    }

아직 온보딩 기간이라 다른 것들과 병행하며 공부를 하고 있습니다! 리뷰 받는 동안 추가적으로 적용해보겠습니다!

wishoon and others added 30 commits February 9, 2022 19:29
Copy link

@choihz choihz left a comment

Choose a reason for hiding this comment

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

안녕하세요, 돌범! 리뷰어 코니입니다.

2단계도 잘 구현해 주셨네요! 피드백을 보시고, 더 궁금한 점이나 의견이 있으시다면 언제든 코멘트 또는 DM 주세요~

Comment on lines 12 to 17
public static void validateCarNames(List<String> carNames) {
checkDuplicated(carNames);
for (String carName : carNames) {
validateInput(carName);
}
}
Copy link

Choose a reason for hiding this comment

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

  1. ,,처럼 입력하면 어떻게 되나요?
  2. a, a처럼 공백이 들어가면 어떻게 되나요?

Copy link
Author

@is2js is2js Feb 18, 2022

Choose a reason for hiding this comment

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

  1. ,,의 검증은 split이후 개별 carName별로 validateInput 걸려서 문제가 없을까 생각했었는데

    1. split()에 2번째 인자(-1)를 주지 않으면 ,,가 모두 다 쪼개지질 않는다 -> split( , -1)으로 수정
        String[] split = ",,".split(","); // count 0
        String[] split1 = ",,a".split(","); // 3
        String[] split2 = ",a,".split(","); // 2
        String[] split3 = "a,,".split(","); // 1
  2. split(,-1)이후 trim()하여, 구분자 주위의 공백을 허용( trim후 개별 carName으로 전달)하도록 수정하였습니다.

      public static List<String> splitWithDelimiter(String value, String delimiter) {
          return Arrays.stream(value.split(delimiter, -1))
                  .map(String::trim)
                  .collect(Collectors.toList());
    }

감사합니다 코니!

if (validateCarsNameSize(carsName)) {
throw new IllegalArgumentException(ERROR_CARS_NAME_DUPLICATED);
private static void checkDuplicated(List<String> carNames) {
if (carNames.stream().distinct().count() != carNames.size()) {
Copy link

Choose a reason for hiding this comment

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

carNames.stream().distinct().count() 같은 코드는 단번에 의미를 파악하기 어렵죠. 변수로 분리해 이름을 부여해 준다면 이해하기 더 수월할 것 같아요.

Copy link
Author

@is2js is2js Feb 18, 2022

Choose a reason for hiding this comment

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

~문 전체가 아니더라도 이렇게 연쇄적인 코드의 변수는 추출하여 이름을 부여하면 더 좋군요! 감사합니다 코니! 수정하였습니다!

long distinctCountOfCarNames = carNames.stream().distinct().count();

private static final String ERROR_NULL_EMPTY_MESSAGE = "빈칸 입력은 허용하지 않는다.";
private static final String ERROR_INCLUDE_BLANK_MESSAGE = "내부에 공백이 포함될 수 없습니다.";
private static final String ERROR_DUPLICATED_MESSAGE = "중복값을 입력할 수 없습니다.";
public static final String ERROR_INVALID_FORMAT_MESSAGE = "입력한 값이 숫자의 형태가 아닙니다.";
Copy link

Choose a reason for hiding this comment

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

접근 제어자 확인해 주세요~

Copy link
Author

Choose a reason for hiding this comment

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

코니, intellij 에서 상수 추출시 기본적으로 public으로 뽑아내는 것을 -> private으로 추출하도록 수정하였습니다!

감사합니다 코니!

public static void validateCarName(String carName) {
validateCarsNameLength(carName);
validateCarsNameBlank(carName);
public static void validateInput(String carName) {
Copy link

Choose a reason for hiding this comment

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

여기서만 사용되는 메서드 같은데 public으로 열어둘 필요가 있을까요?

Copy link
Author

Choose a reason for hiding this comment

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

네 코니!
validateInput()과 validateInputNumber()는 이후 확장성 모든 콘솔input들이 거쳐야할 공통 검증 로직으로 설계하여 열어두었는데

한 곳에서 사용되고 끝난 것에 대해서는 private로 수정하겠습니다

@@ -3,65 +3,51 @@
import java.util.List;

public class Validator {
Copy link

Choose a reason for hiding this comment

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

이처럼 static 멤버만 가지고 있어 객체를 생성하여 사용할 필요가 없는 클래스의 경우, 명시적으로 private 기본 생성자를 선언해 불필요한 객체 생성을 막기도 한답니다.

Copy link
Author

@is2js is2js Feb 18, 2022

Choose a reason for hiding this comment

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

유틸성 클래스에서 생성해놓는 습관을 가져보도록 하겠습니다!
Validator 뿐만 아니라 Util클래스에서도 생성해두었습니다!

감사합니다! 코니!

public static final int FIXED_NUMBER = 4;

@Override
public int generateNumber() {
Copy link

Choose a reason for hiding this comment

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

인터페이스의 이름이 이미 NumberGeneratePolicy이기 때문에, 메서드 이름은 generate() 정도로 해도 충분할 것 같아요.

Copy link
Author

Choose a reason for hiding this comment

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

인터페이스, 클래스명(Type)과 메서드의 이름에 목적어 중복이 안되도록 해야겠네요 !

수정하겠습니다~ 감사합니다 코니!


private static void checkValidLengthOfName(String name) {
int nameLength = name.length();
if (!(CAR_NAME_MIN_LENGTH <= nameLength && nameLength <= CAR_NAME_MAX_LENGTH)) {
Copy link

Choose a reason for hiding this comment

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

조건문을 더 간단하게 만들 수 있지 않을까요? 부정 조건문은 의미를 단번에 파악하기가 어려워요.

Copy link
Author

Choose a reason for hiding this comment

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

저의 경우, 이게 아닌 경우 -> 에러를 발생시켜!가 더 편해서 이런 식으로 작성하였는데 ㅠ 부정조건문이라는 말 자체도 익숙하지가 않네요.

이번 기회에 수정하고 지양하는 방향으로 가보겠습니다!

import racingcar.domain.Name;
import racingcar.domain.Position;

public class CarDto {
Copy link

Choose a reason for hiding this comment

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

CarDto도 domain패키지에 넣어야할까요??

아니요. 지금 위치가 더 적절한 것 같아요.

Copy link
Author

Choose a reason for hiding this comment

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

네 Dto에 대해서 더 알아봤는데,

자동차경주에 대해서는 controller가 사용하는 것
5 Layer 구조에서는 service가 사용하는 것이 더 낫다는 글을보았습니다!

Comment on lines 15 to 19
public static Cars fromNames(Names names) {
return new Cars(names.getNames().stream()
.map(Car::fromName)
.collect(Collectors.toList()));
}
Copy link

Choose a reason for hiding this comment

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

같은 도메인인 CarsNames는 전달할 일이 없으므로 서로 의존하여 필요시 getter를 사용해도 될까요?

필요할 땐 써야죠. 무조건 "getter를 쓰면 안 돼"라고 생각하실 게 아니라, 이런 이야기가 나오는 이유가 무엇인지를 깊이 생각해 보셨으면 좋겠습니다.

Copy link
Author

Choose a reason for hiding this comment

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

금일 네오의 자동차경주 피드백에서 도메인내 객체간 협력시 메세지를 던지자. -> getter를 지양하자는 이야기였던 것 같습니다!

지금의 경우는, 이미 Names에게 새로운 결과물을 만들도록 시키는 경우가 아니라, 생성시 필요한 재료들을 꺼내쓰는 생성자내부이므로 getter가 필요할 것 같습니다!

Comment on lines 6 to 10
private final String name;
private static final int CAR_NAME_MIN_LENGTH = 1;
private static final int CAR_NAME_MAX_LENGTH = 5;
private static final String ERROR_INVALID_CAR_NAME_LENGTH_MESSAGE =
CAR_NAME_MIN_LENGTH + "~" + CAR_NAME_MAX_LENGTH + " 글자 범위 내에서 입력하세요.";
Copy link

Choose a reason for hiding this comment

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

선언 순서를 다시 고민해 볼까요?

Copy link
Author

Choose a reason for hiding this comment

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

상수 (static final) -> final (인스턴스변수) 순으로 배열하고 중간에 빈줄 1개를 삽입하여 구분하겠습니다!

- Util#splitWithDelimiter split()에 2번째 인자 -1 추가
- Validator#validateCarNames에 checkCarNamesEmpty()메서드 추가
- Validator 객체 생성 불필요 명시를 위한 private 생성자 추가
- checkDuplicated 변수 분리를 통한 의도 명확화
- validateInput 사용 범위에 맞게 접근제어자 변경(public->private)
- 인자 명칭이 지나치게 구체적인 것을 추상화
Copy link

@choihz choihz left a comment

Choose a reason for hiding this comment

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

안녕하세요, 돌범!

피드백을 잘 반영해 주셨네요 👍 조금 더 나아질 수 있는 부분과 생각해 보면 좋을 내용들이 있어 코멘트를 드렸어요. 이외에도 더 궁금한 점이나 의견이 있으시다면 언제든 코멘트 또는 DM 주세요~


private Cars getCars() {
try {
return Cars.fromNames(Names.fromInput(InputView.getCarNames()));
Copy link

Choose a reason for hiding this comment

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

지금은 Cars를 만들기 위해 Names의 존재를 알아야 하는 상황인데요, 사실 NamesCars 안에서만 사용되고 있죠. 그렇다면 Names의 존재가 바깥에 노출될 필요가 있을까요? 구현을 드러내고 있는 건 아닐까요?

Copy link
Author

Choose a reason for hiding this comment

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

같은 context에서 사용될 일이 없는, 객체 생성 가운데 활용되는 객체들은 구현을 드러내지 않는 것이 좋을 것 같습니다!

Cars의 생성자명을 fromNames - fromInput으로 수정하고
그 내부에서 Names를 만들고 -> Car -> CarList를 만드는 순으로 정리해보았습니다. Names가 굳이 필요없을 것 같긴한데, 원시값 포장의 연습이라고 생각해야할지.. 흠.. 고민이 됩니다.

public static Cars fromInput(List<String> InputNames) {
    Names names = Names.fromInput(InputNames);
    return new Cars(names.getNames().stream()
            .map(Car::fromName)
            .collect(Collectors.toList()));
}

Comment on lines 12 to 16
public static List<String> splitWithDelimiter(String value, String delimiter) {
return Arrays.stream(value.split(delimiter, -1))
.map(String::trim)
.collect(Collectors.toList());
}
Copy link

Choose a reason for hiding this comment

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

유용하게 사용하려고 따로 분리하신 것이겠지만, 만약 코드의 다른 부분에서 Integer.parseInt() 작업이 필요하다면 굳이 이 메서드를 찾아서 쓸까요?

개인적으로는 이 메서드도 위 코멘트의 대상이었던 메서드와 유사한 성격으로 보입니다.

Copy link
Author

Choose a reason for hiding this comment

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

convert 뿐만 아니라 split + trim정도는 불필요하게 메서드로 추출하지 않도록 수정하겠습니다~!

Comment on lines 47 to 63
private static void checkValidFormat(String inputNumber) {
if (!(inputNumber.chars().allMatch(Character::isDigit))) {
throw new IllegalArgumentException(ERROR_INVALID_FORMAT_MESSAGE);
}
}

private static void validateRoundNumber(String input) {
try {
Integer.parseInt(input);
} catch (NumberFormatException e) {
throw new IllegalArgumentException(ERROR_ROUND_NOT_NUMBER);
private static void checkNullOrEmpty(String input) {
if (input == null || input.trim().isEmpty()) {
throw new IllegalArgumentException(ERROR_NULL_EMPTY_MESSAGE);
}
}

private static void validateRoundMinimumOne(String inputRound) {
if (Integer.parseInt(inputRound) < ROUND_MINIMUM_ONE) {
throw new IllegalArgumentException(ERROR_ROUND_MINIMUM_ONE);
private static void checkIncludeBlank(String input) {
if (input.trim().contains(BLANK)) {
throw new IllegalArgumentException(ERROR_INCLUDE_BLANK_MESSAGE);
}
}
Copy link

Choose a reason for hiding this comment

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

메서드를 사용 순서로 배치중이신 거라면 이 부분의 재정렬이 필요해 보이네요.

Copy link
Author

Choose a reason for hiding this comment

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

메서드 순서대로 재배치하였습니다. 수정하고 나서 다시 체크하는 것이 버릇이 안들어있습니다.
코니만의 노하우가 있을까요??

intellij의 Structure 보는 기능을 활용해보려 합니다 감사합니다 코니!

Comment on lines 22 to 24
private static void validateName(String name) {
checkValidLengthOfName(name);
}
Copy link

Choose a reason for hiding this comment

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

이 메서드가 필요한가요?

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,10 @@
package racingcar.domain;

public class FixedNumberGeneratePolicy implements NumberGeneratePolicy {
Copy link

Choose a reason for hiding this comment

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

이 클래스는 테스트 코드에서만 사용되고 있는데, 여기에 위치하는 것이 적절할까요? 또한, 자바의 람다식에 관해 학습해 보셔도 좋을 것 같아요.

Copy link
Author

@is2js is2js Feb 20, 2022

Choose a reason for hiding this comment

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

초기에는 람다식을 통해 () -> 5를 전략객체에 대입하였다가, 실제 다른 전략객체 확장을 해보고 싶어서 작성하였습니다.!

테스트에만 사용되는 전략객체는 테스트에 위치시키는게 좋을 것 같습니다! 해당 클래스는 삭제하고 람다식으로 대체하겠습니다!


class OutputViewTest {
private final ByteArrayOutputStream output = new ByteArrayOutputStream();
private List<Car> cars = new ArrayList<>();
private final List<Car> carsList = new ArrayList<>();
private Cars cars;

@BeforeEach
public void setUpStreams() {
Copy link

Choose a reason for hiding this comment

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

테스트 메서드들의 네이밍 방식이 다른데, 하나로 통일하는 편이 좋지 않을까요?

Copy link
Author

Choose a reason for hiding this comment

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

셋업 메서드들도 cameCase를 적용하였습니다.

ㅠ_ㅜ 페어 프로그래밍의 거의 모든 부분이 드러내졌네요 ㅎㅎ
다음 미션때는 꼼꼼한 코니의 티칭이 잘 반영하여 페어 프로그래밍 시작부터 잘해보겠습니다!

Comment on lines 12 to 14
@DisplayName("Position from CarDto 값 확인")
@Test
void get_valid_position_from_cardto() {
Copy link

Choose a reason for hiding this comment

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

사소하고 별 것 아닌 것 같은 테스트가 예상치 못한 실수를 막아줄 때도 분명 있지만, 테스트 코드도 유지보수해야 할 코드라는 걸 염두에 두시는 것이 좋습니다. 즉, 의미 있는 테스트 코드를 작성하는 것이 좋습니다.

물론 "의미 있는"이라는 것이 굉장히 주관적이고, 또 장단점이 공존하기 때문에 상황에 맞게 적절히 판단해야 하겠죠 😊

Copy link
Author

Choose a reason for hiding this comment

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

cardto의 구조를 바꿀 때 테스트도 유지보수 해야되는군요

dto에서 값을 확인하는 코드는 이미 유기적으로 간접테스트 되고 있으니 빼도록 하겠습니다!

Comment on lines 22 to 25
System.out.println(
car.getName().getNameValue() + NAME_AND_SCORE_DELIMITER + printDash(
car.getPosition().getPositionValue()));
}
Copy link

@choihz choihz Feb 18, 2022

Choose a reason for hiding this comment

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

getter를 연쇄적으로 사용하고 있는데, 이 방법밖에 없는지 고민할 필요가 있지 않을까요? 이 코멘트와 함께 생각해 볼까요?

DTO의 사용 목적을 다시 생각해 보고 이 구조에 대해서도 고민해 볼까요?
Name, Position 객체의 존재를 DTO를 받는 쪽에서 알아야만 할까요? 어차피 원시값을 꺼내 사용하고 있지 않나요?

Copy link
Author

Choose a reason for hiding this comment

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

해당 부분에 대해 생각해보았습니다.
DTO는 값의 변형을 막으면서 전달하는 객체기 때문에, 중간의 wrapper들이 굳이 없어도 될 것 같다고 판단하였습니다!

dto에서는 wrapper클래스 사용없이 바로 값만 반환되도록 수정하겠습니다~!

이렇게 연쇄적으로 호출되는 것을 디미터법칙 위배 열차전복사고 등이라고 표현하는 것 같더라구요~! 지양하도록

CarDto의 생성자에서는 Car.getName-> Name.getNameValue() -> string 값 추출을 위해 연쇄과정이 필요할 것 같습니다. 여기선 사용해도 될까요??

    public static CarDto from(Car car) {
        return new CarDto(car.getName().getNameValue(), car.getPosition().getPositionValue());
    }

Comment on lines +21 to +24
public static Names from(List<Name> names) {
validateNames(names);
return new Names(names);
}
Copy link

Choose a reason for hiding this comment

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

테스트 코드에서만 사용되고 있네요 🤔

Copy link
Author

Choose a reason for hiding this comment

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

현재 inputView의 기본검증을 이미 끝난 string으로 입력되는 fromInput
테스트나 프로덕션 내부에서 생성되어, 기본검증이 필요한 단일객체Name으로 입력되는 from을 나누어서 구현하고 있습니다. Cars역시 동일한 로직으로 test에서만 생성되고 있습니다!

Name -> Names의 구현도 Car -> Cars처럼, 단일객체에서부터 생성될시 필수검증이 필요하다고 생각합니다!

inputView에서만 이루어지고 있는 검증과 동일하며, 콘솔입력 테스트를 대신할 수 있다는 장점도 가지고 있습니다!

import org.junit.jupiter.params.provider.ValueSource;

class NameTest {
@DisplayName("이름이 1자 미만, 5자 초과일 경우")
Copy link

Choose a reason for hiding this comment

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

"이름이 1자 미만, 5자 초과일 경우" 어떻다는 것인지, 테스트 이름을 분명히 작성해 두면 좋습니다. 특히 테스트가 실패했을 때 원인을 더 빨리 파악할 수 있을 거예요.

Copy link
Author

Choose a reason for hiding this comment

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

@DisplayName("자동차 이름이 1자 미만, 5자 초과하여 유효하지 않은 경우")

형식으로 구체적으로 수정하였습니다.

꼼꼼한 코니의 리뷰 덕분에 지나칠 수도 있는 것들에 대해 다시 돌아보게 되었습니다!

Copy link

@choihz choihz left a comment

Choose a reason for hiding this comment

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

안녕하세요, 돌범!

피드백을 잘 반영해 주셔서 이번 미션은 이제 마무리하려고 합니다 👍 우테코의 첫 미션을 함께하게 되어 즐거웠고, 정말 고생 많으셨어요. 그럼, 다음에 다시 만나요~ 👋

}

public static Name from(String name) {
checkValidLengthOfName(name);
Copy link

Choose a reason for hiding this comment

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

이 작업은 여기서 하는게 적절할까요, 아니면 생성자에서 하는 것이 적절할까요?

Comment on lines +14 to +16
public static CarDto from(Car car) {
return new CarDto(car.getName().getNameValue(), car.getPosition().getPositionValue());
}
Copy link

Choose a reason for hiding this comment

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

CarDto의 생성자에서는 Car.getName-> Name.getNameValue() -> string 값 추출을 위해 연쇄과정이 필요할 것 같습니다. 여기선 사용해도 될까요??

CargetName(), getPosition()이 꼭 Name, Position 객체를 리턴하는 게 좋은지 고민해 보시면 어떨까요?

Comment on lines +15 to +19
public static Names fromInput(List<String> names) {
return new Names(names.stream()
.map(Name::from)
.collect(Collectors.toList()));
}
Copy link

Choose a reason for hiding this comment

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

현재 inputView의 기본검증을 이미 끝난 string으로 입력되는 fromInput
테스트나 프로덕션 내부에서 생성되어, 기본검증이 필요한 단일객체Name으로 입력되는 from을 나누어서 구현하고 있습니다. Cars역시 동일한 로직으로 test에서만 생성되고 있습니다!

하지만 fromInput()에 검증이 끝난 값만 들어온다고 가정하는 것이 적절할까요?

import org.junit.jupiter.params.provider.ValueSource;

class NameTest {
@DisplayName("자동차 이름이 1자 미만, 5자 초과하여 유효하지 않은 경우")
Copy link

Choose a reason for hiding this comment

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

Suggested change
@DisplayName("자동차 이름이 1자 미만, 5자 초과하여 유효하지 않은 경우")
@DisplayName("자동차 이름이 1자 미만, 5자 초과하여 유효하지 않은 경우 예외가 발생한다")

이정도로 결과까지 명확하게 서술하면 더 좋습니다.

Comment on lines +14 to +15
@EmptySource
@ValueSource(strings = {"", "Wishoon", "123456"})
Copy link

Choose a reason for hiding this comment

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

@ValueSource의 빈 문자열과 @EmptySource가 같은 값을 제공하지 않나요?

.isInstanceOf(IllegalArgumentException.class);
}

@DisplayName("자동차 이름이 1~5자로 유요한 경우")
Copy link

Choose a reason for hiding this comment

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

오타가 있네요~

@choihz choihz merged commit e554941 into woowacourse:is2js Feb 21, 2022
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.

None yet

3 participants