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

[1단계 - 자동차 경주] 지니(손진영) 미션 제출합니다. #275

Merged
merged 46 commits into from
Feb 17, 2024

Conversation

jinyoung234
Copy link

@jinyoung234 jinyoung234 commented Feb 15, 2024

안녕하세요 지그! 어찌저찌 마무리 하고 PR 남기게 되었습니다. 첫 미션 리뷰 잘 부탁드리겠습니다. (_ _)

💻 고민 하고 적용한 점

💬 Class 대신 Object Literal

이번 미션을 구현하면서, Class를 사용하기 보단, 객체 리터럴을 활용하였습니다. 궁금한 점에서 더 디테일하게 확인하실 수 있습니다.

Object Literal로 구현 할 때 저희는 아래와 같은 방법을 고려했습니다.

  1. 해당 모듈에 대한 상수 값들은 domain model directory/constant.js에 관리한다.
    • toss/slash 라이브러리에서도 모듈에 대한 상수 값과 공통 상수 값을 따로 관리하는 것에서 영감을 받아서 이 번 미션에서 이러한 방식으로 구현하게 되었습니다.
  2. public method는 객체 내부에, private method는 객체 외부에 위치 시킨다.
    • 단, 객체 외부로 위치 시킬 땐 export 키워드를 추가하지 않았습니다.
  3. function 키워드를 사용하여 top - down 형태로 코드를 전개한다.
    • 함수 표현식을 사용할 경우 강제적으로 bottom-up 방식을 사용하게 되는데, 가독성이 불편해진다고 생각했습니다. (모듈을 이동할 때 코드 line이 1에 위치하기 때문 입니다.)

💬 Validator

validation 할 목록들을 validationTypes 라는 속성의 객체로 저장 한 후, startValidation 이라는 함수 내에서 validationTypes 객체를 배열로 변환 후 하나씩 검증(isValid 함수로 진행)하며 isValid가 false일 경우 에러를 반환하는 형태로 validator 들을 구현했습니다.

💬 분리 할 수 있는 객체는 최대한 분리

저희는 MVC 패턴을 사용하여 Model(Domain), Controller, View로 레이어를 분리했고, Controller의 경우 각 모듈 들에 대해 요청하며 기능이 실행 되는 형태로 설계를 진행했습니다.

따라서, Controller가 요청하는 모듈 들에 대한 책임을 명확히 하는 것이 중요하다고 생각했고, 역할에 맞게 최대한 모듈을 분리함으로써, 메서드 들이 어떤 로직을 수행하는지, 어떤 값을 반환 하는지 예측하기 쉽도록 하였습니다.

💬 jsDoc을 통한 문서화

TypeScript를 사용하지 못하기 때문에, TypeScript 처럼 타입 자동 완성과 문서화 기능을 제공할 수 있는 방법에 대해 고민하다 jsDoc을 사용하자는 결론으로 도출되었습니다.

utils/jsDoc.js 내 각 타입 들에 대해 typedef를 사용하여 타입을 저장 했으며, 각 모듈에서 그 typedef 들을 import 해서 사용하는 형태로 jsDoc을 활용하였습니다.

단, 시간이 부족했던 탓에(정말 죄송합니다 🥲) 모든 모듈에 적용하지 못했던 점이 아쉬웠던거 같습니다.

💬 단위 테스트

저희가 생각하는 단위 테스트는 아래와 같다고 생각했습니다.

코드 베이스 내 단위(가장 작은 코드 조각으로써 함수, 클래스, 메서드에 대해 개별적으로 테스트 하는 것)가 예상대로 작동 하는지 검증하는 단계의 테스트

또한, 각 테스트 모듈은 아래와 특징을 가진다고 생각했습니다.

  • 한 눈에 알아보기 쉬운 네이밍을 가진다.
  • 코드 중복을 최소화 한다.
  • 하나의 작업에 대해 테스트 한다.
  • 외부 테스트 모듈에 영향 받지 않고 독립적이다.
  • 모킹을 최소화 하며 순수한 데이터에 대해 테스트를 한다.

위 개념과 특징을 통해 효과적인 단위 테스트를 하기 위해 아래와 같은 방법 들을 적용했습니다.

✔️ 테스트 내 린트 적용하기

소스 코드에 ESLint를 적용하여 일관성을 보장하듯 테스트 코드에 eslint-plugin-jest를 적용하여 테스트 케이스 오탈자 및 jest 문법 오류를 빠르게 확인하려고 했습니다.

✔️ describe 블록에서 관련 테스트를 그룹화하기

한 테스트 모듈에서 수 많은 test 메서드가 몰려 있다면, 테스트 항목은 많지만 어떤 case에 대해 테스트를 하는지 알기 어렵다고 생각했습니다.

하지만, 이를 describe 블록으로 다시 한번 묶음으로써, 관련 테스트를 그룹으로 분리하여 테스트 파일을 구성하는 것이 가능합니다.

