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

shinsehantan · 2021-02-18T04:55:35Z

안녕하세요. 리뷰어님 :)
행운의 로또 미션 1단계 리뷰 요청드립니다.
로또 미션 데모 페이지입니다.

지난 미션 때, MVC 패턴에 대한 이해없이 맹목적으로 사용했기 때문에 이번 미션 때에는 필요하지 않다면 사용하지 말아야겠다 생각했지만, 알고있는 다른 디자인 패턴이 없고 초기에 패턴을 잡지 않고 시작하려니 그것 역시 막막하여 결국 MVC 패턴을 사용하게 되었습니다. 각 미션마다 적합한 디자인 패턴을 찾기 위해서는 역시 다른 사람들의 작업을 많이 찾아보면서 느끼는 방법밖에 없을까요?
페이지를 실행시키면 reciept-container가 처음부터 숨겨진 상태여야 하기 때문에 classList에 hidden을 추가함으로써 보여짐 여부를 컨트롤 하기로 했습니다. 그런데, index.js에 미리 주어져있던 onModalShow와 onModalClose 함수는 classList에 open을 추가하는 형태를 사용하고 있었습니다. 통일성을 위해서는 reciept-container 역시 open이나 show를 쓰면 좋겠지만 페이지가 로드 되자마자 show를 add하고, 바로 remove하는 작업이 다소 불필요하다고 느껴져서 통일성을 포기하기로 했는데, 잘 선택한 것이 맞을까요?
로또에서 생성된 번호가 1-45 범위 안에 있는지 확인하기 위해 클래스 메서드를 cypress에서 불러오려고 했는데 잘 되지 않았습니다. cypress 테스트 코드에서 클래스의 메서드를 사용할 수 없나요? (ex. Ticket.generateRandomNumbers)
#ticket-image-number-container의 classList에 flex-col이 없는 상태에서 classList.remove('flex-col')을 했을 때 오류가 발생하지 않았습니다. 없는 클래스를 삭제하려고 할 때 오류가 나지 않은 것이 이상한데, 제가 놓친 부분이 있을까요?
(없어도 오류는 나지 않지만, 논리적으로 이상한 것 같아 index.html에서 #ticket-image-number-container의 classList에 flex-col을 추가해둔 상태입니다.)
1000원을 의미하는 '1000'의 변수를 티켓 한 장의 가격의 의미로 쓰는 경우도 있고, 구매 시 지불해야하는 최소 금액의 의미로 쓰는 경우도 있는데, 이 경우에는 하나의 변수에 상수를 두 개 만드는 것이 맞을까요? (ONE_TICKET_PRICE: 1000, MIN_PURCHASE_PRICE: 1000)

페어와 함께 미션을 진행하며 궁금했던 부분을 정리해보았는데,
피드백과 함께 답변주시면 감사하겠습니다 :)

wow9144

안녕하세요.
자세한 내용은 각 코멘트에 남겼으니 참고 부탁드립니다.

질답

View 가 포함된 설계 패턴은 MVC, MVVM, MVP 등 다양합니다. 디자인 패턴은 이런 큰 틀의 설계 뿐만 아니라 객체와 객체의 의존 관계를 어떻게 풀어내는지, 객체 생성은 어떻게 하는지 등 보다 더 작은 코드를 작성 할 때의 패턴도 아우르는 고유명사 처럼 사용됩니다. 그래서 디자인 패턴은 찾아보는게 아니고 문제 해결을 보다 더 쉽게 하는 패턴 이기에 여러가지 문제 상황을 직접 경험하고 해결해나가는 시도가 우선입니다. 문제를 겪지 않고 문제를 해결하는 디자인 패턴부터 관심을 갖으면 공부를 해도 코드에 적용을 못해서 계속 겉돌고 까먹고 잘못 적용하고 하는 경우가 많으니 일단 뭔가를 만들어보고 요구사항이 바뀌는 상황에 많이 노출 되어야 합니다. 그럼 요구사항이 바뀌었을 때 '아 그 때 이렇게 코드를 작성하면 안되는것이었구나' 를 직접 느끼는 시간이 필요합니다. 그러니 말씀하신것처럼 많이 찾아보는것 보다는 직접 작성하고 완성하고 요구사항이 바뀌고 수정하고 같은 싸이클을 빠르게 도는게 더 좋습니다.
세부 코멘트에 따로 작성했습니다.
cypress 를 사용해보지 않아서 제공하는 API가 있는지 여부는 잘 모르겠습니다. 다른 분들 코드를 참고하시거나 문서를 찾아보셔야 할 것 같습니다.
메서드에 어떤 요청을 했을 때 적절히 처리 하지 못하면 에러를 내뱉도록 만드는 경우도 있고 그냥 조용히 무시되도록 만드는 경우도 있습니다. 이 경우는 후자라고 생각하시면 됩니다. 돈이 없는데 로또를 달라고 하면 에러를 뱉어야 하지만 안보이는걸 안보이게 해달라고 하면 이미 안보인다고 에러를 내뱉기 보다는 조용히 무시하는게 메서드를 사용하는 사용자도 편합니다.
세부 코멘트에 따로 작성했습니다.

