-
Notifications
You must be signed in to change notification settings - Fork 446
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단계 - 자동차 경주 구현] 돌범 미션 제출합니다. #330
Conversation
This reverts commit 155dfe5.
public Cars(List<Car> cars) { | ||
this.cars = cars; | ||
} | ||
|
||
public static Cars from(List<String> carsName) { | ||
return new Cars(carsName.stream() | ||
.map(carName -> new Car(carName)) | ||
.collect(Collectors.toList())); | ||
} |
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<Car> cars
를 받으려면 생성자를,List<String> carsName
을 받으려면 정적 팩터리 메서드를 사용해야 하는군요. 이렇게 다르게 구현한 이유가 있을까요?
여기서 제 질문의 정확한 의도는, 왜 하나는 생성자의 형태이고 하나는 정적 팩터리 메서드의 형태냐는 거였어요. 둘 다 Cars
객체를 새로 만들어 리턴하고 파라미터만 다른데, 그렇다면 파라미터가 다른 생성자를 두 개 만들 수도 있으니까요.
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.
fromName
과 같은 네이밍과 함꼐, 생성자와는 다른 인자를 받는 메서드임을 명확하게 드러내도록 해보겠습니다!
추가적으로 생성자는 테스트가 없는 프로덕션에서는 private로, 정적 팩토리 메서드는 public으로 유지될 것 같습니다.
혹은 (2차때 반영을 생각해보고 있습니다!)
- private 기본 생성자
- public from(
List< Car>
) + Cars검증로직(중복 등) - public fromNames(
List< String >
) + Cars검증로직(중복 등)
으로 나누어서, 테스트에서도 정적 팩토리 메서드를 사용가능하도록 고려해보겠습니다!
public Round(String roundNum) { | ||
Utils.validateRoundNumber(roundNum); | ||
Utils.validateRoundMinimumOne(roundNum); | ||
this.roundNum = Integer.parseInt(roundNum); | ||
} | ||
|
||
|
||
public static Round from(String round) { | ||
return new Round(round); | ||
} |
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.
이 클래스의 밖에서
Round
객체를 생성하고자 할 때, 무슨 방법을 써야 할까요? 두 가지 방법을 제공하는 이유는 무엇인가요? 둘은 어떤 차이가 있나요?
여기서 드렸던 이 질문도 위의 내용과 동일합니다. 처음에 정수 하나를 받는 생성자와 정적 팩터리 메서드가 함께 있었죠. Round
를 사용하는 입장에서 볼 때, 이 둘이 어떤 차이가 있는지 혼란스러울 거예요. 지금도 마찬가지인데요, 문자열을 받는 생성자와 정적 팩터리 메서드가 있는데 둘이 하는 일이 동일하지만 이건 내부 구현을 봐야 알 수 있는 거고, 사용하는 입장에서는 혼란스러울 가능성이 높다고 생각합니다.
그리고 이펙티브 자바의 설명 중 "이름을 가질 수 있다"는 장점을 말씀해 주셨는데요. 해당 책에서 제공한 예시를 보면 정적 팩터리 메서드의 이름이 probablePrime()
입니다. 충분히 의미가 있는 이름이죠. 함께 제공해 주신 블로그 링크에서의 예시는 아래와 같네요.
public class Student {
private String id;
private String name;
public static Student withId(String id) {
Student student = new Student();
student.id = id;
return student;
}
public static Student withName(String name) {
Student student = new Student();
student.name = name;
return student;
}
}
id
와 name
모두 String
형이고 둘 중 하나만 받아도 객체를 생성할 수 있게 하고 싶어도, 생성자의 특성상 그럴 수가 없습니다. 하지만 정적 팩터리 메서드를 만들고 적절한 이름을 부여하면 가능하죠.
하지만 지금 구현된 Round
를 보면 int
와 String
을 받는 생성자를 따로 만들 수 있고, 정적 팩터리 메서드는 하는 일이 없습니다. 이펙티브 자바에서 제안한 from
, of
같은 이름들은 저자가 제안한 컨벤션일 뿐, 실질적으로는 어떠한 의미도 더하지 못하고 있죠. 물론 정적 팩터리 메서드에는 이외에도 여러 장점이 더 있는데요, 개인적으로는 그런 장점을 충분히 이해하고 활용하면서 생성자와 공존할 때 발생할 수 있는 혼란의 가능성을 해결할 수 있는 상황이 아니라면 차라리 사용하지 않는 편이 더 낫다고 생각합니다.
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.
고민 결과, 일급컬렉션 같이 여러 생성root( String List or 객체List)로 들어올 경우에는
private 기본생성자
( 2번에서 항상 콜 됨)- inputView검증됨 -> 변환로직만 가지는 public
fromNames
( string List) + 테스트호출용 검증 포함 publicfrom
( 객체 List ) 와 같이 3개정도의 생성자를 만드는게 좋을 것 같습니다.
추가적으로 Round, Car와 같이 생성루트가 1가지로 정해진 것들 혹은 코니 말씀대로 일부인자만 받아야하는 변형이 없는 경우에는
- public 생성자+검증을 1개 생성자에서 해결하는게 좋을 것 습니다.
고민인 것은, inputView에서 재귀를 통해 재입력받도록 설계하여 검증이 이루어지고
테스트에서도 검증을 포함한 생성자를 호출 -> 검증을 포함한 생성자 정의 해야하는데
테스트를 위해 검증을 2번하는 꼴이 되어서... 2개의 검증O/검증X생성자를 설계하려고 했던 것 같습니다.
음... 이 부분은 고민 계속 해보겠습니다.
public Round(int roundNum) { | ||
this.roundNum = roundNum; | ||
} |
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.
문자열을 받아 만드는 경우엔 유효성 체크를 하고 있는데, 정수를 받아 만드는 경우엔 체크하지 않고 있네요.
물론 문자열을 받는 생성자만 처음에 사용하는 것이고, 이 생성자는 게임 과정에서 사용하는 것이므로 괜찮다고 생각하실 수 있지만, 이렇게 코드에 직접 드러나지 않는 암묵적인 지식들이 많아지면 실수 가능성이 높아집니다. 협업하는 경우에도 그렇지만, 코드를 작성한 본인은 이걸 과연 언제까지 기억할 수 있을까요?
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될 Next Round 연산에 String.valueOf( ))
를 활용하겠습니다!
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.
이 부분의 수정으로 인해 프로그램이 동작하지 않고 있어요.
코드의 수정이 있었는데 프로그램을 직접 확인해 보지 않고 push하시면 안 됩니다. 항상 마지막까지 꼼꼼하게 확인하는 습관을 들이시는 것이 좋아요. 그리고 테스트 코드는 모두 성공하는데 실제 코드에는 문제가 있는 상황이 왜 발생하는지도 생각해 보시면 좋을 것 같아요.
문제를 보면, 원래 있던 Round(int roundNum)
생성자에는 0이 들어갈 수 있었고(들어가야만 했고) 이렇게 되었을 때 isValidRound()
를 사용하여 게임의 종료 여부를 판단하고 있었죠. 하지만 이 생성자를 없애버리고 모든 경우에 유효성 검사가 들어가면서, roundNum
값이 0인 Round
는 만들어질 수 없게 되었는데 게임 과정에서는 이걸 만들려고 하죠. 게임 진행에 중요한 isValidRound()
메서드는 여전히 roundNum
값이 0보다 크냐는 의미 없는 질문을 하고 있고요.
이렇게 코드에 직접 드러나지 않는 암묵적인 지식들이 많아지면 실수 가능성이 높아집니다. 협업하는 경우에도 그렇지만, 코드를 작성한 본인은 이걸 과연 언제까지 기억할 수 있을까요?
"결국 코드에 직접 드러나지 않는 암묵적 지식"을 본인도 잊어버리면서 문제가 발생한 건데(이렇게 바로 발생하다니!), 이런 일이 적지는 않답니다 😢
import java.util.List; | ||
import java.util.stream.Collectors; | ||
|
||
public class Utils { |
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.
지나치게 추상적인 이름이네요 😢
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.
클래스명을 -> 단수표현인 Validators
Validator
로 수정하겠습니다! 감사합니다!
import java.util.Collections; | ||
import java.util.List; | ||
|
||
public class RacingRecord { |
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.
학습하셔서 아시겠지만 지금 RacingRecord
의 형태는 DTO라고 보기엔 어려워요. 사실은 Cars
의 getDriveRecord()
를 바로 사용하는 것과 RacingRecord
의 getRacingRecord()
를 사용하는 게 동일하고, 후자는 불필요한 객체 생성만 한 겹 더하고 있다고 할 수도 있죠.
그리고 패키지의 위치도 적절한지 고민해 보아요.
private static final String ERROR = "[ERROR] "; | ||
|
||
|
||
public static void printRacingRecordHeadLine() { |
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.
전체적으로 불필요한 공백을 점검해 보아요.
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.
해당 부분은 code style
로 자동화가 안되는 것 같네요ㅠ! commit전에 수동 확인해야할 사항으로 기록해서 습관화하겠습니다
코니가 첫 리뷰어라 정말 감사네요!
import org.junit.jupiter.params.ParameterizedTest; | ||
import org.junit.jupiter.params.provider.ValueSource; | ||
|
||
class RoundTest { |
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.
테스트 확인하셨나요?
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.
검증 메서드 수정한 뒤 테스트 확인을 안했습니다!
테스트 작성한 프로젝트가 처음이라 많이 헤매고 있는데, 습관을 들이도록 하겠습니다!!
@ParameterizedTest | ||
@ValueSource(strings = {"1", "3", "100"}) | ||
@DisplayName("유효한 round를 생성하는지 검사") | ||
@Test |
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.
@ParameterizedTest
를 사용할 땐 @Test
를 빼도 됩니다. 빼도 되는 게 아니라 빼야 하는군요.
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 racingcar.view.InputView; | ||
import racingcar.view.OutputView; | ||
|
||
public class RacingGameController { |
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 Car findMaxPositionCar() { | ||
return cars.stream() | ||
.max(Car::compareTo) | ||
.orElseThrow(() -> new IllegalArgumentException(ERROR_CAR_EMPTY)); | ||
} |
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.
객체지향이라고 모든 상황에서 객체만 사용해야 하는 건 아닙니다. 그럴 수도 없고요. (물론 그런 극단적인 주장을 하는 경우도 있어요.)
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단계 때 반영되도록 하겠습니다. 다른 프로젝트에서 연습하고 있습니다! (ex> 패키지 구조 등) 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.
안녕하세요 돌범~ 말씀드렸던 것처럼 이번 단계는 머지할게요. 함께 남긴 코멘트 확인 부탁드려요!
OutputView.printRequestInstruction(REQUEST_ROUND_VALUE); | ||
try { | ||
String inputRound = getInput(); | ||
Validator.validateRound(inputRound); |
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.
중복 검증은 항상 좋을까요?
이 코멘트의 의미는, 횟수 숫자 검증을 InputView
와 Round
에서 똑같이 하고 있는데 이것에 대한 생각을 묻는 것이었답니다.
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
이 기회로 제 나름대로 정리를 해보았습니다!
- Cars같은 생성 경로가 2개(List String OR List 객체)인 것은 정적팩토리메서드를 2개(네이밍+검증까지)+ private 생성자(검증x)
- Round처럼, 숫자 VO는 inputView에서 integer로 변환한 체로 검증 -> 생성경로 1개로 취급
- public 기본생성자(검증X, 연산 or 할당 등) <--- inputView에서 검증된 number OR 검증이 필요없는 곳에 사용
- public 정적팩토리메서드(검증O for 테스트 등) <--- 검증하는 곳에서 사용
Round의 중복검증 제거, isValidRound 확인시 검증 안되도록 기본생성자 사용하도록 수정하였습니다!
public Round(int roundNum) { | ||
this.roundNum = roundNum; | ||
} |
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.
이 부분의 수정으로 인해 프로그램이 동작하지 않고 있어요.
코드의 수정이 있었는데 프로그램을 직접 확인해 보지 않고 push하시면 안 됩니다. 항상 마지막까지 꼼꼼하게 확인하는 습관을 들이시는 것이 좋아요. 그리고 테스트 코드는 모두 성공하는데 실제 코드에는 문제가 있는 상황이 왜 발생하는지도 생각해 보시면 좋을 것 같아요.
문제를 보면, 원래 있던 Round(int roundNum)
생성자에는 0이 들어갈 수 있었고(들어가야만 했고) 이렇게 되었을 때 isValidRound()
를 사용하여 게임의 종료 여부를 판단하고 있었죠. 하지만 이 생성자를 없애버리고 모든 경우에 유효성 검사가 들어가면서, roundNum
값이 0인 Round
는 만들어질 수 없게 되었는데 게임 과정에서는 이걸 만들려고 하죠. 게임 진행에 중요한 isValidRound()
메서드는 여전히 roundNum
값이 0보다 크냐는 의미 없는 질문을 하고 있고요.
이렇게 코드에 직접 드러나지 않는 암묵적인 지식들이 많아지면 실수 가능성이 높아집니다. 협업하는 경우에도 그렇지만, 코드를 작성한 본인은 이걸 과연 언제까지 기억할 수 있을까요?
"결국 코드에 직접 드러나지 않는 암묵적 지식"을 본인도 잊어버리면서 문제가 발생한 건데(이렇게 바로 발생하다니!), 이런 일이 적지는 않답니다 😢
woowacourse/atdd-subway-path@89e7df5
woowacourse/atdd-subway-path#249 (comment)
|
하이! 코니! 합격자 110여명 중에 마지막 꼴번으로 붙었다고 생각하며 우테코에 감사함을 느끼면서 살아가고 있는
돌범
입니다.협업을 꼭 하고 싶었는데 이번 기회에 드디어 경험해보았습니다. 리뷰를 보내는 것까지 만만치 않네요! 2일간 페어간의 네트워크 잇슈가 있어, 제출 6시간 전에 해결되는 바람에(?) 페어랑 대면으로 협업을 해보았습니다. 시간이 촉박한 만큼 프리코스 때와 다르게 시도 한 부분에과 남은기간동안 적용해보고 싶은 내용을 Q.로 표기하여 리뷰요청드립니다!
전략패턴 적용
인터페이스 -> 만들어놓고 익명함수로 **테스트에 사용(예정)**하기 위함.
Q1. 랜덤값 >=4 의 조건문에 전략패턴을 적용시,
추상화된 인터페이스를 어디에서 받아야하나
???iterator패턴 적용
각 자동차 경주 횟수 Round객체의 값 감소가 아닌, **새로운 값 객체(VO)**를 반환하도록 작성함.
정적 팩토리 메서드에서의 처리 내용
List<Car> cars
로 받아야개별 Car 접근 가능
+Cars 전체 접근가능
**하다.getter
없이 단일객체(Car)에 대한 조작후 전체 테스트(Cars)가 가능하도록 함.프리코스에서는 stream의
.max()
를 통해서 우승자를 다 가지고 올 수 없어서 (객체 1개만 제공) 값만 가져왓었는데페어인 루키는 그대로 객체를 가져와서 사용했습니다. 객체 지향에서 객체끼리 비교를 위해 좋을 것 같았습니다. Q. stream의 집계함수는 같은 max값을 같더라도 1개만 객체를 뱉어내는데, 해당하는 객체들 동시에 받도록하는 stream의 다른 방법이 있을까용?? 테스트에서 Car객체끼리의 비교를 위해 name과 position을 모두 같을 때만 같은 객체로 취급하는 상태였으므로
this.position == 골라낸 maxCar.position
으로 결국엔 값비교로 끝났는데 말입니다...^^;Output에 대한 테스트
ByteArrayOutputStream
객체를 만들고, @beforeeach에System.setOut(new PrintStream( ))
의 인자로 넣어주면ByteArrayOutputStream 객체.toString()
시 출력Output이 그래도 담겨진다.assertThat( ).contains(" expected 출력 문자열")
로 테스트Dto에 준하는 역할인
RacingRecord(CarsDto)
,Winner(CarsDto)
의 도입프리코스에선
Cars
내부에서StringBuilder
로 모든 결과를 모은 String Result을 return 하였으나이번에는 getter를 제공하는 요구사항 해당 객체(Cars, Winner)들이라고 이해한
DTO
를 View인OuputView.printXXX( )
에 전달하였습니다.Q. 앞으로를 위해서, DTO + service + repository등을 사용해야할까요?? (spring경험이 없지만, spring에서 등장하는 구조로 들었습니다). 그냥 객체(Car에 정보들을 취합시켜서 string으로 return하라고 model에 부탁을 하면 안될까요??
View에서는 DTO객체들을 받아 -> 그 내부에서 StringBuilder or join등을 한다.
getter를 가진 순수 객체 or DTO
등만 인자로 받고, View내부에서 정보를 취합하여 결과를 출력하는 것이 맞을까요?if DTO 사용: View에서 cars -> 개별car 접근시 map toDto로 변환 -> CatDto -> getter
코니! 미리 감사드립니다!