메서드 단위로 나누거나, 작업 단위로 describe로 나누게 되면 어떤 항목을 테스트 하는 것인지 더 직관적으로 확인이 가능하다고 생각했습니다.

또한, 내부 describe 블록은 외부 describe 블록에서 설정 로직을 확장할 수 있기 때문에 추후 테스트 코드의 확장성에도 용이합니다.

✔️ 단위 테스트는 실패할 이유가 하나만 존재하기

단위 테스트는 이름 그대로 코드의 단일 단위만 테스트 하기 때문에, 단일 책임 원칙을 통해 하나의 실패 이유를 가지면 테스트 실패의 원인을 식별하는데 소요되는 시간을 줄일 수 있습니다.

또한, 테스트 내용이 짧아져 이해하기 더 쉬워진다고 생각했습니다.

📢 이슈 상황 해결

💬 RacingGame 모듈 내 racingResult의 참조 문제

import Random from '../../utils/random.js';
import Cars from '../Cars/module.js';

const RacingGame = Object.freeze({
  startRace({ racingCarNames, tryCount }) {
    const cars = new Cars(racingCarNames);

    const racingResult = Array.from({ length: tryCount }, () => {
      const randomMoveValues = Random.pickUniqueNumbersInRange(0, 9, racingCarNames.length);
      return cars.move(randomMoveValues);
    });
    console.log(racingResult);
    return racingResult;
  },
});

export default RacingGame;
class Cars {
  #racingCarDetails;

  constructor(racingCarNames) {
    this.#init(racingCarNames);
  }

  #init(racingCarNames) {
    this.#racingCarDetails = racingCarNames.map((carName) => ({ carName, moveCount: 0 }));
  }

  move(randomMoveValues) {
    randomMoveValues.forEach((randomValues, index) => {
      if (randomValues >= 4) {
        this.#racingCarDetails[index].moveCount += 1;
      }
    });

    return this.#racingCarDetails;
  }
}

export default Cars;

racingResult의 결과 값이 자동차 이름, 이동 횟수가 포함된 객체를 반환하는 2차원 배열이다보니 turn 마다 결과 배열이 racingResult에 들어갈 때 마다 이전 turn의 결과도 함께 영향을 받는 문제 발생 했던 이슈가 있었습니다.

// Cars.js
#updateMoveCounts(randomMoveCounts) {
  const racingCarDetailsCopy = this.#racingCarDetails.map((detail) => ({ ...detail }));

  randomMoveCounts.forEach((randomMoveCount, index) => {
    racingCarDetailsCopy[index] = CarEngine.triggerMove(racingCarDetailsCopy[index], randomMoveCount);
  });

  return racingCarDetailsCopy;
}

moveCars(randomMoveCounts) {
  this.#racingCarDetails = this.#updateMoveCounts(randomMoveCounts);

  return this.#racingCarDetails;
}

racingCarDetails에 대한 copy본을 만든 후 copy 배열 내 존재하는 객체를 update한 후 그 배열을 다시 this.#racingCarDetails에 추가함으로써, 참조 문제를 해결할 수 있었습니다.

🙋 궁금한 점

💬 Class Module vs Object literal

페어를 진행하면서, 클래스를 사용하지 않고 객체를 이용해서 전체적인 구현을 진행했습니다. 우선 클래스를 지양했던 이유는 아래와 같습니다.

✔️ 불 필요한 인스턴스 생성 줄이기

RacingGame.js

#updateRacingStatus() {
  const { minNumber, maxNumber } = RANDOM_NUMBER_RANGE;
  this.#racingStatus = this.#racingStatus.map((currentRacingCarInfo) =>
    RacingCar.from(currentRacingCarInfo).move(pickRandomNumberInRange(minNumber, maxNumber)),
  );
}

RacingCar.js

class RacingCar {
  static MOVE_THRESHOLD = 4;

  static MAX_CAR_NAME_LENGTH = 5;

  #racingCarInfo;

  constructor(racingCarInfo) {
    this.#racingCarInfo = { ...racingCarInfo };
  }

  static from(racingCarInfo) {
    return new RacingCar(racingCarInfo);
  }

  move(randomNumber) {
    const { position: prevPosition } = this.#racingCarInfo;
    return randomNumber >= RacingCar.MOVE_THRESHOLD
      ? { ...this.#racingCarInfo, position: prevPosition + 1 }
      : this.#racingCarInfo;
  }
}

구현 중 RacingCar의 경우 name, position이 있는 객체를 받아 move 함수를 통해 조건이 성립되면 position을 증가 시키도록 설계 했었으며, RacingGame에서 각 레이싱(lap) 마다 RacingCar를 통해 position증가 시켰었습니다.

const MOVE_THRESHOLD = 4;
const MAX_CAR_NAME_LENGTH = 5;

const move = (racingCarInfo, randomNumber) => {
  return randomNumber >= MOVE_THRESHOLD
    ? { ...racingCarInfo, position: racingCarInfo.position + 1 }
    : racingCarInfo;
};

이런 연산을 하기 위해 인스턴스를 만들고 메서드 호출을 하기 보단, 순수 함수를 통해 객체를 넘겨주면 결과 값을 바로 받는 것이 더 직관적이고 간결한 표현을 만들 수 있다고 생각했습니다.

또한, 인스턴스를 생성하지 않기 때문에 성능적으로도 이점을 볼 수 있다고 생각했습니다.

✔️ 예측 가능한 설계 만들기

RacingGame.js

class RacingGame {
  #carNames;

  #moveCount;

  #racingStatus;

  constructor(carNames, moveCount) {
    this.#carNames = carNames;
    this.#moveCount = moveCount;
    this.#racingStatus = this.#initializeRacingStatus();
  }

  #initializeRacingStatus() {
    return this.#carNames.map((carName) => ({ carName, position: 0 }));
  }

  runRace() {
    return Array.from({ length: this.#moveCount }, () => {
      this.#updateRacingStatus();
      return [...this.#racingStatus];
    });
  }

  #updateRacingStatus() {
    const { minNumber, maxNumber } = RANDOM_NUMBER_RANGE;
    this.#racingStatus = this.#racingStatus.map((currentRacingCarInfo) =>
      RacingCar.from(currentRacingCarInfo).move(pickRandomNumberInRange(minNumber, maxNumber)),
    );
  }
}

클래스의 경우 내부 필드를 통해 스스로 값을 변경시킬 수 있습니다.

RacingGame의 경우에도 racingStatus를 내부 상태로 가져, updateRacingStatus 메서드가 값을 바로 반환하는 것이 아닌, racingStatus을 조작하고 있는 것을 알 수 있습니다.

자바스크립트에서 함수의 경우 일급 객체 이기 때문에 변수로 선언이 가능하고, 인자로 함수를 넘겨줄 수 있으며, 함수를 반환할 수 있습니다.

이러한 함수의 특성을 통해 순수 함수로 관리하면 항상 예측 가능한 값을 만들어 내어 좋은 가독성과 유지보수를 가질 수 있다고 생각했습니다.

✔️ 복잡도를 낮추기

class Example {
  	#value;
    
    static CONSTANTS = 1;

	constructor() {
    	// ...
      	this.#value = 1;
    }

	method() {
      	this.#privateMethod();
    	console.log(`example ${this.#value}`);
    }

	#privateMethod() {
		console.log('privateMethod');
	}
}

const example = new Example();
example.method() 
// privateMethod
// example 1

클래스필드, 생성자 함수, 접근 제어자, static추가해야 할 요소가 함수 및 객체에 비해 많은 편 입니다.

const privateMethod = () => console.log('privateMethod');

export const CONSTANTS = 1;

const value = 1;

const method = () => {
  privateMethod();
  console.log(`example ${value}`);
}

method()
// privateMethod
// example 1

하지만 함수는 클래스 보다 직관적이고 보기 쉽다고 생각했습니다.

캡슐화의 경우에도 모듈 시스템(import/export)를 잘 활용하면 접근 제어자 처럼 필요한 것 들만 드러낼 수 있다고 생각했습니다.

/* eslint-disable max-lines-per-function */
import BridgeMaker from './BridgeMaker';
import { GAME, USER, ERROR } from './constants/Message';
import BridgeGame from './domains/BridgeGame';
import Validator from './validator';
import InputView from './view/InputView';
import OutputView from './view/OutputView';
import BridgeRandomNumberGenerator from './BridgeRandomNumberGenerator';
import { STATUS_TABLE } from './constants/bridgeGame';
import { INPUT_EXIT, INPUT_BRIDGE } from './constants/commands';

const { SUCCESS, FAIL } = STATUS_TABLE;

class GameController {
  constructor() {
    this.bridgeGame = new BridgeGame();
  }

  async run() {
    OutputView.print(GAME.START);
    const bridgeSize = await GameController.#createBridgeSize();
    this.#makeBridge(bridgeSize);
    await this.#moveBridge();
  }

