[1단계 - 행운의 로또 미션] 안(안수진) 미션 제출합니다

[jin7969]

안녕하세요! 리뷰어님😆 로또 미션 1 단계를 제출합니다.
리뷰 부탁드립니다!! 🙏

🚀 데모페이지

🎯 기능요구사항

• 로또 구입 금액을 입력하면, 금액에 해당하는 로또를 발급해야 한다.
• 로또 1장의 가격은 1,000원이다.
• 소비자는 자동 구매를 할 수 있어야 한다.
• 번호 보기 토글 버튼을 클릭해 로또 번호를 볼 수 있어야 한다.

테스트 요구사항

• 기능 요구사항을 구현하며, 도메인 로직에 대한 단위 테스트를 Jest 로 작성한다.

🗂 파일 구조

📦src
 ┣ 📂css
 ┃ ┣ 📜index.css
 ┃ ┗ 📜switch.css
 ┗ 📂js
 ┃ ┣ 📂__tests__
 ┃ ┃ ┗ 📜app.test.js
 ┃ ┣ 📂controller
 ┃ ┃ ┗ 📜LottoController.js
 ┃ ┣ 📂model
 ┃ ┃ ┣ 📜Lotto.js
 ┃ ┃ ┗ 📜LottoGame.js
 ┃ ┣ 📂utils
 ┃ ┃ ┣ 📜constants.js
 ┃ ┃ ┣ 📜dom.js
 ┃ ┃ ┣ 📜general.js
 ┃ ┃ ┗ 📜validation.js
 ┃ ┣ 📂views
 ┃ ┃ ┗ 📜LottoGameView.js
 ┃ ┗ 📜index.js

✅ 기타 사항

• mvc 패턴을 사용하여 코드를 작성하였습니다.
• 이번 미션을 진행하면서 unit test를 처음 접하게 되었는데,
  테스트코드를 어떤 식으로 나누어서 작성해야 하는지 어려움이 있었습니다.

추가내용

• 사용자 입장에서 금액을 입력하지 않았으면, 생성된 로또가 없으므로 "번호보기" 스위치가 작동하지 않도록 하였습니다.
• 금액이 입력이 되면, 금액 입력창을 disabled 하여 오류가 나지 않도록 하였습니다.

[roy-jung]

안님 안녕하세요, 리뷰어 정재남입니다.
코드 잘 봤고, 여기서는 전체적인 맥락에서 말씀드리고 싶은 내용 몇가지를 적어보겠습니다.
코멘트 요청사항 확인해보시고 반영 / 이견제시 / 질문 등 해주세요!

• MVC 패턴에 대하여... #103 (review)
• 에러처리를 alert으로만 해주셨는데, try catch를 이용해서 한군데에서 일괄하여 처리하는 방법을 알아보시면 더 좋을것 같아요.
• 클래스에서 외부 노출되지 않는 내부 멤버들은 private으로 관리해주시면 더 좋을것 같습니다.

[roy-jung]

테스트하고자 하는 취지와 실제 테스트 내용이 일치하지 않네요.

[jin7969]

테스트 4개다 말씀하시는 건가요?
테스트 코드는 다시 학습해서 코멘트 남기겠습니다!

[roy-jung]

3번째 테스트의 경우, 이 테스트만으로 999원은 통과될지 어떨지를 확신할 수 있을까요?

4번째 테스트의 경우,
'단위가 1000원이어야 한다'를 테스트하기 위해서는
1000원으로 나누어떨어지지 않는 경우에 대한 테스트와,
1000원으로 나누어떨어지는 경우에 대한 테스트가 모두 필요할 것 같습니다.

[roy-jung]

한 번 생각해보시면 좋겠다는 취지이고 제 말이 100% 맞다는건 아닙니다만, 셀렉터를 상수로 담는 것에 어떤 가치가 있는지 모르겠습니다. 전체 프로젝트에서 한군데에서만 사용한다면 그곳에서만 사용하면 될 것 같고, 두군데 이상에서 사용한다면 그건 역할분리가 제대로 안되었다는 뜻이 되는것 같아서요. 하나의 파일에서 셀렉트한 것을 다른곳에서도 다시 셀렉트할 필요가 있다면 셀렉트한 결과물 자체를 변수화하는 편이 낫지 않을까요?

[jin7969]

