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단계 - 콘솔 기반 로또 게임] 버건디(전태헌) 미션 제출합니다. #263

Merged
merged 45 commits into from
Feb 24, 2024

Conversation

brgndyy
Copy link

@brgndyy brgndyy commented Feb 22, 2024

안녕하세요. 처음 뵙겠습니다!

- 어플리케이션 실행 방법

npm install
npm run start-step1

- 이번 미션에서 신경 쓴점

  1. 추상화
    이번 미션에서 페어와 추상화 관련하여 많은 이야기를 나누고 클래스 분리를 해보았습니다.

가령 당첨번호를 받는 WinLottoNumber에서 LottoNumber 클래스를 상속받아서 당첨번호가 로또 번호 클래스 내에서 유효성 검증을 거치도록 해주었습니다.

  1. 상수의 분리.

저는 지금까지 상수를 분리해줄때 constants 폴더를 만들어서 따로 관리를 해주었었는데, 이번 미션을 진행하면서 페어의 양이 그렇게 많지 않은데도 폴더로 나눈다는것은 오버엔지니어링 같다.라는 관점을 듣게 되었습니다.

그래서 이번에는 아예 각각의 파일 내에서 상수 값들을 선언해주어서 한 파일 내에서 관리를 해주었었는데요.

이러한 관점에 대해서 리뷰어님께서는 어떻게 생각하실지 한번 여쭈어 보고 싶습니다 :)

  1. try-catch 의 분리

저는 지금까지 retryHandler라는 유틸함수를 만들어서, 비동기 함수로 입력값을 받았다가 에러가 발생했을때 다시 해당 함수를 호출해주도록 했는데요.

const retryAsyncFunctionOnError = async (asyncFn, context) => {
  try {
    return await asyncFn.call(context);
  } catch (error) {
    console.error(error.message);
    return retryAsyncFunctionOnError(asyncFn, context);
  }
};

이번에는 try-catch 라는 구조 자체가 하나의 메서드인데 이를 또 분리한다는것 자체, 그리고 분리를 해주려면 call을 사용해야하는데 이것 자체에 동의하지 못하겠다는 페어의 의견이 있었습니다!

리뷰어분께 한번 이에 대한 의견도 여쭈어보고싶습니다 :)

읽어주셔서 감사합니다.

lurgi and others added 30 commits February 20, 2024 13:52
Co-authored-by: brgndyy <brgndyy@gmail.com>
Co-authored-by: JEON TAEHEON <brgndyy@users.noreply.github.com>
Co-authored-by: brgndyy <brgndyy@gmail.com>

fix

Co-authored-by: JEON TAEHEON <brgndyy@users.noreply.github.com>
Co-authored-by: brgndyy <brgndyy@gmail.com>

fix
Co-authored-by: brgndyy <brgndyy@gmail.com>
Co-authored-by: brgndyy <brgndyy@gmail.com>
Co-authored-by: brgndyy <brgndyy@gmail.com>
Co-authored-by: brgndyy <brgndyy@gmail.com>

fix
Co-authored-by: brgndyy <brgndyy@gmail.com>
Co-authored-by: JEON TAEHEON <brgndyy@users.noreply.github.com>
Co-authored-by: brgndyy <brgndyy@gmail.com>
Co-authored-by: brgndyy <brgndyy@gmail.com>
Co-authored-by: JEON TAEHEON <brgndyy@users.noreply.github.com정
Co-authored-by: brgndyy <brgndyy@gmail.com>
Co-authored-by: brgndyy <brgndyy@gmail.com>
Co-authored-by: brgndyy <brgndyy@gmail.com>
Co-authored-by: brgndyy <brgndyy@gmail.com>
Co-authored-by: brgndyy <brgndyy@gmail.com>
Co-authored-by: JEON TAEHEON <brgndyy@users.noreply.github.com>
Co-authored-by: brgndyy <brgndyy@gmail.com>
Co-authored-by: JEON TAEHEON <brgndyy@users.noreply.github.com>
Co-authored-by: JEON TAEHEON <brgndyy@users.noreply.github.com>
보너스 숫자 setBonusNumber 추가