  async #moveBridge() {
    let status = '';
    while (status !== SUCCESS && status !== FAIL) {
      const userMoveType = await GameController.#createBridgeMoveType();
      this.bridgeGame.move(userMoveType);
      status = this.bridgeGame.getStatus();
      OutputView.printMap(this.bridgeGame.getBridge());
    }
    if (status === FAIL) {
      const fail = await this.#fail(status);
      return fail;
    }
    return this.#success(status);
  }

  #success(status) {
    const [top, bottom] = this.bridgeGame.getBridge();
    const count = this.bridgeGame.getCount();
    OutputView.printResult({ top, bottom, status, count, isSuccess: true });
  }

  async #fail(status) {
    const command = await GameController.#createGameCommand();
    if (command === INPUT_EXIT.RESTART) {
      this.bridgeGame.retry();
      this.#moveBridge();
      return;
    }
    const [[top, bottom], count] = [this.bridgeGame.getBridge(), this.bridgeGame.getCount()];
    OutputView.printResult({ top, bottom, status, count });
  }

  /**
   * 유저가 입력한 다리 길이를 통해 다리를 생성 후 BridgeGame 클래스 내 answerBridge 필드에 값을 할당하는 메서드
   * @param {number} bridgeSize - 유저가 입력한 다리 길이
   */
  #makeBridge(bridgeSize) {
    const answerBridge = BridgeMaker.makeBridge(bridgeSize, BridgeRandomNumberGenerator.generate);
    this.bridgeGame.setAnswerBridge(answerBridge);
  }

  /**
   * 유저가 다리 사이즈를 입력하기 위한 메서드
   * @param {*} callback - 유저가 입력한 값을 yield 키워드에서 사용하기 위한 callback
   * @returns {void}
   */
  static #inputBridgeSize(callback) {
    InputView.readBridgeSize(USER.INPUT_BRIDGE_SIZE, (inputValue) => {
      callback(inputValue);
    });
  }

  static async #createGameCommand() {
    const command = await GameController.#inputGameCommand;
    return command;
  }

  static async #createBridgeSize() {
    let bridgeSize = 0;
    while (true) {
      bridgeSize = await GameController.#inputBridgeSize();
      if (Validator.isValidate(!Number(bridgeSize), ERROR.BRIDGE_SIZE)) break;
    }
    return bridgeSize;
  }

  static async #createBridgeMoveType() {
    let moveType = '';
    while (true) {
      moveType = await InputView.readMoving(USER.INPUT_MOVE_TYPE);
      const isIncorrectMoveType = moveType !== INPUT_BRIDGE.D && moveType !== INPUT_BRIDGE.U;
      if (Validator.isValidate(isIncorrectMoveType, ERROR.MOVE_TYPE)) break;
    }
    return moveType;
  }

  static #inputGameCommand(callback) {
    InputView.readGameCommand(USER.INPUT_GAME_COMMAND, (inputValue) => {
      callback(inputValue);
    });
  }
}

export default GameController;

이러한 고민의 결과로 생명 주기가 길어져야 할 경우(예시로 BridgeGame을 든 이유는 다리 건너기 미션에서 다리를 건너지 못할 경우 선택적으로 이전 값을 들고 재 실행 되어야 하기 때문에 예시로 활용하기 좋다고 생각했습니다!) 클래스 모듈을 사용하자는 의견이 나왔습니다.

결론적으로 Cars(이전 turn의 racingCarDetails 값을 기반으로 현재 turn의 racingCarDetails 값을 생성하기 때문)를 제외한 나머지 모듈에 클래스를 적용하지 않았습니다.

저희의 결론에 대해 지그의 의견도 들어보고 싶습니다!

💬 테스트 코드를 반드시 __tests__에 위치 시켜야 할까?

저희는 미션을 구현 하면서, 각 상위 디렉터리 내 모듈 파일(module.js), 테스트 파일, 상수 파일(optional)을 위치하는 형태로 아키텍처를 설계하였습니다.

이렇게 설계 했던 이유는, 유지 보수를 해야 하는 상황이라면 에러가 발생하거나 기능을 추가해야 할 경우 어떤 모듈에서 수정 해야 할 지 빠르게 파악하고 대응하기 위해서 입니다.

따라서 테스트 코드를 tests 디렉터리에 따로 위치 시키지 않았는데 리뷰어님의 의견이 궁금합니다!

chosim-dvlpr and others added 30 commits February 14, 2024 15:15
Co-authored-by: jinyoung234 <sm099@naver.com>
1. 공통 상수를 위한 constants 디렉터리 분리 2. 전체적인 자동차 게임 로직을 수행하는 controller 디렉터리 분리 3. 공통 에러 객체 및 재 입력을 위한 errors 디렉터리 분리 4. jsDoc 타입, Console 모듈을 분리하기 위한 utils 디렉터리 분리 6. 자동차 이름을 검증하기 위한 Validator 디렉터리 분리

Co-authored-by: chosim-dvlpr <bmj05378@gmail.com>
Co-authored-by: chosim-dvlpr <bmj05378@gmail.com>
Co-authored-by: chosim-dvlpr <bmj05378@gmail.com>
Co-authored-by: chosim-dvlpr <bmj05378@gmail.com>
Co-authored-by: chosim-dvlpr <bmj05378@gmail.com>
1. Input message 추가
2. Controller 내의 시도 횟수 구현 추가
3. Validator 추가
4. 입력 메서드 구현