지난 자동차 경주 미션때 페어가 셀렉터를 상수로 사용하는 걸 보고 굳이 저렇게 할 필요가 있을까 생각은 하였습니다.
하지만 e2e 테스트와 여러곳에서 사용하게 되어 셀렉터를 상수로 만들면 오타를 줄일수 있고, SELECTOR. 을 하면 자동으로 목록이 나와 편리 하여서 사용하였습니다.

로이님 말씀대로 "두군데 이상에서 사용한다면 역할분리가 제대로 안되었다" 라는 관점으로 한 곳에서만 사용해야 한다면 저도 필요가 없다고 생각됩니다.

<질문>
이런식으로 다른곳에서도 셀럭트할 필요가 있다면 this.switchInput 파라미터로 넘겨줘서 사용한다는 말씀이신가요??

// controller 파일

export default class LottoController {
  constructor() {
    this.switchInput = $(SELECTOR.SWITCH_INPUT);
  }

    handleLottoNumber(lottoCount) {
      this.lottoGameView.enableSwitch(this.switchInput);
  }
}

// view 파일

export default class LottoGameView {
  constructor() {
     ...
  }

  enableSwitch(switchInput) {
    switchInput.removeAttribute("disabled");
  }
}

[roy-jung]

$(SELECTOR.SWITCH_INPUT) 이 element는 controller가 인스턴스화되기 전에는 존재하지 않는 녀석인가요?
HTML상에 위 element의 생명주기가 JS의 생명주기보다 먼저 시작해서 끝까지 살아있다는 전제하에서는,
위 값을 상수 또는 변수로 만들어두면 안될까요?

const ELEMENTS = {
  ...
  SWITCH_INPUT: $(".switch-input")
}

이렇게 말이죠.

---

질문에 대해서는... 현재의 코드상에서 다른곳에서도 셀렉트할 필요가 있다면, 그럴 필요가 없게끔 할 수 있는 방법은 없을지를 고민해보시면 좋겠다는 취지였습니다.

[jin7969]

셀렉터를 한곳에서 쓰려니 view 파일에 몰빵하는 느낌이 들고, 분리하자니 셀렉터를 여러곳에서 써야하고...
여러가지 시도를 해보다가 결국 처음으로 다시 돌아 왔습니다 ㅠㅠ 처음부터 다시 설계를 해야 할지, 여기서 다른 방법이 있는지 조언을 구하고 싶습니다.

// 구매할 금액 입력
export default class LottoGame {
  constructor() {
    this.lottoGameModel = new Lotto();
    this.lottoGameView = new LottoGameView();
    $(".purchase-form").addEventListener("submit", this.onSubmitPurchase.bind(this));
  }

  onSubmitPurchase(e) {
    e.preventDefault();

    const purchaseAmount = Number(ELEMENTS.PURCHASE_INPUT.value);
    try {
      validatePurchaseAmount(purchaseAmount);
      const lottoCount = Math.floor(purchaseAmount / AMOUNT.UNIT);
      this.lottoGameModel.generateLottoTicket(lottoCount);
      this.lottoGameView.handlePurchasedLotto(this.lottoGameModel.getLottoList(), lottoCount);
    } catch (error) {
      alert(error);
    }
  }
}


// view
export default class LottoGameView {
  constructor() {
    this.lottoNumberList = $(".lotto-number-list");
    this.switchInput = $(".switch-input");
  }

  manageElement() {
    disableElement(ELEMENTS.PURCHASE_INPUT);
    disableElement($(".purchase-button"));
    enableElement(this.switchInput);
  }

  renderPurchaseInfomation(lottoCount) {
    $(".purchase-infomation").innerText = `총 ${lottoCount}개를 구매하였습니다.`;
  }

  renderLottoIcons(lottoCount) {
    this.lottoNumberList.insertAdjacentHTML("beforeend", `<li>🎟️</li>`.repeat(lottoCount));
  }

  renderLottoNumbers(lottoList) {
    lottoList.forEach((numbers) => {
      this.lottoNumberList.insertAdjacentHTML(
        "beforeend",
        `<li>🎟️<span class="lotto-numbers">${numbers}</span></li>`,
      );
    });
  }

  resetLottoList() {
    this.lottoNumberList.replaceChildren("");
  }

  onClickSwitch(lottoList, lottoCount) {
    this.resetLottoList();
    this.lottoNumberList.classList.toggle("show-numbers");
    if (this.lottoNumberList.classList.contains("show-numbers")) {
      this.renderLottoNumbers(lottoList);
      return;
    }
    this.renderLottoIcons(lottoCount);
  }

