[2단계 - 웹 기반 로또 게임] 안톨리니 미션 제출합니다.#462
[2단계 - 웹 기반 로또 게임] 안톨리니 미션 제출합니다.#462Antoliny0919 wants to merge 71 commits intowoowacourse:antoliny0919from
Conversation
JUDONGHYEOK
left a comment
JUDONGHYEOK
left a comment
There was a problem hiding this comment.
안녕하세요 안톨리니! 이번 2단계도 깔끔하게 구현해주셨군요 테스트까지 추가로 꼼꼼하게 작성해주셔서 감명깊게 보았습니다!
현재 구조에서 모듈간의 역할이 명확하게 나뉘어져 있고 1단계 도메인코드를 거의 수정없이 잘사용해주셔서 좋았던 것 같아요! 다만 View가 기능이 늘어날수록 비대해질 여지가 있어보여서 요부분을 의식하시면서 리팩토링 해보시면 좋을 것 같아요!
Q&A
View에서 DOM조작이 이루어지고 있습니다. 어디서 DOM조작이 이루어지면 좋을지 고민이 많았는데 적절한지 궁금합니다.
넵 View에서 DOM조작은 본질적으로 화면을 어떻게 바꿀것인가에 대한 관심사 이기 때문에 해당 책임을 View에 모은 것은 자연스럽다고 생각합니다!
현재 구조에서는 문제 없지만 만약 form이 추가로 확장되었을때 확장 될 때마다 ErrorMessage와 관련된 View로직이 추가되어야할걸로 예상됩니다. 이 부분은 어쩌면 확장을 두려워하게 만들 수 있다고 느껴집니다. Form과 관련된 컴포넌트를 추가하는게 좋을까요?
제 생각에 지금 renderPurchaseAmountErrorMessage와 renderWinningLottoNumberErrorMessage는 사실상 같은 일을 하고 있는데 대상 컨테이너만 달라져서 그렇게 느끼신 것이 아닐까 생각이 됩니다! 말씀하신대로 Form자체를 하나의 단위로 묶어서 캡슐화 해도 좋은 시도가 될 수 있을 것 같아요 하지만 역시나 말씀하신대로 현재 구조에서는 오히려 추상화 비용이 더 클 수 있어서 안톨리니가 고려하시면서 개선해보면 좋을 것 같네요!
입력된 값이 컴포넌트의 일부가 됩니다. 현재는 검증과 값의 변환로직이 있어 괜찮지만 만약 단순한 입력이 존재했을때 XSS같은 공격으로부터 취약하지 않을까? 라는 생각이 듭니다. 다른 프레임워크의 경우에는 이스케이핑을 통해 문제를 해결하는걸 봐왔는데 만약 이러한 상황에 놓인다면 어떤 처리를 통해 보안적으로 더 탄탄하게 만들 수 있을까요?
입력값이 템플릿 리터럴에 바로 들어가면 말씀하신대로 문제가 생길 수 있죠. 가장 간단한 해결책은 createElement와 textContent를 쓰는 방식들이 해결책이 될 수 있을 것 같아요
개인적으로 파일을 잘게 나누기보다 한 곳에서 흐름을 읽을 수 있는 코드를 선호합니다. 파일 분리는 각 파일을 단순하게 만들지만, 전체 흐름 파악을 위한 컨텍스트 이동 비용이 생긴다고 느끼기 때문입니다. 이 부분은 개인차가 있을 것 같기는 하지만... 리뷰어분은 어떤 기준으로 파일 분리 여부를 판단하시는지 궁금합니다.
가장 간단하게 판단하는 기준은 변경의 이유가 다르면 분리하고 항상 같이 바뀌면 함께 두는 것 같아요. 예를 들어 지금 구조에서 component.js와 view.js는 화면이 바뀌면 같이 수정된다는 점에서 합쳐도 될 수 있다고 생각한느데 controller와 view는 도메인 흐름이 바뀔때와 렌더링 방식이 바뀔 떄 라는 서로 다른 이유로 수정되기 때문에 분리되는 것이 맞다고 생각해요!
현재 재시도 로직에서 버그가 발생하고 있습니다! 요걸 우선 해결해보시면 좋을 것 같아요! 수고많으셨습니다 안톨리니!
|
|
||
| renderPurchaseAmountErrorMessage(message) { | ||
| const errorMessageComponent = Component.errorMessage(message); | ||
| this.purchaseAmountErrorMessageContainer.innerHTML = errorMessageComponent; |
| color: var(--error-fg); | ||
| } | ||
|
|
||
| .hidden { |
| Validator.validateLottoNumber(winningLottoNumber); | ||
| Validator.validateBonusNumber(winningLottoNumber, bonusNumber); | ||
| const winningLotto = new WinningLotto(winningLottoNumber, bonusNumber); | ||
| this.lottoMachine.calculateMatchResult( |
There was a problem hiding this comment.
여기서는 lottoMachine의 유무를 체크하지 않네요? 의도가 있으신것일까요?
| this.matchResultDialog.close(); | ||
| }, | ||
|
|
||
| convertHiddenState(targets) { |
There was a problem hiding this comment.
명시적인 show, hide대신에 toggle로 구성해줏니 이유가 있으실까요?
| @@ -5,12 +5,75 @@ | |||
| <title>🎱 행운의 로또</title> | |||
| <meta name="viewport" content="width=device-width, initial-scale=1.0" /> | |||
| <title>Document</title> | |||
There was a problem hiding this comment.
안톨리니가 구현한 코드는 아니지만 title이 두개 있어요 수정되면 좋을 것 같습니다
| <div id="purchase-amount-input-container"> | ||
| <label for="purchase-amount" class="text-body">구입할 금액을 입력해주세요.</label> | ||
| <div class="row-line-between"> | ||
| <input id="purchase-amount" class="font-placeholder" placeholder="금액" required> |
There was a problem hiding this comment.
font-placeholder라는 클래스는 정의해주신 이유가 있으실까요?
|
|
||
| #purchase-lotto-content { | ||
| height: 250px; | ||
| overflow: scroll; |
There was a problem hiding this comment.
overflow: scroll로 지정해주신 이유가 있으실까요?
| const expected = '<table id="lotto-match-result">' | ||
| + '<thead><th>일치 갯수</th><th>당첨금</th><th>당첨 갯수</th></thead>' | ||
| + '<tbody><tr><td>3개 일치</td><td>500</td><td>3</td></tr>' | ||
| + '<tr><td>4개 일치</td><td>1,000</td><td>100</td></tr>' | ||
| + '</tbody></table>' | ||
| expect(lottoMatchResultComponent).toEqual(expected); |
There was a problem hiding this comment.
꼼꼼하게 테스트 코드를 추가해주셨네요 좋습니다! 다만 문자열 완전일치만 검증해서 리팩토링 내성이 낮은데요. 테스트하는 주요 DOM 구조 중심 검증으로 바꾸면 유지보수성이 좋아질 것 같은데 어떻게 생각하세요?
| View.openModal(); | ||
| View.renderMatchResultModal( | ||
| Converter.matchResultSummary(this.lottoMachine.getMatchResultSummary()), | ||
| this.lottoMachine.getRateOfReturn(), |
There was a problem hiding this comment.
제출 후에 다른 form들을 disabled하는 것은 어떠세요?
| @@ -0,0 +1,11 @@ | |||
| const Converter = { | |||
2 participants
학습 목표
이번 미션을 통해 다음과 같은 학습 경험들을 쌓는 것을 목표로 합니다.
제출 전 체크 리스트
리뷰 요청 & 논의하고 싶은 내용
1) 이번 단계에서 가장 많이 고민했던 문제와 해결 과정에서 배운 점
리팩토링 과정에서 어떤 방식으로 전환할지 고민이 많이 되었다.
기존에는
document.createElement를 통해 노드를 하나씩 만드는 구조였다.해당 구조보다 문자열을 사용하여 태그를 렌더링하는게 더 가독성적인 면에서 좋을거라고 판단했다.
예상한대로 문자열인 상태로 전환했을때 가독성 적인 부분이 크게 개선되었다.
하지만 한 가지 고민이 존재하다면 현재 입력값이 렌더링될 컴포넌트의 일부가 되는데 지금 상태는 검증기와 값을 변환하는 과정이 있어 괜찮지만 만약 존재하지 않다면 XSS와 같은 공격에 취약하지 않을까라는 생각도 들었다.
리팩토링 전 코드는 index에 Component를 제외한 로직이 전부 존재했다.
이 로직을 Controller와 View로 전환하면서 각 부분에 어떤 로직이 들어가는게 좋을지 고민해본거 같다.
Controller에는 각 부분의 전체적인 흐름인 유효성 검사, 도메인 객체 생성, View 호출 순서 관리등을 담았다.
View에는 DOM 조작과 렌더링만 하도록 구현했다.
index에는 이벤트리스너에 대한 내용만 담았다.
이렇게 분리했을때 정말 지저분했던 index가 깔끔하게 분리될 수 있었다.
2) 이번 리뷰를 통해 논의하고 싶은 부분
View에서 DOM조작이 이루어지고 있습니다. 어디서 DOM조작이 이루어지면 좋을지 고민이 많았는데 적절한지 궁금합니다.
현재 구조에서는 문제 없지만 만약 form이 추가로 확장되었을때 확장 될 때마다
ErrorMessage와 관련된 View로직이 추가되어야할걸로 예상됩니다. 이 부분은 어쩌면 확장을 두려워하게 만들 수 있다고 느껴집니다. Form과 관련된 컴포넌트를 추가하는게 좋을까요?입력된 값이 컴포넌트의 일부가 됩니다. 현재는 검증과 값의 변환로직이 있어 괜찮지만 만약 단순한 입력이 존재했을때 XSS같은 공격으로부터 취약하지 않을까? 라는 생각이 듭니다. 다른 프레임워크의 경우에는 이스케이핑을 통해 문제를 해결하는걸 봐왔는데 만약 이러한 상황에 놓인다면 어떤 처리를 통해 보안적으로 더 탄탄하게 만들 수 있을까요?
개인적으로 파일을 잘게 나누기보다 한 곳에서 흐름을 읽을 수 있는 코드를 선호합니다. 파일 분리는 각 파일을 단순하게 만들지만, 전체 흐름 파악을 위한 컨텍스트 이동 비용이 생긴다고 느끼기 때문입니다. 이 부분은 개인차가 있을 것 같기는 하지만... 리뷰어분은 어떤 기준으로 파일 분리 여부를 판단하시는지 궁금합니다.
✅ 리뷰어 체크 포인트
1단계
2단계