Co-authored-by: jinyoung234 <sm099@naver.com>
Co-authored-by: jinyoung234 <sm099@naver.com>
1. 무작위 값 생성을 위한 Random 모듈 추가

2. 자동차 경주 결과를 반환하는 RacingGame(도메인 모델) 모듈 추가

3. 자동차들의 이동을 총괄하는 Cars(도메인 모델) 모듈 추가

4. 조건부 자동차 이동을 담당하는 CarEngine(도메인 모델) 모듈 추가

5. Controller 내 자동차 이동 로직 추가

Co-authored-by: chosim-dvlpr <bmj05378@gmail.com>
Co-authored-by: chosim-dvlpr <bmj05378@gmail.com>
Co-authored-by: chosim-dvlpr <bmj05378@gmail.com>
…받는 형태로 변경

1. tryCount 만큼의 row와 racingCarNamesLength 만큼의 column의 randomMoveCount를 생성하는 RandomMoveCountMaker(domain model) 모듈 추가

2. RacingGame 내 startRace 메서드에서 randomMoveCounts를 외부에서 주입 받는 형태로 변경

3. Controller 내 randomMoveCounts를 만들어 RacingGame에 주입하는 로직 추가

Co-authored-by: chosim-dvlpr <bmj05378@gmail.com>
Co-authored-by: chosim-dvlpr <bmj05378@gmail.com>
1. RacingWinnerRecorder 모듈 추가
2. Controller 내 우승자 확인 로직 추가

Co-authored-by: jinyoung234 <sm099@naver.com>
Co-authored-by: jinyoung234 <sm099@naver.com>
1. messages.js를 messages 디렉터리 내 모듈 및 테스트 코드를 위치 시키는 형태로 변경

2. 문자열 포맷을 위한 FORMAT_MESSAGE 구현

3. 자동차 경주 결과 및 우승자 출력을 위한 OutputView 모듈 구현

4. messages 모듈 path 이동에 의해 InputView 및 RacingGameController 경로 변경

5. controller 내 자동차 경주 결과 및 우승자 출력 로직 구현

Co-authored-by: chosim-dvlpr <bmj05378@gmail.com>
Co-authored-by: chosim-dvlpr <bmj05378@gmail.com>
1. CarNameValidator 내 중복된 자동차 이름을 검사하기 위한 duplicateCarNames 객체 추가

2. jsDoc에 duplicateCarNames 속성 추가

Co-authored-by: chosim-dvlpr <bmj05378@gmail.com>
CarEngine, RandomMoveCountMaker, CarNameValidator, TryCountValidator 내 하드 코딩 된 값을 상수로 변경

Co-authored-by: chosim-dvlpr <bmj05378@gmail.com>
1. eslint overrides에 console.js 추가

2. rules 내 설정 추가

3. printWidth 80에서 120으로 변경

Co-authored-by: chosim-dvlpr <bmj05378@gmail.com>
Co-authored-by: jinyoung234 <sm099@naver.com>
Copy link

@zigsong zigsong left a comment

Choose a reason for hiding this comment

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

안녕하세요 지니! 첫 리뷰를 남기게 된 지그입니다.

우선은 코드 위주로 리뷰를 남겼고, MR에 적어주신 내용이 많아 이 부분은 이후에 리뷰 이어가도록 하겠습니다.

한 가지 말씀드리고 싶은 점은, MR 본문이 너무 길고 내용이 방대하여 모두 읽기가 어려웠습니다.

  • 배경지식과 관련된 내용은 생략하고,
  • 궁금했던 점의 코드를 간추려서 작성해주시면

리뷰하기가 더 수월할 것 같습니다.

추가로, 테스트 코드는 잘 작성해주신 것 같아 따로 리뷰 남기지 않았습니다 😉

Comment on lines +3 to +7
const App = Object.freeze({
async start() {
await RacingGameController.run();
},
});
Copy link

Choose a reason for hiding this comment

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

AppObject.freeze()를 사용하신 특별한 이유가 있으실까요?

한 군데(index.js)서만 사용되고, 딱히 외부에서 변경될 가능성이 없는 객체라면 굳이 모든 것을 Object.freeze()해줄 필요는 없을 것 같습니다. App 객체 안에 중요한 정보들이 있는 것도 아닌 것 같아서요

Copy link

Choose a reason for hiding this comment

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

다른 곳들(ex. RacingGameController)도 마찬가지입니다

Copy link
Author

Choose a reason for hiding this comment

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

AppObject.freeze()를 사용하신 특별한 이유가 있으실까요?

한 군데(index.js)서만 사용되고, 딱히 외부에서 변경될 가능성이 없는 객체라면 굳이 모든 것을 Object.freeze()해줄 필요는 없을 것 같습니다. App 객체 안에 중요한 정보들이 있는 것도 아닌 것 같아서요