Co-authored-by: brgndyy <brgndyy@gmail.com>
Co-authored-by: brgndyy <brgndyy@gmail.com>
Co-authored-by: brgndyy <brgndyy@gmail.com>
Co-authored-by: brgndyy <brgndyy@gmail.com>
Co-authored-by: brgndyy <brgndyy@gmail.com>
Co-authored-by: brgndyy <brgndyy@gmail.com>
lurgi and others added 2 commits February 22, 2024 16:18
Co-authored-by: JEON TAEHEON <brgndyy@users.noreply.github.com>
Co-authored-by: brgndyy <brgndyy@gmail.com>
@brgndyy brgndyy changed the title Step1 [1단계 - 콘솔 기반 로또 게임] 버건디(전태헌) 미션 제출합니다. Feb 22, 2024
@brgndyy brgndyy changed the base branch from main to brgndyy February 22, 2024 08:08
Copy link

@igy95 igy95 left a comment

Choose a reason for hiding this comment

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

안녕하세요 버건디~ 구현 전 다이어그램으로 레이어의 관계를 구성해보려는 시도는 좋았던 것 같습니다! 다만 클래스 간 상속 구조에 대해서 좀 더 고민해보시면 어떨까 싶네요 :) 세부 코멘트 확인 부탁드리고 질문에 대한 답변 남기면서 마무리하겠습니다!

상수의 분리.

양이 그렇게 많지 않은데도 폴더로 나눈다는것은 오버엔지니어링 같다.

어떤 코드에 대해 따로 파일을 분리해야 하는가?에 대한 기준점을 '코드의 양'보다는 '코드의 예상되는 사용 범위'에 초점을 맞추어 진행하시는 것을 권장드립니다. 가령 복잡한 유틸 or 상수이더라도 특정 폴더 내만 사용될 것으로 예상된다면 굳이 폴더나 파일을 분리하지 않아도 괜찮습니다. 반대로 짧은 유틸 or 상수이더라도 사용처가 제한되어 있지 않고 두 곳 이상에서 공용으로 사용해야할 경우는 의존성 관리나 재사용성을 위해 분리하는 것이 좋다고 생각합니다.

try-catch 의 분리

예시로 올려주신 유틸의 이름은 수정이 필요해보이긴 하나, '에러가 발생했을 때 동일한 함수를 재호출한다'는 동작 자체는 재사용성의 의미가 있어 보입니다. 다만 this를 외부에서 인자로 받는 경우는 어떠한 부분을 고려하신 건지는 잘 모르겠네요!

