[ccjs 5기] 자동차 경주 스텝1 코드리뷰

[imakerjun]

📝 저자 워크숍

저자 워크숍은 19세기 말, 아이오와 대학에서 시작됐습니다. 이 워크숍은 매우 성공적이어서, 예를 들면 이곳 출신 퓰리처상 수상자만 16명이 될 정도라고 합니다. 아이오와 대학의 저자 워크숍은 빠른 속도로 저자 커뮤니티 사이에 번져 나갔고, 현재는 초중급 작가들이 자신의 실력을 빠르게 높일 수 있는 가장 효과적인 방법 중 하나로 알려져 있습니다. 그리고 하나의 전문 분야에서 무언가 큰 성공을 거두면 다른 분야에도 전이가 됩니다. 시인이자 프로그래머인 리처드 가브리엘은 문학 쪽에서 이 저자 워크숍을 가져와 소프트웨어 쪽에도 적용하였습니다. 그리고 이는 소프트웨어 개발자들에게도 코드라는 저작물을 더 효과적으로 개선시키는데 효과를 주었고 현재는 소프트웨어 뿐만 아니라 마케팅, 요리, 비즈니스 기획 등 다양한 분야에도 걸쳐 적용되고 있습니다.

💻 코드리뷰 저자 워크숍

저자 워크숍은 본래 글쓰기에서 시작되었으며 코드도 하나의 언어로 표현된 저작물이라고 생각하고 진행하는 워크숍입니다. 코드리뷰 저자 워크숍은 아래와 같은 단계로 진행합니다.

1. 코드를 읽어보기

먼저 리뷰할 대상의 코드를 훑어봅니다. 이해가 되지 않는 부분이 있어도 괜찮습니다. 대략 전체를 훑어 본다고 생각하고, 이 때 각 코드에 대한 첫 인상을 메모해둡니다.

2. 저자를 환영하기

저자를 환영합니다. 우리는 비판을 하기 위해 모인 것이 아니고, 저자의 작업물이 더 잘 자라나기 위해 유용한 피드백을 주고 받는 자리라는것을 인지합니다.

3. 코드 일부를 읽기

저자는 코드 중 본인이 중요하다고 생각하는 부분을 선택해 읽습니다. 이 때 주석, 함수의 이름, 변수의 네이밍 등을 이야기할 수 있습니다. 전체 코드를 다 이야기하는 것이 아니고 중요하게 생각한 지점, 이 코드 전체에서 핵심이라고 생각되는 부분을 중심으로 이야기합니다.

4. 코드 요약하기

다른 사람들은 저자가 읽은 코드 부분에 대해 자신들의 이해를 요약하여 말합니다. 이 때 개인적인 평가나 의견보다는 중립적이고 관찰자적인 요약에 집중합니다.

5. 긍정적 피드백

코드에 대한 긍정적인 피드백을 공유합니다. 리뷰하는 사람 입장에서 특히 어떤 부분이 좋았는지, 다른 부분은 바뀌더라도 이 부분만큼은 그대로 남았으면 하는 것은 무엇인지에 대해 이야기합니다. 좋게 느껴진 네이밍, 적절한 주석, 깔끔한 로직 구현 등을 포함할 수 있습니다.

6. 개선 제안하기

개선 제안하는 부분에서 어쩌면 부정적인 피드백이 나올수도 있습니다. 하지만 거기에서 끝나면 안됩니다. 개선 제안이 되야합니다.
그래서 개선을 위한 피드백을 줄 때는 "이 부분이 좋지 않다"라고만 말하는 대신, 어떻게 개선할 수 있을지 구체적으로 제안합니다. "이 함수의 네이밍을 이렇게 바꾸면 어떨까요?" 혹은 "이 주석을 추가하면 이해하는 데 도움이 될 것 같습니다."와 같이 제안합니다.

7. 저자의 질문

저자는 피드백 중에서 이해하지 못한 부분이나 개선 제안에 대해 질문할 수 있습니다. 예를 들어, "왜 이 함수 이름을 바꾸는 것이 더 좋다고 생각하셨나요?"와 같은 질문을 할 수 있습니다.