다른 곳에서 수정할 수 있는 여지를 막으려는 목적으로 read-only 성격의 Object.freeze()를 사용하게 되었습니다.

프로덕션(cli 기반 애플리케이션) 코드라고 가정 했을 때 App 객체 내 start 메서드가 변경 된다면 앱 전체가 멈춰버릴 수 있다고 생각했던거 같습니다. 🥲

지그가 언급해주신 중요한 정보라고 한다면 비즈니스 로직들이 있는 model layer 라고 생각하면 되는지 궁금합니다!

Copy link

Choose a reason for hiding this comment

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

아아 '중요한 정보'라고 언급한 건 특별한 의미는 아니었구요, 외부에서 의도적으로 변경할 만한 프로퍼티가 아니라고 생각했습니다.

비즈니스 로직들이 있는 model layer 라고 생각하면 되는지 궁금합니다!

말씀주신 대로 Car의 moveCount 등이 외부에서 변경될 가능성이 있는 요소들이겠죠?

Copy link
Author

Choose a reason for hiding this comment

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

아! 좋은 인사이트 감사합니다 !! 👍


export default RacingGameController;

async function processUserInput() {
Copy link

Choose a reason for hiding this comment

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

processUserInput 이 함수는 딱히 이 파일 외부에서 쓰이지도 않고, 공용 함수나 유틸성 함수도 아니라 RacingGameController 안에 있어도 상관없을 것 같은데 따로 빼신 이유가 있나요~?

Copy link
Author

Choose a reason for hiding this comment

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

객체 내부에 존재하게 되면 public 하게 외부 모듈에서 접근이 가능하게 되기 때문입니다.

controller 내부 로직을 굳이 외부에 알리지 않고 캡슐화 하고 싶은 목적에 외부에 작성하게 되었습니다.

Copy link

@zigsong zigsong Feb 17, 2024

Choose a reason for hiding this comment

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

controller 내부 로직을 굳이 외부에 알리지 않고 캡슐화 하고 싶은 목적에 외부에 작성하게 되었습니다.

  • controller 내부로직을
  • 외부에 작성했다

라는 것은, 이것이 controller 내부 로직이라고 생각하신 건가요? 외부 로직이라고 생각하신 건가요?

외부에 작성했다면 더 이상 controller 내부 로직이 아닙니다.

그런 의도였다면 processUserInput()은 util 등 별도의 파일에 있는 것이 나을 것 같습니다

Copy link
Author

Choose a reason for hiding this comment

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

다시 생각해보니 지그 의견 처럼 내부 동작에 관련된 로직을 객체 외부로 두게 될 경우 그 모듈의 내부 로직이 아닌 그 모듈과 관련된 함수 정도로 인식할 수도 있겠네요..

제가 생각이 짧았던거 같습니다 👏

src/controller/RacingGameController.js Outdated Show resolved Hide resolved
src/utils/console.js Show resolved Hide resolved
src/utils/console.js Show resolved Hide resolved
src/utils/random.js Outdated Show resolved Hide resolved
@@ -0,0 +1,31 @@
import CarEngine from '../CarEngine/module.js';

class Cars {
Copy link

Choose a reason for hiding this comment

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

클래스 생성 시 Cars와 같은 복수 형태 대신 개별 차를 의미하는 Car로 만들어보는 건 어떨까요?

단일 기능을 나타내는 클래스에 복수의 의미가 들어가있는 것도 어색하고, 비즈니스 로직을 파악하기도 어렵습니다.

Copy link

Choose a reason for hiding this comment

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

추가로 MVC 패턴의 M(Model)을 구현하신듯 한데, 디렉토리명에 domain보다는 model이라고 사용하는 것이 더 일반적인 것 같습니다.

Copy link
Author

Choose a reason for hiding this comment

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

클래스 생성 시 Cars와 같은 복수 형태 대신 개별 차를 의미하는 Car로 만들어보는 건 어떨까요?

단일 기능을 나타내는 클래스에 복수의 의미가 들어가있는 것도 어색하고, 비즈니스 로직을 파악하기도 어렵습니다.

제가 인지하지 못했던거 같습니다. 🥲

그저 Cars를 설계할 때, 각 Turn 마다 차들을 이동시켜야 한다는 목적으로 설계하다보니 위와 같은 모듈이 구현 되었던거 같습니다.

옛날 자료긴 하지만, 데이터 모델링 할 때 복수형을 사용하지 않는다고 하더라구요. 🥲🥲🥲

참고해서 수정해보겠습니다. 좋은 인사이트 감사합니다!!

Copy link
Author

Choose a reason for hiding this comment

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

추가로 MVC 패턴의 M(Model)을 구현하신듯 한데, 디렉토리명에 domain보다는 model이라고 사용하는 것이 더 일반적인 것 같습니다.

도메인 로직임을 명시하고자 domain 이라고 표기 했었는데 참고해서 수정해보겠습니다. 👍

src/domain/Cars/module.js Outdated Show resolved Hide resolved
src/domain/Cars/module.js Outdated Show resolved Hide resolved
}

#updateMoveCounts(randomMoveCounts) {
const racingCarDetailsCopy = this.#racingCarDetails.map((detail) => ({ ...detail }));
Copy link

Choose a reason for hiding this comment

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

racingCarDetailsCopy가 탄생하는 순간 애써 핵심 변수처럼 만들어두었던 racingCarDetails 는 그 의미를 잃게 됩니다. racingCarDetailsCopy 를 만들어내기 위한 도구로 전락하는 느낌이죠…

racingCarDetails에 대한 copy본을 만든 후 copy 배열 내 존재하는 객체를 update한 후 그 배열을 다시 this.#racingCarDetails에 추가함으로써, 참조 문제를 해결할 수 있었습니다.

질문 읽어보았는데요,

우선적으로 Cars와 같이 복수형 모델 사용한 것부터 코드가 지나치게 복잡해졌습니다.

RacingGame.startRace에 인자로 넘겨주는 tryCountrandomMoveCounts가 이미 '각각' 정해진 채로 들어와서 이후 로직을 작성하기가 어려워졌구요.

1, 2, 3회차에 해당하는 randomValues를 미리 모두 정해두지 않고, 각 회차 실행 시점에 생성했다면 코드의 복잡성이나 MR에 작성주셨던 객체 재할당의 문제가 해소될 것 같습니다

@zigsong
Copy link

zigsong commented Feb 15, 2024

잠이 오지 않아 추가로 리뷰 남깁니다. 😴

  • 클래스 vs 일반 모듈

    • �클래스의 경우 내부 필드를 통해 스스로 값을 변경시킬 수 있습니다. > 를 부정적으로 보신 이유가 있으신가요? 그저 JS 패턴 중 하나라고 생각합니다.
    • 클래스는 필드, 생성자 함수, 접근 제어자, static 등 추가해야 할 요소가 함수 및 객체에 비해 많은 편 입니다. > 그렇기 때문에 각각이 하는 역할이 더욱 명확히 드러나는 장점이 있습니다. 캡슐화의 관점에서 일일히 import/export를 구분하지 않게 해주는 일종의 컨벤션인 셈이죠.
    • 이러한 고민의 결과로 생명 주기가 길어져야 할 경우(...) 클래스 모듈을 사용하자는 의견이 나왔습니다. > 앞선 고민과 생명 주기가 어떤 연관성이 있는지 모르겠습니다.
  • 테스트 코드를 반드시 __tests__에 위치?

    • jest와 같은 대중적인 JS 테스팅 프레임워크들에서는 테스트 실행 시 .test.js, .spec.js 등의 확장자로 끝나는 파일이나, __tests__ 디렉토리 안에 있는 파일들을 테스트 파일로 인식하여 자동으로 해당 파일에 대한 테스트를 실행해줍니다.
    • 꼭 따를 필요는 없지만, 추후 테스팅 프레임워크 사용 등에 있어서 유용하니 따르는 것을 권장합니다.

@jinyoung234
Copy link
Author

jinyoung234 commented Feb 15, 2024

잠이 오지 않아 추가로 리뷰 남깁니다. 😴

  • 클래스 vs 일반 모듈

    • �클래스의 경우 내부 필드를 통해 스스로 값을 변경시킬 수 있습니다. > 를 부정적으로 보신 이유가 있으신가요? 그저 JS 패턴 중 하나라고 생각합니다.
    • 클래스는 필드, 생성자 함수, 접근 제어자, static 등 추가해야 할 요소가 함수 및 객체에 비해 많은 편 입니다. > 그렇기 때문에 각각이 하는 역할이 더욱 명확히 드러나는 장점이 있습니다. 캡슐화의 관점에서 일일히 import/export를 구분하지 않게 해주는 일종의 컨벤션인 셈이죠.
    • 이러한 고민의 결과로 생명 주기가 길어져야 할 경우(...) 클래스 모듈을 사용하자는 의견이 나왔습니다. > 앞선 고민과 생명 주기가 어떤 연관성이 있는지 모르겠습니다.
  • 테스트 코드를 반드시 __tests__에 위치?

    • jest와 같은 대중적인 JS 테스팅 프레임워크들에서는 테스트 실행 시 .test.js, .spec.js 등의 확장자로 끝나는 파일이나, __tests__ 디렉토리 안에 있는 파일들을 테스트 파일로 인식하여 자동으로 해당 파일에 대한 테스트를 실행해줍니다.
    • 꼭 따를 필요는 없지만, 추후 테스팅 프레임워크 사용 등에 있어서 유용하니 따르는 것을 권장합니다.

클래스는 인스턴스가 생성될 때 여러 동작들을 수행합니다.

const Person = (function () {
  // 생성자 함수
  function Person(name) {
    this.name = name;
  }

  // 프로토타입 메서드
  Person.prototype.sayHi = function () {
    console.log('Hi! My name is ' + this.name);
  };

  // 생성자 함수 반환
  return Person;
}());

// 인스턴스 생성
const me = new Person('jiny');
me.sayHi(); // Hi! My name is jiny
  1. 생성자 함수 생성
  2. 생성자 함수의 프로토타입에 메서드 추가
  3. 생성자 함수 반환

하지만, 객체 리터럴로 표현하면 다음과 같이 구현할 수도 있습니다.

const Person = {
  sayHi(name) {
    console.log(`Hi! My name is ${name}`)
  }
}

즉, 무분별한 클래스는 불 필요한 코드와 메모리 사용을 초래할 수 있다고 생각했습니다.

또한 모던 자바스크립트 딥다이브의 418p를 참고해보면 저자는 다음과 같이 표현합니다.

사실 클래스는 함수이며, 기존 프로토타입 기반 패턴을 클래스 기반 패턴 처럼 사용할 수 있도록 하는 문법적 설탕입니다.

이 글을 읽었을 때 저는 불 필요하게 자바스크립트의 프로토타입을 직접 접근 하는 경우를 줄일 수 있지 않을까? 라는 생각이 들기도 하였습니다.

멘토님 의견 중 '코드를 읽는 맥락에서 controller 내 processUserInput을 실행 시 무조건 에러가 발생할 것 같은 의미로 해석된다'는 의견을 반영

1. ErrorHandler를 RetryHandler로 변경

2. 메서드 네이밍을 errorWithLogging으로 변경

3. 변경 사항 다른 모듈들에 적용
[ModuleName]/module.js 에서 [ModuleName].js의 형태로 변경
1. 불필요한 모듈이 되어버린 Cars와 CarEngine 제거

2. 자동차 이동을 담당하는 도메인 객체인 Car 구현

3. RacingGame을 class형 모듈로 변경

4. controller 내 racingGame의 선언 방법 변경 및 startRace 메서드 호출 방법 변경
MVC 패턴의 특성에 맞게 domain에서 models로 변경
1. random sort 방식에서 Fisher-Yates Shuffle 알고리즘 방식으로 변경

2. array 간 swap 로직을 swap 함수로 분리
row와 column을 각각 rowIndex와 columnIndex로 변경
변수의 일관성을 위해 CAR_LENGTH_RANGE로 네이밍 후 객체 형태로 변경
@zigsong
Copy link

zigsong commented Feb 16, 2024

안녕하세요 지니~ 추가 리뷰는 내일 마저 드리도록 하겠습니다 🙇‍♀️

@zigsong
Copy link

zigsong commented Feb 17, 2024

즉, 무분별한 클래스는 불 필요한 코드와 메모리 사용을 초래할 수 있다고 생각했습니다.
또한 모던 자바스크립트 딥다이브의 418p를 참고해보면 저자는 다음과 같이 표현합니다.
사실 클래스는 함수이며, 기존 프로토타입 기반 패턴을 클래스 기반 패턴 처럼 사용할 수 있도록 하는 문법적 설탕입니다.
이 글을 읽었을 때 저는 불 필요하게 자바스크립트의 프로토타입을 직접 접근 하는 경우를 줄일 수 있지 않을까? 라는 생각이 들기도 하였습니다.

클래스가 프로토타입 상속의 일종의 문법 설탕라고 보는 의견들도 있지만, 정확히 동일하게 동작하지는 않으며 서로 다른 기능들이 있습니다. 이 점은 추후 찾아서 공부해보시면 좋을 것 같습니다!

그리고 자바스크립트의 프로토타입을 직접 접근하는 것을 부정적으로 보신듯 한데, 미리 정의된 빌트인 객체의 프로토타입에 직접 접근하는 것(ex. String.prototype.includes()의 동작을 직접 바꾸는 것)이 아니라 개발자가 생성한 객체(예시의 Person)라면 크게 문제될 상황은 없습니다. 만약 이것이 문제였다면 자바스크립트에서 class를 내놓지도 않았겠죠?

@zigsong
Copy link

zigsong commented Feb 17, 2024

안녕하세요 지니, 코드와 본문에서 많은 공부와 고민을 하신 게 느껴지네요.

온보딩 미션인 만큼, 추가로 남겨드린 코멘트들은 2단계 또는 다음 미션들에서 함께 고민해보면 좋겠습니다.

이 MR은 여기서 마무리하겠습니다. 수고하셨습니다! ☺️

Copy link

@zigsong zigsong left a comment

Choose a reason for hiding this comment

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

어프루브합니다

@zigsong zigsong merged commit 47fc7c1 into woowacourse:jinyoung234 Feb 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants