[그리디] 김하은 자동차 경주 미션 1,2단계 제출합니다.#170
Conversation
There was a problem hiding this comment.
안녕하세요 하은님 🙋♂️
첫 미션 고생하셨습니다!
하지만 지금 제출해 주신 미션에서 자동차 미션의 2단계의 중요한 요구사항이 하나 빠져 있는것 같아요
[2단계 요구사항 중..]
- "주어진 횟수" 동안 n대의 자동차는 전진 또는 멈출 수 있다.
- **"0에서 9 사이에서 random 값"**을 구한 후 random 값이 4 이상일 경우 전진하고, 3 이하의 값이면 멈춘다.
이 요구사항을 마저 반영하신 다음에 좀 더 디테일하게 리뷰를 남겨 보도록 할게요!!
궁금하셨던 점
Q. Git 사용이 아직 익숙하지 않아 질문드립니다. commit 후에 바로 push를 하는 방식이 일반적인지, 아니면 commit을 여러 개 쌓아둔 뒤 마지막에 한 번에 push하는 방식이 더 권장되는지 궁금합니다.
푸시는 내 로컬에서 작업한 내용(커밋)을 원격 저장소(깃허브)에 올리는 것이기 때문에,
모든 작업이 완료된 뒤 한 번에 push를 해도 괜찮습니다
다만 push 시점 자체보다 더 중요한 것은 커밋의 단위를 어떻게 나누느냐라고 생각해요
내가 작업한 부분이 무엇인지 명확하게 드러나도록 커밋 단위를 나누고 메시지를 작성하면,
나중에 변경 이력을 되돌아볼 때도 훨씬 이해하기 쉽고,
문제가 생겼을 때 어떤 작업에서 변경이 발생했는지도 더 빠르게 파악할 수 있습니다
또 협업 상황에서는 다른 사람이 커밋 내역만 보더라도
어떤 의도로 수정했는지 흐름을 이해하는 데 도움이 되기 때문에,
작업이 잘게 보이더라도 의미 있는 단위로 커밋을 나누는 습관이 더 중요하다고 생각해요
Q. 자바 코딩에 아직 익숙하지 않아 기초 미션임에도 어려움을 느꼈습니다. 특히 for문 안에 if문을 사용하지 않는 조건과 메서드 길이 제한 조건 때문에 RacingGame 클래스의 코드를 여러 메서드로 분리하는 과정이 가장 어려웠습니다.
이 부분은 미션이 왜 이러한 제약 조건을 제시했는지를 고민해 보시면 좋을거 같아요 !
미션에는 다음과 같은 제약이 존재했었는데요,
- indent(인덴트, 들여쓰기) depth를 2를 넘지 않도록 구현한다. 1까지만 허용한다.
예를 들어 while문 안에 if문이 있으면 들여쓰기는 2이다.
힌트: indent(인덴트, 들여쓰기) depth를 줄이는 좋은 방법은 함수(또는 메서드)를 분리하면 된다.
이런 제약은 단순히 코드를 잘게 나누라는 의미보다는,
하나의 메서드가 너무 많은 일을 하지 않도록 만들고,
각 메서드가 조금 더 명확한 역할을 가지도록 연습해보라는 의도에 가깝다고 생각해요. +) 코드 가독성
그래서 메서드를 분리할 때도
단순히 제약을 맞추기 위해 쪼개는 것에서 끝나기보다,
이 메서드가 어떤 역할을 맡고 있는지,
분리했을 때 오히려 의도가 더 잘 드러나는지를 같이 고민해보면 더 좋을 것 같습니다 !
P.S 이번 미션에서 위의 제약을 만족하기 위함이 아니라
메서드들이 하나의 책임만을 가지도록 분리해 보는 연습을 해 보시면 감이 오실거에요
제가 드릴 추가 요구사항
이제 남은 요구사항을 추가로 진행하시게 될 텐데, 제가 드릴 몇 가지 요구사항이 있습니다!
-
메서드 시그니처만으로도 해당 메서드가 무슨 동작을 하는지 의도가 보일 수 있도록 메서드 네이밍을 고민해 주세요.
-
RacingGame클래스의 책임은 무엇일까? 를 고민하면서 빠진 요구사항을 진행해 주세요. -
커밋 메시지는 작업 내용이 드러나도록 구체적으로 작성해 주세요. (⭐️일단은 제목만!!⭐️)
-
이번 자동차 경주 미션에서 구현한 내용을
README.md에 간단히 정리해 주세요.
3번 요구사항의 커밋 메시지는, 보통 많은 팀에서 공통적으로 사용하는 작성 규칙이 있습니다
이런 규칙을 컨벤션이라고 하는데,
커밋 로그만 보아도 어떤 작업이 추가되었는지, 수정되었는지 쉽게 파악할 수 있도록 도와줍니다
국룰처럼 많이 사용되는 방식이 있는데요, 이걸 따라서 한번 해 보시는 것도 좋을것 같아요
위 링크는 공식 문서 같은건데 처음 입문하긴 좀 어려워서
요기 간단 정리된 블로그를 참고하면서 감을 잡아 보시는 걸 추천해요!
처음부터 모든 형식을 완벽히 지키실 필요는 없고,
커밋 제목만 읽어도 작업 내용이 드러나도록 작성해보시면 좋을것 같아요!!
예를 들면 다음과 같이 작성할 수 있어요
- feat: 우승자 판별 기능 구현
- test: 자동차 이동 테스트 추가
- docs: 자동차 경주 미션 내용 README 작성
어떻게 진행할지 방향을 잘 못 잡으시겠다면 물어봐 주셔도 좋고,
스터디원들의 리뷰를 읽어 보는것도 도움이 많이 될거에요 !
화이팅 🔥🔥🔥🔥🔥
P.S 리뷰 반영이 완료 되면 디스코드 스레드에 멘션 남겨 주세요!
| public void move(int randomValue){ | ||
| if(randomValue >= 4){ | ||
| this.position++; | ||
| } | ||
| } |
There was a problem hiding this comment.
이동 조건 값 4는 상수로 분리하는건 어떨까요?
현재 move() 메서드 안에서 4가 직접 사용되고 있는데,
이런 값은 의미를 담은 상수로 분리해 두면 코드를 읽는 사람이 이 4라는 숫자가 무엇을 의미하는지 더 쉽게 이해할 수 있을 것 같아요.
또 나중에 기준값이 바뀌더라도 여러 곳을 찾지 않고 한 곳만 수정하면 되어서 유지보수도 쉬워지고요.
예를 들면 단순한 숫자 4보다,
전진 가능한 최소 값 같은 의미가 드러나는 이름이 있으면 의도가 훨씬 잘 보일 것 같습니다!
| } | ||
| } | ||
|
|
||
| public boolean isAtPosition(int MaxPosition){ |
There was a problem hiding this comment.
다른 코드는 잘 작성해 주셨는데 요기는 아마 실수인거 인거 같아요
MaxPosition은 자바 네이밍 관례상 maxPosition이 더 자연스러워 보입니당
| @Test | ||
| void 자동차가_4이상일때_전진하고_3이하일때_멈추는지_테스트(){ | ||
| Car car = new Car("Pobi"); | ||
|
|
||
| car.move(4); | ||
| assertThat(car.getPosition()).isEqualTo(1); | ||
|
|
||
| car.move(3); | ||
| assertThat(car.getPosition()).isEqualTo(1); | ||
| } |
There was a problem hiding this comment.
현재 이 테스트에서는 전진하는 경우와 전진하지 않는 경우를 한 번에 같이 검증하고 있는데,
이 둘을 나누면 테스트가 무엇을 확인하는지 더 분명해질 것 같아요
그래서 테스트가 실패했을 때도
- 전진해야 하는데 못 한 건지
- 전진하면 안 되는데 전진한 건지
원인을 더 바로 파악할 수 있어요
There was a problem hiding this comment.
단일 자동차의 이동 로직을 RacingTest에서 검증하는 것이 맞을지도 한 번 고민해보시면 좋을 것 같아요
현재 코드를 보면 단일 자동차의 이동 여부는 Car 클래스가 직접 결정하고 있으므로,
이 규칙은 레이싱게임 보다는 개별 자동차의 책임에 더 가깝게 느껴집니다
+) 테스트 클래스 파일을 쉽게 생성하는 인텔리제이 단축키가 있는데
테스트를 하고자 하는 로직이 있는 클래스에서 다음의 단축키를 누르면 간편하게 테스트 클래스를 생성할수 있어요
mac [ cmd + Shift + T ]
windows [ Ctrl + Shift + T ] ?? 윈도우를 쓰진 않아서 맞는지 잘 모르겠네요
There was a problem hiding this comment.
단축키까지 알려주셔서 정말 감사합니다!! CarTest 파일을 만드는데에 써봤어요 ㅎ
| public class Car { | ||
| private final String name; | ||
| private int position; |
There was a problem hiding this comment.
여기 name을 final 로 선언한 것은 매우 좋은것 같네요!
final 로 선언을 했을때 무슨 이점이 있을까요??
There was a problem hiding this comment.
미션에서 name은 한번 지정되면 바뀌지 않는다고 했으므로 final로 선언했습니다! final이 이름이 바뀌지 않는다는 것을 보장해줘 코드를 안전하게..? 만들어주는 이점이 있을 것 같습니다!!! 또한 다른 사람이 코드를 봤을 때 final이라는 단어 하나로 자동차 객체의 불변성을 이해할 수 있을 것 같습니다.
There was a problem hiding this comment.
final은 객체가 한 번 생성된 이후 상태가 바뀌지 않기 때문에 상태를 예측하기 쉬워진다는 장점이 있습니다
저는 개인적으로 가장 큰 장점은 말씀해주신 것처럼,
다른 사람이 이 코드를 보았을 때 “이 값은 절대 바뀌지 않겠구나"를 바로 이해할 수 있다는 점이라고 생각해요
그 덕분에 코드를 처음 읽는 사람도
상태 변경 가능성을 함께 고민하지 않아도 되어서,
코드를 훨씬 더 쉽게 이해할 수 있는 것 같습니다
| @Test | ||
| void 자동차가_4이상일때_전진하고_3이하일때_멈추는지_테스트(){ |
There was a problem hiding this comment.
정답이 있는 부분은 아니지만,
테스트 이름에서 의도는 잘 드러나긴 하는데, 조금 더 개선해보면 좋을 것 같습니다
개인적으로는 메서드명을 ~테스트처럼 마무리하기보다
테스트의 조건과 기대하는 결과가 함께 드러나도록 작성하면 더 명확한것 같더라구요
~테스트라는 표현은 단순히 테스트 코드라는 사실만 전달하는 반면,
에를 들어 4이상일때_자동차는_전진해야_한다처럼 작성하면
이 테스트가 어떤 규칙(명세)을 검증하는지를 더 분명하게 드러낼 수 있기 때문이에요
| public void moveCars(){ | ||
| Random random = new Random(); | ||
| for (Car car : cars){ | ||
| int randomValue = random.nextInt(10); | ||
| car.move(randomValue); | ||
| } | ||
|
|
||
| } |
There was a problem hiding this comment.
moveCars() 를 public 으로 열어둔 이유가 있을까요?? (+ for 반복문 아래 불필요한 줄바꿈은 지워 주세요!)
There was a problem hiding this comment.
헉 별다른 이유 없이 습관적으로 public으로 열어두었던 것 같습니다!! 외부에서 직접 호출할 필요 없는 로직이므로, private로 수정했습니다. 짚어주셔서 감사합니다!
| private int updateMax(int currentMax, int carPosition){ | ||
| if (carPosition > currentMax){ | ||
| return carPosition; | ||
| } | ||
| return currentMax; | ||
| } |
There was a problem hiding this comment.
이 Max 값을 찾기 위한 메서드는 자바 표준 라이브러리(Math)를 이용해서 없애 볼 수 있을 것 같아요
| import java.util.Random; | ||
|
|
||
| public class RacingGame { | ||
| private final List<Car> cars; |
There was a problem hiding this comment.
여기도 final 키워드를 붙여 주셨네요!
그렇다면 퀴즈 입니다
final로 선언된 cars 컬렉션을 cars.remove(0), cars.add(new Car("그리디")) 와 같이 수정할 수 없을까요?
There was a problem hiding this comment.
정답을 모르겠어서 서치해봤습니다 ㅎㅎ
찾아본 결과, final은 cars 변수가 아예 새로운 리스트 객체를 재할당받는 것만 막아줄 뿐, add나 remove를 통해 수정할 수 있습니다!! 좋은 퀴즈 남겨주셔서 감사합니다!
|
|
||
| public class RacingTest { | ||
|
|
There was a problem hiding this comment.
우리가 이 클래스에서 테스트 하고자 하는 대상이 RacingGame이니까
이를 테스트 하는 클래스의 이름 또한 RacingGameTest 로 맞춰주면 어떤 클래스를 검증하는 테스트인지 더 바로 드러날 것 같아요!
| @Test | ||
| void 우승자가_여러명일_경우_공동_우승자를_모두_반환한다(){ |
There was a problem hiding this comment.
RacingGame 에서 테스트를 해 볼만한 것이 공동 우승자 뿐일까요??
현재 테스트가 공동 우승자 상황에만 집중되어 있는데,
게임의 전체 동작을 고려했을 때 다른 케이스들도 함께 검증해보면 좋을 것 같습니다.
예를 들어
- 단일 우승자가 나오는 경우
- 경계 상황(모두 이동하지 않는 경우)
특히 테스트에서는 경계 상황에 대해 내가 의도한 동작을 하는지를 검증해 보는게 특히나 중요한 것 같아요!
| public void race(int tryCount){ | ||
| for(int i=0; i< tryCount; i++){ | ||
| moveCars(); | ||
| } | ||
| } |
There was a problem hiding this comment.
빠졌던 요구사항을 잘 구현해 주셨군요!!
이전에는 자동차를 모두 이동시킨 뒤에 RacingGame이 우승자만 판단했는데,
지금처럼 RacingGame이 이동부터 우승자 결정까지 게임 흐름을 함께 책임지도록 바뀌면서
어떤 점이 더 좋아졌다고 느껴지시나요?
좀 더 명확하게 질문을 드리면
게임을 진행하는 책임을 RacingGame이 더 직접적으로 가지게 되면서 어떤 점이 더 나아진것 같나요?
There was a problem hiding this comment.
응집도가 좋아진 것 같습니다. RacingGame 객체 스스로가 자신의 상태를 변경하고 관리하는 객체지향적인 설계가 된 것 같습니다. 또한 유지보수도 쉬워진 것 같습니다! 외부 코드는 건들일 필요 없이 새로운 룰이 생기면 RacingGame내부만 고치면 되기 때문입니다!
| RacingGame game = new RacingGame(Arrays.asList(pobi, crong, rabbit)); | ||
| List<Car> winners = game.getWinners(); | ||
|
|
||
| assertThat(winners).containsExactly(pobi, crong); |
There was a problem hiding this comment.
containsExactly()는 내부 원소의 순서까지 검증하는 메서드인데요.
현재 요구사항에서 공동 우승자의 반환 순서까지 중요하게 봐야 하는지는 한 번 더 고민해보셔도 좋을 것 같아요
| private int getMaxPosition(){ | ||
| int max=0; |
There was a problem hiding this comment.
현재 코드가 = 앞 뒤에 띄어쓰기가 안 되어 있는거 같아요.
인텔리제이에서는 코드 자동 정렬 기능이 있다는 것을 알고 계시나요?
mac : cmd + shift + L
windows: ctrl + shift + L
코드를 제출하기 전에 한 번씩 정렬을 하고 커밋을 하면 더욱 깔끔해 집니다
| # 🚗 자동차 경주 게임 1,2단계 (Racing Car Game) | ||
|
|
||
| 주어진 횟수 동안 n대의 자동차가 무작위 값에 따라 전진 또는 멈추며, 최종적으로 가장 멀리 이동한 우승자를 가려내는 게임입니다. | ||
|
|
||
| ## 🎯 핵심 요구사항 | ||
| - 주어진 횟수 동안 n대의 자동차는 전진 또는 멈출 수 있다. | ||
| - 각 자동차는 `0에서 9 사이에서 무작위 값`을 구한 후, 그 값이 **4 이상일 경우 전진**하고 **3 이하의 값이면 멈춘다.** | ||
| - 자동차 경주 게임을 완료한 후 누가 우승했는지를 알려준다. (우승자는 한 명 이상일 수 있다.) | ||
|
|
||
| --- |
| @Test | ||
| void 무작위_값이_4_이상이면_전진한다(){ | ||
| Car car = new Car("pobi"); | ||
|
|
||
| car.move(4); | ||
|
|
||
| assertThat(car.getPosition()).isEqualTo(1); | ||
| } |
There was a problem hiding this comment.
테스트 네이밍이 훨씬 명확해 졌네요!
다만 네이밍에서 무작위 값이라는 표현은 조금 어색하게 느껴 지는것 같아요
현재 테스트는 실제로 무작위 값을 생성해서 검증하는 것이 아니라,
4, 3과 같은 값을 직접 전달해서 Car의 이동 규칙을 검증하고 있습니다
또한 Car 객체 자체는 무작위라는 개념을 알지 않고,
단순히 전달된 값에 따라 이동 여부를 판단하는 역할만 가지고 있기 때문에,
무작위라는 표현은 Car의 책임과도 조금 어긋나는 것 같다 라는 느낌을 받았는데요
그래서 테스트 이름도
무작위 값보다는 전달된 값 또는 주어진 값처럼
실제로 Car가 받는 입력을 기준으로 표현해 주는 것이 더 자연스러운 것 같은데 하은님은 어떻게 생각하시나요??
There was a problem hiding this comment.
그게 훨씬 Car 객체에 더 집중된 표현인 것 같아, 이해하기도 쉽고 직관적이어서 좋은 표현인 것 같습니다!!! 감사합니다!
chxghee
left a comment
There was a problem hiding this comment.
고생하셨습니다 하은님!
요청드렸던 부분들을 전반적으로 잘 반영해주신 것 같아요 👍
아직 1, 2단계라 요구사항 자체가 아주 복잡한 편은 아니었는데,
다음 단계부터는 조금씩 설계에 대한 고민이 더 필요해질 것 같아요
스터디에서 다룰 수도 있을거 같은데 다음미션에서는
랜덤 값이 포함된 로직을 어떻게 테스트 할지,
그리고 입력이 들어오고 요구사항이 늘어날 때 어떤 구조로 역할을 분리하고 관리해 나갈지(MVC 패턴)
같은 부분들을 한 번 고민해보시면 좋을 것 같습니다 🙋♂️
안녕하세요. 자동차 경주 미션 제출합니다.
구현 내용
-Car 클래스 생성
-RacingGame 클래스 생성
-기타 기능 구현
궁금한 점
Git 사용이 아직 익숙하지 않아 질문드립니다. commit 후에 바로 push를 하는 방식이 일반적인지, 아니면 commit을 여러 개 쌓아둔 뒤 마지막에 한 번에 push하는 방식이 더 권장되는지 궁금합니다.
==
자바 코딩에 아직 익숙하지 않아 기초 미션임에도 어려움을 느꼈습니다. 특히 for문 안에 if문을 사용하지 않는 조건과 메서드 길이 제한 조건 때문에 RacingGame 클래스의 코드를 여러 메서드로 분리하는 과정이 가장 어려웠습니다.
아직 많이 부족한 단계이지만 리뷰해 주신다면 많은 도움이 될 것 같습니다. 잘 부탁드립니다!
감사합니다.