Skip to content
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

[터틀] 로또 2단계 미션 제출 합니다. #184

Merged
merged 77 commits into from
Feb 27, 2020
Merged

[터틀] 로또 2단계 미션 제출 합니다. #184

merged 77 commits into from
Feb 27, 2020

Conversation

begaonnuri
Copy link
Member

@begaonnuri begaonnuri commented Feb 26, 2020

안녕하세요🙂

저는 이번 2단계 미션을 진행하면서 이것들을 중점적으로 생각했습니다!

  1. 전략패턴을 적용해보자

    • 설계에 부족함을 느껴서 '개발자가 반드시 정복해야 할 객체 지향과 디자인 패턴' 책의 디자인패턴 부분을 중점적으로 읽었는데, 로또를 자동, 수동으로 구매하는 기능에 적용하면 적절할것 같아서 적용해봤습니다.
  2. View의 책임을 줄여보자

    • 1단계 미션을 마치고 코드를 다시 보니, ConsoleOutputView가 출력 이외의 로직을 너무 많이 갖고있는 것 같다고 생각해서 이부분을 개선하는데 중점을 뒀습니다.
  3. Controller 생성

    • 1단계 미션때는 Application이 Controller의 역할을 겸해도 괜찮다고 생각해서 그렇게 진행했습니다. 하지만 2단계 미션에서는 Application의 책임이 많다고 생각해서 Controller를 생성했습니다. 다만, 아직 이부분에 대해 부족해서 적절하게 적용했는지 잘 모르겠습니다.
  4. Mock

    • 테스트코드를 추가하다 보니 중복되는 코드가 너무 많이 생겨서 개선해야겠다고 생각했습니다. 저번 자동차 미션때 테스트 더블을 공부해보면 좋겠다는 리뷰를 받은 적이 있어서 어설프게나마 Mock을 적용해봤는데 부족한 부분 지적해주시면 감사하겠습니다.
  5. 역할과 책임

    • 이 객체의 역할과 책임이 적절한지 생각해보며 미션을 진행했습니다. 부족하거나 적절하지 않은 부분 지적해주시면 감사하겠습니다.

코로나 덕분에 시간이 조금 생겼습니다.
남은 시간동안 공부할 수 있도록 많은 지적과 고민거리 던져주시면 감사하겠습니다!🙇🏻‍♂️
9615643 커밋부터 2단계 커밋입니다!

 1. 정수만 입력 가능
 2. 음수와 0은 입력 불가능
 3. 1,000원 단위로만 입력 가능
 4. 최대 구입 가능 금액 100,000원 제한
 1. 생성자 대신 valueOf로 변경
 2. 테스트코드의 생성자를 valueOf로 수정
 - WinningLottoParser -> LottoNumberParser
 - getNumberOfLotto -> getPurchasedLottoCount
 - isContains -> contains
 - printPurchasedLotto -> printPurchasedAutoLotto
 - 캐싱한 로또 번호를 LottoNumber에서 LottoNumberFactory로 이동
- README.md에 구현할 기능 추가
 - 검증 기능 추가
 - InvalidManualLottoException 클래스 추가
 - 테스트 코드 추가
 - Controller에서 domain을 제어하도록 변경
 - ManualLottoCount 생성자의 인자를 LottoMoney 객체에서 int(구매한 로또 수)로 변경
 - lotto패키지 생성
 - lotto관련 클래스 이동
 - exception패키지 생성
 - exception관련 클래스 이동
 - strategy패키지 생성
 - strategy관련 클래스 이동
 - LottoNumber의 Set을 반환하는 기능 추가
 - Lotto를 반환하는 기능 추가
 - Lottos를 반환하는 기능 추가
 - 테스트코드에 lotto, mock 패키지 추가
 - 로또 당첨자를 관리하는 Map을 WinningResult 클래스로 포장
 - 수익률을 구하는 기능을 WinningLotto에서 WinningResult로 이동
 - printWinningResult에서 하던 비즈니스 로직을 WinningResult로 이동
 - LottoController에서 로또를 구매하는 기능을 메소드로 분리
- ConsoleOutputView의 메소드들이 출력의 책임만 하도록 변경
 - 출력 이외의 로직 제거
 - getter 대신 toString을 사용하도록 변경