8. 저자에게 감사하기

마지막으로, 저자에게 감사의 말을 전하고, 앞으로도 좋은 코드를 작성하기 위한 노력을 격려하고 응원합니다.

[imakerjun]

진영님의 코드 저자 워크숍

#153

[cobocho]

코드를 읽어보기

• 메서드를 거의 분자단위로 쪼갠게 인상 깊었어요
• static을 적절하게 잘 쓰신것 같아요
• 상수화를 디렉토리별로 나눈게 좋아요
• 은닉화도 깔끔한것 같아요

코드 요약하기

• 기본적인 어플리케이션 자체는 MVC 패턴을 통해 작성된다.
• 기능별로 모델을 생성한다.
• 클래스는 인스턴스가 생성된다는 전제 하에 생성한다.
• 모듈 패턴을 사용해 Private한 부분은 최대한 은닉화한다.
• MVC간 간섭을 최대한 줄여 의존성을 없앤다

긍정적 피드백

• 디렉토리 설정이 깔끔해서 이 부분은 계속 유지하면 좋겠어요!
• 클래스를 사용하지 않으면서도 은닉화가 효율적으로 잘 적용된 것 같아요!
• JS의 기본 내장 메서드를 적재적소에 쓰신것 같습니다!

개선 제안하기

export const INPUT_MESSAGE = Object.freeze({
  RACING_CAR:
    '경주할 자동차 이름을 입력하세요(이름은 쉼표(,)를 기준으로 구분).\n',
});

쉼표도 상수화가 적용되면 좋을 것 같아요!

class RacingCars {
  #moveStatus;

  #numberMaker;

