[김민지] 자동차경주 1~2단계 제출합니다 #122
Conversation
There was a problem hiding this comment.
안녕하세요. 민지님! 2단계 구현하느라 고생 많으셨습니다!
코드에서 노력한 흔적이 보이네요! 👍👍
다만 전체적으로 스타일이 통일되지 않았고, 일부 클래스(Winners)에서는 요구사항을 만족하지 않은 것 같아요. 😂
계산기 미션은 첫 미션이라 그렇게 리뷰를 많이 남기지 않았는데, 자동차 미션부터는 앞으로 리뷰를 많이 남길 생각이에요.
너무 과한 리뷰 같거나, 어렵다면 솔직하게 말씀해 주셔도 좋습니다!
리뷰 반영하시고 재요청 부탁드립니다!
PR 메세지에는 구현하면서 의도했던 점, 궁금했거나 어려웠던 것들 등 생각나시는 것들에 대해 편하게 작성해주시면 될 것 같아요!
또는 코드가 어떤 클래스의 메서드가 어떤 동작을 하는지 얘기해주셔도 됩니다!
| //nextInt(10)은 0~9까지의 랜덤한 값을 반환. 그 값이 4보다 크거나 같으면 true 반환 | ||
| return (int)(Math.random() * RANDOM_BOUND) >= MOVE_THRESHOLD; | ||
| } | ||
| } |
There was a problem hiding this comment.
Files changed를 보시면 다음과 같이 경고 같은 아이콘이 표시되고 있어요!
해당 아이콘은 파일의 가장 끝에 개행이 없어서 발생하는데, 개행이 없어도 코드 실행에는 문제가 없으나, 다른곳에서는 문제가 발생할 수 있어요.
wc 명령으로 파일 줄의 개수를 구하는데, 실제 파일의 내용은 두 줄이지만, 한 줄만 출력되는 것을 볼 수 있어요.
이를 해결하기 위해 매 파일마다 확인하기는 너무 불편하니, IntelliJ를 사용하면 간단한 방법으로 막을 수 있어요.
(Settings -> Editor -> General -> Ensure every saved file ends with a line break)
그리고 next-step/java-calculator-unit-playground#79 (comment) 해당 리뷰 참고하셔서 설정하면 앞으로 해당 이슈를 방지할 수 있을 것 같네요!
또한 해당 경고는 Github가 아닌, git에서도 발생하는데, 다음과 같이 git diff 명령을 사용했을 때 다음과 같이 메시지가 출력되는 것을 볼 수 있어요.
| public class Winners { | ||
| public static List<String> findWinnersNames(List<Car> cars){ | ||
| List<String> winners = new ArrayList<>(); | ||
| for(int i = 0; i < cars.size(); i++){ | ||
| if(cars.get(i).getPosition() >= 5){ | ||
| winners.add(cars.get(i).getCarName()); | ||
| } | ||
| } | ||
| return winners; | ||
| } | ||
| } |
There was a problem hiding this comment.
Car 리스트를 받은 뒤, Car 중 position이 5 이상인 Car의 이름을 우승 자동차로 판단하고, 이를 반환하는 클래스네요!
해당 클래스는 정적 메서드로 구현해주셨는데, 정적 메서드를 사용한 이유가 있을까요?
정적 메서드를 사용한다면 어떠한 장단점이 있을 것 같으신가요?
| public static List<String> findWinnersNames(List<Car> cars){ | ||
| List<String> winners = new ArrayList<>(); | ||
| for(int i = 0; i < cars.size(); i++){ | ||
| if(cars.get(i).getPosition() >= 5){ |
There was a problem hiding this comment.
승자는 5 이상의 값이면 판단하는군요!
그런데 왜 특별히 5인가요?
| public class Winners { | ||
| public static List<String> findWinnersNames(List<Car> cars){ | ||
| List<String> winners = new ArrayList<>(); | ||
| for(int i = 0; i < cars.size(); i++){ |
There was a problem hiding this comment.
for문을 사용해주셨네요!
그리고 리스트에 인덱스를 통해 접근해주고 있군요!
하지만 인덱스를 통해 리스트에 접근하는 것은 경우에 따라 피해야 할 방법이 될 수 있어요!
왜 그럴까요?
그리고 반복문은 for문 말고 적어도 3가지 방법을 더 사용할 수 있어요!
해당 방법들은 어떤게 있을까요?
| @Override | ||
| public boolean canMove(){ | ||
| //nextInt(10)은 0~9까지의 랜덤한 값을 반환. 그 값이 4보다 크거나 같으면 true 반환 | ||
| return (int)(Math.random() * RANDOM_BOUND) >= MOVE_THRESHOLD; |
There was a problem hiding this comment.
랜덤한 값을 구하기 위해 Math.random()을 사용해주셨네요!
Math.random()의 반환 값은 double 타입이에요.
하지만 반환 타입은 int형이라 명시적으로 int로 형변환을 해주고 있고, 곱셈을 사용하고 있네요!
Math.random() 메서드는 내부적으로 어떻게 구현되어 있을까요?
그리고 해당 메서드를 사용할 때 주의할 점은 어떤게 있을까요? (Javadoc을 살펴보아요)
그러면 다른 방법으로 형변환 없이 랜덤한 int 값을 얻을 수 있을까요?
주석에 힌트가 있네요 😂
| @@ -0,0 +1,23 @@ | |||
| public class Car { | |||
| private final String carName; | |||
There was a problem hiding this comment.
final 키워드를 사용해주셨네요!
왜 final 키워드를 사용하셨나요? final 키워드를 사용하면 어떤 장점이 있나요?
| public void move(Movable movable){ | ||
| if(movable.canMove()){ | ||
| position++; | ||
| } | ||
| } |
There was a problem hiding this comment.
인터페이스를 사용해서 통제할 수 없는 값에 대해 통제할 수 있도록 구성해주셨군요! 👍👍
그런데 Car 클래스를 인터페이스로 만드는 방법도 있을 것 같은데, 왜 이러한 방법을 선택해주셨나요?
| } | ||
|
|
||
| List<Car> cars = List.of(carA, carB, carC); | ||
| assertThat(Winners.findWinnersNames(cars)).containsExactlyInAnyOrder("Car A","Car C"); |
| Car carA = new Car("Car A"); | ||
| Car carB = new Car("Car B"); | ||
| Car carC = new Car("Car C"); | ||
|
|
||
| for(int i = 0; i < 5; i++){ | ||
| carA.move(()->true); | ||
| } | ||
|
|
||
| for(int i = 0; i < 4; i++){ | ||
| carB.move(()->true); | ||
| } | ||
| for(int i = 0; i < 5; i++){ | ||
| carC.move(()->true); | ||
| } |
There was a problem hiding this comment.
우승자를 구하기 위해 매번 반복문과 move 메서드를 호출해야 하네요!
하지만 다른 테스트가 생긴다면 매번 해당 코드를 작성해줘야 할 것 같아요.
이름과 위치를 받는 생성자를 추가로 만드는 방법도 있을 것 같은데, 생성자를 2개 이상 만드시지 않은 이유가 있을까요?
| public class RandomMove implements Movable { | ||
| //값의 의미나 목적을 코드에서 명확하게 알리고 가독성을 위한 상수 사용 | ||
| //4는 전진 조건, 10은 랜덤 범위를 의미. 추후 변경 시 상수만 수정하면 됨 | ||
| private static final int MOVE_THRESHOLD = 4; | ||
| private static final int RANDOM_BOUND = 10; |
There was a problem hiding this comment.
0에서 9 사이에서 random 값을 구한 후 random 값이 4 이상일 경우 전진하고, 3 이하의 값이면 멈춘다.
라는 미션의 요구사항을 구현한 클래스군요!
만약 해당 클래스의 메서드에서 반환하는 값이 true이면 전진, false면 정지로 판단할 수 있을 것 같아요.
여기서 미션의 요구사항을 조금 더 분석해보면 중요해 보이는 것은 4 이상일 경우 전진하고, 3 이하의 값이면 멈춘다 이라고 판단해볼 수 있을 것 같아요.
해당 요구사항은 반드시 모든 자동차(전기자동차라는 새로운 클래스가 생기더라도)가 이를 지켜야해요.
그렇다면 이 요구사항 지킨 로직은 어디에 존재해야 하는 것이 맞을까요?
새로운 Movable 구현체가 생기면 어떻게 될까요?
이번 단계에서는 메서드를 최대한 작게 나누는 연습을 했습니다.
기능을 나누다 보니 자연스럽게 클래스 분리까지 고민하게 되었고,
else 예약어를 쓰지 않으려고 노력해보니 코드가 더 깔끔해져서 좋았습니다!
또한, 처음엔 어색했던 테스트 코드 작성도 반복하다 보니 점점 익숙해졌고,
테스트 가능한 구조를 고민하면서 객체 설계에 대해 한 번 더 생각해보는 계기가 되었습니다.
혹시 코드나 커밋에 누락되거나 잘못된 부분이 있다면 리뷰 부탁드리겠습니다 🙇♀️
❓궁금한 점
보통 PR 메시지에는 어떤 내용을 포함하면 좋은지 궁금합니다!