-
Notifications
You must be signed in to change notification settings - Fork 192
[클린코드 8기 양지안] 로또 미션 STEP 1 #323
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: inasunnyspot
Are you sure you want to change the base?
Conversation
|
||
get bonusNumber() { | ||
return this.#winningNumbers; | ||
this.#bonusNumber = this.#inputValidator.validateBonusNumber(bonusNumber); |
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.
LottoGameConroller에서 amount, bonusNumber, winnigNumbers는 사실 멤버변수로 필요는 없지만 테스트 코드를 간단하게 작성하기 위해서 만들었습니다. 테스트코드를 위한 코드라는게 어색하지만,, 수업때 이렇게도 할 수 있다고 알려주셔서요. 제대로 이해한게 맞을까요? 근데 또 이렇게 되면 클래스를 사용하는 경우, 3개 이상의 인스턴스 변수를 가진 클래스를 쓰지 않는다.
을 위반하는게 될텐데, 테스트 코드에서만 사용하니까 상관없을지 궁금합니다.
} | ||
} | ||
|
||
export default WinningNumbers; |
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.
그리고 WinningNumbers 클래스를 만들어야할지 고민이 많았는데요, 별 다른 기능은 없고 단순히 당첨 번호와 보너스 번호만 가지고 있는데 없애는게 좋았을까요?
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.
우리가 다음 시간에는 객체지향에 관해서 배우니까 로또 객체를 받아서 WinningNumbers 객체에서 처리해 본다면, 지금 하고 계신 고민이 해소될 수도 있지 않을까요?
); | ||
} | ||
|
||
async startLotto() { |
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.
startLotto에서 로또 게임 프로세스를 전반적으로 관리하도록 만들었는데, 좀 복잡해진 것 같기도 하고.. 함수를 더 분리해야 할까요?
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.
저의 생각에는 굳이 Controller라는 큰 개념이 필요해 보이지 않아 보여요! 왜냐하면, 우리는 단위 테스트를 기준으로 작은 단위로 필요한 기능만 TDD로 사이클을 돌면서 점진적인 리팩터링을 하고 있는 걸 배웠는데, 지금의 상황은 오히려 반대 상황 같아 보입니다!
정리하자면, 저는 Controller를 해체(?)하는 한이 있더라도 이들의 기능을 각기 작은 단위(Lotto, LottoGenerator, WinningNumbers 등)에 역할을 부여해 주는 거라고 생각합니다! 예를 들어, Lotto 번호를 정의할 때에도 1~45 범위인지 검사하는 역할은 Controller가 아니라 Lotto에서 진행해야 하지 않을까요?
@JunilHwang 님! 요거 제가 리뷰 담당인 거 같습니다 ㅎㅎ |
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.
안녕하세요. 지안님!
리뷰 확인했습니다 :)
미션을 열심히 진행해 주셔서 감사 드려요🙏🏻
프로그램 요구사항이 생각보다 별 거 아닌 것 같아 보이지만, 처음이시라면 depth를 1단계로 허용하는 것은 생각보다 어렵습니다ㅠ. 또한, 매개변수를 2가지로 제한한다는 것도 처음엔 익숙지 않죠.
그래서 제가 과감히 제안을 드리고 싶은데요.
제 생각에는 Controller라는 객체를 먼저 떠올리신 다음에 로또를 구현하신 것 같습니다. 어떤 방식으로 개선하시든, 지안님이 편한 방식대로 개선하시되, Controller의 역할을 일부 작은 단위의 클래스로 이양해 보시는 것은 어떨까요? Controller에서 하는 일이 너무 많습니다. 또한, LottoService와도 어떤 기준으로 일을 나누셨는지 불분명해 보여서 납득하기 어려운 부분들이 있었어요. 알고 있는 프로그래밍 패턴을 먼저 적용하지 마시고, 지안님께서 떠올리시는 사고 과정을 먼저 코드로 옮겨보시면서 점진적으로 개선하는 훈련이 필요하실 것 같아요.
이런 막막함은 질문으로 남겨주신 아래의 내용을 살펴보신다면, 감이 조금 오실 수 있을 거라고 생각했어요. 아래의 내용 참고하셔서 코드를 개선해 주시면 감사하겠습니다 :)
또한, 유지보수하기 쉽고, 클린한 코드를 만드는 데에는 정해진 방향성은 있으나, 꼭 하나의 답만 존재하는 것은 아닙니다. 따라서, 너무 부담갖지 마시고 지안님의 생각을 코드로 자유롭게 표현해 주시면 좋을 것 같아요🙂 리뷰어로서 지안님의 사고과정을 도우면서 클린한 코드를 작성하실 수 있도록 돕는 게 제 몫이니까요.
그리고 순수 도메인 로직에 대한 개선과 관련된 피드백까지 한꺼번에 드리기에는 혼란스러우실 수도 있어서, 다음 피드백에서 드릴게요!
조금만 더 고생해 주세요!
감사합니다.
Q&A
매개변수를 2개로 제한하는 이유
- 매개변수로 넘길 것들이 많아진다면, 내가 어떤 순서대로 넘기는지, 그리고 어떤 타입으로 넘기는지 복잡해지지 않을까요? 물론, 매개변수 많이 넘겨야 할 만한 요소가 있을 수도 있겠는데 이 미션에서는 충분히 2개로도 구현할 수 있으실 거예요. 불필요한 요소를 넘기고 있는 건 아닌지도 한번 점검해 보시면 좋겠습니다!
- 다른 의도도 있었는데요. 말씀하신 대로 객체로 넘기는 방법입니다. 2개 이상으로 넘겨야 하는 매개변수가 여러가지라고 생각하신다면, 하나의 object(key:value 한 쌍)를 넘겨보시는 것도 하나의 방법이에요. 그 이유는, object key, value는 순서대로 넘기지 않아도 되니까요! 하지만 자바스크립트의 특성상 타입 검사를 하지 않으므로, 이런 부분들은 고려하셔야겠죠?
도메인, 컨트롤러를 나누는 기준
접근 방법을 조금 개선해 보시면 좋겠습니다. 애매하다고 느끼시더라도 그 순간 최선이라고 생각하시는 객체에 역할을 부여해 보시는 것은 어떨까요? TDD를 다시보기하신다면, 저도 처음에는 자동차 객체에서 무작위 숫자 값을 넣어서 전진 메서드를 다뤘지만, 자동차는 전진 메서드에서 무작위 값의 조건만 가지고 있고, 무작위 값을 주입하는 건 외부에서 해도 된다고 떠올리고 나중에 역할을 외부 객체로 넘겼습니다. 마찬가지로, 로또에서도 이런 접근법을 사용해 보시는 건 어떨까요?
그러기 위해서는, 제 생각에 controller
라는 개념은 지워두시는 편이 좋다고 생각해요. 왜냐하면, 지안님께서 애매하면 controller가 모두 담당하도록 염두에 두셨다고 생각하셨던 건 아니었을까요? 제 생각에는, 최대한 TDD 방식으로 도메인을 구현하시면서 해당 객체의 역할이 아닌 것 같다는 생각이 드신다면, 그때 새로운 객체를 만들어 보시는 것도 좋겠습니다. 단, 그게 controller
와 같이 애매한 개념은 아니었으면 좋겠어요. 이 미션에서 MVC 패턴을 떠올리셨다면 그건 적절하지 못한 방향이라고 생각합니다 🥲. MVC는 진리가 아니라는 점을 꼭 기억해 주세요.
약간 애매하면 일단 controller에 두자.. 하는 마음이 생기는 것 같습니다..
이러한 관점이라면LottoService
도 어떻게 분류하시게 될지 조금 더 감이 오실 거라고 생각합니다!
TDD 진행 기준
- 가장 먼저 작은 단위의 기능 명세서를 만든다
- 확정되지 않아도 좋습니다. 저도 계속 기능 요구사항을 수정하면서 개발했어요! TDD의 본질은 '빠른 피드백'이지만, 그걸 이루기 위해서는 기능 요구사항을 확정짓는다는 마음보다는 떠오르시는 대로 일단 구현부터 하시는 겁니다💪🏻 모든 요구 사항과 역할이 불분명하지는 않을 테니까요.
- 역할을 분리하되, 확정짓지 않는다.
- 자동차 경주에서 move메서드에서 랜덤 값을 생성하는 역할도 초반에 부여했습니다. 하지만, 점진적으로 기능을 하나씩 만들면서 난수 생성 역할을 외부로 이전했습니다. 이처럼, 로또에서도 마찬가지의 방식을 적용해 보시면 좋겠어요.
감이 오지 않으신다면, 자동차 경주로 진행한 TDD로 다시보기를 진행하시면서 제가 작성했던 것과 중간 중간 멈춰 보시면서 똑같이 따라하시되, 어떻게 기능을 확장해 나갔는지를 떠올리시면 됩니다. TDD라는 사이클이 한 번에 이해되고 숙달되기에는 많이 어려운 개념이지요. 분명히 작은 단위부터 연습하시다 보면 충분히 하실 수 있을 겁니다!
@@ -0,0 +1,18 @@ | |||
import Lotto from "../../src/domain/Lotto.js"; | |||
|
|||
describe("Lotto 클래스 테스트", () => { |
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.
로또에서 요구 사항을 기준으로 부합하지 않은 값이 들어온다면, 예외 처리를 어떻게 할지도 테스트를 해야 하지 않을까 싶습니다! 예를 들어 Lotto 클래스에 46
이나 0
이라는 값이 인입된다면, 예외 처리를 해줘야 하지 않을까요?
오히려 컨트롤러 단에서는 Lotto에서 나는 오류를 잡아주는 것이 타당하지 않을까요? 왜냐하면, 컨트롤러에서 하는 일이 너무 많습니다. 피드백에서 다룬 내용은 최대한 작은 단위로 나누면서 역할과 책임을 분명하게 나누는 것에 있으니까요 🙂
} | ||
} | ||
|
||
export default WinningNumbers; |
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.
우리가 다음 시간에는 객체지향에 관해서 배우니까 로또 객체를 받아서 WinningNumbers 객체에서 처리해 본다면, 지금 하고 계신 고민이 해소될 수도 있지 않을까요?
#validateAmount(amount) { | ||
if (/^\d+$/.test(amount)) { | ||
if (amount < LOTTO.PRICE) { | ||
throw new Error(ERROR_MESSAGE.MIN_PRICE); | ||
} | ||
return true; | ||
} | ||
throw new Error(ERROR_MESSAGE.ONLY_NUMBER); | ||
} |
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)을 확인해 보시면요. validate는 일반 동사로 유효성 검사만을 수행해서 유효하지 않은 경우 예외 메시지만 발생시키는 메서드로 활용하자고 피드백을 드렸어요! 그런데 지금 구현하신 메서드에는 유효할 경우 true를 반환하고, 그렇지 않은 경우 예외 메시지를 발생시키고 있네요!
언뜻 이 메서드만 보게 되면, 한 가지 기능만 담당하고 있어 보이기는 하지만요. 제 생각에는 일관성이 조금 아쉽습니다. 왜냐하면, 아래에서 구현하신 isValidateAmount는 이 메서드를 호출해서 결과 값을 참조해 버리고 있는데, 오히려 isValidateAmount에서 자체적으로 true/false의 값을 리턴하고, validateAmount에서 isValidateAmount의 true/false 값에 따라 동작하도록 구현해야 보다 역할과 책임이 명확하지 않을까요?
); | ||
} | ||
|
||
async startLotto() { |
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.
저의 생각에는 굳이 Controller라는 큰 개념이 필요해 보이지 않아 보여요! 왜냐하면, 우리는 단위 테스트를 기준으로 작은 단위로 필요한 기능만 TDD로 사이클을 돌면서 점진적인 리팩터링을 하고 있는 걸 배웠는데, 지금의 상황은 오히려 반대 상황 같아 보입니다!
정리하자면, 저는 Controller를 해체(?)하는 한이 있더라도 이들의 기능을 각기 작은 단위(Lotto, LottoGenerator, WinningNumbers 등)에 역할을 부여해 주는 거라고 생각합니다! 예를 들어, Lotto 번호를 정의할 때에도 1~45 범위인지 검사하는 역할은 Controller가 아니라 Lotto에서 진행해야 하지 않을까요?
안녕하세요. 리뷰어님!
많이 늦었지만 드디어,, step1 제출합니다..!
프로그램 요구사항을 최대한 따르려고 했지만 depth를 1단계까지 허용하거나 매개변수를 2개로 제한하는게 쉽지 않은 것 같습니다..
매개변수를 2개로 제한하는 이유가 있을까요?
혹시 매개 변수가 너무 많아지게 될 경우 객체로 보내는 건 해결 방안이 될 수 있을까요?
그리고 저는 도메인과, 컨트롤러에 대해 나누는 기준이 너무 헷갈리는데 정확한 기준이 있을까요?
약간 애매하면 일단 controller에 두자.. 하는 마음이 생기는 것 같습니다..
LottoService도 컨트롤러라고 생각하는데(각종 도메인을 잘 조합하는 역할..?) 맞는 방향인지 잘 모르겠습니다.
그리고 이번 미션은 TDD인데 도메인을 역할에 따라 분리해야한다는 생각에 테스트 코드를 먼저 작성하지 못하겠더라구요..
TDD란 어떻게 하는 것일까요,,?
도메인을 작게 쪼개서 먼저 기본이 되는 Lotto 테스트 코드 작성하고 Lotto 클래스를 만들고,, 이렇게 진행했어야할까요?...