- 로또마다 enum을 순회해 순위를 찾던 방식에서 map을 캐싱해놓고 찾아오는 방식으로 변경
- 변수로 분리
- 공백 추가
- 변수명 변경
- 메소드 인자 변경
- LottoMoney에서 연산하는 기능을 WinningResult로 이동
- LottoMoney의 money필드를 long에서 int로 변경
- 상금을 LottoMoney 객체에서 int로 변경
- 중복되는 기능의 메소드 제거
- LottoRank 클래스 이동
- WinningLotto 클래스 이동
- WinningResult 클래스 이동
Copy link

@young891221 young891221 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

안녕하세요, 터틀!
패턴이나 여러가지 리펙토링을 열심히 해주신게 느껴집니다 💯
컨트롤러 내부 리펙토링만 처리하면 훨씬 개선될 것 같아 코멘트 남겼습니다. 확인 부탁드려요!

Comment on lines 35 to 36
final int manualCount = manualLottoCount.getCount();
final int autoCount = purchasedLottoCount - manualCount;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이걸 계산해 주는 로직이 mergeManualAndAuto() 여기로 들어가면 좋겠네요

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

인자들의 가독성을 위해 저렇게 따로 계산했었는데, purchaseLotto()의 책임이 너무 많아졌던것 같네요. 감사합니다 :)

return totalLottos;
}

private Lottos mergeManualAndAuto(int manualCount, int autoCount) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

여기 메서드의 기능이 모두 lottoStore로 위임될 수 없을까요?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

저는 LottoStore의 역할이 '사용자가 로또를 구매한다'라고 생각했습니다. 수동로또와 자동로또를 합치는 일은 로직을 한번에 처리하기 위한 행위로 Controller에 있는게 적절하다고 생각했는데, 어떻게 생각하시는지 조금 더 구체적으로 알 수 있을까요?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

음... 개인적으로 컨트롤러라는 클래스의 역할이 모호하다 생각합니다.
보통은 MVC에서 말하는 컨트롤러를 의미하니까요. 여기서는 로또의 중개자 역할을 수행하고 있는데요. 만약, 말씀하신데로 사용자가 로또를 구매한다라고 생각하셨으면 이 메서드 자체도 User라는 클래스로 분리하여 처리하는게 더 맞다고 생각되네요. 어떻게 생각하시나요??ㅎㅎ

Copy link
Member Author

@begaonnuri begaonnuri Feb 27, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

말씀해주신것 처럼 MVC 패턴의 컨트롤러를 흉내내봤습니다. 중개자 역할 이 컨트롤러의 역할이라고 생각하면 mergeManualAndAuto()가 있는게 어색하네요. 말씀해주신 내용을 정리해보면,

  1. 컨트롤러 대신 User 도메인클래스를 만들어 사용자의 행위를 중심으로 로직을 진행한다.
  2. 컨트롤러가 중개자 역할에 집중하도록 한다.

두가지로 개선할 수 있겠네요! 저는 두가지 개선방향에 양다리를 걸치고 있었던것같아요. 조언해주셔서 감사합니다. 2번으로 개선해보겠습니다!

@begaonnuri
Copy link
Member Author

변경을 하고 나니 메소드명이 조금 어색한것 같습니다. 하지만 실제로 하는 행위를 보면 어색하지 않습니다. 메소드명은 행위가 가지는 의미가 적절할까요? 아니면 실제로 행해지는 행위가 적절할까요?

Copy link

@young891221 young891221 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

질문 주신 부분은 케바케겠지만 일반적으로 메서드명은 객체가 행하는 의미적인 측면이 더 강한 것 같습니다. 예를 들어, "코드상에서는 LottoStore가 purchaseLotto()를 한다."라는 의미로 해석하는 측면으로요.
이번 단계 수고하셨습니다ㅎㅎ 머지할게요~!


import static spark.Spark.get;

public class WebUILottoApplication {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

web ui 기능이 벌써 들어와 있네요 ㅎㅎ;

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

앗 conflict 없애다가 추가됐나봐요🙄

@young891221 young891221 merged commit 39853d2 into woowacourse:begaonnuri Feb 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants