[차세진] 로또 미션 Step2 #7
Conversation
|
Warning Rate limit exceeded@JunilHwang has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 28 minutes and 17 seconds before requesting another review. How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 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가 해주는 리뷰입니다!네, 업로드해주신 Github Pull Request JSON 파일을 바탕으로 로또 미션의 요구사항을 고려하여 코드를 분석하고 피드백을 드리겠습니다. 특히 새로운 요구사항이 추가되거나 변경될 수 있는 상황에 대비한 코드의 유연성과 확장성에 초점을 맞추겠습니다.
주요 특징:
a) 확장성 고려:
b) 에러 처리:
c) 국제화(i18n) 지원:
d) 성능 최적화:
a) LottoRule의 설정 외부화: // config.js
export default {
lottoPrice: 1000,
limitPrice: 10000000,
// ... 기타 설정
};
// LottoRule.js
import config from './config.js';
const LottoRule = {
lottoPrice: config.lottoPrice,
limitPrice: config.limitPrice,
// ... 기타 속성
};b) 에러 처리 개선: // ErrorHandler.js
export default class ErrorHandler {
static handle(error) {
// 에러 유형에 따른 처리 로직
console.error('An error occurred:', error.message);
// 사용자에게 친화적인 메시지 표시 로직
}
}
// 사용 예:
try {
// 로직 실행
} catch (error) {
ErrorHandler.handle(error);
}c) 국제화 지원: // i18n.js
const messages = {
en: {
inputMoney: 'Enter the purchase amount:',
// ... 기타 메시지
},
ko: {
inputMoney: '구매 금액을 입력해주세요:',
// ... 기타 메시지
}
};
export default function i18n(key, lang = 'ko') {
return messages[lang][key] || key;
}
// 사용 예:
import i18n from './i18n.js';
console.log(i18n('inputMoney'));
새로운 요구사항 예시와 대응 방안:
이러한 변경사항에 대비하여 현재의 코드 구조를 유지하면서 필요한 부분만 확장하거나 수정할 수 있습니다. 모듈화된 구조와 의존성 주입 패턴 덕분에 새로운 기능을 추가하거나 기존 기능을 수정하는 것이 비교적 용이할 것입니다. |
AI가 해주는 질문에 대한 답변 (1)네, 제공해주신 고민 사항들을 바탕으로 상세한 피드백을 드리겠습니다. 각 섹션에 대해 포맷팅을 개선하고 제목에 대한 강조를 표현하겠습니다. 1. 고민한 부분에 대한 피드백Lotto 클래스의 역할 확장과 LottoMatcher 삭제장점:
개선 제안:
LottoRuleSet 클래스 추가장점:
개선 제안:
2. 리뷰 받고 싶은 부분에 대한 답변cloneDeep 유틸 관련깊은 복사를 위한 간단한 방법: function deepClone(obj) {
if (obj === null || typeof obj !== 'object') {
return obj;
}
if (Array.isArray(obj)) {
return obj.map(deepClone);
}
const clonedObj = {};
for (let key in obj) {
if (obj.hasOwnProperty(key)) {
clonedObj[key] = deepClone(obj[key]);
}
}
return clonedObj;
}이 함수는 대부분의 경우에 잘 작동하지만, 순환 참조나 특별한 객체 타입(예: Date, RegExp)에 대해서는 추가적인 처리가 필요할 수 있습니다. 테스트 케이스 작성 전략
작성 순서:
테스트 주도 개발(TDD) 방식을 따른다면, 각 기능을 구현하기 전에 테스트를 먼저 작성하는 것도 좋은 방법입니다. 이러한 접근 방식을 통해 더 완벽한 테스트 케이스를 작성할 수 있을 것입니다. 그리고 실제 사용 중 발견되는 문제점들은 지속적으로 테스트 케이스에 추가하여 개선해 나가는 것이 중요합니다. |
AI가 알려주는 테스트 케이스 보강
네, 현재 코드에 대해 테스트 케이스를 보강하는 것은 좋은 아이디어입니다. 다양한 시나리오를 커버하는 테스트 케이스를 작성해보겠습니다. 주요 클래스와 기능별로 테스트를 나누어 작성하겠습니다. 1. Lotto 클래스 테스트describe('Lotto 클래스 테스트', () => {
test('유효한 로또 번호로 인스턴스 생성', () => {
const lotto = new Lotto([1, 2, 3, 4, 5, 6]);
expect(lotto.getNumbers()).toEqual([1, 2, 3, 4, 5, 6]);
});
test('중복된 번호로 인스턴스 생성 시 에러 발생', () => {
expect(() => new Lotto([1, 1, 2, 3, 4, 5])).toThrow();
});
test('6개 미만의 번호로 인스턴스 생성 시 에러 발생', () => {
expect(() => new Lotto([1, 2, 3, 4, 5])).toThrow();
});
test('6개 초과의 번호로 인스턴스 생성 시 에러 발생', () => {
expect(() => new Lotto([1, 2, 3, 4, 5, 6, 7])).toThrow();
});
test('1보다 작은 번호로 인스턴스 생성 시 에러 발생', () => {
expect(() => new Lotto([0, 1, 2, 3, 4, 5])).toThrow();
});
test('45보다 큰 번호로 인스턴스 생성 시 에러 발생', () => {
expect(() => new Lotto([1, 2, 3, 4, 5, 46])).toThrow();
});
test('contain 메서드 테스트', () => {
const lotto = new Lotto([1, 2, 3, 4, 5, 6]);
expect(lotto.contain(3)).toBeTruthy();
expect(lotto.contain(7)).toBeFalsy();
});
test('matchNumbers 메서드 테스트', () => {
const lotto1 = new Lotto([1, 2, 3, 4, 5, 6]);
const lotto2 = new Lotto([4, 5, 6, 7, 8, 9]);
expect(lotto1.matchNumbers(lotto2)).toEqual([4, 5, 6]);
});
});2. LottoRuleSet 클래스 테스트describe('LottoRuleSet 클래스 테스트', () => {
let ruleSet;
beforeEach(() => {
ruleSet = new LottoRuleSet();
});
test('기본 규칙 확인', () => {
expect(ruleSet.getPrice()).toBe(1000);
expect(ruleSet.getMinNumber()).toBe(1);
expect(ruleSet.getMaxNumber()).toBe(45);
expect(ruleSet.getNumberCount()).toBe(6);
});
test('당첨 금액 확인', () => {
expect(ruleSet.getPrize(1)).toBe(2000000000);
expect(ruleSet.getPrize(2)).toBe(30000000);
expect(ruleSet.getPrize(5)).toBe(5000);
});
test('유효하지 않은 등수에 대한 당첨 금액 확인', () => {
expect(() => ruleSet.getPrize(6)).toThrow();
});
test('남은 당첨금 분배', () => {
ruleSet.distributeRemainingPrize(10000);
expect(ruleSet.getPrize(1)).toBeGreaterThan(2000000000);
});
});3. LottoSystem 클래스 테스트describe('LottoSystem 클래스 테스트', () => {
let lottoSystem;
beforeEach(() => {
lottoSystem = new LottoSystem();
});
test('로또 구매', () => {
const tickets = lottoSystem.buyLottos(5000);
expect(tickets.length).toBe(5);
tickets.forEach(ticket => {
expect(ticket).toBeInstanceOf(Lotto);
});
});
test('잘못된 금액으로 로또 구매 시 에러 발생', () => {
expect(() => lottoSystem.buyLottos(500)).toThrow();
});
test('당첨 번호 설정', () => {
lottoSystem.setWinningNumbers([1, 2, 3, 4, 5, 6], 7);
expect(lottoSystem.getWinningNumbers()).toEqual([1, 2, 3, 4, 5, 6]);
expect(lottoSystem.getBonusNumber()).toBe(7);
});
test('잘못된 당첨 번호 설정 시 에러 발생', () => {
expect(() => lottoSystem.setWinningNumbers([1, 2, 3, 4, 5, 6], 6)).toThrow();
});
test('당첨 결과 확인', () => {
lottoSystem.setWinningNumbers([1, 2, 3, 4, 5, 6], 7);
const tickets = [
new Lotto([1, 2, 3, 4, 5, 6]),
new Lotto([1, 2, 3, 4, 5, 7]),
new Lotto([7, 8, 9, 10, 11, 12])
];
const result = lottoSystem.checkWinningResult(tickets);
expect(result[1]).toBe(1); // 1등 1개
expect(result[2]).toBe(1); // 2등 1개
expect(result[6]).toBe(1); // 낙첨 1개
});
test('수익률 계산', () => {
lottoSystem.setWinningNumbers([1, 2, 3, 4, 5, 6], 7);
const tickets = [
new Lotto([1, 2, 3, 4, 5, 6]),
new Lotto([7, 8, 9, 10, 11, 12])
];
const profit = lottoSystem.calculateProfit(tickets, 2000);
expect(profit).toBe(100000); // (2,000,000,000 / 2000) * 100
});
});4. 통합 테스트describe('로또 시스템 통합 테스트', () => {
let lottoSystem;
beforeEach(() => {
lottoSystem = new LottoSystem();
});
test('전체 로또 게임 프로세스', () => {
// 로또 구매
const tickets = lottoSystem.buyLottos(5000);
expect(tickets.length).toBe(5);
// 당첨 번호 설정
lottoSystem.setWinningNumbers([1, 2, 3, 4, 5, 6], 7);
// 당첨 결과 확인
const result = lottoSystem.checkWinningResult(tickets);
// 수익률 계산
const profit = lottoSystem.calculateProfit(tickets, 5000);
// 결과 검증
expect(Object.values(result).reduce((a, b) => a + b)).toBe(5);
expect(profit).toBeGreaterThanOrEqual(0);
expect(profit).toBeLessThanOrEqual(400000); // 최대 수익률 400,000%
});
test('여러 회차 로또 게임', () => {
let totalProfit = 0;
let totalCost = 0;
for (let i = 0; i < 5; i++) {
const cost = 10000;
totalCost += cost;
const tickets = lottoSystem.buyLottos(cost);
const winningNumbers = Array.from({length: 6}, () => Math.floor(Math.random() * 45) + 1);
const bonusNumber = Math.floor(Math.random() * 45) + 1;
lottoSystem.setWinningNumbers(winningNumbers, bonusNumber);
const result = lottoSystem.checkWinningResult(tickets);
const profit = lottoSystem.calculateProfit(tickets, cost);
totalProfit += profit;
}
expect(totalProfit).toBeGreaterThanOrEqual(0);
expect(totalProfit).toBeLessThanOrEqual(totalCost * 4000); // 최대 수익률 400,000%
});
});이러한 테스트 케이스들은 다양한 시나리오를 커버하며, 각 클래스의 주요 기능과 예외 상황을 검증합니다. 실제 환경에서는 더 많은 엣지 케이스와 특수한 상황을 고려해야 할 수 있습니다. 또한, 실제 구현에 따라 일부 테스트 케이스를 수정하거나 추가해야 할 수 있습니다. 예를 들어, 실제 랜덤 번호 생성 대신 목(mock) 함수를 사용하여 테스트의 일관성을 유지하는 것도 좋은 방법입니다. 이러한 테스트를 통해 코드의 안정성을 높이고, 향후 변경 사항에 대해 더 자신 있게 대응할 수 있을 것입니다. |
suyeon1218
left a comment
suyeon1218
left a comment
There was a problem hiding this comment.
안녕하세요 세진님!! 이전에 누가 말씀해주셨던 것 같은데 세진님은 객체를 정말 잘 활용하시는 것 같아요. 세진님을 보며 저도 많은 시도를 해보려고 하는데... 문제 해결 능력을 길러주는 스터디를 하게되는 것 같습니다 ㅋㅋ! 바쁘실 텐데 코드를 정성스럽게 작성하기 위해 노력하셨다는 게 느껴져요~!
Lotto 역할 분담
- 우선 contain 이나 amtchNumbers 와 같은 부분이 좋은 것 같아요. 저는 단순히 몇개의 번호가 일치하는지를 보여주는
accord메서드만 작성했는데 세진님의 방식을 보면서 이런 메서드가 테스트하기엔 더 유용하겠구나~ 라는 생각이 들었습니다! - 저도 비슷하게 클래스가 클래스로 존재할 필요가 없는 건 과감히 삭제하고 그냥 객체로 전향시켜 버렸어요! ㅋㅋㅋㅋ...여러 시도를 해보는 건 괜찮지 않을까요...?!
Lotto 규칙 관리
- 저였다면 LottoRule을 상수로 분리하는 선에서 그칠 것 같은데 RuleSet 클래스를 만들어서 여러가지 룰을 관리할 수 있다는 장점이 있는 것 같아요!
- profit을 증가시키는 역할도.. 👍 어떻게 하면 더 실생활에서 사용하는 로또를 만들 수 있을지 고민하신 것 같아요
메서드 복사
- 저도 GPT 한테 물어봤지만... 함수에 대한 복사는 따로 안 해주는 것 같더라구요...! 함수는 대체로 복사할 필요가 없다고 하지만 복사하고 싶다면 클로저를 사용한 방법이 있다는 해답을 내놨어요 ㅎㅎ..!
테스트 케이스
- 저도 이 부분은 아직까지 고민중인 단계인 것 같아요... 최대한 모든 메서드를 커버하려고 하는데 커버할 수 없다면 로직을 변경하는 게 답인 경우도 있는 것 같습니다...!
| if (Array.isArray(obj)) { | ||
| const clonedArray = []; | ||
| obj.forEach((item) => { | ||
| clonedArray.push(cloneDeep(item)); | ||
| }); | ||
| return clonedArray; | ||
| } |
There was a problem hiding this comment.
cloneArray나 cloneObject를 함수로 분리해줄 수도 있을 것 같아요!
| lottoEachUnique: { | ||
| check: ({ numbers, bonusNumber }) => new Set(numbers).size === numbers.length && !numbers.includes(bonusNumber), |
There was a problem hiding this comment.
bonusNumber가 있는걸 봐선 winningLotto에 대한 중복검사인가요, 아니면 일반 ticketLotto에 대해서도 검사할 수 있나요?
| }, | ||
| errorMessage: '모든 로또 번호는 정수여야 합니다.', | ||
| }, |
There was a problem hiding this comment.
정수처리까지 꼼꼼하게 하셨군요...! 저는 깜빡한 것 같네요 메모메모 ✏️
| }, | ||
| validRanks: { | ||
| check: ({ initialRule }) => initialRule.every(({ rank }) => Number.isInteger(rank) && rank > 0), | ||
| errorMessage: '로또 랭킹 규칙에 유효하지 않은 값이(rank) 포함되어 있습니다.', |
| }, | ||
| validMatchCounts: { | ||
| check: ({ rankingRule }) => | ||
| rankingRule.every(({ matchCount }) => Number.isInteger(matchCount) && 0 < matchCount && matchCount <= 6), |
There was a problem hiding this comment.
개인적인 의견이지만 matchCount <= 1 로 써야 뒤에 있는 식과 일관성이 유지되지 않을까요?
| const lottoNumbers = new Set(); | ||
| while (lottoNumbers.size < 6) { | ||
| lottoNumbers.add(getRandomNumber(1, 45)); | ||
| } | ||
| return [...lottoNumbers]; | ||
| }; |
There was a problem hiding this comment.
저는 평소 코드 습관대로 그냥 set이라고 변수명을 선언해버리고 말았는데 이렇게 선언하는 게 더 명시적인 것 같네요...!
| getCountByRank(rank) { | ||
| return this.lottoTickets.filter((lottoTicket) => this.getRank(lottoTicket) === rank).length; | ||
| } |
There was a problem hiding this comment.
수를 추가하는 걸 저는 forEach 를 통해 info[rank] += 1 와 같은 식으로 처리했는데 함수로 분리해주는 게 훨씬 더 좋아보이네요 👍
| this.#ruleSet = initialRule.sort((a, b) => b.rank - a.rank); | ||
| } | ||
|
|
||
| get rulesWithBonusMatch() { |
There was a problem hiding this comment.
그냥 rulesWithBonusMatch 메서드를 만드는 것과는 어떤 차이점이 있나요?
|
@coderabbitai review please |
|
@JunilHwang, based on the details and code snippets provided in the pull request discussions, here's my review focusing on the key areas of concern: 1. Lotto Class Enhancements
2. Error Handling and Validation
3. Test Coverage and Cases
4. Code Structure and Readability
5. Performance Considerations
6. Documentation and Comments
7. Future Enhancements
Overall, the PR shows a strong understanding of object-oriented principles and JavaScript capabilities. Keep up the good work, and consider the suggestions to further enhance the application. Actions performedReview triggered.
|
|
@coderabbitai summary please |
|
@JunilHwang, here's a summary of the pull request based on the discussions and code snippets provided: Overview
Discussions
Suggestions
This summary encapsulates the key aspects of the pull request and the discussions held. If there are specific areas you would like to delve deeper into or additional details you need, please let me know! Actions performedSummary regeneration triggered. |
seeyoujeong
left a comment
seeyoujeong
left a comment
There was a problem hiding this comment.
코드 잘 읽었어요!
점점 깔끔해지는 코드 읽기 좋았어요!
처음부터 꼼꼼하게 작성하기 위한 기준같은것이 있을까요 !
경험이 쌓이면 될듯합니다! (리뷰하는 사람 경험 부족) 지금 이 부족한 부분을 채우기 위해선 우리의 동반자 ai 친구한테 물어보면 좋지않을까요??
ai도 사실 우리들의 경험을 먹고 성정한 친구니까요! (리뷰하는 사람 ai를 좀 불편해함, 뭔가 무서움, 자리 뺏길거같음, 하지만 써야함)
| export default class Lotto { | ||
| constructor(lottoProps) { |
There was a problem hiding this comment.
여기에 type, numbers, bonusNumber를 적어주면 더 읽기 편할 것 같아요!
| lottoTicket.contain(this.winningLotto.bonusNumber) === bonusMatch && | ||
| lottoTicket.matchNumbers(this.winningLotto).length === matchCount, | ||
| ); | ||
| const matchedRuleWithoutBonus = this.#ruleSet.rulesWithoutBonusMatch.find( | ||
| ({ matchCount }) => lottoTicket.matchNumbers(this.winningLotto).length === matchCount, |
| get profitAmount() { | ||
| return this.lottoRankingResult.reduce((sum, { count, profit }) => sum + profit * count, 0); | ||
| } | ||
|
|
||
| get leftPaidAmount() { | ||
| return Math.max(0, this.paidAmount - this.profitAmount); | ||
| } | ||
|
|
||
| increaseProfit() { | ||
| this.#ruleSet.increaseRankProfit(this.leftPaidAmount); | ||
| } | ||
|
|
||
| get profitRatio() { | ||
| return parseFloat(((this.profitAmount / this.paidAmount) * 100).toFixed(2)); | ||
| } | ||
|
|
||
| get paidAmount() { | ||
| return this.#lottoData.paidAmount; | ||
| } | ||
|
|
||
| get lottoTickets() { | ||
| return cloneDeep(this.#lottoData.lottoTickets); | ||
| } | ||
|
|
||
| get winningLotto() { | ||
| return cloneDeep(this.#lottoData.winningLotto); | ||
| } | ||
|
|
||
| get ticketCount() { | ||
| return this.#lottoData.lottoTickets.length; | ||
| } |
There was a problem hiding this comment.
오 깔끔합니다 ㅎㅎ 제 코드 짜면서 든 생각이 있어요 클래스의 요소들(정적 프로퍼티, 인스턴스 프로퍼티, 정적 메서드, 인스턴스 메서드, getter, setter)의 배치 순서를 통일하는게 좋더라구요! 자신만의 규칙을 만드는거죠!
| if (this.template.hasOwnProperty(templateKey)) { | ||
| console.log(this.format(templateKey, messages)); | ||
| } else { | ||
| console.log(...messages); | ||
| } | ||
| } | ||
|
|
||
| lineBreak() { | ||
| console.log(''); | ||
| } |
There was a problem hiding this comment.
console.log 대신 만드신 print를 써도 되지않을까요??
| hasAllRanks: { | ||
| check: ({ rankingRule }) => { | ||
| const ranks = rankingRule.map((data) => data.rank).sort((a, b) => a - b); | ||
| return rankingRule.length > 0 && ranks.every((rank, index) => rank === index + 1); | ||
| }, | ||
| errorMessage: '로또 랭킹 규칙에 모든 등수가 빠짐없이 있어야 합니다.', | ||
| }, |
| payAmountInteger: { | ||
| check: ({ payAmount }) => Number.isInteger(payAmount), | ||
| errorMessage: '로또 결제 금액은 숫자(정수)여야 합니다.', | ||
| }, | ||
| payAmountUnit1000: { | ||
| check: ({ payAmount }) => payAmount % 1000 === 0, | ||
| errorMessage: '로또 결제는 1000원 단위로만 가능합니다.', | ||
| }, | ||
| }; |
There was a problem hiding this comment.
음수 검사도 해줘야 해요!
이유: -1000 % 1000 === 0 -> true
4 participants
주중에 로또 개발할 시간이 안날 것 같아서, 바쁘기 전에 호다닥해서 올립니다 !
따라서
리뷰하기 + 리뷰반영하기도 틈틈히 호다다닥 할 예정입니다 .... (그치만 장담할 수는 없습니다..ㅏ...엉엉 🥲)미션을 수행하면서 고민한 부분
고민한 부분 요약 : Lotto 클래스의 자체적인 역할을 부여하기 + LottoSystem 의 일을 덜기 (잘 되었는지는...모르겠습니다 ! ! !)
Lotto 클래스가 하는 일을 확장하고 싶어서, 비교하는 로직을 여기로 넣었습니다. 약간
array1.concat(array2)이런 배열 내장 메소드처럼 로또도 하나의 자료형처럼 내장 메소드가 있으면 좋겠다! 생각해서 진행했습니다.아래와 같이 사용할 수 있습니다.
이렇게 되니까 당첨로또와 로또티켓을 비교해주는 LottoMatcher 클래스가 통째로 무쓸모가 되어버려서... 삭제했습니다 !
뭔가 헬퍼 역할을 하는 클래스가 이제 LottoPayment 하나만 남았기 때문에, 코드 일관성이 떨어진 것 같아 고민중 입니다 🤔
일단 bonusNumber 의 비교는 contain 으로 충분한 것 같아서 matchNumbers 에서는 일반번호만 비교합니다 !
하지만 이 역시 순수하게 Lotto 만을 고려한 작업이 아니라, 전체적인 로직을 고려한 일로 느껴지기 때문에 개선 방법을 고민중 입니다 🤔 (무한 고민..)
규칙을 관리해주는 클래스를 추가했습니다 (LottoRuleSet)
기존에는 LottoSystem 에서 하던 몇가지 일들을 분리하기 위해서 ... 여러 시도를 해보다가 규칙을 관리하는 친구를 만들었습니다
남은 액수를 받아서 각 순위의 profit 을 증가시키는 역할도 있습니다 .. ! (규칙에 distribute 속성을 추가했습니다)
이런... 느낌... ⬇️
리뷰 받고 싶은 부분 + 궁금한 점
위에 작성한.. 고민한 부분의 결과(?)에 대해 의견이 궁금합니다 !
기존에 제가 구현한 cloneDeep 유틸이 인스턴스 메소드는 복사를 하지 않더라구요 ! 그래서 애를 좀 먹었는데... 이런 복사 함수를 간단하게 만들 수 있는 팁이 있을까요 ??
테스트 코드를 여러개를 작성해도, 직접 로또게임을 하면서 확인해보니 커버하지 못하는 케이스가 꼭 몇개씩 있었어요! 그래서 테스트케이스추가+재수정 과정을 여러번 거쳤습니다. 처음부터 꼼꼼하게 작성하기 위한 기준같은것이 있을까요 ! 음... 아니면 보통의 작성 순서라던지..!