[2단계 - 행운의 로또 미션] 클레어(안은소) 미션 제출합니다.#409
Conversation
- 리팩터링 - UI 구현 - 이벤트 처리 - 동적 UI 렌더링 - 웹 표준 및 접근성
- 리뷰어 피드백 반영 (woowacourse#370 (comment))
- 기존의 view 폴더 하위에 있던 validation폴더를 상위 계층 폴더로 이동 - 기존의 ui로직에만 잇던 유효성 검증 로직을 domain 로직에도 추가
- 기존의 vlidationUtils 에서 validations로 변경 - validations을 validateDomain, validateUtils, validateUI로 분리
- 테스트 코드를 폴더 구조에 맞게 변경 - describe 를 사용하여 테스트 코드 추가
- 헤더 스타일링 - lotto-game-view 스타일링 - footer 스타일링
- css 폴더 구조 변경 - 로또 구매 섹션 디자인 구현
- 구매 수량 표시 - 로또 번호 목록 표시
- 당첨 번호 입력 폼 - 보너스 번호 입력 폼 - 결과 확인하기 버튼
- purchase-section의 height를 32px로 고정
- 당첨 결과 섹션
- 당첨 통계 테이블
- 수익률 표시
- 게임 재시작 버튼
- LottoGameState: 옵저버 패턴을 활용한 상태 관리 구현 - 로또 구매/당첨 정보 상태 관리 - 상태 변경 시 구독자에게 알림 - LottoGameView: DOM 조작 및 렌더링 로직 구현 - 로또 구매 목록 동적 렌더링 - 당첨 통계 및 수익률 표시 개선 - 상태 변경에 따른 화면 업데이트 - LottoGameController: 사용자 입력 처리 및 이벤트 핸들링 - 구매/당첨 확인/재시작 이벤트 처리 - 비즈니스 로직과 뷰 상태 동기화
- 확인에서 구입으로 수정
JUDONGHYEOK
left a comment
JUDONGHYEOK
left a comment
There was a problem hiding this comment.
안녕하세요 클레어!! 2단계도 너무너무 수고많으셨습니다!🙇🏻♂️ 1단계 리뷰가 도움이 되었다니 다행이네요😄 이번 단계에서는 옵저버 패턴을 적용해주셨네요! 옵저버 패턴이 현재 반응형 프로그래밍의 핵심이 되는만큼 프론트엔드에서는 꼭 알아야할 패턴중 하나라고 생각하는데 이렇게 직접 미션중에 필요성을 느끼고 구현해주신 모습이 인상적이였어요!!
1. test 관련
테스트가 많이 보완되었다는 것이 느껴지네요! 피드백을 잘 받아들여주셔서 감사합니다!😊 validation 관련 테스트에 대해 질문해주셨는데, 이상적으로는 각 유효성 검증 함수도 개별적으로 테스트하는 것이 좋다고 생각해요.
도메인에서 이를 테스트를 하기위해서는 잘못된 구매 금액으로 로또를 초기화하면 에러가 발생한다.와 같은 모호한 말보다는 명확한 경계값을 갖고 테스트하고 케이스를 추가하는 것이 좋아보입니다!
2. MVC패턴사용
현재는 MVC패턴이라고 하기에는 어려움이 있어요. 자세한 이야기는 코멘트에 남겨두었지만 현재 구조에서는 Controller와 View 간의 책임 경계가 약간 모호한 부분이 있어요. Controller가 DOM에 직접 접근하고, View가 상태를 직접 구독하는 구조인 것 같은데 MVC패턴과는 거리가 있는 것 같습니다.
다만 목적이 분명한 코드라고 생각이 되어서 꼭 구조를 수정할 필요는 없을 것 같아요. 만약에 이번 미션에서 꼭 MVC패턴의 장단점을 얻고자 MVC패턴을 적용해야겠다고 생각하신다면 모델의 역할을 강화하면서 Controller가 명확히 Model과 View를 중재하는 형식으로 변해야한다고 생각해요!
제 생각에는 어떤 패턴의 적용에 의의를 두기보다는 애플리케이션의 특성에 따라 지금처럼 클레어만의 구조를 적용하시고 문제점이 무엇인지 되돌아보며 추가적으로 패턴에 대해 학습하면서 적용해나가시면 더 얻어갈 수 있는 점들이 많다고 생각합니다!
3. 커밋단위
현재 클레어의 말대로 특정 커밋들이 작업단위가 크긴 한 것 같아요. 팀이나 개인에 따라 다르긴 하지만 보통 기능단위로 커밋을 진행하는 것 같아요. 예를 들어 로또의 발행기능이나 당첨 기능처럼요. 또 기능단위에서 수정사항이 너무 많다면 이를 또 구조에 따라 나눌 수도 있을 것 같습니다.
4. 리팩토링 방향성
말씀해주신 방향성은 모두 좋은 것 같습니다. 추가적으로 제가 제안드리고 싶은 것은 구조의 명확성이나 DOM접근들을 효율적으로 할 수 있는 방법에 대해 고민해보시면 좋을 것 같아요!
코드의 구조나 설계에서 클레어만의 생각을 엿볼 수 있어서 재밌게 리뷰할 수 있었던 것 같아요!😁 기능적으로는 문제없이 동작하지만 좀 더 개선할 수 있는 부분들이 남아있는 것 같아 RequestChanges하겠습니다! 너무 수고많으셨어요 클레어!!🦍
| - [x] 재시작하는 경우 구입 금액 입력부터 게임을 다시 시작한다. | ||
| - [x] 종료하는 경우 프로그램을 종료시킨다. | ||
|
|
||
| ## 2단계 기능 작업 목록 |
index.html
Outdated
| <link rel="stylesheet" href="/javascript-lotto/css/main.css" /> | ||
| <script type="module" src="/javascript-lotto/src/step2-index.js"></script> |
There was a problem hiding this comment.
여기 경로 때문에 로컬에서는 구동이 안되는 것 같은데요 확인 부탁드려도 괜찮을까요?
There was a problem hiding this comment.
헉 그렇네요 ! 배포에 신경쓰느라 로컬에서 구동이 안되는걸 미처 확인 못헀네요 ..! 수정헀습니다 (commit : bc20a35)
감사합니다 !
src/step2-index.js
Outdated
| import LottoGameController from './View/Web/LottoGameController.js'; | ||
|
|
||
| document.addEventListener('DOMContentLoaded', () => { | ||
| setTimeout(() => { |
There was a problem hiding this comment.
setTimeout()은 현재 실행 중인 스크립트가 완료된 후에 콜백 함수가 실행되도록 예약하는 것으로 알고있어요.
저는 DOM이 완전히 준비되고 모든 리소스가 로드된 상태에서 로또 게임 애플리케이션이 초기화되도록 보장하는 안전 장치로서 setTimeout을 사용했는데요, 찾아보니 DOMContentLoaded 이벤트는 이미 HTML이 완전히 로드되고 파싱된 후에 발생한다고 하네요 ...! DOM 요소들은 이미 사용 가능한 상태이니 .. setTimeout()은 지워도 될 것 같아요.
수정했습니다! (commit : 66be12a)
src/View/Web/LottoGameView.js
Outdated
|
|
||
| #initializeModalEvents() { | ||
| const modalOverlay = document.querySelector('.modal-overlay'); | ||
| const modalCancelButton = document.querySelector('.modal-cancle-container'); |
There was a problem hiding this comment.
여기 cancle오타 인것 같아요. vscode사용하시면 cspell을 이용해보시는 것은 어떨까요?
There was a problem hiding this comment.
헉헉헉 이 오타는 전혀 못봤네요 ... 오 cspell 사용해볼게요!
그런데 cspell을 사용하면 큰 파일을 열었을 때는 많이 느려질 수도 있다고 하는데용 그런 경우는 없으셨나요?(단순 궁금입니다.)
(commit : 986b639)
src/View/Web/LottoGameView.js
Outdated
| modalOverlay.style.visibility = 'hidden'; | ||
| lottoListSection.style.visibility = 'hidden'; | ||
| winningNumbersSection.style.visibility = 'hidden'; |
There was a problem hiding this comment.
직접 스타일을 조작하기보다는 css class를 사용하시는 것은 어떨까요? 또 visible, hidden이 반복되니 메소드로 추상화할 수도 있을 것 같아요
src/View/Web/LottoGameController.js
Outdated
|
|
||
| class LottoGameController { | ||
| #state; | ||
| #view; |
There was a problem hiding this comment.
여기서는 view를 전혀 사용하고 있지 않네요. 사실 지금 구조는 MVC패턴이라기보다는 클레어만의 구조인 것 같아요.(현재 패턴도 훌륭하다고 생각합니다!) view에서 state를 직접 의존하고 view는 state의 변화에 observe하는 옵저버패턴인 것 같습니다! 컨트롤러는 말그대로 view와 모델 사이의 통신을 중재하는 역할이라고 생각하는데, 현재 구조상으로는 controller에서는 단순히 엘리먼트에 event handler를 할당하고 state를 업데이트하는 로직만을 담당하고 있는 것 같아요.
제 개인적인 생각이지만 �현재 옵저버를 활용한 구조에서 꼭 Controller를 활용해야하나 싶기도 하구요. 클레어가 느끼시기에 계층이 그래도 존재하는 것이 좋겠다 싶으시면 좀 더 계층간의 의존성이나 역할을 분명히 하면 좋을 것 같다는 생각이 들어요! 굳이 MVC패턴에 생각이 휩쓸리지 않으셨으면 좋겠다는 생각입니다!
There was a problem hiding this comment.
앗 초기에 view를 사용할 줄 알고 넣어놨다가 안빼놨네요..! 해당 내용은 반영해서 수정했습니다. (commit : refactor(LottoGameController): 사용하지 않는 view 제거)
패턴에 대한 명확한 가이드라인이 제 스스로 세워지지않아서 무작정 알려진 MVC 패턴을 써야겠다는 생각으로 구현을 하다보니 이번에는 동키콩말대로 MVC 패턴이라기보다는 모호한 저만의 구조가 된 것 같아요.
동키콩의 피드백을 듣고 보니 계층간 의존성이나 역할이 좀더 명확하게 분리되면 좋을 것 같다는 생각이 드네요.!
| #numbers; | ||
|
|
||
| constructor(numbers) { | ||
| validateLottoNumbers(numbers); |
There was a problem hiding this comment.
오 validate로직이 이동하셨군요. 개인적으로 도메인에 종속된 validation이라면 이 흐름이 더 자연스럽다고 생각해요!
eslint.config.js
Outdated
| 'import/prefer-default-export': 'off', | ||
| 'import/extensions': 'off', |
There was a problem hiding this comment.
사실 페어의 eslint.config.js를 가져온 내용이라, 왜 사용하는지 스스로 명확히 몰라서 찾아봤습니다!
-
import/prefer-default-export:
- 이 규칙은 모듈에서 단일 export만 있을 경우 default export를 사용하도록 강제하는 규칙입니다.
- 'off'로 설정시 장점 :
- 나중에 추가 export가 필요할 때 코드 수정이 더 적게 필요합니다
- TypeScript와 같이 사용할 때 named export가 더 타입 안전합니다
-
import/extensions:
- 이 규칙은 import 문에서 파일 확장자(.js, .jsx 등)를 명시적으로 포함하도록 강제하는 규칙입니다
이렇게 정리했을때, import/prefer-default-export는 off여도 크게 상관없을 것 같지만, import/extensions는 off로 명시할 필요가 없다고 생각되네요!
수정하여 반영하였습니다! (commit : chore: 사용하지 않는 eslint 제거)
| sourceType: 'module', | ||
| globals: { | ||
| ...globals.node, | ||
| ...globals.jest, |
There was a problem hiding this comment.
이제 웹환경도 추가되었으므로 browser도 추가하시는 것은 어떠실까요??
There was a problem hiding this comment.
오앗 이건 로컬에서 추가하고 커밋을 안했네요 ..! 언급해주셔서 감사합니다!
수정했습니다! (commit : beb4b96)
src/Domain/LottoService.js
Outdated
| class LottoService { | ||
| static initializeLotto(purchaseAmount) { | ||
| validatePurchaseAmount(purchaseAmount); | ||
| const lottoManager = new LottoManager(); |
There was a problem hiding this comment.
늦은 피드백이지만 LottoService를 static메소드로 구성할 이유가 있는지 궁금해요! LottoManager를 필드로 활용해서 메소드 파라미터의 수를 줄여도 좋을 것 같고 WinningLotto에 대한 의존성을 위임하여 결합도를 낮출수도 있을것 같구요!
- 리뷰어 리뷰 반영 (woowacourse#409 (comment))
- CSS와 JavaScript 파일 경로를 상대 경로로 변경 - 로컬 환경에서 정상 구동되도록 수정 - 리뷰어 리뷰 반영 (woowacourse#409 (comment))
- 리뷰어 리뷰 반영 (woowacourse#409 (comment))
- DOMContentLoaded 이벤트 핸들러 내의 setTimeout 제거 - LottoGameController의 requestAnimationFrame 제거 - 리뷰어 리뷰 반영 (woowacourse#409 (comment)) - 리뷰어 리뷰 반영 (woowacourse#409 (comment))
|
@eunsoA 안녕하세요! 로또 미션 제출일이 지나 일주일 됐고 다음 미션이 진행 중이라, 이 PR이 열려 있으면 클레어가 미션 하기에 신경 쓰일까 걱정돼요. 혹시 언제쯤 다시 요청 주실 수 있으실까요? |
|
@JUDONGHYEOK 앗 안녕하세요 동키콩! |
- 리뷰어 리뷰 반영(woowacourse#409 (comment))
- static 메소드를 인스턴스 메소드로 변경하여 상태 관리 개선 - LottoManager를 클래스 필드로 관리하여 의존성 명확화 - 리뷰어 리뷰 반영 (woowacourse#409 (comment))
- 테스트 코드를 인스턴스 기반으로 수정하고 beforeEach 추가
|
안녕하세요 동키콩! 🦍 |
JUDONGHYEOK
left a comment
JUDONGHYEOK
left a comment
There was a problem hiding this comment.
리뷰반영해주신 것 모두 확인했습니다!! 확실히 객체들 간의 책임이 더 명확해진 것 같아요!
모든 요구사항을 만족하셔서 머지하겠습니다~~!! 수고많으셨어요 클레어~~
2 participants
안녕하세요 동키콩!
이번 2단계 리뷰도 잘 부탁드립니다 :)
학습 목표
이번 미션을 통해 다음과 같은 학습 경험들을 쌓는 것을 목표로 합니다.
제출 전 체크 리스트
리뷰 요청 & 논의하고 싶은 내용
1) 이번 단계에서 가장 많이 고민했던 문제와 해결 과정에서 배운 점
2) 이번 리뷰를 통해 논의하고 싶은 부분
1단계 리뷰에서 테스트 코드 추가와 description에 관한 이야기를 해주셔서 수정 보완해보았는데요, 더 수정할 부분이 있을지 궁금해요. 제가 놓친 부분들이 있을까요? 다른분들의 코드를 보니 validation에 대해서도 각각 test를 작성하신 분들도 있던데 이런 것도 필요할지 궁금해요.
크루분들 중에 MVC 패턴을 쓰시는 분들이 많더라구요. 저는 이전에 MVC 패턴을 적용해본적이 없고, 어떤 장단점이 있는지 궁금해서 이번 2단계에서 MVC를 적용해보았습니다. 실제로 MVC가 현업에서 많이 쓰이는지, 그리고 제가 적용한 방식이 맞는지 의문이 듭니다.
저의 경우 도메인 - ui - 기능 연동 이렇게 생각하고 구현했는데 기능 연동 부분이 꽤나 단위가 크다는 생각이 들어요. 그래서 이번 2단계의 경우 '정적 / 동적 렌더링 구분, 이벤트와 UI 연동, 기능 연동'이라는 세단계를 구분하지 않고 하나의 커밋에 때려 넣었던 것 같아요. 보통 동키콩의 경우 어떤 단위로 작업을 진행하는지 궁금합니다!
오늘 PR을 올리지만 개인적으로 수정할 사안들이 많이 보여서 이어서 리팩터링을 진행할 생각입니다. 제가 생각하는 우선 수정할 내용은 아래와 같은데요, 이와 더불어 더 수정할 점이 있다면 알려주세요!
✅ 리뷰어 체크 포인트
1단계
2단계
1단계에서 자세한 리뷰로 많이 생각해볼 수 있었어요
2단계도 잘 부탁드립니다! 감사합니다 :)