wow9144 · 2021-02-21T04:32:31Z

src/js/Model/Ticket.js

+
+
+  generateRandomNumbers() {
+    const randomNumbers = Array.from({ length: Number.LOTTO_MAX_NUMBER }, (_, i) => i + 1);


이 시점의 이 변수는 1~45까지의 숫자를 가진 배열이기 때문에 randomNumbers는 적절한 이름이라고 보기 어렵습니다.
메서드 이름에서 generateRandomNumbers 로 이미 의도가 명확히 표현 되었기 때문에 내부에서는 그저 numbers 로 해도 충분 할 것 같습니다.

generateRandomNumbers로 생성해서 별 생각없이 randomNumbers로 이름 지었는데, 생각해보니 이 시점에서는 이미 의미를 가진 배열이 되어서 numbers로 해도 괜찮을 것 같네요! 감사합니다.

wow9144 · 2021-02-21T04:51:34Z

src/js/Util/constants.js

+  ONE_TICKET_PRICE: 1000,
+  LOTTO_MAX_NUMBER: 45,
+  TICKET_NUMBER_LENGTH: 6,
+  MIN_PURCHASE_PRICE: 1000,


질문 5에 대한 답변입니다.

최소 한게임만 구입 가능하기 때문에 이 경우에 굳이 MIN_PURCHASE_PRICE이 존재 할 필요는 없긴 합니다.
그러나 의미상 2개로 나누는게 필요할 경우가 생긴다면
MIN_PURCHASE_PRICE = this.ONE_TICKET_PRICE
하시면 하나의 값을 2개의 의미로 한 곳에서 관리 할 수 있습니다.
만약 최소 구입 가격이 변경 된다면 그 때 값을 직접 할당하여 서로 연관을 끊어주시면 됩니다.

저희는 constants.js를 클래스로 관리하고 있지 않아서
MIN_PURCHASE_PRICE = this.ONE_TICKET_PRICE를 사용하기는 어려웠습니다.
this를 쓰지 않고도 하나의 값을 2개의 의미로 관리할 수 있는 방법이 있을까요?

const ONE_TICKET_PRICE = 1000 export let STANDARD_NUMBER = { ONE_TICKET_PRICE: ONE_TICKET_PRICE, MIN_PURCHASE_PRICE: ONE_TICKET_PRICE };

으로 하시면 될 것 같습니다.

let을 사용하면 되는군요! 적용했습니다. 감사합니다 :)

wow9144 · 2021-02-21T06:50:42Z

src/js/index.js

+class App {
+  constructor() {
+    this.intializeTickets();
+    onPurchaseResultHidden();


이런 경우라면 html/css 에서 디폴트로 아예 숨겨놓고, 보여줘야 할 때 js로 보여줘야 합니다.

질문 2번의 답변이 되기도 합니다.
화면을 조작 할 기능을 js에서 제공하고 있다고 하여 화면의 이 기능으로만 화면을 컨트롤 해야 할 필요는 없습니다.
어쨌든 화면은 최초의 상태를 가지고 그려질 것이니까요.
그래서 최초에 안보여야 하는것은 CSS를 이용하여 안보이는 상태로 그리면 됩니다.

네! 최초로 보여지는 상황에 맞추어 index.html에 hidden을 넣어두고 필요할 때 remove해서 보여주는 것으로 수정했습니다.
그리고 존재하지만, 보여지는 부분에서만 숨김 처리하는 visibility hidden보다 입력 전에 dom element가 노출되지 않게 하는 display none이 더 적합할 것 같아 css는 display none으로 변경하였습니다 :)

wow9144 · 2021-02-21T06:57:47Z

src/js/index.js

-const onModalShow = () => {
-  $modal.classList.add('open')
-}
+class App {


App의 역할이 무엇인가 고민해보시고 그 역할에 맞는 일을 App 이 하고 있는지 검토해보시면 좋을 것 같습니다.
App은 전체 프로그램을 구동시키는 역할을 하기에 컨트롤러 뷰 모델을 생성하고 구동하는 역할이지 않을까 싶습니다.
그래서 컨트롤러에게 요청 해야 할 것은 요청해야 하는데 App 이 컨트롤러와 뷰의 일까지 직접하고 있는것으로 보입니다.
tickets 와 ticketCount 로 별도로 관리할 필요 없이 tickets 와 tickets.length 로 할 수 있을 것 같습니다.
그러나 이 상태들이 App이 알고 있어야 하는 정보는 아닌것으로 생각됩니다.
다른 옮길 더 적절한 곳이 있을지 고민해보시면 좋을 것 같습니다.

App의 역할에 대해 다시 고민해보는 계기가 되었습니다.
App은 초기에 전체 프로그램을 구동시키는 역할만 할 수 있도록 기능을 덜어낸 뒤 Model의 구조 또한 함께 바꿔보았습니다.

wow9144 · 2021-02-21T06:59:52Z

src/js/Controller/submitController.js

+} from "../View/receiptView.js";
+import { onPurchaseResultShow } from "./viewController.js";
+
+export const handlePurchaseAmountSubmit = () => {


이름에 비해 너무 많은 일을 하고 있는것 같습니다.
내부의 책임들을 적절히 분산시킬 필요가 있을 것 같습니다.
input에 입력한 금액을 다룬다는 이름이지만, 실제로는 로또를 발행하는 기능까지 이 안에 다 들어있는것으로 보입니다.

기존에 담당했던 입력한 금액에 대한 검증, 티켓 생성, 티켓 렌더 기능 중 일부를 분리하여
입력한 금액에 대한 검증, publishTickets(티켓을 생성하고 렌더)함수를 호출하는 일까지만 담당하도록 변경했습니다. :)

wow9144 · 2021-02-21T07:03:25Z

src/js/View/receiptView.js

+  const ticketImageNumberContainer = $(Element.TICKET_IMAGE_NUMBER_CONTAINER);
+  let ticketImageNumberTemplate = "";
+
+  tickets.map((lotto) => {


Array의 메서드인 forEach와 map 을 언제 어떻게 쓰는지 추가적인 학습이 필요할 것 같습니다.

forEach 메서드는 단순히 반복문을 대체하기 위해 사용하는 함수이고, map 메서드는 요소값을 다른 값으로 매핑하여 새로운 배열을 생성하기 위해 사용하는 함수이므로 이 부분에서는 단순 반복을 위한 forEach 사용이 더 적절했던 것 같습니다.

wow9144

코드가 diff 없이 거의 다 새로 만들어지고 주요 모델의 설계를 뒤흔드는 변화가 있는것으로 보여서 히스토리 파악이 힘들었습니다. ㅠㅜ 세부 코멘트 확인 부탁드립니다.

wow9144 · 2021-02-23T12:51:34Z

src/js/Util/constants.js

+  ONE_TICKET_PRICE: 1000,
+  LOTTO_MAX_NUMBER: 45,
+  TICKET_NUMBER_LENGTH: 6,
+  MIN_PURCHASE_PRICE: 1000,


const ONE_TICKET_PRICE = 1000 export let STANDARD_NUMBER = { ONE_TICKET_PRICE: ONE_TICKET_PRICE, MIN_PURCHASE_PRICE: ONE_TICKET_PRICE };

으로 하시면 될 것 같습니다.

wow9144 · 2021-02-23T13:58:14Z

src/js/Model/Ticket.js

@@ -0,0 +1,20 @@
+import { STANDARD_NUMBER } from "../Util/constants.js";
+
+export default class Ticket {


사용되지 않는 파일은 지우는게 좋습니다.
그리고 동일한 TicketBundle이라는 새파일을 만드시는것보다 이 파일을 수정하시는게 좋았을 것 같습니다.

Ticket 모델 삭제를 했어야 하는데ㅠㅠㅠ 혼란드려서 죄송합니다.

wow9144 · 2021-02-23T14:18:10Z

src/js/Model/TicketBundle.js

+    this.ticketBundle = [];
+  }
+
+  makeTicketBundle(ticketLength) {


이전 코드에서 Ticket이 등장하여 로또용지인가보다 하고 별 코멘트 없이 넘어갔었는데,
makeTicketBundle이 등장하면서 제가 잘못 생각한듯 느껴집니다. (객체를 생성해서 export 한것도)
모델이 이렇게 바뀌어 코드 전체가 바뀐듯 한 느낌인데 코드만 보고는 의도를 알기 힘들어서 의견을 먼저 여쭤봅니다.

Ticket의 역할이 무엇인가요?

Ticket이 ticketBundle을 가지고 있는것이 어떤 의도인가요? (Ticket이 여러개 있는것이 ticketBundle일것으로 생각되는데, 그렇다면 ticketBundle = [ticket, ticket, ticket..] 일텐데 ticket이 ticketBundle을 가지고 있음으로써 ticket이 무엇인지 모르겠습니다.)

new Ticket()하여 하나의 객체를 export 하고 있는데, 하나만 존재해야 하는 이유는 무엇인가요?

Ticket 모델의 이름은 TicketBundle이 더 적절한 것으로 생각되어 TicketBundle로 수정했습니다. 동일한 의미인데 파일/클래스명이 TicketBundle이면 어색하지 않나 싶어서 두 이름을 다르게 만든 것이 혼란을 야기한 것 같습니다😢

TicketBundle 클래스는 메서드로 각 티켓의 숫자를 만드는 generateRandomNumbers와 ticket의 묶음인 ticketBundle을 만드는 makeTicketBundle을 가지고 있으며, ticketBundle의 상태를 관리합니다.

이 부분은 이해하셨던 것이 맞는데 역시 네이밍 때문에 혼란을 드린 것 같습니다.

TicketBundle은 ticket들의 묶음이라 한 게임당 1개만 필요하기 때문에 여러 곳에서 생성해서 사용하지 않아도 된다고 생각해 export default new TicketBundle()을 사용했습니다. 이것 역시 네이밍에서 비롯된 문제일 수 있겠네요ㅠㅠ

wow9144

설명해주신 의도부분이 충분히 이해되었습니다. :)
2단계로 넘어가서 계속 이어가보시죠!

jum0 and others added 24 commits February 16, 2021 18:26

initial commit

63f6cc2

docs : README.md 수정

d89e543

docs : README.md 수정

24cf90e

chore : eslint, prettier, cypress 설정

2ded2e3

docs : 1단계 기능 구현 목록 작성

035b29b

docs : 구현할 기능 추가

4ebd50f

test : '초기 상태에서 입력 창 아래 부분이 숨김 처리 되어 있는 것을 확인한다' 추가

fc93141

feat : 입력 창 아래에 숨김 처리하는 기능 구현

20fa710

docs : 구현한 기능 체크

8779834

docs : '문자는 입력 불가능하다' 기능 추가

8542ea9

test : '구입 금액 입력창에 숫자 입력을 할 수 있다' 추가

e460aea

feat : 구입 금액을 입력하고 검증하는 기능 구현

9aedb61

docs : '로또의 개수만큼 로또 용지 그림이 출력된다' 기능 추가

ddb7628

feat : 구입한 로또의 개수만큼 로또가 생성되고, 텍스트에 반영되는 기능 구현

98d576c

test : '구입 금액만큼 로또 용지를 출력할 수 있다' 추가

1ae1e60

feat : 로또의 개수만큼 로또 용지 그림이 출력되는 기능 구현

d9e3480

test : '구입 금액만큼 로또 용지를 출력할 수 있다' 수정

371c75a

test : '토글 버튼을 클릭하면 각 로또의 번호가 출력된다' 추가

fd66806

feat : 토글 버튼을 클릭하면 수직으로 정렬되고 각 로또의 번호들이 출력되는 기능 구현

3265b48

test : '토글 버튼을 클릭하면 각 로또의 번호가 출력된다' 수정

dbdddee

fix : 로또들이 정렬이 안되는 버그 수정

ea4ef59

refactor : 상수 처리

5ab537b

chore : README.md 가이드 이미지 추가

2c073e4

docs : 데모 페이지 추가 및 이미지 수정

93cfdc4

pocojang requested a review from wow9144 February 18, 2021 05:01

pocojang assigned wow9144 Feb 18, 2021

wow9144 reviewed Feb 21, 2021

View reviewed changes

jum0 and others added 3 commits February 22, 2021 14:23

refactor : early return 방식으로 수정

1db11e3

refactor : randomNumbers 변수명 numbers로 수정

b7829a2

git 테스트

49181d2

jum0 and others added 8 commits February 22, 2021 15:50

refactor : 함수명 및 파일 위치 변경

d2022f6

refactor : 상수 네이밍 변경

4aff052

refactor : 불필요한 함수 삭제 및 css 숨김 처리 방식 변경

311768b

refactor : 문자열 리터럴 삽입 방식 변경(내장 함수 사용)

00186b4

refactor : handlePurchaseAmountSubmit 함수 기능 분리

27a9728

refactor : map 메서드를 forEach 메서드로 변경

c75a0b3

refactor : App, Model 수정

156bbfe

refactor : 상수 그룹 네이밍 변경

5ddb0c1

wow9144 reviewed Feb 23, 2021

View reviewed changes

shinsehantan added 3 commits February 24, 2021 11:17

refactor : 상수 처리 변경

3ca6761

refactor : 사용하지 않는 파일 삭제

da88fc6

refactor : Ticket 클래스명 TicketBundle로 수정

cd2009c

wow9144 approved these changes Feb 25, 2021

View reviewed changes

wow9144 merged commit bcfe3e9 into woowacourse:shinsehantan Feb 25, 2021



		generateRandomNumbers() {
		const randomNumbers = Array.from({ length: Number.LOTTO_MAX_NUMBER }, (_, i) => i + 1);

		@@ -0,0 +1,20 @@
		import { STANDARD_NUMBER } from "../Util/constants.js";

		export default class Ticket {

Conversation

shinsehantan commented Feb 18, 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!

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!

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!

wow9144 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants