[2단계 - 행운의 로또 미션] 신세한탄(신세희) 미션 제출합니다 by shinsehantan · Pull Request #56 · woowacourse/javascript-lotto

shinsehantan · 2021-02-26T15:09:09Z

안녕하세요. 리뷰어님 :)
@jum0 와 함께 로또 미션을 진행한 신세한탄입니다.
2단계 미션을 진행하며 궁금했던 사항을 정리해보았습니다.

게임이 진행될 때, 모델의 프로퍼티들에 기존의 값을 덮어씌우는 방식으로 진행하고 있습니다. 그래서 게임을 다시 시작하는 경우, 모델의 값이 남아있더라도 다음 게임에 영향을 미치지 않습니다. 그럼에도 불구하고 다시 시작을 할 때, 모델의 프로퍼티 값들을 초기화하는 것이 좋은지 궁금합니다.

    this.ticketBundle = Array.from({ length: ticketLength }, () =>
      this.generateRandomNumbers()
    );
  }

alert 메시지를 상수 처리할 때, 메시지 내용을 기준으로 네이밍을 하는 것이 나은지, 검사하는 메서드를 기준으로 네이밍을 하는 것이 나은지 궁금합니다. 예를 들어, 다음과 같은 메서드가 있습니다.

isBlankIncluded(array) {
    alert(메시지를 상수처리할 부분)
    Return;
}

alert 메시지가 “공백은 포함될 수 없습니다“인 경우, 메시지를 기준으로 BLANK_IS_FORBIDDEN라고 하는 것이 나은가요, 메서드를 기준으로 BLANK_INCLUDED라고 하는 게 나은가요? 현업에서는 어떤 방식을 더 선호하고, 많이 사용하는지 궁금합니다.

그럼 2단계 미션 피드백도 잘 부탁드립니다! 🧚🏼‍♀️🧚🏼‍♂️

…에 있는 지 확인한다' 추가

wow9144 · 2021-02-27T11:24:59Z

질문에 대한 답변

명시적으로 값을 초기화 해주는게 경우에 따라 좋을 수도 안좋을 수도 있습니다. 지금과 같은 경우는 하나의 객체를 돌려쓰고 있고 게임을 다시 시작한다는 명시적인 과정이 있기 때문에 그 시점에 리셋을 해주는게 좋을 것 같습니다. 새 게임이 시작된다면 기존 객체가 아닌 새로운 객체로 시작을 한다면 문제가 없을테지만, 기존 객체를 돌려쓰는데 어느 시점에 값을 덮어쓰기 때문에 괜찮다는 것은 다시 말해 덮어쓰기 전 어느 시점에 접근 하는 로직이 새로 들어온다면 안괜찮을 수도 있다는 말이 되기도 합니다. :) 그래서 버그의 가능성을 줄이기 위해서라도 리셋을 해주는게 좋을 것 같습니다. 또는 값을 덮어쓰는 방식이 아닌, 매번 새로운 객체를 반환하도록 이뮤터블하게 코드를 전개해가는 방법도 있습니다.
메시지를 기준으로 네이밍 하는게 좋습니다. 전혀 다른 메서드에서도 해당 메시지를 사용하게 될 수도 있기 때문이죠.
그렇다고 너무 메시지와 동일하게 구체적으로 이름을 지어버리면 메시지만 바꿔도 될 것을 이름까지 바꿔야 할 수 있으니 이름에서 구현에 따라 달라질만한 구체적인것은 최대한 배제하는게 좋습니다.
예를 들면 화면에 input 이 있고 "입력된 값이 유효하지 않습니다. 올바른 값을 입력해주세요." 라는 메시지가 있었다고 가정 할 때 이름을 INVALID_INPUT_VALUE 라고 했는데 나중에 이 input의 디자인이 select 박스로 바뀌게 되면 메시지를 바꿔야 하는데 메시지를 바꿔놓고 보니 이름도 바꿔야 할꺼 같고 뭐 그렇게 되는것 입니다. - -;
이럴 때는 기존의 메시지를 바꾸는게 맞는것인지 또 다른 상수를 만들어야 하는게 맞는것인지는 물론 상황에 맞게 또 판단을 해야겠지만... 여튼 그렇습니다. :)
결론은 메서드가 아닌 메시지입니다. (얼럿 메시지는 자신을 호출하는 메서드를 모른다는 관점에서 생각해보시면 이해하기에 좀 더 쉽습니다.)

wow9144

Result를 중심으로 개선해야 할 부분들이 있을 것 같습니다. 세부 코멘트 확인 부탁드립니다 :)
차근차근 잘 나아가고 계십니다! 지금 처럼 계속 화이팅!

