-
Notifications
You must be signed in to change notification settings - Fork 92
[자동차 경주] 이상현 미션 제출합니다. #120
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
base: idealhyun
Are you sure you want to change the base?
Conversation
- util 형태로 생성
- 게임 시작 메소드 - 라운드 만큼 시행 - Car 리스트로부터 가장 큰 distance 값 받아오기 - 가장 큰 distance를 기준으로 같은 값을 가진 Car들을 우승자로 정하기
- 랜덤한 값 테스트를 위한 인터페이스 생성
- 자동차 이름 5자 이하 유효성 검증 - 자동차 이름 공백 유효성 검증
- , 를 기준으로 이름 나누는 기능
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.
첫 번째 제출 수고하셨습니다!
몇몇 부분들에 대해서 코멘트 남겼어요.
주어진 컨벤션 룰과 4단계 내용에 조금 더 집중해보면 좋을 것 같아요.
만약에 주어진 상황에서 출력이 콘솔이 아닌 gui가 된다면? 또는 웹 응답이 된다면? 어디가 바뀌어야 하고 얼만큼 영향을 받을지 생각해보면 더 도움이 될것 같습니다.
리뷰 남긴부분에 대한 답변 부탁드리고 궁금한 부분들 계속 물어봐주세요!
@@ -14,6 +14,8 @@ dependencies { | |||
testImplementation platform('org.assertj:assertj-bom:3.25.1') | |||
testImplementation('org.junit.jupiter:junit-jupiter') | |||
testImplementation('org.assertj:assertj-core') | |||
testImplementation('org.mockito:mockito-core:5.11.0') |
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.
오 mockito를 쓰셨군요...
@@ -0,0 +1,5 @@ | |||
package domain; | |||
|
|||
public interface NumberGenerator { |
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.
NumberGenerator라는 Interface를 만들었군요 혹시 이 Interface는 어떻게 쓰이나요?
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.
@ExtendWith(MockitoExtension.class) | ||
public class CarRaceTest { | ||
@Mock | ||
NumberGenerator randomNumberGenerator; |
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.
@mock 어노테이션은 어떤역할을 하나요?
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.
@mock 어노테이션은 NumberGenerator 객체를 mock 객체로 생성할 수 있게 해줍니다. 이를 통해서 제가 원하는 값을 반환하게 지정할 수 있습니다.
import java.util.Random; | ||
|
||
public class RandomNumberGenerator implements NumberGenerator { | ||
private final int MAX_RANDOM_NUMBER = 10; |
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.
위의 값들은 static을 안 썼고, 아래 Random은 static을 썼군요. 어떤 차이가 있나요?
그리고 자바에서 static이 붙고 안붙는게 변수이름에 어떤식으로 적용이 될까요?
(참고 자료 : 구글 자바 네이밍 컨벤션)
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.
static을 붙이면 해당 객체를 생성할 때 하나의 인스턴스를 공유합니다. 상수로 작성하려했다면 static을 붙여야했습니다.
private static final
은 UPPER_SNAKE_CASE
, private final
은 lowerCamelCase
로 작성해야합니다. 컨벤션을 완전 무시하고 작성했었네요. 😢
또한, static final
이더라도 내용이 가변될 수 있다면 lowerCamelCase
로 작성해야합니다.
src/main/java/domain/Car.java
Outdated
} | ||
|
||
private void validate(String name) { | ||
if(name.length() > MAX_CAR_NAME_LENGTH ){ |
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.
discord에서 이야기했던 것처럼 포맷팅을 한번 해보면 좋을 것 같아요.
xml을 등록하고 reformat code를 하면 알아서 다 바뀐답니당
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.
적용해보겠습니다!
} | ||
} | ||
|
||
private void recordOutput(Car car) { |
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.
요것도 출력과 관련있는 내용인 것 같아요. 만약에, 저희가 gui를 제공하도록 바뀌어야한다고 하면 어디가 영향을 받을까요?
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.
private final StringBuilder gameRoundsOutput
, recordOutput()
들이 영향을 받을 것 같습니다.
이러한 부분들이 carRace의 상태라 생각했었는데 출력을 위한 형태였던 것 같습니다.
그럼 이러한 로직들은 OutputView에서 carRace의 상태를 가지고 문자열로 바꿔서 출력하는 형태로 진행하는게 더 적절한건가요?
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.
추후 출력이 변경되었을 때의 생각을 해보면 됩니다.
현재 상태라면 만약 출력형식이 gui 또는 웹 응답 형태가 되었을 때, CarRace의 코드 또한 바뀌어야 할 것입니다. 이러한 형태는 그리 좋지 못합니다.
우리가 생각했을 때 출력에 대한 책임은 OutputView가 온전히 들고있어야 하는데, 그렇지 못하단 이야기거든요. CarRace도 들고있는 것이죠.
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에서 carRace의 상태를 가지고 문자열로 바꿔서 출력하는 형태로 진행하는게 더 적절한건가요?
좀더 핑퐁을 하면서 유도하고 싶지만 우리는 시간이 많지 않으니 😅
넵 그렇습니다.
view가 domain형태로 된 객체를 직접 다룸으로써 출력에 대한 책임을 온전히 가질 수 있습니다.
우리는 내부 연산결과를 우리가 약속한 CarRace라는 도메인 객체로 OutputView에 제공하면서, view에 대한 의존을 끊을 수 있는거죠.
이에 대해서 깊게 생각해보시면 좋을 것 같습니다. 이번 단원의 주요한 내용이거든요. 4단계의 힌트에 대해서 파보는 걸 추천드립니다.
import java.util.Arrays; | ||
import java.util.List; | ||
|
||
public class CarNameParser { |
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.
CarNameparser는 어떤 의도로 만들었고, 지금상황에서 필요할까요?
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
에서 자동차 이름과 쉼표로 이루어진 문자열을 받고, 쉼표를 기준으로 이름들을 분리해주는 행위를 누군가 해야하는데 InputView
는 입력만 받는 역할을 해야된다고 생각했고, 이 역할을 맡길 객체가 떠오르지 않아서 CarNameparser
라는 객체를 만들어서 역할을 분리하려했습니다.
위의 역할 분리가 적절하지 않았다면 불필요한 객체인 것 같습니다.
이러한 의도를 가졌었는데, 의도가 적절한 것일까요?
객체지향은 이러한 역할(책임?)을 객체에게 잘 분리해야한다고 생각했는데 분리할 때 어떤 점을 고려하면 좋을까요?
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.
추상화와 클래스를 쪼개는 것은 미래에 코드가 변경되었을 때 이 변경을 쉽게하기 위해 미리 현재에 지불하는 비용 같은 것입니다.
앞서 말했듯이 추후에 파싱로직이 복잡해지거나, 추가 되는 경우엔 잘 쪼갰다고 볼 수 있겠죠.
하지만, 그런 경우가 안 온다면 어떨까요? 갑자기 그냥 파싱로직이 없어지거나 애당초 입력을 할 때 객체처럼 들어온다면?
spring에 대해서 아시는지 기억이 잘 안나지만, 추후 배우는 spring 에서는 웹요청을 자동으로 객체에 매핑해줍니다. 이런 경우 CarNameParser는 무의미해지겠죠. 그냥 지워질 것입니다.
심지어 이를 유지보수하기 위해 테스트코드까지 짰다면 이 테스트코드에 투자한 시간 또한 무의미해질 수도 있습니다.
그런 클래스를 분리할 시간에, 우리는 다른 코드 한 자 치는게 더 나은 선택일 수도 있습니다. 개발자의 시간은 곧 돈과 다름 없거든요.
그렇기에 우리는 추후에 변경될 가능성
그리고 추후에 변경되었을 때의 이펙트
를 생각하면서 쪼개야합니다.
import java.util.List; | ||
|
||
public class CarNameParser { | ||
private final String DELIMITER=","; |
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.
이 값이 불변이라면 다른 방식으로 표현해보면 어떨까요??
static을 붙인 변수와 붙이지 않은 변수는 실제로 어떤 차이가 있나요?
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.
private static final String DELIMITER = ",";
으로 수정해야할 것 같습니다.
static을 붙이지 않으면 객체마다 해당 변수 메모리를 차지해서 메모리가 낭비됩니다.
@DisplayName("게임 라운드 정수 입력 테스트: 예외 발생") | ||
void invalidInputGameRounds(String input) { | ||
// Given | ||
System.setIn(new ByteArrayInputStream((input + "\n").getBytes())); |
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.
InputViewTest를 해보셨군요. 해보니까 view를 테스트하는건 어떘나요?
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.
입력값의 데이터 타입에 대해 유효성 검증을 하는게 맞나? 라는 생각을 했었던 것 같습니다.
|
||
@Nested | ||
@DisplayName("자동차 생성 - 이름 테스트") | ||
class InvalidCarNameTest { |
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.
Nested를 잘 나눠주신것 좋습니다.
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.
몇몇 코멘트 남겼습니다. 미션은 여기서 approve해두겠습니다. 수고하셨습니다.
몇몇 코멘트를 남겼는데, InputView,OutputView에 대한 내용은 꼭 읽어보고 답 남겨주세용
@@ -0,0 +1,5 @@ | |||
package domain; | |||
|
|||
public interface NumberGenerator { |
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.
src/main/java/domain/Car.java
Outdated
private final int MOVE_STANDARD_NUMBER = 4; | ||
private final int MAX_CAR_NAME_LENGTH = 5; | ||
|
||
public Car(String name){ |
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.
좋은 접근 방법입니다. 👍
왜 그렇게 나눴는지에 대해서 질문을 던져보고 더 디테일하게 상현님의 생각을 듣고싶지만, 리뷰 핑퐁의 시간이 모자란 관계로 제 생각을 전달드리겠습니다.
유효성을 검증하는 것 또한 일종의 책임입니다. 그리고 책임이 어딨는지 알기 도와주는 방법은 유사한 요구사항이 변경되었을 떄, 그리고 그 책임을 맡는 클래스가 바뀌었을 때 를 생각해보면 됩니다.
View는 입출력에 대한 책임, Car는 자동차 라는 도메인 모델에 대한 책임을 갖고 있습니다.
만약 입력이 console이 아닌 웹 요청으로 바뀌면 어떨까요? 아니면 gui의 클릭으로 바뀐다던가?
그런 경우를 생각해보면 데이터 타입(정수, 실수, 문자열 등)에 대한 포맷 유효성은 InputView에 있는게 맞을 것입니다.
반대로, 객체 규칙(이름 5자 넘기지 않기)
이 InputView에 있는데 입력이 gui로 바뀌면 어떻게 될까요?
아마 기존 (이름 5자 넘기지 않기)
를 위한 로직이 복붙될 것 같습니다. 그렇다는 것은 객체 규칙이 InputView의 책임이 아니란 말도 되죠.
결론적으로 생각해주신 방향과 저도 동일하게 생각합니다!
import java.util.Arrays; | ||
import java.util.List; | ||
|
||
public class CarNameParser { |
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 java.util.Arrays; | ||
import java.util.List; | ||
|
||
public class CarNameParser { |
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.
추상화와 클래스를 쪼개는 것은 미래에 코드가 변경되었을 때 이 변경을 쉽게하기 위해 미리 현재에 지불하는 비용 같은 것입니다.
앞서 말했듯이 추후에 파싱로직이 복잡해지거나, 추가 되는 경우엔 잘 쪼갰다고 볼 수 있겠죠.
하지만, 그런 경우가 안 온다면 어떨까요? 갑자기 그냥 파싱로직이 없어지거나 애당초 입력을 할 때 객체처럼 들어온다면?
spring에 대해서 아시는지 기억이 잘 안나지만, 추후 배우는 spring 에서는 웹요청을 자동으로 객체에 매핑해줍니다. 이런 경우 CarNameParser는 무의미해지겠죠. 그냥 지워질 것입니다.
심지어 이를 유지보수하기 위해 테스트코드까지 짰다면 이 테스트코드에 투자한 시간 또한 무의미해질 수도 있습니다.
그런 클래스를 분리할 시간에, 우리는 다른 코드 한 자 치는게 더 나은 선택일 수도 있습니다. 개발자의 시간은 곧 돈과 다름 없거든요.
그렇기에 우리는 추후에 변경될 가능성
그리고 추후에 변경되었을 때의 이펙트
를 생각하면서 쪼개야합니다.
} | ||
} | ||
|
||
private void recordOutput(Car car) { |
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.
추후 출력이 변경되었을 때의 생각을 해보면 됩니다.
현재 상태라면 만약 출력형식이 gui 또는 웹 응답 형태가 되었을 때, CarRace의 코드 또한 바뀌어야 할 것입니다. 이러한 형태는 그리 좋지 못합니다.
우리가 생각했을 때 출력에 대한 책임은 OutputView가 온전히 들고있어야 하는데, 그렇지 못하단 이야기거든요. CarRace도 들고있는 것이죠.
} | ||
} | ||
|
||
private void recordOutput(Car car) { |
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에서 carRace의 상태를 가지고 문자열로 바꿔서 출력하는 형태로 진행하는게 더 적절한건가요?
좀더 핑퐁을 하면서 유도하고 싶지만 우리는 시간이 많지 않으니 😅
넵 그렇습니다.
view가 domain형태로 된 객체를 직접 다룸으로써 출력에 대한 책임을 온전히 가질 수 있습니다.
우리는 내부 연산결과를 우리가 약속한 CarRace라는 도메인 객체로 OutputView에 제공하면서, view에 대한 의존을 끊을 수 있는거죠.
이에 대해서 깊게 생각해보시면 좋을 것 같습니다. 이번 단원의 주요한 내용이거든요. 4단계의 힌트에 대해서 파보는 걸 추천드립니다.
return cars.stream().filter(car -> car.getDistance() == maxDistance).map(Car::getName) | ||
.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.
return cars.stream().filter(car -> car.getDistance() == maxDistance).map(Car::getName) | |
.toList(); | |
} | |
return cars.stream() | |
.filter(car -> car.getDistance() == maxDistance)\ | |
.map(Car::getName) | |
.toList(); | |
} |
stream의 경우엔 한줄에 하나씩 연산하도록 하는게 가독성에 좀 더 도움이 됩니당.
자동차 경주 - 초간단 애플리케이션
학습 내용
코드 설명
회고