-
Notifications
You must be signed in to change notification settings - Fork 452
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단계 - 자동차 경주 구현] 유콩(김유빈) 미션 제출합니다. #391
Conversation
- 자동차 이름의 길이 제한 수정(5글자 포함) - 에러 메시지 출력 추가
생성된 인스턴스가 아닌 또 다른 생성자를 호출하여 제대로 값이 저장되지 않아 수정함
- view 에서 model 생성자를 호출했던 로직을 controller 로 이동
- view(output) 에서 Car 의 상태를 모르도록 Car 가 직접 처리하도록 수정함
- Cars 테스트에서 사용하기 위해 추가 - 위치를 입력받는 과정이 추가됨에 따라 테스트 추가
- private 메소드를 사용하지 않아도 같은 내용의 테스트를 실행할 수 있도록 코드 수정 - Car 객체를 입력을 받는 Cars 생성자 추가 - 생성자 추가에 따른 테스트 코드 추가
- CarTest 의 drive 를 테스트하는 것과 유사하여 삭제
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.
안녕하세요 유콩! 코드가 전체적으로 깔끔해지고 리팩토링도 잘 해주셨네요🙂 고치신 부분들에 있어 궁금한 부분이 있어 코멘트 남겼습니다 확인 부탁드려요.
- 테스트를 위한 생성자
- 유콩이 해당 결론을 내리기까지의 생각을 조금 더 공유해주시면 좋을 것 같아요. 테스트코드를 위한 코드가 왜 필요했는지, 그리고 적절하지 않다고 생각했으나 생성자까지는 그러해도 된다고 결론을 내린 이유에 대해 공유해주시면 조금 더 좋을 것 같습니다.
- Input 변수 내부 초기화 vs 파라미터
- 유콩이 말한 부분도 맞는 이야기입니다! 그래서 유콩이 외부 입력값을 직접변경하여 사용하고 계셨군요! 요 부분에 대해서도 코멘트 남겼으니 확인해주세요!
- 추가로 자동차이동과 관련해서
전략패턴
이라는 키워드로 검색을 해보시면 조금 더 의미를 알 수 있을 것 같아요. 그리고 파라미터로 ArrayList 을 받지 않고 List을 받는 이유도 같이 고민해보시면 좋을 것 같아요. 위 두가지가 제가 말씀드린 Input 변수 내부 초기화와 같은 맥락으로 말씀드린거였습니다!
private static final int CUSTOM_DELIMITER_PART = 1; | ||
private static final int NUMBER_PART = 2; | ||
|
||
public int splitAndSum(String text) { | ||
text = replaceEmpty(text); |
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.
바로 다음 글에 남긴 답변처럼, 기존의 값을 변경하는 것이 프로그램이 확장될 때 읽기 쉬운 코드가 아닐까 합니다. 입력받은 빈 값을 생략하여 다시 재할당한다면 그것은 결국 더하기에서 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.
좋은 관점인 것 같습니다🙂
다만 파라미터로 전달된 값을 내부적으로 변경한다는 것을 외부는 알고 있을까요? 예를들어 A > B를 호출했을 때 B에서는 A의 값 C를 C`로 바꾸지만 A는 그 부분을 충분히 인지할 수 있을지에 대해서도 고민해보면 좋을 것 같아요.
현재의 프로그램에 대해서 유콩이 작성한 것이 매우 자연스럽다고 생각하지만, 기본적으로 조금 더 복잡한 프로그램에서 외부에서 전달받은 값을 내부에서 변경하는 것과, 내부에서는 해당 값을 변경하지 않는 것 중 어떤 것이 사이드이펙트 / 이해하기 편한 코드에 가까운지 계속해서 고민해나가시면 좋을 것 같아요.
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.
좋은 부분에 대해서 고민한 것 같습니다🙂 우리가 어떤 메소드를 사용한다는 것은 내부 동작을 모르고도 그 메소드를 사용할 수 있기에 추상화되어 있다고 하는데요. 같은 맥락에서 내가 예상하지 못한 형태(내가 사용하고 있는 변수가 달라지는 것)은 수정해주신대로 지양하는 것이 좋을 것 같아요.
if (isEmpty(text)) { | ||
return WRONG_VALUE_RESULT; | ||
text = "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.
요구사항을 능동적으로 해석하고 유콩만의 룰을 만드셨네요👏 이 부분에 있어서 질문이 생기는데요.
- 기존에는 0을 상수화하여 사용하시다가 현재는 다시 매직넘버로 변경한 이유가 있을까요?
- 프로그램을 사용하는 사용자는, 빈 값을 입력했는데 0으로 간주되는 것이 맞는 방향일까요? 맞다라는 절대 기준은 없지만 저의 관점에선
어 입력 실수 했는데 왜 0이지?
라는 생각이 들 것 같아요. 조금 더 친절한 프로그램이 되기 위해 빈 값이 입력되면 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.
- 기존에는 0을 상수화하여 사용하시다가 현재는 다시 매직넘버로 변경한 이유가 있을까요?
지난번 피드백에서 제 나름 정의한 상수화의 기준
중 재사용성, 의미있는 값 등에 해당되지 않아 매직넘버로 변경하였습니다. 그러나 아래에 더하기, 곱하기에 대해 답변을 달다보니 오히려 더하기 기능이기 때문에 0으로 치환 했다면 매직넘버를 다시 상수로 변경하는 것이 적절하다는 생각이 드네요.😅
- 프로그램을 사용하는 사용자는, 빈 값을 입력했는데 0으로 간주되는 것이 맞는 방향일까요? 맞다라는 절대 기준은 없지만 저의 관점에선 어 입력 실수 했는데 왜 0이지?라는 생각이 들 것 같아요. 조금 더 친절한 프로그램이 되기 위해 빈 값이 입력되면 0으로 바뀌어서 진행된다는 안내멘트가 있으면 어떨까요?
테스트 코드에서 빈값 또는 null 이 입력 되었을 때 에러를 띄우는 것이 아닌 0을 반환하라는 요구사항이 있었습니다.
@Test
public void splitAndSum_null_또는_빈문자() throws Exception {
int result = StringCalculator.splitAndSum(null);
assertThat(result).isEqualTo(0);
result = StringCalculator.splitAndSum("");
assertThat(result).isEqualTo(0);
}
그것을 저는 빈 값을 잘못된 값으로 간주하지 않고 연산을 진행해야 한다, 라는 의미로 해석 하였고 만약 빈 값이 섞여서 (ex. "1,:3") 들어오면 에러를 띄우는 것이 아닌 그에 맞는 연산을 해야한다고 판단했습니다. 제가 구현하는 기능이 계산기의 더하기
기능이고 더할 때 결과값에 영향을 미치지 않으면서 연산에 포함 시키려면 0으로 바꾸어 나머지 연산(1+3)을 진행하는 것이 적절하다 생각했습니다. 만약 제가 구현하는 기능이 곱하기
였다면 결과값에 영향을 미치지 않기 위해 빈 값을 1로 바꾸었을 것 같습니다. 각 연산에 영향을 끼치지 않는 값으로 대체하는 것이 사용자의 입력 실수를 방지하는 것이라 생각합니다.
안내메시지는 생각하지 못했습니다.😅 지금 프로그램 대로라면 사용자는 직접 빈 값을 입력을 해봐야 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.
프로그램에 대한 유콩의 이해가 깊은게 느껴지네요🙂 잘못된 입력에 대해서도 프로그램은 정상동작해야한다 라는 관점에 대해서 잘 지켜주신 것 같아요. 말씀드린대로 유콩의 이해도와 프로그램 사용자의 이해도는 다를 수 있으니 이 부분에 대한 메시지도 잘 추가해주셨네요👏
private static final int CAR_LENGTH_LIMIT = 5; | ||
|
||
private final String name; | ||
private int position = 0; | ||
|
||
public Car(String name) { | ||
checkValid(name); | ||
checkValidName(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.
생성자 오버로딩을 사용할 때는 부생성자들이 항상 마지막엔 주 생성자를 호출하는 형태가 일반적인데요.
그 이유에 대해 고민해보면 좋을 것 같아요.
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번째 생성자가 1번 생성자를 호출하고 있는데, 이 방향이 맞을까요?
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.
제가 말씀드린 이유는 각 파라미터에 대한 일관된 검증을 하기 위함인데요. String name만 가지고 생성하는 생성자의 경우 Position 에 대한 Validation 을 수행하지 않기에 발생할 수 있는 이슈들을 제거하기 위해 호출의 방향을 변경하는 것이 좋을 것 같다고 생각했습니다.
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.
name 만으로 생성자를 호출할 경우 검증이 필요없는 0이 position 에 대입되므로 검증이 필요 없을 거라 생각했습니다. 혹시나 다른 개발자가 초깃값을 수정할 가능성이 있기에 잘못된 객체 생성을 막기 위해서는 검증이 포함되는 구조로 바꾸는 것이 좋겠군요. 테스트를 위한 생성자를 삭제하면서 해당 생성자는 삭제했지만 다음 미션때는 그 부분을 고려하도록 하겠습니다. 감사합니다!
public void drive(boolean directing) { | ||
if (directing) { | ||
move(); | ||
} | ||
} | ||
|
||
public String step() { | ||
return name + " : " + STEP.repeat(position); |
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.
스텝이라는 네이밍은 차를 전진한다라는 의미로 받아들여질 여지도 있는 것 같아요. 조금 더 직관적인 이름(자동차가 전진한 것 현 위치를 보여준다)이 없을까요?
추가로 질문주신 부분에 대해 같이 얘기해보면 좋을 것 같습니다. 유콩이 생각하신대로 포지션을 가지고 있으며 스스로 능동적인 객체가 되기 위해선 자동차가 상태값을 가공하여 넘겨준다는 관점은 매우 좋은 것 같습니다.
다만 자동차라는 객체가 담당하는 핵심적인 역할이 출력과 관련이 있는가?
라는 관점에서도 고민이 필요한 것 같아요. 자동차와 관련된 비즈니스 로직들이 변경되었을 때 자동차 코드를 변경하는건 매우 자연스럽고 당연한 일인 것 같아요. 그렇다면 화면에 출력하는 형태가 - 에서 = 로 바뀌었을 때 자동차 클래스가 변경되는 것이 어떤 위험을 야기할 수 있을까요? 조금 더 말씀드리자면 뷰의 영역은 다양한 사용자가 존재할 수 있기 때문에 각 사용자에게 맞는 형태로 뷰를 보여줘야하는 경우가 잦은데 이 때 이 로직이 모두 우리의 모델객체안으로 들어온다면 자동차가 수행해야하는 비즈니스 요구사항에 따른 변경이 아닌, 단순히 뷰를 위한 변경이 잦아지고 많아질 것 같아요.
유콩이 생각하신대로 객체가 능동적으로 자신의 상태를 관리한다라는 측면에선 분명 옳은 선택일 수 있지만, 그 객체가 가지고 있는 핵심적인 로직은 무엇인가 / 레이어간(MVC)의 역할은 잘 분리되어 있는가 라는 측면에서 본다면 다른 답이 나올 수도 있는 것 같아요.
유콩이 선택한 방식과 출력은 뷰에서 가공/담당하는 방식의 장단은 분명 존재하기에 이 장단에 대해서 이해하는게 중요할 것 같아요.
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.
step 을 showNowPosition
으로 수정했다가 출력하는 방식을 바꾸어 사용하지 않아 삭제했습니다!
MVC 패턴에 관한 생각은 아래 피드백에 댓글을 달았습니다. 상세한 설명 감사합니다!!😁😁
} | ||
Output.roundResult(cars); | ||
Output.newLine(); |
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.
MVC구조로 리팩토링하는 것이 미션
인만큼 이 부분도 고민해보면 좋을 것 같아요.
MVC 구조라고 했을 때 각 구성요소간의 의존관계의 방향은 어떤 형태를 띄어야하는가? 예를 들어 모델은 뷰를 알아도 되지만 뷰는 모델을 알면 안된다거나, 서로 아는 것이 자연스럽다거나 등 유지보수하기 좋은 코드
라는 관점에서 MVC 구성요소간의 이상적인 의존방향을 고민해보면 좋을 것 같습니다.
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.
카일의 피드백을 보기 전까지는, MVC 패턴의 의존 관계는 Model 과 Controller, Controller 와 View 로만 상호작용을 할 수가 있고 View 는 어떤 프로그램이든간에 독립적으로 있어야 하며 Model 은 폐쇄적이어야 한다 라고 생각했습니다. 뷰는 말 그대로 입력/출력 에 관한 객체이므로 그에 관한 가공 및 처리는 Controller 또는 Model 이 맡아야 하며 Model 은 실제 값을 가지는 객체이므로 외부 객체가 값을 알아선 안되기 때문입니다. MVC 패턴에 대해 정확하게 이해하고 있는 것이 아니기 때문에 각 객체가 정의하는 역할에 벗어나서는 안된다 생각하였고 그러다보니 각 역할에 대한 기능을 하도록 끼워맞추기를 했던 것 같습니다.
제가 작성하는 코드는 단순한 문제 풀이가 아닌 이후에 특정 서비스를 제공하는 소스가 될텐데 그 관점을 생각 못했습니다. 제공하는 서비스의 성격에 따라 보안이 중요시되어 Model 의 정보 유출에 민감한 경우
가 있을 것이고, 카일이 예를 들어준 보여지는 정보가 수정이 잦은 경우
도 있을 것입니다. 혹은 그 외에 제가 생각하지 못한 또 다른 예시들이 있을 수도 있구요.
자동차 경주 미션에서는 사실 자동차가 가지는 정보(이름, 위치)가 크리티컬한 정보가 아니기도 하고 일반 사용자를 대상으로 하는 프로그램이기 때문에 UI(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.
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.
- View 는 어떤 프로그램이든간에 독립적으로 있어야 한다.
제가 이해한 MVC 는 모델은 단순하게 값을 저장하고 수정하는 역할만 하며 컨트롤러에게 값을 전달한다, 뷰는 입력과 받아온 값을 출력만 한다, 그 사이의 컨트롤러는 모델에서 값을 받아와 뷰에게 전달한다 입니다. 독립적이어야 한다의 의미는 요구사항의 수정까지는 생각하지 못했고 뷰는 모델과 컨트롤러의 역할을 몰라도 된다의 의미였습니다.
- 코드 수정
모델에 있던 뷰 메소드는 카일의 피드백대로 객체를 분리하여 해결하였습니다!
private boolean isCars(String[] names) { | ||
return names.length >= CAR_LIMIT; | ||
} | ||
|
||
private boolean isCars(List<Car> cars) { | ||
return cars.size() >= CAR_LIMIT; |
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개씩 작성되어있는데, 혹시 이유가 있을까요?
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.
생성자를 오버로딩 하다보니 각 타입에 맞는 검증 메소드를 새로 생성했었습니다. 카일의 피드백을 보고 생성자 내부 구조를 수정하였고 수정함에 따라 사용하지 않은 중복적인 메소드들은 삭제했습니다!
@@ -24,13 +24,22 @@ public void run() { | |||
} | |||
|
|||
private void init() { | |||
cars = input.carName(); | |||
attempt = input.attempt(); | |||
cars = createCars(input.carName()); |
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.
요 부분과 논외이지만, 유콩이 생각하는 서비스
라는 패키지의 Game 객체의 역할은 무엇이라고 정의하고 계신가요? 같이 생각해보고 고민해보면 좋을 것 같습니다.
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.
저는 service
패키지 내의 객체들은 비지니스 로직을 수행하는 역할로 부여하였습니다. MVC 패턴으로 볼 때 Controller 의 역할을 담당하며 Controller 내에서도 Model 과 View 를 단순하게 연결짓는 것과 비지니스 로직(모델 상태정보 변경, 안내문 출력 등) 의 기능들이 방대하지 않아 service 하나로 통합하였습니다.
이번 프로그램에서 service 의 RacingCarGame 은 자동차 객체들의 상태값을 변화시키는 기능을 호출하며 사용자에게 입력 메시지 또는 에러메시지 등의 안내를 하는 뷰를 불러와 모델과 뷰의 중간에서 프로젝트의 기능들을 관리하도록 하였습니다. 최종 우승자를 출력하는 과정에서 모델에서 뷰를 호출하였지만 되도록 모델과 뷰가 의존되지 않도록 코드를 작성하였습니다.
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.
요 부분에 대해서는 일반 서비스
라는 것은 MVC에 포함된 요소가 아닌데 어떤 이유로 도입된 패키지일까요? 추가로 비즈니스 로직(모델 상태정보 변경, 안내문 출력)이라고 작성해주셨는데, 모델 상태정보 변경과 안내문 출력 모두가 비즈니스 로직일지 고민해보면 좋을 것 같아요.
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.
서비스와 컨트롤러 역할에 대해 다시 찾아보았습니다.
-
controller
모델과 뷰를 연결짓는다. 프로그램을 실제로 이용하는 사용자의 역할을 맡는다. -
service
사용자가 접하는 기능을 구현한다. 자동차 경주 구현 미션을 예로 들면 사용자에게 우승할 자동차를 예측하는 기능을 제공할 수 있다.
서비스는 MVC 패턴에 대해 찾아보다가 접했고 필요에 따라 MVC 에 포함할 수 있다고 생각해 추가했습니다. 그러나 제공하는 기능만을 두고 봐도 사용자가 자동차 경주 게임을 실행한다, 라고 생각했을 때 controller 가 되는 것이 맞겠네요. 패키지명은 수정했습니다!
(예시로 적었던 모델 상태정보 변경은 모델, 안내문 출력은 뷰 겠네요.😅😅)
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.
조금 더 명확하게 각 역할을 분리해나가고 계신 것 같아요🙂 서비스는 현재 범위에서 제외하고, 컨트롤러에와 뷰 / 모델에 대해서 잘 이해하고 계신 것 같습니다.
try { | ||
Output.getAttempt(); | ||
String inputValue = scan.nextLine(); | ||
return new Attempt(inputValue); | ||
return inputText(); |
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.
뷰에서는 되도록 모델과는 연관이 없는 코드를 작성하고 싶었습니다. 입력 객체의 역할은 단순하게 사용자의 입력을 받아 반환하는 것이고 그 결과값을 받아와 생성자를 호출하는 것은 Controller(service) 의 역할이라고 생각해 수정하였습니다.
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.
위 피드백에 답변 남겼습니다.😁
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.
뷰가 도메인을 모르기 위해서 변경해주셨군요👏
답변은 여기다 남기는게 맞나요...?🤣
CarsTest 에서 여러 자동차들 중 가장 많이 이동한 위치(maxPosition)를 찾고 그 위치의 값을 확인하는 테스트 항목이 있었습니다. 그러나 Cars 의 메소드 중에서 자동차 객체들 중 가장 큰 위치 값을 가지는 Car 를 반환하는 메소드(findMaxPositionCar) 의 접근제어자는 private 이었고 해당 메소드를 사용하지 않고 간접적으로 동일한 테스트를 할 수 있는 방법을 고민해봤습니다. 테스트를 위한 구현 코드는 PR 보낼 때 작성했듯이 애초에 구현 코드가 제대로 작성되었는지를 확인하는 것이 테스트 코드기에 목적에 맞지 않아 적절하지 않는다고 생각합니다. (원하는 값을 셋팅하기 위해 특정 변수를 생성한다던가 등의 비지니스 로직과는 연관이 없는 것은 예외라고 생각합니다..) 그러나 생성자는 내부 중요 로직이 노출되지 않으면서 사용자의 편의성을 높일 수 있다고 생각합니다. 테스트 혹은 프로그램이 확장됨에 따라 활용도가 높아질 수 있을 것 같습니다. 객체를 생성하는 방법이 추가되는 것 뿐이라고 생각합니다. |
- CarTest 에서 true, false 를 지정하는 것이 아닌 해당 범위에 속해있는 값이 뽑힐 때 이동/대기하도록 수정 - 랜덤 값을 임의로 지정할 수 있게 하기 위해 랜덤 값을 뽑는 로직을 별도로 분리
전략 패턴에 대해 공부해보고 그에 맞춰서 수정해보았습니다! 전략 패턴으로 바꾸니 테스트 코드에서 람다식을 이용해 랜덤 값의 결과를 임의로 지정할 수 있군요! 실제로 적용해보니 어떤 값을 입력받고 출력하는 것이 테스트에 용이한지 파악하는 것이 조금 어려워서 추가 공부가 필요할 것 같습니다. 감사합니다! |
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 checkValid(String names) { | ||
@Override | ||
public String toString() { | ||
return "name : " + name + ", position : " + position; |
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단계 요구사항에서 domain 패키지의 객체는 view 패키지 객체에 의존하지 않는다.
요게 있는 것으로 알고 있어요.
이 요구사항이 의미하는 바를 조금 더 깊게 고민해보면 좋을 것 같아요. 단순히 패키지 의존만을 의미하는 것인지, 아니면 도메인 패키지가 뷰의 로직을 알면 안된다인지 생각해보면 좋을 것 같아요.
if (isDuplicated(names)) { | ||
throw new IllegalArgumentException("자동차 이름을 모두 다르게 입력해주세요."); | ||
@Override | ||
public String toString() { |
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.
마찬가지로 도메인 도메인에 toString이 있는데 위에서 남긴 고민에 대해서 같이 고민해보아요.
|
||
public interface Movable { | ||
|
||
int randomNumber(); |
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.
전략패턴 도입 👏 다만 Movable 이라는 인터페이스가 랜덤넘버를 반환하는 것이 부자연스럽지 않나요?
추가로 전에 말씀드렸던 Input에 대해서 말씀드리고 싶었던 부분은 아래와 같아요.
- 전략패턴을 도입하는 경우 인터페이스 형태로 파라미터를 전달받게 되는데, 그렇다면 Input타입도 내부에서 구체클래스를 선언하는 것이 아니라 인터페이스를 파라미터로 받는 형태로 변경되겠죠?
- 그렇게 되면 우리는 Scanner NextLine()이라는 테스트 할 수 없는 입력에 대해서도 지금 Movable을 테스트할 수 있는 것처럼 테스트할 수 있을 것 같아요🙂
- 결국 인터페이스를 파라미터로 받고 외부에서 그 구현클래스를 주입했을 때 얻어지는 장점은 여기서 사용한 전략패턴과, Input 인터페이스 + 파라미터화 한 이후에 외부에서 받는 경우의 맥락이 맞닿아 있어서 말씀드렸어요ㅎㅎ
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.
자동차의 움직임을 결정하기 때문에 이름이 Movable
인 만큼 반환값을 랜덤 값이 아닌 움직임에 대한 여부를 반환하도록 수정하였습니다!
안그래도 입력 객체를 수정하기 전에는 Input 에서 입력 + 생성자 호출이 있어 제가 원하는 입력에 대한 테스트가 어려웠습니다. 생성자를 호출함으로써 간접적으로 테스트 하는 방향으로 바꾸었지만 다음 미션에서 제가 임의로 지정할 수 없는 값에 대해 테스트를 해야 하는 상황이 온다면 전략 패턴을 이용하면 좋을 것 같습니다. 감사합니다.😁
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.
좋은 관점인 것 같아요. 인터페이스를 통해 이룰 수 있는 것은 여러가지가 있지만, 요번 미션에서는 테스트하기 어려운 것을 분리하여 외부에서 주입받는 형태로 만들어 테스트가능하게 한다
라는 점에 대해서 인지하고 가시면 충분할 것 같습니다!
@@ -0,0 +1,9 @@ | |||
package racingcar.domain; | |||
|
|||
public class MovableImpl implements Movable { |
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.
Impl은 일반적으로 구현체에서 자주 사용하는 네이밍전략인데요.
Impl 보단 랜덤한 값으로 갈지 말지를 정한다는 네이밍이 적절하지 않을까요?
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.
DecisionToMoving
으로 바꾸었습니다!
if (directing) { | ||
move(); | ||
} | ||
public Car(String name, int position) { |
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.
- 유콩이 말한 테스트에서만 사용하는 생성자라는 것과 그리고 단순히 생성자가 하나 늘어난다라는 관점도 일리가 있지만, 유콩의 코드를 유지보수하는 다른 개발자는 사용할 수 있는 단순히 테스트만을 위해 존재한다고 생각하기 어려울 것 같아요.
해당 생성자가 테스트만을 위해 추가된 생성자인지 실제로 코드 내에서 사용하기 위해 추가된 생성자인지는 다른 개발자도 포함하여 저도 시간이 오래 지나면 알아보기 힘들 것 같습니다. 이 생성자는 구현 코드 내에서 사용하면 안된다! 의 이유가 존재한다면 그 생성자는 추가하면 안될것 같습니다. 그 이유는 생성자마다 다를 것 같습니다.
public Car(final String name, final int position) -> 추가 X
자동차 경주 미션은 말 그대로 자동차들이경주
하는 프로그램이므로 경주를 진행하기 위해서는 자동차들의 출발지점이 같아야 합니다. 이 생성자가 존재하면서 다른 개발자는어떠한 경우에 자동차의 위치를 고정시켜서 사용해도 되는거지?
라는 혼란이 올 것 같습니다.
public Cars(final List cars) -> 추가 O
이미 자동차 객체를 검증했기 때문에 검증된 자동차 객체들의 리스트를 묶어도 된다고 생각합니다. Cars 를 생성하는 당시에 문자열로 입력했을 때의 검증을 모두 포함되어 있기 때문입니다.
public Attempt(final int attempt) -> 추가 O
위의 두 개의 생성자와 같이 여러 방식으로 생성자를 호출할 수 있다면 시도횟수는 본래 숫자가 저장되니 숫자로도 저장할 수 있어야 할까? 해서 추가했습니다. 시도횟수의 생성자는 테스트를 위해 추가한 것은 아니며 추가되었기 때문에 테스트를 추가한 것입니다.
테스트만을 위한 생성자를 추가한다 하더라도 추가한 생성자가 프로그램 내 요구사항(ex. 자동차들의 출발지점 동일) 또는 로직(ex. 자동차 이동 시 1칸씩 이동) 등 대전제를 반하는 내용이 있다면 그 생성자는 추가하지 않아야 한다고 생각합니다. 하지만 그렇지 않다면 추가해도 된다고 생각합니다. 다른 개발자가 테스트만을 위한다고 생각하지 않아도 요구사항에 반하지 않으며 선택의 폭이 넓어져 활용도가 높아질 것 같습니다...🥺
- 유콩이 해당 생성자를 추가한 이유는 최대값을 구해오기 위한 로직들(차를 각각 이동시키는 것)이 테스트코드에 추가되는 것이 바람직하지 않다라고 생각하신 것 같아요. 이 부분이 바람직하지 않은 이유가 있을까요?
Cars 의 테스트 중 우승자에 관한 테스트를 추가하고 싶었고 테스트를 하려면 findWinners 메소드를 사용해야 했습니다. findWinners 메소드는 Cars 내에 있기 때문에 테스트 대상이 되는 객체는 Cars 여야 하고 Cars 로 묶인 자동차들의 위치를 임의로 수정하기 위해 테스트를 위한 생성자를 떠올렸습니다. car 의 drive 메소드의 접근 제어자가 public 이기 때문에 일단은 다른 방식으로 코드를 수정했습니다!
그것과는 별개로 카일의 피드백에 의견을 말해보자면, 동일한 메소드(ex. private 메소드를 테스트 하기 위해 private 메소드를 테스트로 복사해와 사용함) 를 테스트에 추가한다면 유지보수 측면에서 이후에 수정되면 동일한 메소드가 있는지 확인하는 과정이 필요하다고 생각합니다!!
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.
- 해당 생성자가 테스트만을 위해 추가된 생성자인지 실제로 코드 내에서 사용하기 위해 추가된 생성자인지는 다른 개발자도 포함하여 저도 시간이 오래 지나면 알아보기 힘들 것 같습니다. 이 생성자는 구현 코드 내에서 사용하면 안된다! 의 이유가 존재한다면 그 생성자는 추가하면 안될것 같습니다. 그 이유는 생성자마다 다를 것 같습니다.
- 맞습니다. 생성자는 상황에 따라 이유에 따라 추가될 수 있다고 생각합니다. 유콩이 내린 결론으로 가는 것이 맞는 것 같아요. 현재 이 케이스에선 테스트를 위한 부적절한 생성자 (포지션을 임의로 설정하는)가 포함되었으니 코멘트드렸던 것 이구요.
- 유콩이 해당 생성자를 추가한 이유는 최대값을 구해오기 위한 로직들(차를 각각 이동시키는 것)이 테스트코드에 추가되는 것이 바람직하지 않다라고 생각하신 것 같아요. 이 부분이 바람직하지 않은 이유가 있을까요?
- 저는 이 부분에서 자동차의 위치를 전략패턴을 통해 충분히 원하는 형태로 움직인 후, 우승자를 찾으면 되는데 그 부분이 구현/테스트 되어 있는데 생성자를 추가해서 이를 해결하려고 한 이유가 무엇일까 라는 질문이었습니다.
- 라운드 별 결과를 출력해야 하는 과정에서, Model 에서 View(Output) 를 호출하지 않기 위해 별도 객체 생성
- 자동차의 '움직임'에 관한 객체기 때문에 의미가 더 가깝게 이동 여부를 반환하도록 수정
- 메소드 내에서 값이 수정되지 않음을 명시적으로 알려줌
- DS_Store 추가
- 테스트만을 위한 생성자였음 - position 을 임의로 지정할 수 있다라는 것을 내포하고 있기 때문에 삭제
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 Input input; | ||
|
||
public RacingCarGame() { | ||
input = new Input(); |
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.
정답은 없지만, 내부에서 선언하여 같은 값을 사용한다면 위의 RoundResult처럼 같이 선언과 동시에 초기화하거나 객체 생성자에서 초기화하는 일관된 형태를 띄면 좋을 것 같습니다.
} | ||
|
||
@Override | ||
public String toString() { |
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.
메소드 위치의 절대적으로 옳은 위치는 없지만, toString이 단순히 값을 편히 보기위한 편의기능이라면 아래의 private메소드들이 이 객체의 역할을 더 드러내는 것으로 보여 프라이빗과 투스트링의 위치를 수정하는건 어떨까요?
attempt = createAttempt(input.attempt()); | ||
} | ||
|
||
private Cars createCars(final String carName) { |
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.
요 부분은 취향차이긴 하지만, 저는 new Cars(carName) 자체만으로도 createCars라는 의미를 충분히 전달하는 것 같습니다.
return new Cars(carName); | ||
} | ||
|
||
private Attempt createAttempt(final String attempt) { |
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.
ditto
import java.util.Collections; | ||
import java.util.List; | ||
|
||
public class RoundResult { |
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.
출력을 위한 객체를 분리해주셨군요👏 조금 더 나아가서, 출력을 위해 필요한 값을 필드로 가지는 객체로 만들어보는 건 어떨까요?
현재는 실제 출력의 포맷을 만들어내는 형태로 구현해주셨는데, 출력이 매우 복잡한 경우 이런 객체를 사용하는 것은 좋은 것 같습니다. 다만 현재는 출력을 위한 단순 데이터 덩어리의 DTO를 만들어내고, Output에서 출력의 형태 (전진은 - 로 표현, : 로 구분 짓기)를 담당해도 좋을 것 같습니다.
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.
추가로 해당 클래스는 뷰를 위한 로직들로만 구성되어 있는데 도메인에 현재 속해있는데요. 이 클래스가 어디에 위치하면 좋을지도 고민해보면 좋을 것 같습니다.
조금 더 많은 고민을 원하시면 위에서 말씀드린 데이터 덩어리의 DTO를 만들었을 때는 어느 패키지로 가는 것이 좋을지도 같이 고민해보면 좋을 것 같습니다.
for (String name : names) { | ||
cars.add(createCar(name)); | ||
} | ||
public Cars(final String[] names) { |
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 void play() { | ||
public void play(final RoundResult roundResult) { |
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.
다른 코멘트와 마찬가지로 이 부분도 조금 더 고민해보면 좋을 것 같아요.
현재 RoundResult는 완전히 뷰를 위한 로직들을 수행하고 있어요. 그렇다는 말은 도메인인 Cars가 RoundResult를 import해서 사용하는 것 만으로도 도메인이 뷰에 의존하는 형태가 되는데요.
이 부분을 이름과, 값이 들어있는 DTO 리스트 형태로 반환하는 형태로 변경하면 어떨까요?
try { | ||
Output.getAttempt(); | ||
String inputValue = scan.nextLine(); | ||
return new Attempt(inputValue); | ||
return inputText(); |
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.
뷰가 도메인을 모르기 위해서 변경해주셨군요👏
@@ -75,7 +84,7 @@ private Car findMaxPositionCar() { | |||
.orElseThrow(() -> new NoSuchElementException("max 값을 찾을 수 없습니다.")); | |||
} | |||
|
|||
private List<String> findSamePositionCar(Car target) { | |||
private List<String> findSamePositionCar(final Car target) { |
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단계 코드를 제출합니다. ㅎ_ㅎ
지난번 1단계 때 너무 늦게 제출해서 빠르게 제출하고 싶었는데 그다지 빠르진 않은것 같네요...🤣
2단계는
MVC 패턴으로 리팩토링하라
였는데 이미 구조가 나뉘어져 있어서 코드만 일부 수정했습니다.지난 피드백 관련
1단계 피드백에서 구현 또는 답변하지 못하고 넘어간 것에 대해 결론을 내려봤습니다.
1. Model 의 상태값 출력 위치
지난 피드백을 보고 Model 이 가지는 상태값을 출력할 때 getter 를 쓸것인지, Model 에서 View 의 역할을 할 것인지 고민이 많았습니다. 일단 저는 getter 를 써서 모델이 가지는 상태값을 노출하는 방법보다 모델에서 정제된 값을 넘겨주는 방식을 택했습니다. 모델과 뷰 만을 놓고 봤을 때 뷰는 그저 입력/출력을 담당하는 객체이고 그 외의 객체들이 어떤 역할을 하는지와는 연관이 없어야 한다고 생각했습니다. 모델은 객체가 가지는 상태값을 관리하는 주체기 때문에 모델에서 자기 자신이 가지는 값에 대한 처리를 완료한 이후 외부로 내보내는 것이 적절하다고 생각합니다.
질문
1. 테스트를 위한 생성자
CarsTest 에서 제가 원하는 위치(position)에 있는 Car 를 생성하기 위해 실제 구현 코드 내에서는 사용하지 않는 생성자를 추가하였습니다. (
Car(String name, int position)
) 테스트만을 위한 코드는 애초에 구현한 코드를 테스트한다는 테스트 코드의 역할이 맞지 않아 적절하지 않으나 생성자의 경우 구현 코드와는 거리가 멀다고 생각합니다. 이후에 프로그램의 크기가 커짐에따라 사용될 가능성이 있을 수 있고 객체를 생성할 때 검증을 하므로 생성자에 한해 가능하다고 결론을 내려 추가했습니다. 카일님은 어떻게 생각하시는지 궁금합니다. 👀2. Input 변수 내부 초기화 vs 파라미터
Input
이라는 변수를 내부 생성자에서 초기화하는 것과, 외부에서 해당 값을 파라미터로 주입해주는 것에는 어떤 차이가 있을까요? 랜덤을 테스트하는 것과 이어서 고민해보면 좋을 것 같아요→ 지난번 피드백 중 일부 내용입니다.
이 문제에 관해 고민을 해보았지만 질문의 내용을 정확히 이해하지 못해 생각한 내용만이라도 적어봅니다. 내부 생성자에서 초기화를 한다면 해당 인스턴스에 속해있는 값일 것이고 외부에서 파라미터로 입력받는다면 외부에서 공용으로 쓰는 input 값일 것입니다. input 을 공유하는 범위에 속해있는 경우 상태값을 공유할 수 있습니다.
나름 생각을 해보려했으나 랜덤을 테스트하는 것과 어떤 연관성이 있는지 모르겠습니다...🥲
리뷰 감사합니다. 좋은 하루 보내세요! 😊