.prettierrc copy Outdated Show resolved Hide resolved
test.each([
{
numbers: [1, 2, 3, 4, 5, 6],
winObj: { winNumbers: [1, 2, 3, 4, 5, 6], bonusNumber: 7 },
Copy link

Choose a reason for hiding this comment

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

obj, arr, data 등 도메인보다는 자료구조에 초점을 맞춘 네이밍은 다른 대체할 만한 네이밍이 없지 않은 이상 되도록 지양하는 것이 좋겠습니다.

Copy link
Author

Choose a reason for hiding this comment

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

짚어주셔서 감사합니다!

혹시 그러면 winNumberInfo 는 어떠실까요 ?

단순히 winNumbers라고 하기에는 당첨번호만 포함하고 bonusNumber는 포함하지 못하는 듯한 느낌이 드는데 이러한 공통됐지만 속성은 다른 경우에 보통 어떤식으로 네이밍을 해주어야할지 여쭈어 보고싶습니다!

Copy link

Choose a reason for hiding this comment

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

괜찮을 것 같습니다~ 네이밍에 대한 일관적인 원칙은 없지만, 최대한 하위 데이터를 아우르는 이름 or 사용 목적에 부합한 이름을 짓는 것이 좋겠습니다. 만약 네이밍이 어렵다면 AI를 사용해보시는 것도 권장드립니다~

Screenshot 2024-02-24 at 1 33 22 PM

Copy link
Author

Choose a reason for hiding this comment

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

짚어주셔서 감사합니다! 한번 활용해보도록 하겠습니다 ~

__tests__/Lotto.test.js Outdated Show resolved Hide resolved
__tests__/LottoMachine.test.js Outdated Show resolved Hide resolved
__tests__/LottoNumber.test.js Outdated Show resolved Hide resolved
src/Domain/LottoNumber.js Outdated Show resolved Hide resolved
Comment on lines 18 to 19
this.#numbers = numbers.sort((a, b) => a - b);
this.#validLottoNumbers();
Copy link

Choose a reason for hiding this comment

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

데이터의 가공과 유효성 검증의 우선순위를 비교하자면 유효성 검증이 더 높기 때문에 순서가 바뀌어야 할 듯 싶네요!

Copy link
Author

Choose a reason for hiding this comment

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

만약에 저 유효성 검증 코드와 필드 선언 코드의 순서가 뒤바뀌었을시에, 유효성 검증 함수 안에는 전부 인자가 들어가게 되는데, 혹시 이러한 부분은 감수를 해야 하는 부분일까요 ?

1. 필드 먼저 선언했을시

  1. 먼저 필드로 선언한다.
  2. 유효성 검증 함수를 해주다가 검증에 실패했을시에 예외처리한다.

2. 유효성 검증 먼저 했을시

  1. 유효성 검증을 해준다.
  2. 유효성 검증이 전부 끝나야지만 필드로 선언해준다.
  #validLottoNumbers() {
    this.#numbers.forEach((number) => {
      this.#validInRange(number);
    });
    this.#validDuplicate(numbers);
    this.#validLength(numbers);
  }

  #validInRange(number) {
    if (number < LOTTO_NUMBER_RANGE.MIN || number > LOTTO_NUMBER_RANGE.MAX) {
      throw new Error(ERROR_MESSAGES.OUT_OF_RANGE);
    }
  }

  validInRangeWrapper(number) {
    return this.#validInRange(number);
  }

  #validDuplicate(numbers) {
    if (new Set([numbers]).size !== LOTTO_LENGTH) {
      throw new Error(ERROR_MESSAGES.DUPLICATE);
    }
  }

  #validLength(numbers) {
    if (numbers.length !== LOTTO_LENGTH) {
      throw new Error(ERROR_MESSAGES.INVALID_LENGTH);
    }
  }

Copy link

@igy95 igy95 Feb 24, 2024

Choose a reason for hiding this comment

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

LottoNumber와 직접적으로 연관되지 않은 유효성 검증 함수를 모두 클래스 바깥으로 분리하면, 현재 고민과는 관계없이 자연스레 인자를 넣어주게끔 수정해야 될 것 같습니다.

그리고 현재의 고민보다는 조건이든, 동작이든 앞뒤 순서의 의존 관계를 고려해서 호출될 필요가 없는 함수를 미리 호출하지 않도록 코드를 작성하시는 것이 성능 관점에서도 부합한 코드를 작성할 수 있습니다.

하단 예시에서 isValidfalse일 경우 바로 null을 반환하는 것과 result를 계산하고 isValid를 봐주는 것중 어느 것이 성능적으로 더 나은 코드라고 할 수 있을까요?

// 1.
function getResultOrNull(isValid: boolean) {
  if (!isValid) return null;

  return calculateComplexLogic();
}

// 2.
function getResultOrNull(isValid: boolean) {
  const result = calculateComplexLogic();

  if (!isValid) return null;
  
  return result;
}

Copy link
Author

Choose a reason for hiding this comment

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

calculateComplexLogic()으로 예를 드니까 확 와닿네요!

짚어주셔서 감사합니다 : )

src/Domain/Lotto.js Outdated Show resolved Hide resolved
src/Domain/LottoMachine.js Outdated Show resolved Hide resolved
src/View/OutputView.js Outdated Show resolved Hide resolved
@igy95 igy95 self-assigned this Feb 22, 2024
Copy link

@igy95 igy95 left a comment

Choose a reason for hiding this comment

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

안녕하세요 버건디! 빠르게 리뷰 반영해주시느라 고생 많으셨습니다 ㅎㅎ 추가적인 코멘트 남겨두었으니 확인 부탁드리고 다음 단계에서 뵐게요! 🙌

@igy95 igy95 merged commit d2d2375 into woowacourse:brgndyy Feb 24, 2024
@brgndyy brgndyy deleted the step1 branch February 26, 2024 12:41
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.

3 participants