Conversation
| parseInput(input) { | ||
| return input.split("").map(Number); | ||
| } |
There was a problem hiding this comment.
[논의]
저는 입력값을 가공하는 부분은 Model의 역할보다는 Controller의 역할이라고 생각해서 Controller의 이벤트 리스너 안에 넣어주었는데, 어떤 방식이 더 적합한지 이야기해보면 좋을 것 같습니다.
There was a problem hiding this comment.
Model에는 핵심 비즈니스 로직이 들어가야 하고, 입력값 검증 및 가공은 핵심 비즈니스 로직과는 거리가 멀다고 생각이 들어서 Controller로 책임을 옮기도록 하겠습니다!
There was a problem hiding this comment.
입력값 검증에는 게임 규칙이 포함이 되어 있어서 입력값 검증은 Model에, 입력값 가공의 경우는 Controller에 두었습니다.
| const DIGIT_COUNT = 3; | ||
| const MIN_NUMBER = 1; | ||
| const MAX_NUMBER = 9; | ||
| export const ERROR_MESSAGE = "1~9 사이 서로 다른 숫자 3개를 입력하세요."; |
There was a problem hiding this comment.
[배움]
코드 내에 하드코딩된 숫자(매직 넘버)는 코드를 처음 접한 사람에게 의미가 직관적으로 와닿지 않기 때문에 의미를 담은 상수로 분리하신 부분 좋은 것 같습니다! 저도 해당 부분을 리팩토링 예정이었는데, 도움이 되었습니다.
| isValidInput(input) { | ||
| if (input.length !== DIGIT_COUNT) return false; | ||
| if (!/^[1-9]+$/.test(input)) return false; | ||
| return new Set(input.split("")).size === DIGIT_COUNT; | ||
| } |
There was a problem hiding this comment.
[배움]
입력값 검증에 정규식을 사용해 코드 길이를 줄이고, Set의 특성을 잘 살린 코드라는 게 보여서 좋은 것 같습니다.
저도 중복 검사에 Set을 사용했지만 단순히 조건 검사용으로 사용해서 배열을 반환하는 Set을 쓰는 게 맞나 고민이 들었었는데, 이런 식으로 코드를 작성하면 가독성도 좋아지고, Set의 특성도 더 잘 활용할 수 있는 것 같습니다.
| play(input) { | ||
| const userNumbers = this.parseInput(input); | ||
| const strikes = this.countStrikes(this.computerNumbers, userNumbers); | ||
| const balls = this.countBalls(this.computerNumbers, userNumbers); | ||
| return this.formatResult(strikes, balls); | ||
| } |
There was a problem hiding this comment.
[질문]
스트라이크/볼 계산, 결과 계산/포맷 계산 등의 아주 작은 단위로 분리한 부분이 인상깊었습니다! 이번 미션에서 목표로 하셨던 단일 책임 원칙과 관련이 있을까요?
프로그래밍 요구사항에서는 컴퓨터숫자와 유저입력값을 모두 받아와서 결과값을 반환하도록 제시되어 있는데, 유저입력값만 받아서 결과를 계산하는 play 메서드와 결과 문자열을 반환하는 formatResult 메서드로 분리하신 이유가 있나요?
There was a problem hiding this comment.
단일 책임 원칙을 엄격히 지키고자 했는데 결과(문자열)를 반환하기 위한 일련의 과정(스트라이크/볼 계산, 결과 포맷)을 play 메서드에 넣어도 단일 책임 원칙에 어긋난다는 생각은 안들어서 수정했습니다!
| constructor() { | ||
| this.$userInput = document.querySelector("#user-input"); | ||
| this.$result = document.querySelector("#result"); | ||
| this.$restartButton = document.querySelector("#game-restart-button"); | ||
| } |
There was a problem hiding this comment.
[질문]
클래스를 잘 몰라서 하는 질문인데, 저는 생성자 함수 방식을 사용해서 DOM 요소를 그냥 풀어놨어요. 그런데 유성님은 constructor()로 묶어놓은 게 신기하네요. 클래스 방식을 사용하면 꼭 constructor로 묶어줘야 하나요?
There was a problem hiding this comment.
최신 자바스크립트 문법인 클래스 필드를 사용하면 아래처럼 constructor 밖에 바로 선언해도 현재 코드와 똑같이 동작합니다.
$userInput = document.querySelector("#user-input");
$result = document.querySelector("#result");
$restartButton = document.querySelector("#game-restart-button");constructor는 객체가 생성될 때 가장 먼저 딱 한 번 실행되는 함수로 '초기화'의 목적을 보여준다고 생각합니다. DOM 요소를 가져오는 작업이 View 객체를 생성할 때 가장 먼저 이루어져야 하는 초기화 단계라고 생각해서 그 의도를 명확히 보여주려고 constructor 안에 묶어두었습니다.
| init() { | ||
| document.querySelector("#submit").addEventListener("click", () => { | ||
| this.onSubmit(); | ||
| }); | ||
| document | ||
| .querySelector("#game-restart-button") | ||
| .addEventListener("click", () => { | ||
| this.onRestart(); | ||
| }); | ||
| } |
There was a problem hiding this comment.
[제안]
Controller에 DOM 선택자가 있으면 결합도가 높아지고, MVC 패턴의 역할 분리가 위배된다고 생각합니다. View에서 이벤트를 바인딩하는 메서드를 제공하고, Controller는 실행할 로직을 콜백함수로 View에 전달하는 방식을 사용하는 건 어떨까요?
There was a problem hiding this comment.
예령님 제안을 확인하고 고민해보니 현재 저의 코드는 HTML 구조나 id가 변경되면 View뿐만 아니라 Controller 코드까지 수정해야한다는 점에서 결합도가 높고, DOM을 찾는 행위는 View에 더 적합하다는 생각이 드네요. 제안 감사합니다!
| export default class BaseballGame { | ||
| constructor() { | ||
| this.computerNumbers = this.generateComputerNumbers(); | ||
| } |
There was a problem hiding this comment.
[제안]
Model은 Controller가 넘겨주는 숫자들을 받아서 계산만 해주는 계산기(유틸리티)의 역할만 한다고 생각합니다! 정답값을 생성하는 것은 Controller의 역할로 이동하는 것이 어떨까요?
There was a problem hiding this comment.
MVC에서 각각의 역할에 대해 고민하다가 Model이 모든 비즈니스 로직을 가져야한다는 생각에 빠졌습니다. 그래서 리팩토링 과정에서 Controller에서 Model로 옮겼습니다. 예령님 제안처럼 Controller가 정답 숫자를 생성해서 View로부터 받은 입력 숫자와 함께 Model로 전달하는 것이 적합하다는 생각이 드네요. 이에 맞게 수정하겠습니다!
| // 게임 재시작 | ||
| reset() { | ||
| this.computerNumbers = this.generateComputerNumbers(); | ||
| } | ||
| } |
There was a problem hiding this comment.
[제안]
초기화와 관련된 로직도 Controller의 역할에 더 적합해보여요! 어떻게 생각하시나요?
Updated the refactoring section header in README.md.
2 participants
개인 목표 달성 여부
미달성 이유: Cypress를 처음 접하다 보니 모든 코드를 완벽히 이해하는 데 한계가 있었습니다. 미달성한 부분은 아래 스터디 제안을 통해 보완하고자 합니다.
리뷰어에게