  handlePurchasedLotto(lottoList, lottoCount) {
    this.manageElement();
    this.renderPurchaseInfomation(lottoCount);
    this.renderLottoIcons(lottoCount);
    this.switchInput.addEventListener("click", () => this.onClickSwitch(lottoList, lottoCount));
  }
}

[roy-jung]

설계는 이대로도 문제 없는데, 최초에 제가 말씀드리고자 했던 취지가 그거였어요. "view에 몰빵"!
셀렉터라는게 view에서 element를 선택하자는 거니까 view에 의존적인게 당연하죠. 당연히 view에 의존적일 수밖에 없는 내용이라면 view에 몰아넣는게 합리적이지 않나요?

모든 크루들이 view와 controller의 의존관계를 끊어내려 하기보다 오히려 controller가 적극적으로 view에 개입하고 있는데, 그 이유는 제가 추측하기로는 크루분들 사이에 eventListener를 controller에서 등록해야 한다는 강박 같은게 자리잡은게 아닌가 싶거든요.. 왜 그런 컨센서스가 생긴건지 모르겠는데, 이벤트등록은 뷰에서 하는게 맞습니다. 이벤트 발생시 컨트롤러에서의 동작은 별도의 함수를 마련, 호출함으로써 해결하면 되구요.

당장 구조를 뜯어고치기엔 다음 진도도 나가야 하니, 내용 참고하셔서 2단계때 리팩토링을 해보셔도 좋고, 다음 미션때 새롭게 시도해보셔도 좋을 것 같습니다.

[roy-jung]

인스턴스화하고 bindEvents를 호출하는 명령이 반드시 순차적으로 일어나는 거라면,
bindEvents 메서드를 constructor에 넣어두는 편이 낫지 않나요?

[jin7969]

#103 (comment)
같은 크루인 록바의 코멘트를 보고 이해 하였습니다!!

[roy-jung]

이 둘이 분리되어 있어야 할 이유가 없는 것 같아요.

[jin7969]

#114 (comment)
똑같이 이해 하면 될까요?

[roy-jung]

넵

[roy-jung]

전적으로 lottoGameView에 대한 내용인데 controller가 들고 있어야 할 이유가 있나요?

[roy-jung]

#82 (comment)

[jin7969]

저도 처음에 똑같이 생각을 하였습니다. 1~45 숫자 배열을 만들고 랜덤으로 인덱스를 선택해서 푸쉬한다
하지만 #103 (comment) 로이님 코멘트를 보고 다시 작성하였습니다. 랜덤숫자를 뽑는 함수도 필요없고, for 문도 사용하지 않아 휠씬 더 깔끔한거 같습니다. 👍

// before

  generateRandomNumber() {
    const lottoNumbers = Array(RANGE_MAX)
      .fill()
      .map((num, index) => index + RANGE_MIN);
    for (let i = 0; i < 6; i += 1) {
      const randomIndex = getRandomNumber(lottoNumbers.length);
      this.numbers.push(lottoNumbers[randomIndex]);
      lottoNumbers.splice(randomIndex, 1);
    }
  }

// after

  generateRandomNumber() {
    const { LENGTH_MIN, LENGTH_MAX, RANGE_MIN, RANGE_MAX } = LOTTO_NUMBER;

    const lottoNumbers = Array(RANGE_MAX)
      .fill()
      .map((_, index) => index + RANGE_MIN);
    shuffleArray(lottoNumbers);
    this.numbers = lottoNumbers.slice(LENGTH_MIN, LENGTH_MAX);
  }

[jin7969]

fill() 을 사용하여 배열을 만들었지만, 배열에 값을 넣을 필요도 없고 ( fill(0) ), 스프레드 연산자가 가독성이 더 좋아 수정 하였습니다.

const lottoNumbers = [...Array(RANGE_MAX)].map((_, index) => index + RANGE_MIN);


// 시간도 체크해 보려고 하였으나, 브라우저나 터미널마다 달라서 정확하게 파악이 안되었습니다.

console.time("스프레드");
const arr = [...Array(100000)].map((_, index) => index + 1);
console.timeEnd("스프레드");

[roy-jung]

좋네요! 성능은 상대적으로 안좋기는 하지만 가독성이나 이해도의 측면에선 확실히 이득이 있는 방법이죠 ㅎㅎ

[roy-jung]

for문 대신 다른 방법은 없을까요?

[jin7969]

로또티켓 한 장을 만드는 클래스와 생성된 모든 로또 티켓 클래스를 따로 관리 하였으나, 로또티켓 한 장 클래스는 필요가 없는거 같아
삭제하고 하나의 로또 클래스에서 관리하도록 하였습니다.

