Skip to content
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

[Step 3] 자동차 경주 #5718

Open
wants to merge 12 commits into
base: 6democratickim9
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 10 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions src/main/java/CarRacingGame/CarRacingGame.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
package CarRacingGame;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

java에서 pakcage컨벤션은 소문자를 원칙으로 합니다 😄


public class CarRacingGame {
static CarRacingGameUtils cars = new CarRacingGameUtils();

public static void main(String[] args) {

int numberOfCars = InputView.inputNumberOfCars();
int numberOfAttempts = InputView.inputNumberOfAttempts();

cars.initialCarSettings(numberOfCars);
cars.printMovingCars(numberOfAttempts);
}
}

44 changes: 44 additions & 0 deletions src/main/java/CarRacingGame/CarRacingGameUtils.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
package CarRacingGame;

import java.util.HashMap;
import java.util.Map;
import java.util.Random;

public class CarRacingGameUtils {
public Map<String, String> cars;

// TODO. 초기 차 인풋을 받고 차의 이동거리가 1인 상태에서 시작하는지,
// 차의 이동거리가 0 인상태에서 시작되는 것인지 확인 필요
public Map<String, String> initialCarSettings(Integer numberOfCars) {
cars = new HashMap<>();
for (int carNum = 1; carNum <= numberOfCars; carNum++) {
cars.put("car" + carNum, "-");
}
return cars;
}

public void printMovingCars(int numberOfAttempts) {
System.out.println("실행 결과");
for (int attempt = 0; attempt < numberOfAttempts; attempt++) {
PrintView.printCars(cars);
moveCars();
System.out.println();
}
}

public void moveCars() {
cars.forEach((carNumber, currentPosition) -> {
String newPosition = makeDistanceOfCars(currentPosition);
cars.put(carNumber, newPosition);
});
}

public String makeDistanceOfCars(String currentPosition) {
Random random = new Random();
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Random 객체가 매번 초기화 될 필요는 없을 것 같습니다.
정적 상수로 추출되어도 괜찮을 것 같아요 😄

// TO BE
public static Random RANDOM = new Random();
    
public String makeDistanceOfCars(String currentPosition) {
    int randomValue = RANDOM.nextInt(10);
    if (randomValue >= 4) {
        currentPosition += "-";
    }
    return currentPosition;
}

int randomValue = random.nextInt(10);
if (randomValue >= 4) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

10, 4와 같은 매직넘버는 정적 상수로 추출되면 훨씬 코드 가독성을 높일 수 있는 부분입니다 😄

요 내용 한번 정리하고 가시죠 🙏

매직넘버 치환.
https://hoonmaro.tistory.com/44

currentPosition += "-";
}
return currentPosition;
}
}
Copy link

@neojjc2 neojjc2 Oct 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

요구사항 구현 잘 해주셨습니다.
그리디 알고리즘(Greedy Algorithm) 쪽에 익숙하신 느낌이기도 합니다.
최소한의 코드로 요구사항을 만족시켜주셨네요 👍

크게 3가지로 의견을 드리고 싶습니다.

1. 먼저 테스트 관점입니다 (TDD)

현재 작성하신 코드 기반으로 테스트를 잘 작성해주셨습니다만, 정확한 테스트라고 보긴 다소 어렵습니다.

@Test
@DisplayName("Set the numbers of the cars and check cars move properly")
public void testMoveCars() {
    int numberOfCars = 2;
    carRacingGameUtils.initialCarSettings(numberOfCars);
    carRacingGameUtils.moveCars();
    carRacingGameUtils.cars.forEach((car, position) -> {
        assertTrue(position.equals("-") || position.equals("--"));
    });
}

요구사항은

  • 전진하는 조건은 0에서 9 사이에서 random 값을 구한 후 random 값이 4이상일 경우이다.

