[조수연] 로또 미션 Step2 #6
Conversation
- accord: 비교할 두 배열을 받아서 일치하는 개수를 반환하는 함수 - generateRandomNumberArray: 배열의 개수를 받아서 그 개수만큼 랜덤 번호를 생성하는 함수
- LottoRule 의 RANK_BY_ACCORD 프로퍼티가 잘못 작성되어 있던 문제 - Lotto 가 LottoRule 을 상속받지 않도록 변경
- Lotto 의 static 이 아니라 LottoRule 의 static 을 참조하도록 변경
- 재귀 호출 시 메서드 이름 정정
- hasOwnProperty 가 아닌 hasProperty 메서드를 사용해서 난 에러 - hasOwnProperty 대신 in 으로 프로퍼티 검사
- 수ㅗ수점이 반환되는 문제 수정
- 객체를 순회하지 못했던 문제 해결
- accord 메서드가 제대로 동작하지 않는 문제 - 배열의 길이가 더 큰 값을 집합으로 만들어 비교하도록 함
- 당첨 결과를 나타낼 수 있는 메서드 이름으로 수정
- getRank 에서 bonusCorrect 의 연산 범위를 제대로 지정해주지 않은 문제 수정 - 일치하는 번호 개수와 보너스 번호 개수에 따라 랭크가 제대로 매겨지지 않는 문제 수정
- 잘못된 메서드 이름으로 호출하던 문제 수정
- 얕은 복사를 하지 않고 프로퍼티를 추가하던 문제 수정
WalkthroughThe new changes introduce configurations for ESLint and Prettier to standardize code formatting and linting. Several new classes and functions are added to handle a lottery game's input, output, validation, and core logic, along with corresponding unit tests. Utility functions for random number generation, currency formatting, and asynchronous input reading are also included, ensuring a comprehensive setup for the lottery application. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant View
participant Input
participant Output
participant LottoController
participant Lotto
participant WinLotto
User->>View: Start game
View->>LottoController: Initialize
LottoController->>Input: Request purchase amount
Input->>User: Prompt for amount
User->>Input: Enter amount
Input->>LottoController: Return amount
LottoController->>Lotto: Generate tickets
Lotto->>LottoController: Return tickets
LottoController->>Output: Display tickets
Output->>User: Show tickets
LottoController->>Input: Request winning numbers
Input->>User: Prompt for numbers
User->>Input: Enter numbers
Input->>LottoController: Return numbers
LottoController->>WinLotto: Set winning numbers
WinLotto->>LottoController: Return validation
LottoController->>Output: Display result
Output->>User: Show result
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
AI가 해주는 리뷰입니다!코드 분석:이 Pull Request는 로또 게임의 기본 구조와 기능을 구현하고 있습니다. 주요 변경사항은 다음과 같습니다:
강점:
개선점:
구체적 제안:
import { describe, test, expect, vi } from 'vitest';
import LottoController from '../domain/LottoController';
describe('LottoController', () => {
test('getLottos should return correct number of lottos', async () => {
const mockView = {
inputMoney: vi.fn().mockResolvedValue(5000),
};
const controller = new LottoController({ view: () => mockView, lotto: Lotto, winLotto: WinLotto });
const lottos = await controller.getLottos();
expect(lottos).toHaveLength(5);
});
});
class LottoError extends Error {
constructor(message) {
super(message);
this.name = 'LottoError';
}
}
// 에러 발생 시
throw new LottoError('유효하지 않은 로또 번호입니다.');
// 에러 처리
try {
// 로직
} catch (error) {
if (error instanceof LottoError) {
console.error('로또 게임 오류:', error.message);
} else {
console.error('알 수 없는 오류:', error);
}
}
// config.js
export default {
lottoPrice: 1000,
limitPrice: 10000000,
minNumber: 1,
maxNumber: 45,
defaultLength: 6,
winLength: 7,
// ... 기타 설정
};
// LottoRule.js
import config from './config.js';
const LottoRule = {
...config,
// ... 기타 메서드
};
export default LottoRule;
generateLottoNumbers: () => {
const numbers = new Set();
while (numbers.size < LottoRule.defaultLength) {
numbers.add(Math.floor(Math.random() * LottoRule.maxNumber) + LottoRule.minNumber);
}
return Array.from(numbers);
},학습 포인트:
추가될 수 있는 요구사항 예시와 대응:
이러한 변경사항들은 기존 코드의 구조를 크게 해치지 않으면서 새로운 요구사항을 수용할 수 있도록 합니다. 설정의 중앙화와 객체지향적 설계 덕분에 새로운 기능 추가나 규칙 변경이 비교적 쉽게 이루어질 수 있습니다. |
seeyoujeong
left a comment
seeyoujeong
left a comment
There was a problem hiding this comment.
코드 잘 읽었습니다! 거의 새로 짜신듯해서 고생하신게 눈에 보였네요. ㅎㅎ
getRank 코드를 어떻게 수정하면 더 깔끔해질까 고민하다가 뇌절이 와버린...
그래도 명색이 Domain에 있는데 클래스로 선언하는 게 좋은지 잘 모르겠네요... 다른 분들의 의견이 궁금합니다!
읽기 편하면 괜찮지않을까요?! ㅎㅎ
다른 분들은 적절한 유효성 검사가 어느정도라고 생각하는지 궁금합니다...!
입력이랑 인수같은 외부에서 내부로 들어오는 부분은 하는게 좋다고 생각해요!
winLotto에서도 자신만의 private프로퍼티를 가지게 하고 싶은데 다른 방법이 있다면 알려주시면 감사하겠습니다.
수연님 말씀대로 이름을 바꾸는게 가장 간단하죠 ㅋㅋㅋ 저같은 경우는 보너스 번호는 따로 받다보니 코드를 좀더 줄일 수 있었네요.
for 문 내부에서 if문을 쓰는 더 나은 방법이 있다면 알려주시면 감사하겠습니다 🥹
머리를 굴려봤지만 결국 gpt에게 도움을 요청했네요 ㅎㅎ
| if (lotto.length === 1) { | ||
| lotto = lotto[0].split(',').map((number) => number.trim()); | ||
| } | ||
| return [...lotto].map((number) => Number(number)).sort((a, b) => a - b); |
| return [...lotto].map((number) => Number(number)).sort((a, b) => a - b); | ||
| } | ||
|
|
||
| get() { |
There was a problem hiding this comment.
get, set은 범용적인 이름이니깐 구체적인 이름을 쓰면 좋아요!
구체적이지 않은 이름을 사용하면 이 클래스를 사용하는 사람이 한번더 생각하게 된달까요?! ㅎㅎ 예를 들어, get 메서드를 쓸때, 사용자가 '이건 뭘 가져오는거지?'라고 생각을 하게 되는거죠!
그래서 여기서는 get 보다는 로또의 숫자들을 가져오는거니깐 getter를 써서 numbers라고 해도 좋고, 아니면 더 구체적으로 lottoNumbers 이렇게 해도 괜찮을 것 같아요.
저같은 경우는 인스턴스 이름을 lotto로 할 확률이 높다고 생각해서 numbers라고 했어요.
lotto.numbers라고 쓸걸 예상하고 ㅎㅎ
| if (restart === 'n' || restart === 'N') { | ||
| break; | ||
| } |
There was a problem hiding this comment.
this.getRestart에서 문자를 가져오기전에 toLowerCase를 쓰면 대문자 검사를 안해도 됩니다!
| } | ||
|
|
||
| async getLottos() { | ||
| const validator = createValidator(LottoValidation); |
There was a problem hiding this comment.
Lotto클래스에도 생성해서 쓸때마다 생성해서 쓰고 있어서 validator 폴더의 LottoValidator에서 미리 만들어서 그걸 export하는게 좋아보여요!
| if (LottoRule.winningInfo[rank].checkBonus === false) { | ||
| return rank; | ||
| } | ||
| if (LottoRule.winningInfo[rank].checkBonus && bonusCorrect) { | ||
| return rank; | ||
| } |
There was a problem hiding this comment.
if (!LottoRule.winningInfo[rank].checkBonus || bonusCorrect) {
return rank;
}이렇게 할 수도 있겠네요!
| try { | ||
| const { postProcessFn, validate } = options; | ||
| this.validator({ message, options, postProcessFn, validate }); | ||
|
|
||
| const input = await this.#rl(message); | ||
| const postInput = postProcessFn ? postProcessFn(input) : input; | ||
| validate && validate(postInput); | ||
|
|
||
| return postInput; | ||
| } catch (error) { | ||
| console.log(error); | ||
|
|
||
| return this.readInput(message, options); | ||
| } |
There was a problem hiding this comment.
step1때 저도 이런 방식으로 했었는데, 이번엔 예외 처리를 더 밖에서 하는걸로 수정해봤어요! 뭐가 나은지는 모르겠지만 ㅎㅎ
| function makeLottoMocking(...lotto) { | ||
| return new Lotto(...lotto); | ||
| } |
There was a problem hiding this comment.
모킹을 하지않아서 makeLotto로 이름을 수정하는게 좋아보여요!
|
@coderabbitai review please |
|
@JunilHwang: I will review the changes. Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
Outside diff range, codebase verification and nitpick comments (17)
src/utils/createValidator.js (1)
1-25: Implementation ofcreateValidatorfunctionThe
createValidatorfunction is well-designed, providing a flexible and reusable way to handle validations across the project. The use of closures and higher-order functions is appropriate here. However, the error message in lines 15-17 could be more descriptive to aid debugging.- `${keyType} 가 초기화 시 전달한 validation 내부에 존재하지 않습니다.` + `${keyType} is not defined in the provided validation rules.`src/View/View.js (2)
25-42: Consider validating user input in view methods.While the view methods correctly delegate input handling to the
Inputclass, adding an extra layer of validation here could prevent issues if theInputclass is bypassed or misused.
4-23: Ensure proper sanitization of template variables in theOutputclass.The
formatmethod in theOutputclass does not sanitize user inputs before inserting them into the template strings, which could lead to XSS vulnerabilities. Consider adding input sanitization to prevent such security risks.
- Location:
src/View/Output.js, lines 19-27Analysis chain
Ensure proper handling of external templates in the constructor.
The constructor initializes
InputandOutputobjects and sets up templates for various display functions. It's crucial to ensure that these templates are correctly managed and do not introduce any security risks, such as XSS vulnerabilities.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for any direct user input being inserted into templates without proper sanitization. rg --type js -A 3 $'this.output.format'Length of output: 683
Script:
#!/bin/bash # Description: Search for the implementation of the `format` method in the `Output` class. ast-grep --lang javascript --pattern $'class Output { $$$ format($_, $_) { $$$ } $$$ }'Length of output: 1336
src/domain/LottoController.js (2)
48-62: Improve validation ingetwinLottomethod.The method uses complex validation logic that could be simplified or better documented to improve maintainability.
64-72: Ensure robust validation ingetRestartmethod.The
InputValidatordoes not have a specific validation rule forrestart. To ensure robust validation, consider adding a validation rule forrestartinInputValidator.
src/validator/View/InputValidator.js: Add a specific validation rule forrestart.Analysis chain
Ensure robust validation in
getRestartmethod.The method handles user decisions to restart the game. It's crucial to ensure that the validation is robust and handles different types of user inputs effectively.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for robustness in restart validation. rg --type js -A 3 $'getRestart'Length of output: 559
Script:
#!/bin/bash # Search for the implementation of InputValidator rg --type js 'InputValidator' # Search for the inputRestart method in the codebase rg --type js 'inputRestart'Length of output: 654
Script:
#!/bin/bash # Extract the implementation of InputValidator rg --type js -A 10 'const InputValidator' src/validator/View/InputValidator.js # Extract the implementation of inputRestart method rg --type js -A 10 'async inputRestart' src/View/View.jsLength of output: 755
src/__tests__/Lotto.test.js (5)
32-42: Handle non-numeric characters more robustly in test descriptions.The test description uses
$lottowhich might not be replaced correctly in the output. Ensure that dynamic values in test descriptions are properly formatted to reflect the actual values being tested.
44-51: Improve clarity in test description for duplicate numbers.The description could be clearer. It currently reads as if any duplication in the numbers would throw an error, which might confuse whether it's about format or content.
- '중복되는 숫자($lotto)로 초기화할 경우 에러를 반환한다.' + 'If the Lotto is initialized with duplicate numbers in $lotto, it should throw an error.'
53-62: Clarify the test case for incorrect number count.The test description could be more descriptive to immediately convey the expected outcome without needing to read the test body.
- '로또번호($lotto)의 개수가 6이 아니면 에러를 반환한다.' + 'An error is thrown if the Lotto is initialized with a number count other than six ($lotto).'
64-75: Enhance readability and accuracy of the test for number range validation.The test checks if each number is within the 1 to 45 range, but the description could be clearer to reflect this specifically.
- '로또번호($lotto)의 각 번호가 1~45 사이의 숫자가 아니면 에러를 반환한다.' + 'An error is thrown if any number in the Lotto initialization is outside the 1-45 range ($lotto).'
78-82: Confirm the expected behavior ofget()method.The test checks the
get()method, but it's unclear what the method is supposed to return based on the test alone. Clarifying this in either the test description or by adding a comment would help.src/__tests__/WinLotto.test.js (7)
32-42: Handle non-numeric characters more robustly in test descriptions.Ensure that test descriptions accurately reflect the tested values, especially when using dynamic placeholders.
44-51: Improve clarity in test description for duplicate numbers.Adjust the wording to make it clear that the error is specifically about the initialization with duplicate numbers.
- '중복되는 숫자($lotto)로 초기화할 경우 에러를 반환한다.' + 'An error is thrown if WinLotto is initialized with duplicate numbers in $lotto.'
53-62: Clarify the test case for incorrect number count in WinLotto.The description should explicitly state the expected number count to avoid confusion.
- '로또번호($lotto)의 개수가 7이 아니면 에러를 반환한다.' + 'An error is thrown if the WinLotto is initialized with a number count other than seven ($lotto).'
64-75: Enhance readability and accuracy of the test for number range validation in WinLotto.Make the description more specific to the range check being performed.
- '로또번호($lotto)의 각 번호가 1~45 사이의 숫자가 아니면 에러를 반환한다.' + 'An error is thrown if any number in the WinLotto initialization is outside the 1-45 range ($lotto).'
78-82: Verify the behavior ofisBonusCorrectmethod.The test checks the
isBonusCorrectmethod, but it's unclear how this method works based on the test alone. Adding a comment or more descriptive test could help.
91-96: Clarify thegetRankmethod's functionality and expected results.The test for
getRankcould use more detailed descriptions to explain how the rankings are determined based on the lotto numbers.
99-124: Confirm the structure and values ingetWinningResult.The test for
getWinningResultmethod checks the output against a structured object. Ensure that the structure and values in the expected result match the actual functionality of the method.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
Files selected for processing (21)
- .eslintrc (1 hunks)
- .prettierrc (1 hunks)
- eslint.config.js (1 hunks)
- package.json (2 hunks)
- src/View/Input.js (1 hunks)
- src/View/Output.js (1 hunks)
- src/View/View.js (1 hunks)
- src/tests/Lotto.test.js (1 hunks)
- src/tests/WinLotto.test.js (1 hunks)
- src/domain/Lotto.js (1 hunks)
- src/domain/LottoController.js (1 hunks)
- src/domain/LottoRule.js (1 hunks)
- src/domain/WinLotto.js (1 hunks)
- src/main.js (1 hunks)
- src/utils/createValidator.js (1 hunks)
- src/utils/formatCurrency.js (1 hunks)
- src/utils/generateRandomNumber.js (1 hunks)
- src/utils/readline.js (1 hunks)
- src/validator/View/InputValidator.js (1 hunks)
- src/validator/View/OutputValidator.js (1 hunks)
- src/validator/domain/LottoValidator.js (1 hunks)
Files skipped from review due to trivial changes (3)
- .eslintrc
- .prettierrc
- src/utils/generateRandomNumber.js
Additional context used
Biome
src/View/Input.js
[error] 26-26: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
src/validator/domain/LottoValidator.js
[error] 6-6: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.(lint/suspicious/noGlobalIsNan)
[error] 23-23: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.(lint/suspicious/noGlobalIsNan)
src/domain/WinLotto.js
[error] 8-10: This constructor is unnecessary.
Unsafe fix: Remove the unnecessary constructor.
(lint/complexity/noUselessConstructor)
Additional comments not posted (10)
src/utils/formatCurrency.js (1)
1-11: Well-structured currency formatting function.The
formatCurrencyfunction is implemented usingIntl.NumberFormat, which is a robust choice for handling localized number formatting. The default parameter forcurrencyis set to 'ko-KR', which should be documented if this function is intended for broader use to clarify its default behavior.eslint.config.js (1)
1-9: Comprehensive ESLint and Prettier configuration setup.This configuration effectively integrates global settings and recommended practices, ensuring a consistent coding style across the project. It would be beneficial to include comments explaining the choice of specific plugins and configurations for future maintainability.
src/main.js (1)
1-18: Proper use of asynchronous function and module imports inmain.The updated
mainfunction correctly initializes and runs theLottoControllerusing asynchronous operations, which is suitable for handling potentially long-running operations like those in a lottery system. Ensure that error handling is robust, especially around the asynchronous operations inlottoController.run().package.json (2)
6-6: Formatting adjustment intypefieldThe adjustment to remove unnecessary spaces in the
typefield is a good practice as it helps maintain consistency and clarity in the file.
16-21: Addition of ESLint and Prettier dependenciesThe addition of ESLint and Prettier dependencies is a positive change, ensuring that the project adheres to standardized coding practices. It's crucial to verify that these versions are compatible with the project's current setup.
src/utils/readline.js (1)
3-23: Implementation ofreadLineAsyncfunctionThis function provides asynchronous reading of input, which is a useful utility for the project. However, the use of
arguments.lengthis not recommended in modern JavaScript due to potential performance issues and the fact that it does not work with arrow functions. Consider using rest parameters instead.- if (arguments.length !== 1) { + if (query.length !== 1) {The validation for the query type is correctly implemented, ensuring robust error handling.
Likely invalid or redundant comment.
src/validator/domain/LottoValidator.js (1)
3-45: Validation logic appears robust and modular.The structure of
LottoValidatoris well-organized, separating concerns into different validation categories. This modular approach enhances readability and maintainability.Tools
Biome
[error] 6-6: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.(lint/suspicious/noGlobalIsNan)
[error] 23-23: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.(lint/suspicious/noGlobalIsNan)
src/View/View.js (1)
44-62: Effective use of templates for output formatting.The methods
printPurchaseQuantity,printLottos,printWinningResult, andprintRevenueeffectively use templates to format output, which enhances maintainability and readability.src/domain/WinLotto.js (1)
44-67: Ensure accuracy ingetWinningResultcalculations.This method calculates the total prize and other details based on the lotto results. It's important to verify that all calculations are correct and consider edge cases where the input might not be as expected.
src/domain/LottoController.js (1)
6-33: Review overall structure and flow ofLottoController.The
LottoControllerclass effectively orchestrates the game flow, handling user inputs and outputs through its methods. Ensure that all methods are correctly implemented and consider adding more detailed error handling.
| const OutputValidator = { | ||
| templateKey: { | ||
| checkType: (value) => | ||
| typeof value === 'string' || 'templateKey 의 타입이 문자열이 아닙니다.', | ||
| }, | ||
| templateVariables: { | ||
| checkType: (value) => | ||
| typeof value === 'object' || | ||
| 'templateVariables 의 타입이 객체가 아닙니다.', | ||
| }, | ||
| }; | ||
|
|
||
| export default OutputValidator; |
There was a problem hiding this comment.
Refactor validation methods for consistent return types.
The OutputValidator methods currently return either a boolean or a string error message. This inconsistency can complicate error handling in the consuming code. Consider refactoring these methods to either always return a boolean or throw an error with the message.
- typeof value === 'string' || 'templateKey 의 타입이 문자열이 아닙니다.',
+ if (typeof value !== 'string') throw new Error('templateKey 의 타입이 문자열이 아닙니다.');
+ return true;
- typeof value === 'object' || 'templateVariables 의 타입이 객체가 아닙니다.',
+ if (typeof value !== 'object') throw new Error('templateVariables 의 타입이 객체가 아닙니다.');
+ return true;Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const OutputValidator = { | |
| templateKey: { | |
| checkType: (value) => | |
| typeof value === 'string' || 'templateKey 의 타입이 문자열이 아닙니다.', | |
| }, | |
| templateVariables: { | |
| checkType: (value) => | |
| typeof value === 'object' || | |
| 'templateVariables 의 타입이 객체가 아닙니다.', | |
| }, | |
| }; | |
| export default OutputValidator; | |
| const OutputValidator = { | |
| templateKey: { | |
| checkType: (value) => { | |
| if (typeof value !== 'string') throw new Error('templateKey 의 타입이 문자열이 아닙니다.'); | |
| return true; | |
| }, | |
| }, | |
| templateVariables: { | |
| checkType: (value) => { | |
| if (typeof value !== 'object') throw new Error('templateVariables 의 타입이 객체가 아닙니다.'); | |
| return true; | |
| }, | |
| }, | |
| }; | |
| export default OutputValidator; |
| export default class Output { | ||
| #template; | ||
| constructor({ template }) { | ||
| this.#template = template; | ||
| this.validator = createValidator(OutputValidator); | ||
| } | ||
|
|
||
| static print(...message) { | ||
| console.log(...message); | ||
| } | ||
|
|
||
| static lineBreak() { | ||
| console.log(''); | ||
| } | ||
|
|
||
| format(templateKey, templateVariables) { | ||
| this.validator({ templateKey, templateVariables }); | ||
| let templateString = this.#template[templateKey]; | ||
|
|
||
| Object.entries(templateVariables).forEach(([variable, value]) => { | ||
| templateString = templateString.replaceAll(`%{${variable}}`, value); | ||
| }); | ||
|
|
||
| return templateString; | ||
| } | ||
| } |
There was a problem hiding this comment.
Implementation of Output class
The Output class is well-structured, using private variables and a validation system to ensure robust output formatting. The use of template strings in the formatting function is a good practice. However, consider adding error handling for missing template keys to enhance the robustness of the method.
+ if (!this.#template[templateKey]) {
+ throw new Error(`Template key '${templateKey}' not found.`);
+ }Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export default class Output { | |
| #template; | |
| constructor({ template }) { | |
| this.#template = template; | |
| this.validator = createValidator(OutputValidator); | |
| } | |
| static print(...message) { | |
| console.log(...message); | |
| } | |
| static lineBreak() { | |
| console.log(''); | |
| } | |
| format(templateKey, templateVariables) { | |
| this.validator({ templateKey, templateVariables }); | |
| let templateString = this.#template[templateKey]; | |
| Object.entries(templateVariables).forEach(([variable, value]) => { | |
| templateString = templateString.replaceAll(`%{${variable}}`, value); | |
| }); | |
| return templateString; | |
| } | |
| } | |
| let templateString = this.#template[templateKey]; | |
| if (!templateString) { | |
| throw new Error(`Template key '${templateKey}' not found.`); | |
| } | |
| Object.entries(templateVariables).forEach(([variable, value]) => { | |
| templateString = templateString.replaceAll(`%{${variable}}`, value); | |
| }); | |
| return templateString; |
| try { | ||
| const { postProcessFn, validate } = options; | ||
| this.validator({ message, options, postProcessFn, validate }); | ||
|
|
||
| const input = await this.#rl(message); | ||
| const postInput = postProcessFn ? postProcessFn(input) : input; | ||
| validate && validate(postInput); | ||
|
|
||
| return postInput; | ||
| } catch (error) { | ||
| console.log(error); | ||
|
|
||
| return this.readInput(message, options); | ||
| } |
There was a problem hiding this comment.
Potential stack overflow risk and inefficient validation order.
The recursive call in the catch block (this.readInput(message, options)) could lead to a stack overflow if the error persists. Consider using a loop or other error handling strategy instead.
Additionally, validating input before actually reading it might lead to inefficiencies or incorrect behavior. Consider moving the validation to after the input is read.
Lastly, the use of optional chaining is recommended to simplify the code:
- validate && validate(postInput);
+ validate?.(postInput);Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| try { | |
| const { postProcessFn, validate } = options; | |
| this.validator({ message, options, postProcessFn, validate }); | |
| const input = await this.#rl(message); | |
| const postInput = postProcessFn ? postProcessFn(input) : input; | |
| validate && validate(postInput); | |
| return postInput; | |
| } catch (error) { | |
| console.log(error); | |
| return this.readInput(message, options); | |
| } | |
| try { | |
| const { postProcessFn, validate } = options; | |
| this.validator({ message, options, postProcessFn, validate }); | |
| const input = await this.#rl(message); | |
| const postInput = postProcessFn ? postProcessFn(input) : input; | |
| validate?.(postInput); | |
| return postInput; | |
| } catch (error) { | |
| console.log(error); | |
| return this.readInput(message, options); | |
| } |
Tools
Biome
[error] 26-26: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
| const InputValidator = { | ||
| message: { | ||
| checkType: (value) => | ||
| typeof value === 'string' || 'message 는 문자열 타입이어야 합니다.', | ||
| checkLength: (value) => | ||
| value.trim().length >= 1 || 'message 는 1글자 이상이어야 합니다', | ||
| }, | ||
| options: { | ||
| checkType: (value) => | ||
| typeof value === 'object' || 'options 는 객체 타입이어야 합니다.', | ||
| checkHasPostProcessFn: (value) => | ||
| 'postProcessFn' in value || | ||
| 'options 는 postProcessFn 프로퍼티를 가지고 있어야 합니다.', | ||
| checkHasValidate: (value) => | ||
| 'validate' in value || | ||
| 'options 는 validation 프로퍼티를 가지고 있어야 합니다.', | ||
| }, | ||
| postProcessFn: { | ||
| checkType: (value) => | ||
| value === undefined || | ||
| typeof value === 'function' || | ||
| 'postProcessFn 는 함수여야 합니다.', | ||
| }, | ||
| validate: { | ||
| checkType: (value) => | ||
| value === undefined || | ||
| typeof value === 'function' || | ||
| 'validate 는 함수여야 합니다.', | ||
| }, | ||
| restart: { | ||
| checkValue: (value) => | ||
| value === 'y' || | ||
| value === 'Y' || | ||
| value === 'n' || | ||
| value === 'N' || | ||
| '재시작 여부는 영문자 y 혹은 n 의 값이어야 합니다.', | ||
| }, | ||
| }; | ||
|
|
||
| export default InputValidator; |
There was a problem hiding this comment.
Enhance validation error handling and clarity.
The validation functions use a pattern that might not be immediately clear (condition || 'error message'). Consider using explicit if statements for better readability and error handling:
- checkType: (value) => typeof value === 'string' || 'message 는 문자열 타입이어야 합니다.',
+ checkType: (value) => {
+ if (typeof value !== 'string') {
+ throw new Error('message 는 문자열 타입이어야 합니다.');
+ }
+ return true;
+ }This approach makes the code easier to understand and debug.
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const InputValidator = { | |
| message: { | |
| checkType: (value) => | |
| typeof value === 'string' || 'message 는 문자열 타입이어야 합니다.', | |
| checkLength: (value) => | |
| value.trim().length >= 1 || 'message 는 1글자 이상이어야 합니다', | |
| }, | |
| options: { | |
| checkType: (value) => | |
| typeof value === 'object' || 'options 는 객체 타입이어야 합니다.', | |
| checkHasPostProcessFn: (value) => | |
| 'postProcessFn' in value || | |
| 'options 는 postProcessFn 프로퍼티를 가지고 있어야 합니다.', | |
| checkHasValidate: (value) => | |
| 'validate' in value || | |
| 'options 는 validation 프로퍼티를 가지고 있어야 합니다.', | |
| }, | |
| postProcessFn: { | |
| checkType: (value) => | |
| value === undefined || | |
| typeof value === 'function' || | |
| 'postProcessFn 는 함수여야 합니다.', | |
| }, | |
| validate: { | |
| checkType: (value) => | |
| value === undefined || | |
| typeof value === 'function' || | |
| 'validate 는 함수여야 합니다.', | |
| }, | |
| restart: { | |
| checkValue: (value) => | |
| value === 'y' || | |
| value === 'Y' || | |
| value === 'n' || | |
| value === 'N' || | |
| '재시작 여부는 영문자 y 혹은 n 의 값이어야 합니다.', | |
| }, | |
| }; | |
| export default InputValidator; | |
| const InputValidator = { | |
| message: { | |
| checkType: (value) => { | |
| if (typeof value !== 'string') { | |
| throw new Error('message 는 문자열 타입이어야 합니다.'); | |
| } | |
| return true; | |
| }, | |
| checkLength: (value) => | |
| value.trim().length >= 1 || 'message 는 1글자 이상이어야 합니다', | |
| }, | |
| options: { | |
| checkType: (value) => | |
| typeof value === 'object' || 'options 는 객체 타입이어야 합니다.', | |
| checkHasPostProcessFn: (value) => | |
| 'postProcessFn' in value || | |
| 'options 는 postProcessFn 프로퍼티를 가지고 있어야 합니다.', | |
| checkHasValidate: (value) => | |
| 'validate' in value || | |
| 'options 는 validation 프로퍼티를 가지고 있어야 합니다.', | |
| }, | |
| postProcessFn: { | |
| checkType: (value) => | |
| value === undefined || | |
| typeof value === 'function' || | |
| 'postProcessFn 는 함수여야 합니다.', | |
| }, | |
| validate: { | |
| checkType: (value) => | |
| value === undefined || | |
| typeof value === 'function' || | |
| 'validate 는 함수여야 합니다.', | |
| }, | |
| restart: { | |
| checkValue: (value) => | |
| value === 'y' || | |
| value === 'Y' || | |
| value === 'n' || | |
| value === 'N' || | |
| '재시작 여부는 영문자 y 혹은 n 의 값이어야 합니다.', | |
| }, | |
| }; | |
| export default InputValidator; |
| value.every((number) => !isNaN(number)) || | ||
| '숫자로 변환할 수 없는 문자가 포함되어 있습니다.', | ||
| checkDuplicate: (value) => { | ||
| const set = new Set(value); | ||
|
|
||
| return set.size === value.length || '중복되는 숫자가 포함되어 있습니다.'; | ||
| }, |
There was a problem hiding this comment.
Replace isNaN with Number.isNaN in common validation.
Similar to the previous comment, using Number.isNaN ensures that the type is correctly checked without coercion.
- value.every((number) => !isNaN(number)) ||
+ value.every((number) => !Number.isNaN(number)) ||Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| value.every((number) => !isNaN(number)) || | |
| '숫자로 변환할 수 없는 문자가 포함되어 있습니다.', | |
| checkDuplicate: (value) => { | |
| const set = new Set(value); | |
| return set.size === value.length || '중복되는 숫자가 포함되어 있습니다.'; | |
| }, | |
| value.every((number) => !Number.isNaN(number)) || | |
| '숫자로 변환할 수 없는 문자가 포함되어 있습니다.', | |
| checkDuplicate: (value) => { | |
| const set = new Set(value); | |
| return set.size === value.length || '중복되는 숫자가 포함되어 있습니다.'; | |
| }, |
Tools
Biome
[error] 23-23: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.(lint/suspicious/noGlobalIsNan)
| set(...lotto) { | ||
| const validator = createValidator(LottoValidator); | ||
| const normalizedLotto = Lotto.normalize(...lotto); | ||
|
|
||
| validator({ common: normalizedLotto, defaultLotto: normalizedLotto }); | ||
| this.lotto = normalizedLotto; | ||
| } |
There was a problem hiding this comment.
Improve method naming and add detailed error handling.
The set method could be more descriptively named to indicate that it sets the lotto numbers after validation and normalization. Consider renaming it to setLottoNumbers or similar. Additionally, enhance error handling to provide more detailed feedback on validation failures:
- validator({ common: normalizedLotto, defaultLotto: normalizedLotto });
+ try {
+ validator({ common: normalizedLotto, defaultLotto: normalizedLotto });
+ } catch (error) {
+ throw new Error(`Validation failed: ${error.message}`);
+ }This change helps in debugging and makes the code more robust.
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| set(...lotto) { | |
| const validator = createValidator(LottoValidator); | |
| const normalizedLotto = Lotto.normalize(...lotto); | |
| validator({ common: normalizedLotto, defaultLotto: normalizedLotto }); | |
| this.lotto = normalizedLotto; | |
| } | |
| set(...lotto) { | |
| const validator = createValidator(LottoValidator); | |
| const normalizedLotto = Lotto.normalize(...lotto); | |
| try { | |
| validator({ common: normalizedLotto, defaultLotto: normalizedLotto }); | |
| } catch (error) { | |
| throw new Error(`Validation failed: ${error.message}`); | |
| } | |
| this.lotto = normalizedLotto; | |
| } |
| test('WinLotto 클래스의 lotto 프로퍼티를 콤마로 숫자를 구분한 문자열로 초기화할 수 있다.', () => { | ||
| const winLotto = makeWinLottoMocking('1, 2, 3, 4, 5, 6, 7'); | ||
| expect(winLotto.accord(correctLotto)).toBe(7); | ||
| }); |
There was a problem hiding this comment.
Consider renaming makeWinLottoMocking to makeWinLotto.
Similar to the previous file, the function name suggests mocking but is used for actual instantiation. Renaming it would improve clarity.
- function makeWinLottoMocking(...lotto) {
+ function makeWinLotto(...lotto) {
return new WinLotto(...lotto);
}Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| test('WinLotto 클래스의 lotto 프로퍼티를 콤마로 숫자를 구분한 문자열로 초기화할 수 있다.', () => { | |
| const winLotto = makeWinLottoMocking('1, 2, 3, 4, 5, 6, 7'); | |
| expect(winLotto.accord(correctLotto)).toBe(7); | |
| }); | |
| test('WinLotto 클래스의 lotto 프로퍼티를 콤마로 숫자를 구분한 문자열로 초기화할 수 있다.', () => { | |
| const winLotto = makeWinLotto('1, 2, 3, 4, 5, 6, 7'); | |
| expect(winLotto.accord(correctLotto)).toBe(7); | |
| }); |
| constructor(...lotto) { | ||
| super(...lotto); | ||
| } |
There was a problem hiding this comment.
Remove unnecessary constructor in WinLotto.
The constructor in WinLotto is redundant as it only calls the parent class constructor with the same parameters.
- constructor(...lotto) {
- super(...lotto);
- }Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| constructor(...lotto) { | |
| super(...lotto); | |
| } |
Tools
Biome
[error] 8-10: This constructor is unnecessary.
Unsafe fix: Remove the unnecessary constructor.
(lint/complexity/noUselessConstructor)
| getRank(lotto) { | ||
| const bonusCorrect = this.isBonusCorrect(lotto); | ||
| const accordCount = this.accord(lotto) - (bonusCorrect ? 1 : 0); | ||
|
|
||
| for (const rank in LottoRule.winningInfo) { | ||
| if (accordCount === LottoRule.winningInfo[rank].accord) { | ||
| if (LottoRule.winningInfo[rank].checkBonus === false) { | ||
| return rank; | ||
| } | ||
| if (LottoRule.winningInfo[rank].checkBonus && bonusCorrect) { | ||
| return rank; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| return 'none'; | ||
| } |
There was a problem hiding this comment.
Optimize the getRank method to reduce complexity.
The method can be simplified by using a more direct approach to checking conditions, as suggested by a previous reviewer.
- if (LottoRule.winningInfo[rank].checkBonus === false) {
- return rank;
- }
- if (LottoRule.winningInfo[rank].checkBonus && bonusCorrect) {
- return rank;
- }
+ if (!LottoRule.winningInfo[rank].checkBonus || bonusCorrect) {
+ return rank;
+ }Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| getRank(lotto) { | |
| const bonusCorrect = this.isBonusCorrect(lotto); | |
| const accordCount = this.accord(lotto) - (bonusCorrect ? 1 : 0); | |
| for (const rank in LottoRule.winningInfo) { | |
| if (accordCount === LottoRule.winningInfo[rank].accord) { | |
| if (LottoRule.winningInfo[rank].checkBonus === false) { | |
| return rank; | |
| } | |
| if (LottoRule.winningInfo[rank].checkBonus && bonusCorrect) { | |
| return rank; | |
| } | |
| } | |
| } | |
| return 'none'; | |
| } | |
| getRank(lotto) { | |
| const bonusCorrect = this.isBonusCorrect(lotto); | |
| const accordCount = this.accord(lotto) - (bonusCorrect ? 1 : 0); | |
| for (const rank in LottoRule.winningInfo) { | |
| if (accordCount === LottoRule.winningInfo[rank].accord) { | |
| if (!LottoRule.winningInfo[rank].checkBonus || bonusCorrect) { | |
| return rank; | |
| } | |
| } | |
| } | |
| return 'none'; | |
| } |
| async getLottos() { | ||
| const validator = createValidator(LottoValidation); | ||
| const money = await this.view.inputMoney({ | ||
| postProcessFn: (value) => Number(value), | ||
| validate: (value) => validator({ money: value }), | ||
| }); | ||
| const quantity = LottoRule.exchange(money); | ||
|
|
||
| return [...new Array(quantity)].map( | ||
| () => new this.lotto(LottoRule.generateLottoNumbers()), | ||
| ); | ||
| } |
There was a problem hiding this comment.
Optimize getLottos method for better performance.
The method uses a map over a newly created array to generate lotto numbers, which could be optimized to reduce overhead.
- return [...new Array(quantity)].map(
- () => new this.lotto(LottoRule.generateLottoNumbers()),
- );
+ return Array.from({ length: quantity }, () => new this.lotto(LottoRule.generateLottoNumbers()));Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async getLottos() { | |
| const validator = createValidator(LottoValidation); | |
| const money = await this.view.inputMoney({ | |
| postProcessFn: (value) => Number(value), | |
| validate: (value) => validator({ money: value }), | |
| }); | |
| const quantity = LottoRule.exchange(money); | |
| return [...new Array(quantity)].map( | |
| () => new this.lotto(LottoRule.generateLottoNumbers()), | |
| ); | |
| } | |
| async getLottos() { | |
| const validator = createValidator(LottoValidation); | |
| const money = await this.view.inputMoney({ | |
| postProcessFn: (value) => Number(value), | |
| validate: (value) => validator({ money: value }), | |
| }); | |
| const quantity = LottoRule.exchange(money); | |
| return Array.from({ length: quantity }, () => new this.lotto(LottoRule.generateLottoNumbers())); | |
| } |
3 participants
미션을 수행하며 어려웠던 점
중복되는 유효성 검사 줄이기
validator를 클래스나 함수로 구현하신 걸 보고 저도 중복 로직 검사를 줄이기 위해createValidator를 만들었습니다. 세진님의createValidator아이디어와 react-hook-form에서 validate 프로퍼티를 등록해줌으로써 검사하는 방법을 적절히 합치고 싶었어요.if로 제어처리를 해주던 게 사라지니 많이 깔끔해진 느낌... 인데 아예if문을 사용하지 않는 게 좋은 방법은 아닌 것 같단 생각도 들어요. 적절히 사용해야 하는데 아직 validator를 사용해야하는 곳과 if문을 사용하는 곳의 기준을 못 잡고 있어서 이 점은 앞으로 차차 제 기준을 만들어나가야 할 것 같아요.TDD 시도해보기
다형성
기타
궁금한 점
LottoRule을 객체로 관리하는 방법에 대해
static키워드만으로 선언이 되어 있었고, 인스턴스가 따로 필요하지 않을 것 같아 이번엔 아예 객체로 바꾸었습니다.매개변수의 유효성 검사에 대해
리뷰받고 싶은 점
WinLotto의 private프로퍼티
depth 지못미...
Summary by CodeRabbit
Viewclass to handle user input and output for a lottery game simulation.Lottoclass for managing and comparing lotto numbers.LottoControllerclass to manage the lottery game flow.WinLottoclass to handle winning lotto numbers and ranking.Input,Output, andLottodata integrity.LottoandWinLottoclasses to ensure functionality.