[1단계 - 자동차 경주] 해리(최현웅) 미션 제출합니다.

[hwinkr]

리뷰어님! 반갑습니다 :) 😊 빙봉과 페어로 미션을 진행했습니다. 잘 부탁드려요!

설계도

본격적으로 미션을 해결하기 전, 페어와 대화를 나누면서 설계도를 그려보고 구현해야 하는 기능을 간단하게 작성해보았습니다.

페어와의 공통 질문

페어와 미션을 해결 하면서, 매듭 짓지 못한 의문점이 있어 질문하게 되었습니다.

1. getter를 어디까지 지양해야하는지?

프리코스를 진행하면서, **“객체는 객체스럽게 사용한다“**와 “getter를 지양한다.” 라는 공통 피드백을 받았었습니다.

그 이유에 대해서 페어와 얘기를 나누어 보았을 때, 도메인 내부에서 가지고 있는 상태와 관련된 로직(동작)을 해당 도메인이 책임 지는 것이 아니라, 외부에서 getter를 사용해서 상태를 가져오고 로직(동작)을 구현한다면 도메인의 상태가 예상치 못하게 변경될 수 있는 문제 때문이라고 정리 했습니다!.

그래서 이번 자동차 미션을 구현할 때, 자동차 위치 상태를 가지고 있는 Car 도메인이 우승자를 결정하는데 사용되는 로직을 구현하도록 했는데요, OutputView에서 각 라운드의 자동차 위치를 출력하기 위해서는 Car 도메인 내부에서 getter를 통해 어쩔수 없이 상태를 외부에 보내야 헸는데 결국은 getter를 사용하게 되었으니 getter를 지양하라는 것의 의미가 사용할 수 없는 곳에서는 어쩔수 없이 사용하되, 최대한 사용을 줄여야 한다는 것일까요? 궁급합니다!

2. 유효성 검사는 어느 영역에서 하는게 좋을까요?

컨트롤러에서 모든 유효성 검사를 수행하는 방향으로 미션을 진행하였습니다. 뷰, 컨트롤러, 도메인 중 어느 영역에서 유효성 검사를 수행하여야 하는지 페어와 고민을 해보았습니다.

우선 뷰 영역에서는 입력과 출력에 대한 책임만을 가지도록 하는 것이 적합하다고 생각하여 유효성 검사 수행 영역 후보지에서 제외하였습니다.

'경주 시도 횟수'와 같은 특정 입력의 경우 도메인을 따로 생성하지 않고 함수의 인자로만 전달되는 상황이 생겨서 통일성을 위해 도메인에서 유효성 검사를 하지 않고 컨트롤러에서 수행하게 되었습니다.
통일성을 버리더라도 각 도메인에 대한 유효성 검증은 해당 도메인 내부에서 하는게 맞을까요?

[JeongBin0227]

getter를 어디까지 지양해야하는지?

getter를 지양한다 는 원칙은 아마 객체 지향 프로그래밍에서 객체의 캡슐화와 정보 은닉을 하여, 객체의 상태를 외부에서 직접 접근하는 대신 객체가 스스로의 상태를 관리하고 행동할 수 있도록 메서드를 통해 상호작용하게 하려는 목적에 근거해서 나온말이 아닌가 생각됩니다.

위와 같은 내용은 이상적인 내용, 즉 getter 를 지양하는게 좋다이지 절대로 쓰면 안된다는 아니라고 생각합니다. 실제로 이번에 해리도 느끼셨겠지만, 완전히 getter를 사용하지 않는 것은 현실적이지 않다고 생각합니다. 예를 들면 UI 를 위한 데이터 표시도 그렇고 로깅하는것도 꼭 객체의 상태를 읽어야합니다.