입니다만, 작성하신 테스트코드는 항상 참(True) 을 테스트하는 코드 입니다.
만약 중간에 버그가 발생해서 전진하지 못한 상황이라 할지라도 기본 값이 -이기 때문에 해당 코드는
정확한 테스트 코드는 아닙니다. 실제로 변화하는 요인을 통제하고 우리가 구현한 비즈니스 코드가 정확하게
동작하는가 에 대한 관점에서 봤을 때는 조금 더 보완이 되어야 하는 코드라고 생각합니다.

아 물론 Random객체에 대한 테스트 코드가 필요하다는 의견은 아닙니다.
이미 java 기본 라이브러리 이기 때문에 Random자체를 테스트할 필요는 없고,
Random객체를 제어해 가면서 테스트가 가능한 구조가 되어야 한다는 의견입니다 🙏

테스트에 대해서 한번 정리하고 가시면 좋을 것 같습니다.

테스트더블.
https://beomseok95.tistory.com/295

2. 유지보수 관점입니다.

현재 구조에서는 자동차가 이동한 거리를 - 값을 통해서 알 수 있습니다.
만약 매 라운드에서 1위를 한 차가 어떤 차인지, 혹은 round를 진행함에 있어서
사고가 발생해서 경기 중인 자동차를 모두 멈춰야 하는 요구사항이 생긴다면
현재 구조에서는 아주 많은 수정이 필요하게 됩니다.

자동차라는 객체와 자동차를 움직이게 하는 조건 (즉 수정이 앞으로 많이 발생할 것으로 예상되는 부분)은 분리를 해서
앞으로 수정사항이 발생했을 때 기존 코드의 변경을 최소화 해야 한다고 생각합니다.

물론 지금 구조에서도 가능합니다 예를들어,
-값을 기준으로 Map<String, String>에서 매회 1등을 한 자동차를 찾을 수 있긴 합니다만
만약 출력 형식이 -이 아닌 *로 변경해야 하는 요구사항이 발생했다고 가정했을 때
-*로 변경하면 위에서 예로 든 1등을 한 자동차를 찾는 로직도 -*로 변경해야 합니다.
(출력 형식과 전혀 관련 없는 로직에 변경이 생깁니다.)
그 관점에서 봤을 때 아래 요구사항에 대한 추가적인 개선이 필요해 보입니다.

  • 핵심 로직을 구현하는 코드와 UI를 담당하는 로직을 구분한다.

그리고 자동차를 움직이게 하는 조건에 대해서는 꾸준하게 수정사항이 발생할 수 있는 확률이 가장 높은 부분임에도 불구하고
지금 구조는 아주 strict 하다고 생각합니다.
이 부분도 함께 개선되면 좋을 것 같습니다 😄

public String makeDistanceOfCars(String currentPosition) {
    Random random = new Random();
    int randomValue = random.nextInt(10);
    if (randomValue >= 4) {
        currentPosition += "-";
    }
    return currentPosition;
}

요 내용 한번 참고해주시면 좋을 것 같습니다 🙇

전략패턴.
https://jackjeong.tistory.com/108

3. 마지막으로 객체지향 관점입니다.

제가 만약 여러가지 게임을 제공하는 일종에 게임랜드 앱을 만들고
그 안에 민주님이 제공해주신 자동차경주 게임을 넣는다고 가정해보겠습니다.

저는 앱을 통해 사용자의 입력 및 출력을 하려고 합니다.
하지만 자동차경주 게임을 포팅하려면 지금 구조에서는 초기화는 어떻게 한다고 할지라도
게임실행 결과는 console로 밖에 볼 수 없는 구조를 가지고 있습니다.

int numberOfCars = InputView.inputNumberOfCars();
int numberOfAttempts = InputView.inputNumberOfAttempts();

cars.initialCarSettings(numberOfCars);
// console 화면에 출력하는 기능과 자동차경주를 진행하는 moveCars(); 가 강 결합 되어있는 구조.
cars.printMovingCars(numberOfAttempts);

따라서 민주님께 수정을 요청할 수 밖에 없는 구조입니다.
객체지향의 SOLID중 개방-폐쇄 원칙 (Open-Closed Principle)에 따라 기존 코드를 변경하지 않고 새로운 기능을 추가할 수 있어야 좋은 구조라고 생각합니다 🙇

그렇게 하려면 일단 객체들간에 역할과 책임을 좀 나눌 필요가 있다고 생각됩니다.

요 내용한번 보시면 좋을 것 같습니다 😄

MVC 패턴.
https://m.blog.naver.com/jhc9639/220967034588

25 changes: 25 additions & 0 deletions src/main/java/CarRacingGame/InputView.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
package CarRacingGame;

import java.util.Scanner;

public class InputView {
private static final Scanner scanner = new Scanner(System.in);

public static int inputNumberOfCars() {
System.out.println("자동차 대수는 몇 대 인가요?");
int numberOfCars = scanner.nextInt();
if (numberOfCars <= 0) {
throw new IllegalArgumentException("Number of cars should be positive");
}
return numberOfCars;
}

public static int inputNumberOfAttempts() {
System.out.println("시도할 회수는 몇 회 인가요?");
int numberOfAttempts = scanner.nextInt();
if (numberOfAttempts <= 0) {
throw new IllegalArgumentException("Number of attempts should be positive");
}
return numberOfAttempts;
}
}
12 changes: 12 additions & 0 deletions src/main/java/CarRacingGame/PrintView.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
package CarRacingGame;

import java.util.Map;

public class PrintView {

public static void printCars(Map<String, String> cars) {
cars.forEach((car, position) -> {
System.out.println(position);
});
}
}
11 changes: 7 additions & 4 deletions src/main/java/StringAddCalculator/StringCalculatorUtils.java
Original file line number Diff line number Diff line change
Expand Up @@ -5,17 +5,20 @@

public class StringCalculatorUtils {

private static final String DELEMETER = ",|:";

public static String[] splitStringCalculator(String inputs) {
String DELEMETER = ",|:";
String splitedString = inputs;
String currentDelimeter = DELEMETER;

if (inputs.startsWith("//")) {
int separatorIndex = inputs.indexOf("\n");
DELEMETER = inputs.substring(2, separatorIndex);
currentDelimeter = inputs.substring(2, separatorIndex); // 사용자 정의 구분자를 currentDelimeter에 저장
splitedString = inputs.substring(separatorIndex + 1);
}

return splitedString.split(DELEMETER);
return splitedString.split(currentDelimeter);

}
public static List<Integer> convertInputs(String[] inputs) {
List<Integer> result = new ArrayList<>();
Expand All @@ -29,7 +32,7 @@ public static List<Integer> convertInputs(String[] inputs) {
private static int toPositiveInteger(String input) {
int number = Integer.parseInt(input);
if (number < 0) {
throw new RuntimeException("There is wrong value included: " + input);
throw new IllegalArgumentException("There is wrong value included: " + input);
}
return number;
}
Expand Down
39 changes: 39 additions & 0 deletions src/test/java/CarRacingUtilsTest.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
import CarRacingGame.CarRacingGameUtils;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.DisplayName;
import org.junit.jupiter.api.Test;

import java.util.Map;

import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertTrue;

public class CarRacingUtilsTest {

private CarRacingGameUtils carRacingGameUtils;

@BeforeEach
public void setUp() {
carRacingGameUtils = new CarRacingGameUtils();
}

@Test
@DisplayName("Set the numbers of the cars and check proper numbers of items")
public void testInitialCarSettings() {
carRacingGameUtils.initialCarSettings(3);
Map<String, String> cars = carRacingGameUtils.initialCarSettings(3);
assertEquals(3, cars.size());

}

@Test
@DisplayName("Set the numbers of the cars and check cars move properly")
public void testMoveCars() {
int numberOfCars = 2;
carRacingGameUtils.initialCarSettings(numberOfCars);
carRacingGameUtils.moveCars();
carRacingGameUtils.cars.forEach((car, position) -> {
assertTrue(position.equals("-") || position.equals("--"));
});
}
}