Lotto.js 삭제
LottoGame.js -> 파일명 변경 Lotto.js

for문 대신 스프레드 연산자로 배열을 만들고 map 함수를 이용하여 구현을 하였습니다.

generateLottoTicket(count) {
  this.lottos = [...Array(count)].map(() => this.generateLottoNumber());
}

[roy-jung]

고생하셨고, step-2에서 다시 뵙겠습니다.
몇가지 코멘트 추가했는데, 이 내용들은 step-2때 검토/반영해주세요 :)

[roy-jung]

console은 지워주세요 :)

[roy-jung]

외부에서 직접 접근하지 못하도록 private 설정을 해두면 어떨까요?

[roy-jung]

.map(this.generateLottoNumber)로도 될거예요.

[jin7969]

제가 새로고침인 줄 알고 Reviewers 에서 버튼을 잘못눌려서 리뷰요청이 된거 같아요 ㅠㅠ 정말 죄송합니다. 🙏
제대로 해서 리뷰요청 할려고 하였는데... 어쩔수 없이 2단계에서 마무리 하도록 하겠습니다.

코멘트로 설명을 대체 하겠습니다.

1. 테스트

로또 번호 생성 테스트와 금액 유효성 테스트를 나누어서 작성을 하였습니다.
로또 번호 테스트에서 1~45 범위 테스트도 추가 하였으며, 유효성 테스트는 가능한 것과 불가능한 것으로 나누어서 작성하였습니다.
불가능 테스트에서 toThrow() 로 에러를 던지는 것으로 하면 될지, expect(isValidMinimumAmount(amount)).toBeFalsy() 로 false 를 하는게 나을지 고민을 하였고, 저는 에러를 던지는 것이 '불가능을 테스트 한다'에 포함이 된다고 생각되어 아래와 같이 작성하였습니다. 로이님의 의견이 궁금합니다.

describe("구입할 금액 유효성 테스트", () => {
  test("1000원 이상 이어야 한다.", () => {
    const amount = 1000;
    expect(!isValidMinimumAmount(amount)).toBeTruthy();
  });

  test("1000원 미만으로 입력할 수 없다.", () => {
    const amount = 999;
    expect(() => validatePurchaseAmount(amount).toThrow(ERROR_MESSAGES.MINIMUM_AMOUNT_IS_SMALL));
  });

  test("1000원 단위로 입력 가능하다.", () => {
    const amount = 2000;
    expect(!isValidAmountUnit(amount)).toBeTruthy();
  });

  test("1000원 단위가 아니면 입력할 수 없다.", () => {
    const amount = 2200;
    expect(() => validatePurchaseAmount(amount)).toThrow(ERROR_MESSAGES.NOT_DIVIDED_INTO_THOUSAND);
  });

2. 역할 분리

직접 부딪혀 보니 이해가 잘 되었습니다. 감사합니다 :)
로이님 말씀대로 이벤트는 컨트롤에서 해야 한다는 강박이 있었습니다. 2단계에서는 패턴 보다는 역할 분리와 기본기에 집중해서 구현을 하겠습니다.

3. private 설정

셀렉터에 대한 코멘트를 받고 수정을 한 뒤, 마지막으로 private 설정을 하고 리뷰요청을 할려고 하였습니다.. 아쉽네요ㅠㅠ
2단계에서 반영을 하면서 작성하겠습니다.

4. 추가 내용

!isValidMinimumAmount() 이처럼 부정문을 사용하여 검사를 하였으나, 부정문이 있으면 한번더 생각을 해야 하므로 밑에 내용으로 수정하였습니다.

// utill
export const isValidMinimumAmount = (amount) => {
  return amount < AMOUNT.MINIMUM;
};

export const isValidAmountUnit = (amount) => {
  return amount % AMOUNT.UNIT !== 0;
};

// validation
  if (isValidMinimumAmount(amount)) {
    throw new Error(ERROR_MESSAGES.MINIMUM_AMOUNT_IS_SMALL);
  }

  if (isValidAmountUnit(amount)) {
    throw new Error(ERROR_MESSAGES.NOT_DIVIDED_INTO_THOUSAND);
  }

element 속성을 설정하는 함수를 dom.js 로 분리하여 사용하였습니다.

export const disableElement = (element) => {
  element.setAttribute("disabled", true);
};

export const enableElement = (element) => {
  element.removeAttribute("disabled");
};