-
Notifications
You must be signed in to change notification settings - Fork 6
[홍건우] 로또 미션 Step1 #4
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: HongGunWoo
Are you sure you want to change the base?
Conversation
seeyoujeong
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
코드 잘 읽었습니다!
취향차이인 부분이 있다보니 리뷰를 몇개 못남겼지만... 열심히 읽었답니다! ㅎㅎ
random으로 생성되는 값에 대한 test 코드를 작성할 때 어떤 방식으로 구현해야할지 아직 감이 잘 오지 않는 것 같습니다.
제가 알기로는 랜덤 숫자를 생성하는 함수를 모킹해서 반환값을 지정해주고 테스트하는걸로 알고 있습니다!
오버 엔지니어링과 확장성을 생각한 개발은 뭐가 다를까요?
흠... 확장성과 오버 엔지니어링 그 중간을 계속 고민해봐야될 것 같아요.. 확장성을 고려하면서 필요 이상의 기능은 만들지 않는... 어렵네요 ㅎㅎ
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
제가 알고 있는 뷰는 데이터를 보여주는 역할을 하고 컨트롤러가 입력을 받아서 모델의 데이터를 조작하는 걸로 알고 있어요!
그래서 지금 뷰에 있는 기능들을 컨트롤러로 옮겨야 하지 않을까 생각이 듭니다!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
다시 생각해보니 ask 메서드를 통해 나오는 query가 있어서 뷰에 포함시켜도 될 것 같네요.. 어렵군요!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
저도 많이 고민했던 부분인데 console로 입력받는 부분에 필요한 query문이 만약 웹 서비스 형식으로 구현되어있다면 query 출력은 view가 담당하는게 맞지 않을까 생각해서 view로 위치시켰습니다!
| (ticket) => | ||
| ticket.matchedCount === info.matchCount && | ||
| (info.includeBonus | ||
| ? ticket.includedBonusNumber === info.includeBonus | ||
| : true) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이 부분을 분리해서 함수로 만들면 가독성이 높아질듯합니다!
| result.formattedResult.forEach( | ||
| ({ count, includeBonus, matchCount, reward }) => { | ||
| this.view.printMessage( | ||
| `${matchCount}개 일치${ | ||
| includeBonus ? ', 보너스 볼 일치' : '' | ||
| } (${reward}원) - ${count}개` | ||
| ); | ||
| } | ||
| ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
저의 출력 함수도 이렇게 구현해봐야겠어요 ㅎㅎ
| @@ -0,0 +1,32 @@ | |||
| import { Lotto } from '../model/Lotto.js'; | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
확장자 js로 해도 잘 작동되나요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
문제없이 잘 작동하는데 문제가 될만한 부분이 있을까요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
아 타입스크립트인데 js로 되어있어서 궁금해서 여쭤봤어요 ㅎㅎ
jgjgill
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
수고하셨습니다..! 🙇♂️
random으로 생성되는 값에 대한 test 코드를 작성
으음 해당 테스트에서 중요한 부분은 랜덤이 아닌 그 결과값인 것 같아요.
그래서 랜덤 그 자체에 관심을 가지는 것이 아니라 랜덤에서 나올 값만 관심을 가지면 되지 않을까 싶어요.
근데 테스트 코드를 고려하지 않고 무지성 코드를 작성하면 이 부분이 와닿지가 않더라구요.
테스트 코드를 작성할 때 아 이 부분을 분리했어야 했다는 것을 깨달았던 것 같습니다.
결국은 경험이 답이다? 😇
| winningInfo: [ | ||
| { | ||
| matchCount: 4, | ||
| reward: 5000, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
5_000 가독성이 더 좋아질 것 같습니다..!
| }, | ||
| ], | ||
| }, | ||
| '2': { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
오 로또 규칙이 달라졌을 때 사용되는 타입인가요??
1, 2, 3 보다 의미있는 이름으로 하면 더 이해하기 좋을 것 같아요..!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
제가 로또를 잘 몰라서 규칙 이름 짓기가 애매하더라구요 ㅋㅋㅋㅋㅋ gpt도움을 좀 받아야겠습니다.
| const tickets: Lotto[] = []; | ||
|
|
||
| for (let _ = 0; _ < ticketCount; _++) { | ||
| const numbers = this.generateRandomNumbers(); | ||
| tickets.push(new Lotto(numbers)); | ||
| } | ||
|
|
||
| this.tickets = tickets; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이 부분 map으로는 사용할 수 없었을까요??
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
배열이 아닌 map 구조로 관리하는 걸 말씀하신 건가요?
지금 구조에서는 map으로 관리해야하는 필요성을 느끼지 못해서 배열로 설정했었는데 step2 진행해보면서 생각해보겠습니다!🙇♂️
| while (numbers.size < this.lottoRule.lottoType.selectedCount) { | ||
| const minNumber = this.lottoRule.lottoType.minNumber; | ||
| const maxNumber = this.lottoRule.lottoType.maxNumber; | ||
|
|
||
| numbers.add(getRandomNumber(minNumber, maxNumber)); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1만 100억 번 나오는 억까를 당한다면? 🤪 (딴지 걸기)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
아앗.... 랜덤 방식 준일님 방식으로!!...
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);
},학습 포인트:
추가될 수 있는 요구사항 예시와 대응:
이러한 변경사항들은 기존 코드의 구조를 크게 해치지 않으면서 새로운 요구사항을 수용할 수 있도록 합니다. 설정의 중앙화와 객체지향적 설계 덕분에 새로운 기능 추가나 규칙 변경이 비교적 쉽게 이루어질 수 있습니다. |
AI 답변 2네, PR 본문의 추가 내용을 포함하여 상세한 피드백을 드리겠습니다. PR 리뷰 피드백 (확장)1. 코드 분석 (추가)PR 본문에서 언급된 주요 포인트:
2. 강점 (추가)
3. 개선점 (추가)
4. 구체적 제안
5. 학습 포인트 (추가)
6. 향후 요구사항 변경 대비 (추가)
추가 답변
이러한 피드백을 통해 코드의 구조와 테스트 방식을 개선하고, 향후 요구사항 변경에 더 유연하게 대응할 수 있을 것입니다. 특히 MVC 패턴의 적용과 랜덤 값 테스트에 대한 접근 방식을 개선하면 코드의 품질과 유지보수성이 크게 향상될 것입니다. |
|
@coderabbitai review please |
|
@JunilHwang, thank you for requesting a review. Here's a detailed analysis based on the current implementation and the objectives outlined in your pull request: Review SummaryStrengths
Areas for Improvement
Specific Suggestions
Additional Observations
Learning Points
Future Requirements
ConclusionOverall, the project is well-structured with a clear focus on maintainability and scalability. Enhancing the test coverage and refining the MVC implementation will further improve the quality of the application. Keep up the good work! Actions performedReview triggered.
|
|
Warning Rate limit exceeded@JunilHwang has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 29 minutes and 14 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 (
|
미션을 수행하면서 어려웠던 점
TDD로 구현하기
TDD를 적용하기 위해 사용자 입장에서의 기능 명세와 개발자 입장에서의 상세 요구 사항을 작성해서 작성한 순서대로 구현하려고 했습니다.
요구사항대로 구현하다가 다른 기능이 추가되면 어떻게 하지? 라는 생각에 결국 TDD 구현이 점점 무너지는 것 같았습니다.😭
클래스를 사용하는 경우, 3개 이상의 인스턴스 변수를 가진 클래스를 쓰지 않는다.
해당 조건을 신경쓰면서 구현이 생각보다 쉽지가 않았습니다. 결국 LottoGame 모델이 인스턴스 변수가 3개가 되어버렸는데 아쉬움이 많습니다🥲.
나는 타입스크립트 바보다...
항상 세팅되어있는 프로젝트만 만지다가 다시 직접 세팅하려니 관련 세팅을 전부 까먹었네요...
test폴더와 src 폴더를 분리해 각각 환경에 맞는 tsconfig를 구축하고 싶었는데 다시 공부해야겠습니다🔥
리뷰 받고 싶은 부분
예를 들어 랜덤으로 생성된 값과 특정한 값을 비교해서 비교 결과를 알아야하는 경우에는 어떻게 하면 좋을까요?
TDD로 생각해봤을 때 랜덤값에 대한 비교보다는 결과값을 비교하는 함수에 대한 분리 및 비교해야하는 대상들을 인자로 넘겨주는 방식으로 구현하는 것이 더 맞는 방향인지 궁금합니다.
궁금한 점