[그리디] 이고은 자동차 경주 미션 1,2 단계 제출합니다. #148
Conversation
There was a problem hiding this comment.
안녕하세요, 고은님!
빨리 제출해주셨네요 엄청 부지런하신 것 같아요 :)
질문에 대한 답변
혹시 Math.max를 사용하지 않고 짤 수 있는 방법이 있을까요? 며칠 고민해 보았는데 도무지 생각이 안납니다🥹
제가 처음에 다른 부분이랑 착각했었는데요, 다시 수정하겠습니다 ㅎ
저같은 경우는 stream을 사용해서 최댓값을 얻었어요.
//우승자 위치 찾는 메서드
public int findWinnerPosition(List<Car> cars){
return cars.stream()
.map(Car::getPosition)
.reduce(Integer::max)
.orElse(0);
}고은님이 질문하신 의도가 궁금한데 내장함수를 사용하고 싶지 않으셨던 거라면 WinnerPosition이라는 클래스를 만들어서 최댓값을 갱신시키는 메서드를 넣어볼 수도 있을 것 같아요. 물론 객체로서 충분한 책임이 부여될 것 같다면 만드셔도 좋지만, 그게 아니라면 저는 내장함수를 그대로 사용하는 것도 나쁘지 않다고 생각합니다!
전체 리뷰
-
EOF
각 파일 마다 개행을 추가하지 않으면 발생하는 개행 문자 표시입니다.
인텔리제이 설정을 통해 자동으로 해결 가능해요!
Setting > Editor > General > Ensure every saved file ends with a line break -
명명 규칙
보통 메서드명에 동사를 쓰는 경우가 많아서 클래스명은 보통 명사로 짓는 게 익숙하다고 느껴지는 것 같아요
CarMove나RaceManage같은 경우 명사나 명사구 형태로 수정하는 것은 어떨까요?
클래스명 명명 규칙
코드를 혼자만 본다면 상관없겠지만 협업의 측면에서 다른 사람들이 볼 때 헷갈릴 만한 점은 수정하는 것이 낫다고 생각합니다 :) -
테스트 작성
우승자를 구하는 기능이 의도대로 동작하는지 테스트한다.
2단계 요구사항인 테스트 코드가 작성되기 전인 것 같아요!
테스트 코드가 처음이시라면 아래 학습테스트를 통해 학습한 후 적용하면 도움이 될 것 같습니다 :)
JUnit5Test
AssertJTest -
단일책임원칙
여러 일을 하는 메서드가 꽤 있는 것 같아요! 대표적인 메서드를 리뷰 댓글로 남겼는데 이외에도 레이싱+출력을 모두 수행하는 메서드 등이 있는 것 같은데 고은님이 생각하시기에 메서드가 여러 일을 담당하고 있다면 분리해보는 건 어떨까요?
|
|
||
| public class Car { | ||
| private String name; | ||
| private int position=0; |
There was a problem hiding this comment.
코드스타일은 아래 단축키를 통해 한 번에 정렬 가능합니다!
- 윈도우 : Crlt + Alt + L
- 맥 : Command + Option + L
src/main/java/Car.java
Outdated
|
|
||
| public void move(){ | ||
| position++; | ||
| // System.out.println(position); |
src/main/java/CarMove.java
Outdated
| import java.util.Random; | ||
|
|
||
| public class CarMove { | ||
| public void randomMove(Car car){ |
There was a problem hiding this comment.
현재 이 메서드가 하는 역할이 다양한 것 같아요.
- 랜덤 숫자 생성하기
- 자동차가 움직이는 기준 충족하는지 확인하고 움직이기
단일책임원칙(SRP)에 따라 분리해보는 것은 어떨까요?
만약 분리하기로 결정하셨다면 각각의 메서드가 하는 역할이 어떤 클래스에 어울리는지 고민해봐도 좋을 것 같습니다!
src/main/java/CarMove.java
Outdated
|
|
||
| int randomNumber = random.nextInt(10); // 0부터 9까지의 랜덤 숫자 생성 | ||
| System.out.println("랜덤변수" + randomNumber); | ||
| if(randomNumber >= 4){ |
There was a problem hiding this comment.
- 4이상-> 전진, 4미만->멈춤
고은님이 README에도 작성해주신 것처럼 이러한 비즈니스 요구사항이 있었는데요,
만약 이 요구사항에 익숙하지 않은 사람이 코드를 처음 본다면 4가 어떤 의미를 가지는지 알기 어려울 수도 있을 것 같아요
'4'라는 기준점에 의미를 부여해보는 것은 어떨까요?
매직넘버 상수화는 또한 관리 포인트를 한 곳에 집중시킬 수 있다는 장점도 있습니다.
현재는 한 곳 밖에 안 쓰이지만 기준 체크하는 로직이 여러 군데 쓰인다고 가정하면 상수화시킨 부분만 수정하면 되기 때문에 관리가 훨씬 용이해지는 장점이 있습니다 :)
src/main/java/RacingCar.java
Outdated
| @@ -0,0 +1,24 @@ | |||
| import java.util.Scanner; | |||
|
|
|||
| public class RacingCar { | |||
There was a problem hiding this comment.
클래스 이름이 RacingCar여서 Car클래스와 헷갈릴 여지가 있는 것 같아요!
다른 이름으로 바꿔보는 건 어떨까요?
src/main/java/Winner.java
Outdated
| @@ -0,0 +1,31 @@ | |||
| public class Winner { | |||
|
|
|||
| public void findWinner(Car[] cars) { | |||
There was a problem hiding this comment.
여러 클래스가 메서드 파라미터로 Car[] 배열을 받고 있는 것을 보니 지금 코드의 중심 데이터는 자동차들의 목록인 것 같습니다. 이 목록 자체를 Cars라는 클래스로 만들어 관리하면 어떨까요?
Cars 클래스가 내부에 List<Car>를 가지고 우승자를 찾거나 모든 자동차를 움직이는 등 자동차 목록에 관련된 모든 책임을 직접 처리한다면 RaceManage나 Winner 같은 클래스가 자동차 목록을 직접 다루는 게 아니라 Cars라는 잘 만들어진 객체에게 요청만 보내면 되니까 좀 더 객체지향적인 코드가 될 수 있을 것 같아요!
public class Cars {
private List<Car> cars;
...
}There was a problem hiding this comment.
추천해주신 대로 코드 수정을 한번 해 보았는데
이름들이 Car cars car 비슷비슷해서 중간에 한 번 헤맸습니다.
다른 이름을 설정할까 싶다가도 Car, cars가 제일 깔끔한 것 같아서 일단 좀 헷갈리지만 car,cars 이렇게 썻습니다.
혹시 멘토님이시라면 이름 설정을 어떻게 하실지 궁금합니다!!
There was a problem hiding this comment.
저도 처음 미션 할 때 Car, Cars로 지정했습니다!
이후 미션에서도 만약 단일 객체면 단수형, 단일 객체를 모아둔 리스트는 복수형으로 이름 지었어요, 사실 전 제일 직관적이라고 생각했어서 만약 고은님이 너무 헷갈리신다면 다른 이름으로 바꾸셔도 좋습니다! ex) CarCollection 등등
저는 지피티한테 가장 흔히 사람들이 사용하는 이름 후보 좀 뽑아달라고 한 뒤에 제가 알아듣기 쉬운 이름으로 정하는 편이에요 ㅎㅎ
|
리뷰 감사합니다! 단일 책임 원칙에 따라 분류를 해 보다가 Winner클래스에서 분류한 Cars(여기도 이름이 비슷비슷..) 클래스에서도 여러 일을 하길래 한번 더 나눌까 하다가 오히려 너무 많아지고 복잡한 느낌이 들어서 여기서 멈췄습니다..!! 저번 코드리뷰에서 작성해 주신 단일 책임 원칙에 대한 파일 링크를 쭉 읽어 봤는데 오히려 너무 많이 나누는 것도 별로라고 하더라고요.. 아직 적정선에 대한 감이 잘 안오는데 하다보면 늘겠죠?!?! |
There was a problem hiding this comment.
질문에 대한 답변
단일 책임 원칙에 따라 분류를 해 보다가 Winner클래스에서 분류한 Cars(여기도 이름이 비슷비슷..) 클래스에서도 여러 일을 하길래 한번 더 나눌까 하다가 오히려 너무 많아지고 복잡한 느낌이 들어서 여기서 멈췄습니다..!! 저번 코드리뷰에서 작성해 주신 단일 책임 원칙에 대한 파일 링크를 쭉 읽어 봤는데 오히려 너무 많이 나누는 것도 별로라고 하더라고요.. 아직 적정선에 대한 감이 잘 안오는데 하다보면 늘겠죠?!?!
현재 미션은 규모가 작은 프로젝트라고 생각하는데요, 그래서 사실 대규모 협업 프로젝트의 경우 말씀해주신 “너무 많이 나누는 경우”가 발생할 수 있지만, 현 미션에서는 최대로 나누어도 워낙 소규모이기 때문에 걱정하시는 상황이 발생하지 않을 것 같아요. 초록 스터디 미션을 통해서 단일 책임 원칙을 엄격히 지킴으로써 객체지향을 연습해본다는 마인드로 최대한 나누어도 될 것 같습니다!
혹시 리뷰 반영하다가 많이 헷갈리거나 도움이 필요하시다면 편하게 디엠 주셔도 되고, 댓글 아무때나 달아주셔도 됩니다! 저도 리뷰어로 리뷰하는 것은 처음이라.. 제 말이 이해가 되지 않으신다거나 하면 바로 연락 주세요 ㅎ
전체 리뷰
-
EOF
지난번 말씀드린 EOF 설정이 아직 반영되지 않은 것 같습니다..! 다시 한 번 확인 부탁드려요!
-
커밋 메시지
저는 커밋의 단위를 하나의 기능을 완성하거나, 기능이 너무 크다면 제 스스로 단계를 나누어 커밋 메시지를 작성하는 편입니다! 아래는 제가 처음 자동차 경주 미션을 수행했을 때 생성한 커밋들인데요, 사실 이렇게 커밋 메시지를 자세히 작성하면 좋은 점은 책임 소재를 찾기 쉽다는 것 같아요. 작업하고 나서 문제가 생겼을 때, 어떤 커밋 작업과 관련된 문제인지 빠른 파악이 가능하다는 것이 가장 큰 장점이라고 생각합니다. 또한 제 코드를 보는 사람들에게 “저 이런 작업 흐름을 통해 코드를 작성했어요”라고 알릴 수 있는 장점도 있다고 생각해요. 지도 같은 느낌?
지금은 feat, refactor, fix 이런 커밋 종류들도 붙여주고 있어요! 인터넷 검색해보시면 커밋 메시지에 대한 글이 많아서 한 번 찾아보셔도 좋을 것 같습니다 ㅎㅎ 고은님만의 규칙을 만드셔도 좋구요!
- MVC
사실 MVC는 4단계 요구사항인데요, 현재 고은님 작업속도가 빠르시기도 하고, View가 여러 곳에 밀접하게 엮인 부분이 많아보여서 미리 나누는 연습을 해보셔도 좋을 것 같습니다!
MVC 패턴
| public void randomMove(Car car){ | ||
| Random random = new Random(); | ||
|
|
||
| int randomNumber = random.nextInt(10); // 0부터 9까지의 랜덤 숫자 생성 |
There was a problem hiding this comment.
이 10도 매직넘버 상수화를 적용시켜보면 어떨까요?
비즈니스 규칙과 연관되어 있으니까요!
|
|
||
| int randomNumber = random.nextInt(10); // 0부터 9까지의 랜덤 숫자 생성 | ||
| System.out.println("랜덤변수 : " + randomNumber); | ||
| car.move(randomNumber); |
There was a problem hiding this comment.
리뷰를 반영한 흔적이 보이네요 :)
그런데 아직 RandomMove는 1. 랜덤 생성과 동시에 2.자동차를 움직이는 명령을 내리고 있는 것 같아요!
혹시 이 부분에 대해서는 완전한 분리를 이루지 않은 이유가 있는지 궁금합니다!
저는 완전한 분리(car.move 호출을 다른 곳에서)를 제안하고 싶은데요, 이유는 다음과 같습니다.
현재 RandomMove는 랜덤값에 의존하므로 단위테스트가 어려울 것 같아요.
완전히 분리하게 되면 각각의 책임이 명확해집니다. Random을 생성하는 클래스와 움직임을 담당하는 곳이 분리되므로 유지보수 및 확장이 용이하게 변할 것 같아요! 예를 들어, 새로운 자동차를 움직이는 비즈니스 규칙이 추가된다고 가정하면, 결합되어 있는 메서드의 경우 로직이 복잡해질 수 있습니다.
추가로 이 randomMove 메서드는 각각의 자동차가 움직일 때마다 호출되는 메서드 입니다!
그러므로 자동차 수 x 라운드 수 = 호출횟수가 되는데요,
아마 제일 많이 호출되는 메서드일 거라 생각하는데 자동차를 움직일 때마다 항상 Random() 객체를 생성하고 있어
(물론 이 미션에서의 성능차이는 미세하지만) 성능 면에서 불필요한 객체 생성이 반복되고 있는 셈입니다.
이 부분도 한 번 고려해보면 어떨까요?
고은님의 생각을 들려주시면 감사하겠습니다!
| assertThat(f1.getPosition()).isEqualTo(1); | ||
| } | ||
|
|
||
| void CarStop(){ |
There was a problem hiding this comment.
CarStop 메서드에 @test 어노테이션이 빠져있어서 실제로는 테스트가 실행되지 않을 것 같아요.
이 부분 한번 확인해보시면 좋을 것 같습니다!
현재는 자동차의 이동에 대한 부분만 테스트가 작성되어 있는 것 같습니다! 시간이 되신다면 테스트를 더 다양하게 만들어 보시는 것은 어떨까요? 테스트 커버리지를 확인하는 방법
| @@ -0,0 +1 @@ | |||
|
|
|||
다른 부분은 만족스러운데 Winner쪽 코드들이 depth 2 이상은 쓰지 않는다는 규칙이 있어 많이 헤맸습니다. car를 2차원 배열로 받아볼까도 고민했는데, 코드가 너무 길고 복잡해져서 Math.max를 사용했습니다.
혹시 Math.max를 사용하지 않고 짤 수 있는 방법이 있을까요? 며칠 고민해 보았는데 도무지 생각이 안납니다🥹