따라서 결론은 getter의 사용을 최소화하고, 불가피하게 사용해야 할 경우에는 객체의 상태를 변경하는 로직은 포함하지 않도록 주의하면서 객체의 상태를 외부로 전달할 때는 가능한 불변성을 유지하거나, 복사본을 전달하여 원본 객체의 상태가 외부에 의해 예상치 못하게 변경되는 것을 방지하면 된다고 생각합니다.

유효성 검사는 어느 영역에서 하는게 좋을까요?

유효성 검사 위치는 만들고 있는 애플리케이션의 아키텍쳐나, 기획에 따른 비즈니스 규칙에 따라 달라질거 같아요.

예시로 주신 '경주 시도 횟수'와 같은 특정 입력의 경우, 사용자의 입력쪽이고, 간단한 검사이기 때문에 프레젠테이션 계층, 즉 뷰에서 유효성 검사를 하면 사용자 경험을 향상시킬수 있을 거 같아요.

통일성을 위해 모든 유효성 검사를 컨트롤러에서 수행할 수도 있지만, 이는 도메인 모델이 캡슐화하고 보호하는 것을 방해할 수 있다고 생각해요.

즉 가능하면 유효성 검사는 해당 도메인 내부에서 수행하는 것이 바람직하나, 유동적으로 필요하면 다른곳에서도 유효성 검사를 할 수 있다고 생각합니다.

[JeongBin0227]

안녕하세요 해리~
미션하느라 고생많으셨습니다.

해리가 주신 질문이나 고치면 좋은것들 남겨놨으니 편하게 읽어보시고 다시 질문 및 수정 요청 주시면 좋을거 같아요! 미션 하느라 고생많으셨습니다! 💯

[JeongBin0227]

여기서 왜 reduce 를 사용했는지 궁금해요~
reduce 는 누적된 값을 사용하기위해서 적절한 함수인데 여기서는 forEach 가 더 적절할거 같아요

[hwinkr]

mockRandoms([1, 2, 3])

pickNumberInRange
.mockReturnValueOnce(1)
.mockReturnValueOnce(2)
.mockReturnValueOnce(2)

모킹한 pickNumberInRange 함수가 지정한 값을 반환하도록 하려면 함수의 호출을 누적시켜야 하지 않을까라고 생각해서 reduce를 사용했었는데, 생각해보니 동일한 모킹 함수를 사용하니 인자로 들어온 numbers 배열을 forEach로 순회하면서 독립적으로 호출하는 것이 더 괜찮을 것 같습니다! 의견 감사드립니다

[JeongBin0227]

테스트 케이스 제목에서는 이런 추상적인 문장보다는
정상적인 전진과 정지가 올바르게 출력되는지 테스트 구체적인 문장이 더 좋습니다.

[hwinkr]

확실히 추상적인 문장 보다는 구체적인 문장이 테스트의 의도를 더 잘 드러내는 것 같아서 좋은 것 같네요!

[JeongBin0227]

전체적으로 mockQuestions와 getLogSpy 함수가 반복되고 있는데,
beforeEach 를 사용해서 코드의 중복을 줄여보면 어떨까요?

[hwinkr]

ApplicationTest.js에 있는 모든 테스트 케이스가 mockQuestions 함수를 호출하니 이를 test.each를 사용해서 중복을 줄여보려고 했으나, 예외가 있는 경우와 그렇지 않은 경우의 테스트 진행 방식이 달라서 두 경우를 분리해서 테스트를 진행하는 것으로 수정해보았습니다. 예외가 있는 경우에는 mockQuestions를 호출한 후 테스트 진행 방식이 동일해서 test.each를 사용해서 중복을 줄여봤습니다!

describe("자동차 경주 입력에 대한 예외 테스트", () => {
  const TEST_CASES = [
    {
      inputs: ["harry,harry", "harry,bong", "5"],
      expectedErrorMessage: ERROR_MESSAGES.carNameUniqueness,
    },
	//...
  ];

  let logSpy;

  beforeEach(() => {
    logSpy = getLogSpy();
  });

  test.each(TEST_CASES)(
    "각 예외 상황에 맞는 에러 메시지를 콘솔에 출력하는지 테스트",
	//...

[JeongBin0227]

예외가 있는 경우와 그렇지 않은 경우의 테스트 진행 방식이 달라서

제가 전체적인 내용을 몰라서 이 부분은 파악하지 못했는데,
test.each를 사용해서 중복을 줄여봤습니다! 이부분이 해리의 대처가 좋았던거 같아요 👍

[JeongBin0227]

테스트에서 따로 선언하지 않고
상수로 선언해서 사용하는건 잘하신거 같아요 👍

[JeongBin0227]

이 두함수가 비슷하게 쓰이고 있네요. 앞으로 또 이렇게 유저한테 입력을 받는게 생기면 비슷한 코드가 또 중복될 수 있을거 같아요.
그래서 이 두함수를 추상화 시켜보는건 어떨까요?

힌트를 주자면, #processInput(inputViewMethod, validator, errorMessage) 이런식의 함수를 만들고, 해당 함수를 통해 입력을 받고, 유효성검사를 하고, 유효한 입력을 반환하거나 오류가 발생하면 사용자에게 메시지를 출력할수 있을거 같아요.

[hwinkr]

프리코스를 진행할 때도, 입력을 많이 처리해야 하는 상황이 발생해 아래와 같이 추상화된 함수를 만들었었습니다.

// 프리코스 3주차 로또 미션에서 구현한 함수

const processInputWithRetry = async (inputCallback, processInputCallback, errorCallback) => {
  try {
    const input = await inputCallback();
    processInputCallback(input);
  } catch (error) {
    errorCallback(error.message);
    await processInputWithRetry(inputCallback, processInputCallback, errorCallback);
  }
};

export default processInputWithRetry;

하지만 이번 미션을 진행할 때는 입력을 받는 경우가 2번 밖에 없어 이 함수의 구현에 대해 생각해보지 않았는데, 확장성을 고려해 보았을 때 2번 이상 동일한 함수가 사용된다면 추상화된 하나의 함수를 구현하고 필요한 곳에서 호출하는 방향이 더 좋은 것 같습니다. 앞으로 "해당 애플리케이션이 확장된다면 유지보수를 위해 어떤 것을 고려해볼 수 있을까"에 대해 생각을 해보며 구현해 봐야겠네요. 좋은 의견 감사드립니다!

[JeongBin0227]

좋아요 👍
다만 제가 해리에게 오해를 줄수있을까봐 조금더 보충하면,
이 부분은 리팩토링에 포함되는데, 처음부터 확장성과 유지보수를 고민할 필요는 없을거 같아요~

즉, 처음부터 리팩토링을 고민하기 보단 코드를 먼저 짜보고, 확장성이나 유연성의 기준으로 코드를 개선해볼 수 있을까를 고민해보는게 좋을거 같아요.
회사에서는 좋은 코드도 중요하지만 제일 좋은 코드는 버그없이 잘돌아가면서 기한안에 작성한 코드가 제일 좋은 코드라고 생각해요. 그래서 리팩토링, 좋은 코드 늪에 빠져 고민하긴 보단, 해리가 생각한 구조대로 먼저 코드를 짜고 그다음에 여러 상황들을 고려하면서 개선하는게 좋다고 생각해요. 그 과정에서 테스트 코드가 큰역할을 준다고 생각합니다

[hwinkr]

좋은 말씀 감사드립니다!

[JeongBin0227]

nit) makeRoundResult 은 어떨까요?

[JeongBin0227]

이 구현방식이 잘못된건 아니지만, findMaxPosition 와 디펜던시가 생기는 단점이 있을거 같아요.
저라면

judgeWinners() {
  let maxPosition = 0;
  const winners = [];

  this.#cars.forEach(car => {
    const currentPosition = car.getPosition();
    if (currentPosition > maxPosition) {
      maxPosition = currentPosition;
      winners.length = 0; // 우승자 목록 초기화
      winners.push(car.getName());
    } else if (currentPosition === maxPosition) {
      winners.push(car.getName());
    }
  });

  return deepFreeze(winners);
}