wow9144 · 2021-02-27T11:46:51Z

src/js/Model/Result.js

+    });
+
+    this.matchingCounts = tmpMatchingCounts;
+    $(ELEMENT.WIN_NUMBER_CONTAINER).dataset.totalPrize = totalPrize;


Model은 View 를 알지 못합니다. DOM에 접근 하는 코드가 존재해서는 안됩니다.

넵 따로 dataset을 사용하지 않고 receiptView에서 submitController에 totalPrize를 요청, submitController에서는 Result에서 리턴한 totalPrize값을 받아와서 receiptView에 전달하는 방식으로 변경해보았습니다.

뷰가 모델로부터 데이터를 받을 때에는 반드시 컨트롤러에서 받아야 한다는 것을 염두에 두면서 바꾸려니 쉽지 않았습니다ㅠㅠ

사실, totalPrize를 Result에 저장하면 가장 편할 것 같은데 총 수익금은 수익률을 계산하는 과정에서만 사용되고 직접 렌더 되거나 하는 일은 없기 때문에 저장까지는 불필요할 것 같아서 저장하지 않았는데 아직도 맞는 판단인지 잘 모르겠습니다.

저장해야 하는 정보와 그렇지 않은 정보를 어떤 기준으로 정해야 할까요?

wow9144 · 2021-02-27T12:48:43Z

src/js/Model/Result.js

+} from "../Util/constants.js";
+import { $ } from "../Util/querySelector.js";
+
+class Result {


이 클래스를 보고 전체 코드를 다시 훑어보게 되었습니다.
그리고 문제를 하나 찾았는데, 대부분의 메서드를 void형으로 작성하고 계신다는 점이었습니다.
객체가 자신의 상태를 바꾸는 메서드를 공개해놓고 메서드로 요청이 들어오면 그 요청에 따라 자신이 관리하는 상태가 변경되어야 한다면 자신의 상태를 바꾸고 요청에 맞는 결과값을 응답으로 리턴해줘야 하는곳에서는 리턴을 해줘야 하는데 리턴을 하지 않고 상태만 바꾸고 전혀 다른곳에서 이 상태를 가져다 쓰는 식으로 코드가 전개되는 경향을 발견했습니다.
이런 패턴으로 코드가 전개되면, 지속적으로 상태가 변경되기 때문에 저장해둬야 하는 값이 아니라 일시적으로 계산만 하고 결과를 반환해도 되는 값 조차 전부 내부 상태로 두는식으로 코드가 작성되기 때문에 사용되지도 않는 불필요한 프로퍼티가 생길 수 있습니다.
또한 버그 발생시 디버깅이 힘들어집니다.
이 값이 나오면 안되는 값인데 어디서 이 값이 세팅 되었는지 추적하기 힘들 수 있습니다.

따라서 불필요한 프로퍼티가 있지는 않은지, 메서드의 시그니처(파라미터와 리턴 타입)가 적절한지 한 번 살펴보시면 좋을 것 같습니다.

winning, rank 등을 계산하는 책임을 TicketBundle에 두지 않고(몰아두셨던 분들이 많았었는데) 따로 빼신 부분은 잘 하신 부분입니다.
이름만 Result 라는 너무 추상적인 이름보다는 조금 더 구체적이면 더 좋지 않을까 싶습니다. :)

피드백을 반영하기 위해 흐름을 따라가면서, 리턴 없이 저장만 하고 필요한 곳에서 모델을 찾아 가져다 쓰는 상태가 얼마나 디버깅에 취약할 지 짐작이 되었습니다..!

적절히 리턴하고 필요한 곳에서 파라미터로 받아서 사용하도록 바꾸다보니 Result 모델에 있던 ranks도 저장할 필요가 없었던 값인 것을 발견했습니다.

Result모델의 이름은 WinningResult로 변경했습니다!

wow9144 · 2021-02-27T12:50:41Z

src/js/Util/constants.js

 export const ALERT_MESSAGE = {
  INVALID_NUMBER: "문자 및 공백은 입력 불가능합니다.",
-  INVALID_RANGE: "1000원 이상, 5000원 이하만 입력 가능합니다.",
+  INVALID_MONEY_RANGE: "1000원 이상, 5000원 이하만 입력 가능합니다.",


ONE_TICKET_PRICE, MAX_PURCHASE_PRICE 가 변경 되었을 때 지금은 여기도 같이 직접 변경해줘야 합니다.
휴먼 에러가 나기 쉬운 지점이므로 여기서도 ONE_TICKET_PRICE, MAX_PURCHASE_PRICE 를 쓰도록 개선하는게 좋아보입니다.

alert message내부에서도 상수를 쓰면 좋군요!!! 바로 적용했습니다 :)

