[클린코드 8기 김민석] 로또 미션 STEP1 #333
Conversation
There was a problem hiding this comment.
안녕하세요 민석님!
시간이 지나서 로또 피드백 강의에서 학습하신 내용이 많이 휘발되셨을 텐데, 그런데도 불구하고 이렇게 열심히 진행해 주셔서 감사드려요🙏🏻
다만, 민석님께서 작성하신 코드 중에서 굵직한 개선이 필요해서 자잘한 피드백을 더 남겨드리기보다는, 큰 개선사항에 집중해 보시면 좋겠습니다.
1. 컨벤션 준수하기
- 프로그래밍 요구사항에서 자바스크립트 코드 컨벤션을 지킬 것을 권장하고 있습니다. 민석님의 코드는 대부분 클래스 코드의 메서드가 파스칼 케이스로 시작하는군요ㅠ. 코딩 컨벤션을 대폭 개선해 주시면 감사하겠습니다. AirBnb JavaScript Style Guide를 참고하여 코드를 개선해 주세요!
- 메서드는 be동사(is) 또는 일반동사로 시작해야 한다고 생각합니다. 하지만, 몇몇 메서드들은 일반동사의 느낌보다는 과거분사 형태의(MatchedCount) 이름으로 명명되어서 이것이 어떤 동작을 수행하는지와 어떤 값을 반환할 것인지 이름만 보고 추론하기가 매우 어려웠습니다.
2. 도메인/입출력 구별하기
- 현재,
domain디렉터리에 있는 클래스에서 입출력을 그대로 진행하고 있습니다. 도메인에서는 console.log나 입력 코드와 관련된 부분에 의존하지 않고 도메인 역할만 수행해야 하지 않을까요? 마찬가지로 domain 바깥에 있는 코드들은 입출력 그 자체에만 집중하거나, Factory와 같은 클래스는 다른 도메인에서도 종속되지 않고 이용할 수 있는 형태로 개선이 필요하지 않을까요? 그런데 Factory를 어떤 의도로 설계하셨는지 제가 알 수 없어서 자세한 조언을 드리기는 어려울 것 같아요. 참고만 해주세요! 추후 domain 쪽으로 편입하셔도 괜찮고 없애셔도 됩니다. 충분히 상황 공유만 해주세요!
3. 입력 값 검증하기
- 현재 구입 금액에
-1000,Infinity와 같은 유효하지 않은 숫자를 입력하거나1999와 같이 1000으로 나눠떨어지지 않는 값을 입력하더라도 코드가 동작합니다. 어떻게 개선할 수 있을까요? - 또한, 이런 사용자가 유효하지 않은 입출력 값을 입력하였을 때에는 도메인 테스트 코드에서 예외 테스트 케이스도 만들어서 별도로 검증해야 하지 않을까요?
너무 고생 많으셨습니다!
마저 보완하시면서 성장해 보시죠 💪🏻
There was a problem hiding this comment.
class에서는 생성자를 제외하고 PascalCase는 사용하지 않는 것으로 알고 있습니다🙂. 제가 봐왔던 코드들 중에서는 생소하네요. 혹시 대문자로 메서드 이름을 정하시는 이유가 있을까요?
src/domain/lotto-seller.js
Outdated
| this.lottoOperator = lottoOperator; | ||
| } | ||
|
|
||
| Sell(money) { |
There was a problem hiding this comment.
모든 클래스의 메서드가 대문자로 시작하네요 ㅠㅠ
src/domain/lotto-winner-number.js
Outdated
| this.bonusNumber = bonusNumber; | ||
| } | ||
|
|
||
| MatchedCount(lotto) { |
There was a problem hiding this comment.
이 메서드의 역할에 따른 이름을 명명하신 이유가 궁금합니다.
제가 동료의 입장에서 이 메서드를 확인했을 때 개수를 매칭한다는 역할 이외에는 어떤 값을 반환하는지를 예측하기 어려워서요.
src/domain/lotto-winner-number.js
Outdated
| let winnerNumber = this.numbers.slice(); | ||
| let result = 0; | ||
| for (let expectedNumber of lotto.expectedNumbers) { | ||
| const idx = winnerNumber.findIndex(n => n === expectedNumber); | ||
| if (idx === -1) { | ||
| continue; | ||
| } | ||
|
|
||
| winnerNumber.splice(idx, 1); | ||
| result += 1; | ||
| } |
There was a problem hiding this comment.
선언형 프로그래밍으로 작성한다면, 어떻게 개선해 볼 수 있을까요?
|
|
||
| describe('로또', () => { | ||
| describe('번호는', () => { | ||
| test('1부터 99까지다.', () => { |
There was a problem hiding this comment.
99까지인가요? 테스트 코드를 작성하신 의도는 유효 범위 숫자를 벗어난 값이 들어오면 예외가 발생하는 코드로 이해했는데, 명세도 어떤 내용을 테스트하는가가 분명하게 드러나면 좋지 않을까요?
src/factory/factory.js
Outdated
| static Create() { | ||
| const min = 1; | ||
| const max = 99; | ||
| const numbers = this.generateRandomLottoNumber(6, min, max); |
There was a problem hiding this comment.
디렉터리가 domain 안에 포함되어 있지 않으므로, 뭔가 도메인에 종속되지 않으면서 재사용성을 노리신 것으로 이해했습니다.
그런데 generateRandomLottoNumber에 의존하고 있습니다.
src/factory/factory.js
Outdated
| return new Lotto(numbers); | ||
| } | ||
|
|
||
| static generateRandomLottoNumber(count, min, max) { |
| // TODO : mocking 방법 알아낸 후 구현 | ||
|
|
||
| // test('로또 당첨 번호를 뽑는다.', async () => { | ||
| // // given | ||
| // const lottoOperator = new LottoOperator(); | ||
| // const rl = readline.createInterface({ | ||
| // input: process.stdin, | ||
| // output: process.stdout, | ||
| // }) | ||
| // mock() | ||
|
|
||
| // // when | ||
| // const lottoWinnerNumber = await lottoOperator.DrawWinningNumber(rl); | ||
|
|
||
| // // then | ||
| // expect(lottoWinnerNumber instanceof LottoWinnerNumber).toBeTruthy(); | ||
| // }) | ||
|
|
|
|
||
| describe('로또 사업자', () => { | ||
|
|
||
| test('요구된 종류와 갯수의 로또를 발행한다.', () => { |
There was a problem hiding this comment.
요구된 종료외 개수라는 점이 모호합니다. 요구된 종류란 무엇이고, 개수는 몇 개로 설정하였나요? 이런 것이 테스트 케이스에 분명하게 드러나고 기대하는 결과까지 명세에 언급되어야 하지 않을까요?
There was a problem hiding this comment.
이곳도 테스트 명세를 구체적으로 작성해 주시면 좋겠어요!
2 participants
늦게 올려 죄송합니다.
주말에 시간을 꽤 투자했는데 이제서야 올리네용.. 원래 이렇게 코드가 많이 나오는 미션이 맞나요🥲
TODO