개요
우아한테크코스 1단계 자동차 경주 미션을 진행하며 고민한 지점과, 새롭게 알게 된 것들을 정리하는 차원에서 본 회고를 작성하게 되었다.
단계
1. 단위 테스트에 대한 개념 정리
먼저 단위 테스트에 대한 개념을 코치와 함께 정리하는 시간을 가졌다. 항상 테스트를 하면서 자문자답을 하는 느낌이었어서, 굳이 테스트 코드가 왜 내 프로그램에 존재해야 하는지를 그동안 이해할 수 없었지만, 코치와의 대화를 통해 위 의문에 대한 답을 어느 정도 해소할 수 있었다.
내 코드가 안전한 코드임을, 남에게도 또 "나에게도" 증명할 수 있다
단위 테스트를 진행하는 핵심적인 이유가 이 문장에 담겨 있는 것 같다. 또 내 프로그램을 테스트해야하는 순간이 올 때, 이것이 잘 짜여진 프로그램인지를 어느정도 판단할 수가 있다. 테스트하기 좋은 코드 일수록 좋은 프로그램이다. 이유는 큰 문제를 작은 단위로 해결할 수 있고, 시스템이 커질 때마다, 작은 단위로 문제를 해결하지 못하면 유지보수가 복잡해지기 때문이다.
테스트의 파라미터를 넣을 때는 검증하고자 하는 로직이 무엇인지, 값의 끝단이 무엇인지(경계값)을 잘 판단하는 것이 중요하다.
- 자세한 내용은 이곳에 정리해 놓았다
2. 좋은 코드에 대한 본인만의 기준 정립하기
다음은 좋은 코드 품질에 대해 이야기하는 시간이었다. 일반적으로 좋은 코드 품질을 가르는 몇 가지 통상적 기준에 대해 알 수 있었다. 가장 인상 깊었던 몇 가지 포인트를 정리하자면
- 가장 기본적인 것, 내가 원하는 대로 동작해야 한다. 소프트웨어의 근본적 목적은 문제 해결이다
- 읽기 쉽고, 보기 좋아야 한다. 그러나 읽기 쉬운것과 보기 좋은 것은 다르다.
- 읽기 쉬운것은 네이밍 등 코드 로직을 머릿속으로 복잡하게 생각하지 않고도 코드의 의도록 파악할 수 있는 것이다.
- 보기 좋은 것은 컨벤션을 지키거나, depth, 메소드 줄 제한을 지키는 등 정돈이 잘 된 코드이다.
- 자세한 내용은 이곳에 정리해 놓았다
-> 이를 통해 내가 가장 중요하게 두어야 할 기준을 정리해봤다
- 코드 품질 중 네이밍이 가장 중요하다고 생각했다. 네이밍만 잘 작성해도 의도 전달이 편했다. 따라서 네이밍이 조금 길어도 코드의 의미를 확실히 설명하는 것이 좋겠다.
- 컨벤션 체크리스트를 만들어서 작업 후 확인하자
- 이 코드가 왜 최적의 판단이었는지를 리뷰어에게 설득 시키자
3. 1단계 - 자동차 경주 구현
자세한 1단계 코드와 요구사항은 이곳의 rawfishthelgh 브랜치를 참고하자
중요 고민 지점 : 자동차의 위치를 어디서 관리할까?
PR 전
미션을 시작하면서 요구사항을 정리하고, 먼저 했던 생각은 "가장 핵심적인 객체가 무엇일까?" 였다. 나는 이 질문에 "자동차"라는 답을 했고, 처음에는 다음과 같이 자동차를 정의했다.
- 자동차는 본인의 이름을 알고 응답할 수 있다.
- 자동차는 본인의 위치를 알고 응답할 수 있다.
그러나, 순간 "자동차가 본인의 위치를 왜 알아야 하지?" 라는 의문이 들기 시작했다. 왜냐하면 자동차의 위치를 자동차의 상태라고 말할 수 있는지가 모호했다. 단순히 외부에서 자동차를 바라봤을 때 그 위치인 것일수도 있다고 판단했기 때문이다.
따라서 자동차의 위치를 CarRepository를 만들어 그 안에서 관리하고, 자동차 객체는 단순히 본인의 이름만 알고 응답할 수 있는 것으로 그 기능을 수정해서 1차 PR을 올렸다.
//첫 PR Car 도메인 코드
public class Car {
private final Name name;
public Car(Name name) {
this.name = name;
}
public String getName() {
return name.getName();
}
}//첫 PR CarRepositoryimpl 코드
public class CarRepositoryImpl implements CarRepository{
private Map<Car, Integer> carBoard;
@Override
public void cycleCars(MovingStrategy movingStrategy) {
for (Car car : carBoard.keySet()) {
move(car, movingStrategy.getNum());
}
}
@Override
public void move(Car car, int number) {
if (isAllowedToMove(number)) {
carBoard.put(car, carBoard.get(car) + ADD_POINT);
}
}
@Override
public boolean isAllowedToMove(int number) {
if (number >= MOVING_STANDARD_NUM) {
return true;
}
return false;
}
@Override
public List<Car> findWinners() {
int maxValue = Collections.max(carBoard.values());
return carBoard.keySet().stream()
.filter(x -> carBoard.get(x) == maxValue)
.collect(Collectors.toList());
}
@Override
public Map<Car, Integer> getCarBoard() {
return carBoard;
}
@Override
public void insertCarBoard(Map<Car, Integer> cars) {
this.carBoard = cars;
}
}리뷰 후
위 고민(자동차의 위치값 관리 주체)에 대한 질문에 다음과 같은 답을 받을 수 있었다
Car가 위치값을 가지는 경우,
Car가 위치값을 가지지 않는 경우
-> 두 경우 모두 자동차의 "움직인다"라는 행동에는 변함이 없다.
자동차의 "움직인다"라는 행동은 코드로 어떻게 표현될 수 있으며, 이 행동은 누구에게 맡겨야 할까?
Car가 위치값을 가질 경우
-> 자신이 움직인 거리를 알 수 있다. 자신의 위치값에 대해 응답할 수 있다
Car가 위치값을 가지지 않는 경우
-> 위 질문에 대해 답할 수 없다
어떤 객체가 "행동"할 수 있을까? 누군가에 의해 수동적으로 움직이는 객체가 필요한가, 능동적인 객체가 필요한가?
CarRepository가 너무 많은 역할을 갖고 있지 않은가?
심지어 CarRepository를 CarService로 이름을 바꿔도 전혀 어색하지 않을 것 같다
-> 과연 Repository의 역할을 하고 있는 것인지 생각해 보라
추가 피드백 내용
- Controller에서 초기화면 출력, Car 생성, 게임 시작까지의 책임을 두었고, 결과 화면 출력을 GamePlay에 넘겼다. 이를 왜 따로 관리 했는가? 컨트롤러의 책임은 무엇으로 설정했는가?
- 구현 객체가 1개인데 왜 인터페이스화를 진행했는가?
- 컨벤션은 꼼꼼하게 확인 하자(쓸데없는 공백, 미사용 import문)
- 단순히 원시값을 감싸 클래스로 분리할 이유가 있나? 한 클래스에 묶인 상수들은 서로 연관되지 않은 것 같다. 열거형을 사용할 때는 각 상수의 의미가 명확할 때 사용하자(플래그성(GO,STOP,WAIT), 색깔(YELLOW, GREEN, BLUE) emd)
- 네이밍에 더 집중하자, 이동 숫자를 반환하는 클래스 이름이 MovingStrategy다. "정책에서 숫자를 반환한다" 어색하지 않은가?
- 내부 테스트에서만 쓰이는 메소드는 항상 private으로 선언하고, 상위 추상화 단계에서의 public에서 테스트 하자. 다만 private이 연계적으로 많아질 수록 테스트하고 싶은 욕구가 생길 수 있다. 이때는 한 클래스에 너무 많은 책임이 몰려있지 않은지를 생각하고, 클래스 분리를 고려해 보자
- 컬렉션 인스턴스를 그대로 반환하기 보다는 값을 복사해 반환하자
- Util 성 클래스 (정적 팩토리 메소드) 등에서는 생성자를 private으로 막아두자
- DisPlayName과 Test 메소드 네이밍을 한 번에 고민하는게 귀찮으면, 그냥 메소드를 한글 설명으로 써도 된다
- 단순히 값을 반환하기만 하는 객체, 포장의 필요성이 있을까?
- 생성자로 값을 초기화하는 방식이 아닌, 일종의 set 방식을 사용해서 값을 초기화했다. 로직 중에 해당 메소드를 호출하면 값이 초기화되지 않을까? 해당 객체를 사용하기 위해서는 무조건 해당 메소드를 호출해야 하나? 이러면 NPE 문제가 발생할 수 있다. 생성자를 통해 값을 초기화하는 방식을 고민하자
4. 2단계 - 자동차 경주 리팩터링
2단계 코드는 이곳의 step2 브랜치에서 확인 가능하다
리팩토링 내용
1. Car 객체의 역할 재설정
리뷰어의 피드백을 듣고 Car 객체의 역할을 다시 고민해 보았다. 이에 대해 생각해 보니 위치의 관리 역할을 누구에게 위임해야 할지 생각이 명확해질 수 있었다.
Car의 역할
- Car는 스스로 움직일 수 있다
- Car는 숫자를 보고 움직일 수 있는지 판단할 수 있다
- Car는 독립적인 본인의 위치를 갖는다
위와 같은 정의를 내린 이유는 이렇다
- position은 Car의 인스턴스 하나가 생길 때 마다 부여 및 종속되어야 하는 값이므로, 이를 동등하거나 독립적으로 관리할 수 없다
- 그러므로 Car는 본인의 상태인 position을 스스로 관리할 수 있어야 한다
- 따라서 이를 늘리거나, 늘릴 수 있는 지 판단하는 것도 Car의 역할이다
2. CarRepository 삭제 및 List를 관리하는 일급컬렉션 Cars 사용, Set 메소드로 컬렉션을 집어넣지 않고, final 선언후 생성자 주입으로 초기화
position을 Car에서 관리하는 순간 Map으로 이름과 위치를 관리했던 CarRepository의 필요성이 적어졌고, 기존의 많은 역할을 분리할 수 있겠다는 생각이 들었다.
먼저 개별 차량의 이동, 이동 가능여부 판단 등의 역할은 Car 객체로 이동시켰고, 이제는 Car라는 객체를 리스트로 관리하여 관련 역할(차량 전체 이동, 리스트 중 우승자 판단)만 끄집어내면 된다고 생각했다. 따라서 List를 private으로 관리하는 Cars 라는 일급컬렉션을 생성했다.
또한 기존 처럼 set 메소드로 컬렉션을 넣는 것이 아닌 final 필드로 선언하고 생성자로 주입해 초기화했다.
여기서 Cars의 역할은 본인의 컬렉션 상태에 대답 및 변경을 수행한다.
1) 차량들에 이동 명령을 내린다
2) 차량들 중 우승자들을 찾는다
//Car를 리스트로 관리하는 Cars 클래스
public class Cars {
private final List<Car> cars;
public Cars(List<Car> cars) {
this.cars = cars;
}
public void moveCars(NumberGenerator numberGenerator) {
for (Car car : cars) {
car.move(numberGenerator.getNum());
}
}
public List<Car> findWinners() {
int winnerValue = cars.stream().max(Comparator.comparingInt(Car::getPosition)).get().getPosition();
return cars.stream().filter(
car -> car.getPosition() == winnerValue)
.collect(Collectors.toList());
}
public List<Car> getCars() {
return cars;
}
}3. 입력, 출력의 흐름을 GamePlay 하나에서 관리. InputController 삭제 후 GamePlay가 컨트롤러로 이동
피드백을 받았던 “컨트롤러의 역할을 무엇으로 설정했는가”에 대해서 고민했고 다음과 같은 정의를 내려보았다.
컨트롤러는 어떠한 입력을 받았을 때 어떻게 처리할지 알려주어야 한다
따라서 초반 InputController를 사용해 차량 입력을 받을 때 게임을 시작함을 드러낸다는 전제를 깔았지만, 입출력의 흐름이 개별 클래스에서 관리되다 보니 타인이 읽기 힘들 것 같다는 생각이 들었다.
따라서 모든 입출력 흐름을 GamePlay 클래스에 위임하고 컨트롤러 패키지로 이동시킨 뒤, InputController 클래스의 역할은 그 명을 다했다고 생각하여 삭제했다.
4. 추가 변경 포인트
-
피드백을 받은 것 처럼 “정책에서 숫자를 반환한다”라는 개념이 어색해 보였다. 하지만 네이밍을 조금 바꿔보면 납득이 가지 않을까라고 생각해서 클래스의 이름을 NumberGenerator로 변경해 봤다
-
ENUM을 언제 사용해야 할 지를 명확히 정해야겠다고 생각했다. 같은 ENUM 클래스에 사용하는 상수들이 명확한 연관관계와 공통점을 바탕으로 분기해야 할 때만 사용하고, 지금처럼 단순히 매직넘버, 매직스트링을 감쌀 때는 각자 클래스 안에서 사용하기로 결정을 내렸다.
-
내부 클래스에서만 쓰이는 private 메소드에 대해서는 테스트를 하지 않기로 결정했다
궁금증
- Getter의 사용이 “필연적이다”라고 생각이 들 때는 getter를 사용해도 되는 것인가? 혹은 getter의 사용은 오직 출력 계층과 테스트를 위해서만 존재해야 하는가?
: Cars 라는 클래스 안에서 게임에 참여하는 Car 객체들을 관리하고 있다. 게임이 끝나면 각 Car의 position 값을 비교하여 우승자들을 가려야 한다. 그러므로 각 Car들의 position 값을 getter을 통해 Cars까지 끌고 와서 비교해야 하는 동작은 필연적이라고 생각했다. 첫 pr때 position을 따로 관리했었던 이유도 이와 같다.
따라서 위와 같이 컬렉션에 함께 담긴 객체들의 세부 필드 값을 기준으로 비교를 수행해야 할 때, getter의 사용을 허용할 수 있는가에 대해 궁금했다.
- 게임 진행 순서를 Controller에서 드러내는 것이 맞는가?
: 위에 언급했듯 모든 입출력과 게임진행의 흐름을 GamePlay에서 드러내고 있는데, 나는 Service의 개념을 “받아온 정보를 비즈니스 로직에 맞게 가공한다”라고 생각했다. 따라서 “게임의 순서를 관리한다” 라는 개념이 서비스에 있지 않다고 봤다.
또한 Controller에 이 클래스를 넘긴 이유는 요청의 진입 시점을 드러내고 진입 이후 어떤 순서로 요청이 처리되는지를 담당하는 것이 Controller의 역할이라고 생각했기 때문이다
그러나 “게임이 진행되는 순서”를 비즈니스 로직으로 봐야 하는지, 아니면 단순히 순서로만 봐야 하는지가 궁금했다.
두 번째 피드백
질문에 대한 답변
- 게임 진행 순서를 Controller에서 드러내는 것이 맞는가?
: "서비스 로직 = 요구사항" 이라고 생각하면 명확해진다. 게임 진행 요구사항을 보자. 요구사항은 입력 -> 순서에 맞게 실행 -> 출력 으로 정리될 수 있을 것이다. 그렇다면 "게임의 순서" 역시 넓은 의미에서 서비스일 것이다.
게임이 진행되는 순서는 사용자가 아닌 시스템이 관리한다. 그렇다면 게임의 순서는 서비스에 있는것이 자연스러울 것이다.
그렇다면 입출력도 서비스 로직이 아니라고 생각할 수 있지 않나?
-> 그렇게 볼 수도 있지만 입출력은 console 뿐만이 아닌 api call이나 file i/o로도 관리될 수 있으므로, 2단계에서 도메인과 입출력을 분리하라는 요구사항이 생겼을 것이다.
- Getter의 사용이 필연적으로 판단 된다면?
: 일단 Setter의 사용은 무조건적으로 피하는게 맞다.
그러나 getter를 쓸지 말지에 대한 결정은 본인이 클래스에 대한 책임을 무엇으로 두었는지가 일단 핵심이다.
1) 위치를 비교하는 책임을 Cars 에다 둘 것이다
-> 지금처럼 Cars에서 Getter를 사용해 비교한다
2) 위치를 비교하는 책임을 Car 에다 둘 것이다.
-> 다음과 같이 Car 안에서 비교할 수도 있다
#Car.java
public class Car implements Comparable<Car> {
...
public boolean isSamePosition(Car car) {
return this.position == car.position;
}
@Override
public int compareTo(Car o) {
int o1 = this.position;
int o2 = o.position;
if (o1 > o2) {
return 1;
}
return -1;
}
#Cars.java
public class Cars {
...
public List<Car> findWinners2() {
Car winner = cars.stream()
.max(Car::compareTo)
.orElse(new Car("dummy"));
return cars.stream()
.filter(it -> it.isSamePosition(winner))
.collect(Collectors.toList());
}어떤 방식이 더 편리해 보일까? 고민해 보자
추가 피드백 내용
메소드 안에서 객체 초기화를 하는 것, 최대한 피하자
이번 pr에서 GamePlay의 gameStart 메소드 코드는 다음과 같았다
public void gameStart() {
OutputView.printInputCarNamesNotice();
Cars cars = new Cars(CarFactory.buildCars(InputView.inputCarNames()));
OutputView.printInputTryTimesNotice();
int tryTimes = InputView.inputTryTimes();
printResultNotice();
play(cars, tryTimes, new RandomNumberGenerator());
printWinner(cars.findWinners());
}Cars 객체가 gameStart 안에서 초기화 된다. 즉, 객체 내부의 구현이 GamePlay에 의존되는 것을 볼 수 있다.
-> 이러면 캡슐화가 위반된다
따라서 Cars를 1. GamePlay 생성자의 인스턴스로 사용하거나, 2. gameStart 메소드의 파라미터로 넣어주는 것이 더 자연스러울 것이다. 1번을 진행하면 파라미터 개수를 줄일 수 있다는 장점이 있다.
따라서
public class GamePlay {
private final Cars cars;
private final NumberGenerator numberGenerator;
public GamePlay(Cars cars, NumberGenerator numberGenerator) {
this.cars = cars;
this.numberGenerator = numberGenerator;
}
public void gameStart() {
OutputView.printInputCarNamesNotice();
OutputView.printInputTryTimesNotice();
int tryTimes = InputView.inputTryTimes();
printResultNotice();
play(cars, tryTimes);
printWinner(cars.findWinners());
}
...
}다음과 같이 생성자로 Cars를 주입받을 수 있도록 변경해 보았다.
단순 값에 대해서, 상수화를 진행해야 할 경우에 대한 기준을 세워라
매직넘버에 대해 상수화를 진행하는 것은 좋은 행동이긴 하나, 너무 단순한 경우에는 상수화를 안 해도 로직이 쉽게 읽힌다. 다른 사람이 코드를 볼 때, 상수화를 안 하면 읽기 힘들다고 생각하는 부분에 한해서만 상수화를 진행하자
-> 앞으로는 단순 숫자나 문장의 경우 상수화를 진행하지 않고, 구분자나 출력에 필요한 도구(ex, “-“)이거나, 상수가 특별한 의미를 갖는 경우에 한하여 상수화를 진행하기로 했다
If문 안의 조건식에 대해서도 메소드로 추출하자
private void validateNameLength(int nameSize) {
if (nameSize < MIN_NAME_LENGTH || nameSize > MAX_NAME_LENGTH) {
throw new IllegalArgumentException("[ERROR] 1에서 5사이의 이름 길이만 입력 가능합니다.");
}
}위의 코드를
private void validateNameLength(int nameLength) {
if (!isValidNameLenght(nameLength)) {
throw new IllegalArgumentException("[ERROR] 1에서 5사이의 이름 길이만 입력 가능합니다.");
}
}
private static boolean isValidNameLenght(int nameLength) {
return nameLength >= MIN_NAME_LENGTH && nameLength <= MAX_NAME_LENGTH;
}다음과 같이 조건식마저도 메소드로 분리하고 네이밍을 한다면 더 쉽게 읽힐 것이다
기존> NameLength를 검사하네 > 1미만 5초과이면 예외를 뱉는구나.
변경 > NameLength를 검사하네 > 유효한 크기의 이름이 아니면, 예외를 뱉는구나, > 유효한 크기의 이름은 어떤 기준이지? 1이상 5이하가 유효한 크기의 이름이구나.
검증 기준은 요구사항의 하나라서 적절한 이름을 사용할 때 해당 기준을 필요시 살펴보기 용이하다.
후기
사실 이전에 자동차 미션을 진행해본적이 있어서 처음에는 자신감이 있었으나, 하면 할 수록 놓치는 부분이 많다고 생각했다. 리뷰어와의 지속적 피드백과 리팩토링을 통해 많은 것을 얻어갈 수 있었다.
또 리뷰어의 질문에 대한 답변을 어떤 식으로 이끌어가야 하는지를 배웠다(pr 메세지보다는 코멘트로), 어떤 부분을 고민해봐야 하는지를 알고 에너지를 쏟는 과정에서 코드가 깔끔해질 수 있었다.
다음 미션에서는 타인의 PR과 코드를 많이 참고해야겠다고 생각했고, 리뷰어에게 본인의 의견을 더 명확히 피력해야겠다고 생각했다.
또한 "행동할 줄 모르는 클래스"는 만들지 않아야겠다고 생각했다. 단순히 값만 반환하는 클래스가 이에 해당한다.
그러나 행동할 줄 알더라도, 클래스의 네이밍과 클래스의 행동이 일치하도록 해야겠다는 생각도 들었다.
클래스의 책임을 명확히 나누고, 그 책임을 잘 분산시켜 보자. 미션을 수행하면서 자연스럽게 체득할 수 있을 거라 믿는다