이렇게 해서 코드 중복 제거하면서 디펜던시 없게 했을거 같아요~

[hwinkr]

아하.. 그렇군요!
하지만, 이 방식으로 구현하면 judgeWinners 메서드가

• 가장 멀리간 자동차를 찾는다.
• 가장 멀리간 자동차와 같은 거리의 자동차를 winners 배열에 담는다.

이렇게 2가지 책임을 모두 가지게 되는건 아닐까요? 저는 우승자를 찾을 때 2가지 과정을 거치니 Car 도메인의 isAheadOf 동작을 사용하는 메서드(findMaxPosition)와 isSamePosition 동작을 사용하는 메서드(judgeWinners) 2가지로 한 함수가 하나의 일만 하도록 분리를 시켜서 구현을 했었습니다! 클래스 내부 메서드 사이에 디펜던시가 생기면 어떤 문제가 발생할 수 있을까요? 궁금합니다!

[JeongBin0227]

이렇게 2가지 책임을 모두 가지게 되는건 아닐까요?

저는 디펜던시의 관점에서 생각했는데, 해리 말을 들어보니 두가지 책임이 생길거 같네요~
저는 고민해봤는데 이럴경우 책임도 분리하고 의존성을 최소화하기 위해

judgeWinners() {
  const maxPosition = this.#findMaxPosition();
  const winners = this.#calculateWinners(maxPosition);

  return deepFreeze(winners);
}

#findMaxPosition() {
  return Math.max(...this.#cars.map(car => car.getPosition()));
}

#calculateWinners(maxPosition) {
  return this.#cars
    .filter(car => car.getPosition() === maxPosition)
    .map(winner => winner.getName());
}

요런식으로 짜봤을거 같아요~ 이러면 의존성도 최소화하고 책임도 명확해질거 같아요

클래스 내부 메서드 사이에 디펜던시가 생기면 어떤 문제가 발생할 수 있을까요? 궁금합니다!

많은 문제가 있는데, 우선 디펜던시가 있으면 확장성이나 재사용성이 힘들어 집니다. 또한 테스트도 힘들어지고, 다른 사람들이 봤을때 디펜던시 코드도 같이 확인해야하기 때문에 가독성도 떨어집니다.

[hwinkr]

const winners = this.#cars
	.filter((car) => maxPositionCar.isSamePosition(car))
	.map((winner) => winner.getName());

이 부분을 한번 더 메서드로 분리해줘서 디펜던시를 줄일 수 있는 것이였군요! 앞으로 잘 활용해볼 수 있을 것 같아요

[JeongBin0227]

이렇게 util 함수를 만들어 재사용하는건 좋은거 같아요 👍

[JeongBin0227]

1과 5를 매직넘버로 쓰이는데, 따로 변수로 저장해도 좋을거 같아요

[hwinkr]

상수 분리에서 이 부분을 놓쳤었네요..감사합니다!

[JeongBin0227]

Suggested change

	if (
	!property ||
	typeof property !== 'object' ||
	Object.isFrozen(property)
	) {
	return;
	}
	deepFreeze(property);
	if (
	!property ||
	typeof property !== 'object' ||
	Object.isFrozen(property)
	) {
	return;
	}
	deepFreeze(property);if (typeof property === 'object' && property !== null && !Object.isFrozen(property)) {
	deepFreeze(property);
	}

이렇게도 바꿀수 있을거 같네요

[JeongBin0227]

안녕하세요~ 해리!

첫번째 미션 수행하느라 고생많으셨습니다!
수정해주신 부분 확인했고, 추가로 질문해주신 부분 답변 남겼으니 확인해보시면 좋을거 같아요!

첫 PR 머지 축하드리고 고생하셨습니다~ 💯