wow9144 · 2021-02-27T13:13:10Z

src/js/Controller/submitController.js

+  closeModal,
+} from "../Handler/elementHandler.js";
 import TicketBundle from "../Model/TicketBundle.js";
+import Result from "../Model/Result.js";


import 해온 Result가 클래스 또는 생성자 함수가 아니므로(export 하는 쪽에서 객체를 생성해서 넘겨줬기 때문) Result가 아닌 result 가 더 좋을 것 같습니다.
아래 Result를 사용하는 쪽에서 관련 설명 이어가겠습니다.

TicketBundle도 동일하게 export하는 쪽에서 객체를 생성해서 넘겨주는 방식을 사용했기 때문에 import ticketBundle하여 사용하는 것으로 변경했습니다 :)

wow9144 · 2021-02-27T13:15:27Z

src/js/Controller/submitController.js

+};
+
+const setNumbers = (winningNumbers, bonusNumber) => {
+  Result.setWinningNumbers(winningNumbers);


Result.setWinningNumbers() 는 Result 가 클래스이고 setWinningNumbers 는 스태틱 메서드로 생각되게 합니다.
근데 스태틱 메서드가 아니므로 Result가 아니라 result 가 되어야 합니다.
아래 Result 로 쓰고 있는곳 전부 이에 해당됩니다.

아직 클래스 사용에 미숙해서 export default new Result();를 사용하면 클래스를 export하는 동시에 생성할 수 있다는 정도만 알고, export한 이후에 제대로 사용하는 방법을 몰랐던 것 같습니다.

export로 내보낸 Result를 import result from "../Model/Result.js"로 import하면 import한 이름인 result로 사용할 수 있고, 여러 곳에서 import하더라도 동일한 하나의 인스턴스를 참조한다는 것을 공부하게 되었습니다 :)

wow9144

코멘트 드린 부분은 3단계로 넘어가서 고민 하셔도 될 것 같습니다.
화이팅!!

wow9144 · 2021-02-28T12:54:02Z

src/js/View/receiptView.js

-export const printWinningResult = () => {
+export const printWinningResult = (matchingCounts) => {
  const winningCounts = $$(ELEMENT.WINNING_COUNT);
  const totalPrize = getTotalPrize();


MVC 패턴에서 View와 Model은 Controller를 모릅니다.
따라서 View가 Controller의 기능에 의존하면 안되고 Controller로 부터 주입을 받아야 합니다.
getTotalPrize를 직접 호출 하지 말고 필요한 값은 파라미터로 받아서 그리기만 해야 합니다.

의존의 의미를 자의적으로 해석했던 것 같습니다. 직접 호출하지 않고 파라미터로 받아서 그릴 수 있도록 변경했습니다 :)

wow9144 · 2021-02-28T12:55:29Z

src/js/View/receiptView.js

 import { ELEMENT } from "../Util/constants.js";
 import { $, $$ } from "../Util/querySelector.js";
-import result from "../Model/Result.js";
 import { getTotalPrize } from "../Controller/submitController.js";


View 파일에서 Controller 파일을 import 하면 의존의 방향이 잘못된 것이라고 볼 수 있습니다.

View에서 이 함수 하나만 import 해서 사용하고 있는 것이 어색하다고 생각하기는 했는데, 역시 잘못된 거였네요. 앞으로 import로 의존의 방향을 한 번 더 체크해볼 수 있을 것 같습니다 :) 감사합니다.

wow9144 · 2021-02-28T12:58:46Z

src/js/View/receiptView.js

  const winningCounts = $$(ELEMENT.WINNING_COUNT);
  const totalPrize = getTotalPrize();
  const money = Number($(ELEMENT.TICKET_IMAGE_NUMBER_CONTAINER).dataset.money);
  const earningRate = ((totalPrize - money) / money) * 100;


이런 계산 로직은 View에서 직접 계산 하기보다는 외부에서 계산되어 주입되는게 좋습니다.

넵 외부에서 계산해서 주입하는 방식으로 변경했습니다 :)

wow9144 · 2021-02-28T13:03:54Z

src/js/Model/WinningResult.js

+
+  setRanks(ticketBundle) {
+    let ranks = [];
+    ranks = ticketBundle.map((ticket) => {


const ranks = ticketBundle.map(....
하셔도 되셨을것 같습니다. :)

앗 빠르게 수정했습니다 :)

wow9144 · 2021-02-28T13:15:34Z

src/js/Model/WinningResult.js

+    this.matchingCounts = [];
+  }
+
+  initializeWinningResult() {


constructor 에서 초기화했던 프로퍼티들을 지금처럼 프로퍼티 전체를 다시 초기화 하는 형태의 메서드가 필요하게 되면 십중팔구 이 메서드가 필요했던것이 아니라 처음부터 객체를 새로 생성해서 썼어야 하는것일겁니다.
(하나를 추가하는데 매번 두 곳 이상의 코드가 똑같이 수정되면 이는 중복으로써 처읍부터 하나였어야 할 확률이 매우 높습니다.)
다시 말해 WinningResult 에서 필요한 프로퍼티가 하나 늘어날 때 마다 constructor와 initializeWinningResult 두 곳을 똑같이 수정해야 한다면 이는 중복이고 관리의 어려움이 따르므로 제거해야 할 대상이 됩니다.

SubmitController를 클래스형으로 변환해서 모든 모델을의 객체를 새로 생성하는 방식으로 수정해보았는데... 이 구조가 괜찮은 구조인지는 잘 모르겠습니다ㅠㅠ 3차 PR에서 다시 한 번 피드백 부탁드립니다.

wow9144 · 2021-02-28T13:17:07Z

src/js/Model/WinningResult.js

+  }
+
+  setMatchingCounts(ranks) {
+    const rankInfo = [


calculateTotalPrize 의 rankInfo 와 중복으로 보입니다.
선언은 한군데서 하고 양쪽에서 가져와 쓰는걸로 변경 하시면 좋을 것 같습니다.

네 수정해보았습니다 :)

wow9144 · 2021-02-28T13:25:12Z

src/js/Util/validator.js

  return !/^[0-9]+$/.test(money);
 };
+
+export const isValidWinningNumbers = (array) => {


파라미터의 이름을 array로 쓰는것은 아무 정보를 전달하지 못하기 때문에 좀 더 의미 있는 이름을 써주시는게 좋습니다.
array가 중요한게 아니라 어떤 array인지가 중요합니다.
배열이라면 복수형으로 이름을 지어주시면 되고 이경우에는 winningNumbers의 유효성을 검사하는 것이기 때문에 array 대신 winningNumbers 가 더 적절하고, 함수 이름에 이미 winning 이 들어가 있으니 정보의 번복을 피하는 차원에서 numbers 정도로 해주셔도 괜찮습니다.
값이 만약 숫자가 아닌 문자열로 들어올 수도 있고 내부에서 문자면 숫자로 변환하는 등의 로직이 있을 때는 numbers 보다는 values 정도로 해주면 좀 더 미묘한 차이의 의도까지 전달 할 수 있습니다.

3단계 기능이 추가되면서, 이 함수로 기존의 검증 대상이던 당첨 번호들 외에도 수동구매로 입력한 로또 번호들selfPurchaseLottoNumbers까지 검증을 하게 되었습니다.

함수의 이름은 isValidWinningNumbers에서 isValidNumbers로 변경했고, 파라미터의 이름도 Numbers외에 공통점을 아우르는 더 좋은 이름이 떠오르지 않네요...

여러 곳에서 사용하는 검증 함수의 경우 이렇게 의미가 생략되어도 괜찮을까요?

shinsehantan · 2021-03-23T03:01:34Z

🧚🏼‍♀️🧚 레벨 1의 마지막 미션을 진행하고 있는 시점에 남겨보는 행운의 로또 미션 셀프 리뷰 🧚🏼‍♀️🧚

내가 생각했을 때 바보같지만 나눠보고 싶은 질문

MVC 패턴에서 MVC의 명확한 역할, 각각이 알아야 할 정보와 알면 안 되는 정보(이걸 진짜 아직도 잘 모르겠다..........)
util 폴더에 있어야 할 파일과 그렇지 않은 파일 (constants랑 추상화되지 않은 validator가 util에 있어도 되나?)

그때는 몰랐지만 지금은 알 수 있는 것

if문을 대체하는 방법
오브젝트 리터럴 사용법
object freeze와 deep freeze

그때도 몰랐고 아직도 잘 모르겠는 것

웹 접근성에 대해 아직 전혀 모른다.
상태를 사용해야 하는 메서드가 있을 때, 직접 로컬 스토리지에서 가져와서 사용하면 안 되는지? 어떤 시점에 파라미터로 넘겨받는 것이 좋은지?

반복되고 있는 실수 및 습관이 있다면?

3단계까지 고려해서 구조를 어느 정도 그려보고 시작해야 하는데, 항상 1단계 기능까지만 고려하고 시작해서 중간에 구조를 뒤엎게 된다.

기존에 작성하던 방식에서 코드를 단순히 가져와서 사용한 경우는 없는지?

코드는 아니지만 eslint나 pritter 등 초기 설정을 페어가 설정하거나 이전에 사용하던 설정을 그대로 사용했고, 어떤 속성이 어떤 역할을 하는지 전혀 알려고 하지 않았다.
querySelector 유틸 함수 만들어둔 뒤로 추가적인 고민없이 계속 복붙해서 쓰고있다.

확실하게 말할 수 있는 발전한 1가지

for 문은 더 이상 사용하지 않는다.

다시 로또 미션 코드를 작성한다면 어떻게 작성할 것인가?

상태값의 변경은 무조건 상태값을 변경하는 역할을 하는 메서드를 통해서만 변경할 것이다.
SubmitController의 책임을 나눠볼 것이다. (3단계 안 끝나서 진짜 해야함^^)

갈 길이 멀다 신세한탄... 🤦🏻‍♀️

shinsehantan and others added 15 commits February 26, 2021 15:58

docs : step2 구현할 기능 목록 작성

25e1e28

feat : 당첨 번호와 보너스 번호 입력 기능 구현

065f24a

feat : 당첨 번호와 보너스 번호 입력 값을 검증하는 기능 구현

c652a31

test : '당첨 번호, 보너스 번호 입력 검증을 할 수 있다.' 추가

cfeed94

test : 상수 처리 에러 수정

69ade71

feat : 티켓별 숫자 일치 개수 및 전체 당첨 개수 저장하는 기능 구현

8f36ec6

docs : README.md 수정

664a0ac

feat : 모달창에 당첨 개수, 수익률을 반영하는 기능 구현

63048c7

test : '결과 확인 버튼을 누르면 모달창이 나타난다' 추가

3fa2e62

refactor : 상수 처리

989e3f3

feat : 모달창을 닫는 기능 구현

d10512c

feat : 다시 시작하는 기능 구현

a1fe75d

test : '다시 시작하기 버튼을 누르면 초기화 돼서 다시 구매를 시작할 수 있다'와 '생성된 번호가 1부터 45 범위 안…

6546e19

…에 있는 지 확인한다' 추가

refactor : 테스트 케이스 상수 처리

917352d

refactor : 주석 삭제 및 함수명 변경

a27a62e

pocojang requested a review from wow9144 February 26, 2021 15:23

pocojang assigned wow9144 Feb 26, 2021

wow9144 suggested changes Feb 27, 2021

View reviewed changes

shinsehantan added 8 commits February 28, 2021 00:17

fix : Result 사용을 result로 변경

95dc095

fix : TicketBundle 사용을 ticketBundle 로 변경

95f2b6e

refactor : alert 메시지 내부 숫자 상수 처리

daeeb86

refactor : Model에서 DOM에 접근하지 않도록 로직 수정

f3a7b2e

refactor : void 형식 메서드를 return 하는 메서드로 변경 및 모델 이름 변경

f1c2bac

refactor : alert 메시지 리네이밍

4abc68a

feat : 다시 시작할 때 모델 초기화하는 기능 추가

8aa1756

refactor : cypress test 주소 변경

b864ef3

wow9144 approved these changes Feb 28, 2021

View reviewed changes

wow9144 merged commit 9c96a94 into woowacourse:shinsehantan Feb 28, 2021

Conversation

shinsehantan commented Feb 26, 2021

Uh oh!

wow9144 commented Feb 27, 2021

Uh oh!

wow9144 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shinsehantan Feb 27, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shinsehantan Feb 27, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wow9144 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shinsehantan commented Mar 23, 2021

내가 생각했을 때 바보같지만 나눠보고 싶은 질문

그때는 몰랐지만 지금은 알 수 있는 것

그때도 몰랐고 아직도 잘 모르겠는 것

반복되고 있는 실수 및 습관이 있다면?

기존에 작성하던 방식에서 코드를 단순히 가져와서 사용한 경우는 없는지?

확실하게 말할 수 있는 발전한 1가지

다시 로또 미션 코드를 작성한다면 어떻게 작성할 것인가?

갈 길이 멀다 신세한탄... 🤦🏻‍♀️

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

shinsehantan Feb 27, 2021 •

edited

Loading

shinsehantan Feb 27, 2021 •

edited

Loading