  constructor(numberMaker) {
    this.#moveStatus = {};
    this.#numberMaker = numberMaker;
  }
...

constructor 대신 필드에 디폴트로 설정해도 좋을 것 같아요!

const Validator = (function Validator() {
  const validateRacingCars = (userInput) => {
    const racingCars = convertStringToArray(userInput, SEPERATOR_SYMBOLS.COMMA);
    if (isInvalidLengthRacingCars(racingCars))
      throw new RangeError(
        ERROR_MESSAGE.INVALID_RANGE(CAR_MIN_LENGTH, CAR_MAX_LENGTH),
      );
    if (isDuplicateRacingCars(racingCars))
      throw new SyntaxError(ERROR_MESSAGE.DUPLICATE_CAR_NAMES);
  };

  return {
    check(userInput, message) {
      switch (message) {
        case INPUT_MESSAGE.RACING_CAR:
          validateRacingCars(userInput);
          break;
        default:
          throw new Error(ERROR_MESSAGE.NOT_MESSAGE);
      }
    },
  };
})();

Validator가 확장될 가능성도 있으니 check()가 어떤걸 체크하는지 메서드명에 포함되면 더 좋을듯해요!

[vitabyfrank]

1. 대략적인 첫인상

• 관심사 분리를 잘하신것 같아요!
• 함수명도 잘 신경써주신것 같아요! 굿굿 !

4. 코드 요약하기

• 객체 지향적인 코드를 사용하면서, 관심사마다 책임 분리하기.
• 즉시 실행함수를 사용하여 InputView를 리턴하는데 인상 깊었다.
• constants에 관심사 기준으로 파일로 분리했다.

5. 긍정적 피드백

• 관심사 분리에 많은 고민을 하며, 코드를 작성한 부분이 그대로 드러났어요! 디테일한 부분이 좋았습니다.
• freeze를 사용하여, 휴먼 에러를 제거하는 부분이 인상 깊었어요!

6. 개선 제안하기
   OutputView에서 print만 찍는것도 좋지만, 관심사로 분리 하시는건 어떻게 생각하실까요?

[bxxmi]

첫인상

• 테스트 코드 작성 시 jest의 기능을 잘 활용하시는 것 같다.
• mvc 패턴으로 개발 시 모델을 잘게 나누시는 게 인상적이다.
• object.freeze 활용을 잘하시는 것 같다.

코드 요약

• 하나의 Car model로 구성하는 방법도 있었을텐데 그 안에서 다루는 데이터들도 관리의 용이성을 위해 model을 분리하셨다.
• 사용자에게 값을 입력받을 때 그 행위에 집중할 수 있도록 구성하셨다.
• 즉시실행함수를 사용하셨다.

긍정 피드백

• 상수 값을 각 사용처에 맞게 분리해서 관리하는 것이 좋아보인다.
• 에러 메세지의 객체화
• 메소드 이름의 구체화 -> 어떤 기능을 수행하는지 파악하기 쉬웠다.
• jest의 다양한 기능을 활용한 테스트 코드 작성

개선 사항 제안

• utils/car.js의 createResultArray 함수 로직 중 split 메소드를 2번 사용하고 계신데, 가독성과 불필요한 반복을 줄이기 위해 하나로 정리해서 사용하는 것은 어떨까요? 또한, map 함수 내 인자로 s 라는 키워드 보다는 result와 같이 명확한 키워드를 사용하는 것은 어떨까요?

export const createResultArray = (racingResult) =>
  racingResult
    .at(-1)
    .split('\n')
    .map((result) => {
      const splitResult = result.split(' : ');
      const [racer, distance] = [splitResult[0], splitResult[1].length];
      return [racer, distance];
    });

그리고 지금은 인덱스 값을 직접 넣고 있는데 만약, racingResult에 다른 값들이 들어오게 된다면 해당 인덱스를 하드 코딩해서 사용하는 것은 조금 위험하지 않을까?! 라는 생각도 들었습니다!🤔

[HoseokNa]

코드를 읽어보기

• 파일이 많이 있어서 세부적으로 구현하셨다고 느꼈어요.
• Test 코드도 폴더로 분리되어 있어서 인상 깊었습니다.

코드 요약하기

• 본인의 역할만 하기 위해 책임을 나누었다.
• 캡슐화에 신경을 써서 노출할 정보만 사용할 수 있게 구성했다.
• 인스턴스로 만들 필요가 없는 곳은 클래스 대신에 즉시함수를 활용했다.

긍정적 피드백

• 저와 달리 jest의 여러 함수들을 활용해서 좋았습니다.
• 테스트 코드에서 util함수와 상수관리를 할 생각을 못했는데 좋은 방법이라고 느꼈습니다.
• 책임이 명확하게 분리 되어 있어서 확인하기 좋았습니다.
• 에러 메세지 정의를 인자를 받도록해서 조건이 변경이 됐을 때도 유연하게 사용할 수 있을 것 같습니다.

개선 제안하기

• 개선 보다는 질문이 될 수 있을 것 같은데 아래 처럼 작성한거랑 즉시 실행함수로 만드신 것과 차이가 있는걸까요? 🤔 (저도 잘 몰라서)

const createUserInputByQuestion = async (message) => {
    const readMachine = createReadMachine();
    const userInput = await readMachine.question(message);
    readMachine.close();
    return userInput;
  };

const getUserInput = async (message) => {
  const userInput = await createUserInputByQuestion(message);
  Validator.check(userInput, message);
  return userInput;
};

const processUserInput = (userInput, message) => {
  switch (message) {
    case INPUT_MESSAGE.RACING_CAR:
      return convertStringToArray(userInput, ',');
    default:
      return userInput;
  }
};

export const async input = (message) => {
  const userInput = await getUserInput(message);
  return processUserInput(userInput, message);
}

• NumberMaker.getRacingCarRandomNumbers는 테스트 작성을 위해 만드신걸까요?
  • 확인했을 떄는 사용되는 곳이 테스트 코드에만 있는 것 같았어요.
  • 필요한 곳에서 NumberMaker.createRandomNumber을 사용할 수 있을 것 같아요.

저자의 질문

저자에게 감사하기

[yondo123]

첫 인상

• 필요에 따라 즉시실행함수를 쓰셨다.
• 다양한 jest 메서드를 활용해서 테스트케이스를 꼼꼼하게 작성한 흔적이 보인다.

코드 요약하기

• 은닉화를 위해서 즉시실행함수를 쓰셨다.
• 책임 분리를 위해 MVC 패턴을 도입하였으며 맡은 책임만 수행할 수 있도록 model, view, controller로 분리하였다.
• 인스턴스가 사용하지 않는 메서드들은 static 메서드로 작성하였다.

긍정적 피드백

• MVC 패턴을 적극적으로 활용하여 분리를 시도하신것이 인상깊었습니다.
• 경계 값 상수를 오류 메시지까지 도입하는 부분이 좋았어요.

개선점 제안하기

• getRacingCarRandomNumbers 함수에서 할당된 racingCarNumbers를 단순히 map을 수행하려는 의도시라면 간결하게 메서드 체이닝을 활용해보시는 것은 어떨까용~?🙇🏻

//NumberMaker.js
  getRacingCarRandomNumbers(racingCars) {
    const racingCarNumbers = [...racingCars];
    return racingCarNumbers.map(() => this.createRandomNumber());
  },

//제안
getRacingCarRandomNumbers(racingCars) {
   return [...racingCars].map(() => this.createRandomNumber());;
  },

[jinyoung234]

Q1. App 클래스에서 컨트롤러를 설정할수 있다면 어떨까요?? 예를들어 App의 constructor에서 인자를 받아서 컨트롤러에 전달하는 방식

→ 혹시 어떤 이유 인지 들어볼 수 있을까요?

Q2. check()가 어떤걸 체크하는지 메서드명에 포함되면 더 좋을듯해요! @cobocho

→ check()의 역할이 사실 validate~의 메서드들을 추상화 하려는 목적이었는데, 혹시 더 좋은 이름이 있을까요 ..? ㅜ

Q3. OutputView에서, print만 찍는것이, 관심사로 분리 하시는건 어떻게 생각하실까요? @devfrankkim

→ 혹시 다시 설명 한번만 부탁드려도 될까요 ..? ㅜㅜ 이해를 잘 못했습니다!

@HoseokNa 아 private 적인 부분은 외부에서 아예 사용할 수 없도록 하기 위해 IIFE를 통해 캡슐화하는 것이 목표였습니다!!

레퍼런스는 찾아보고 댓글에 남겨두겠습니다!

[cobocho]

A2. Validation을 MVC에서 누가 담당하냐에 따라서 달라질것 같은데 만약 model별로 Validate를 작성한다면 지금 방식이 가장 적절할것 같고 Validator를 통합하여 관리한다면 메서드명에 체크 대상이 포함되면 좋지 않을까 싶었어요!

[imakerjun]

A2. 추상화를 너무 많이 하면, 오히려 가독성 측면에서 어려움이 생기는 경우가 있는것 같아요. check라는 메서드로까지 추상화를 하는 목적이 무엇인지가 드러나면 좋을것 같아요~!

[vitabyfrank]

Q3.

GameController 
  static #printRaceResult(results) {
    OutputView.print(OUTPUT_MESSAGE.RESULT);
    results.forEach((result) => {
      OutputView.print(`${result}\n`);
    });

controller에서 View까지 같이 해결하는 것 같아서, print 메세지 찍는것도 좋지만, view만 따로 관심사로 빼는건 어떤가 생각했습니다 :)

[jinyoung234]

다들 감사합니다 :)

[jinyoung234]

A2. Validation을 MVC에서 누가 담당하냐에 따라서 달라질것 같은데 만약 model 별로 Validate를 작성한다면 지금 방식이 가장 적절할것 같고 Validator를 통합하여 관리한다면 메서드명에 체크 대상이 포함되면 좋지 않을까 싶었어요!

@cobocho 아..!! 통합해서 관리하는 거니 체크 대상이 있으면 좋겠네여 좋은 의견 공유 감사합니다 :)!!!!