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

[엘리] 로또 미션 제출합니다. #126

Merged
merged 23 commits into from
Feb 26, 2020

Conversation

YebinK
Copy link

@YebinK YebinK commented Feb 20, 2020

안녕하세요! 엘리라고 합니다 :D
이번 리뷰 잘 부탁드려요!

src/main/java/domain/LottoNumber.java Outdated Show resolved Hide resolved
src/main/java/domain/LottoResult.java Outdated Show resolved Hide resolved
Copy link

@jaeyeonling jaeyeonling left a comment

Choose a reason for hiding this comment

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

안녕하세요 엘리 리뷰를 맡은 재연링입니다.
요구사항에 맞게 잘 구현해주셨어요!
몇 가지 코멘트 남겼으니 확인하시고 궁금하신 점 있으면 DM 주세요 :)
좋은 주말 보내세요~

src/main/java/LottoApplication.java Outdated Show resolved Hide resolved
src/main/java/LottoApplication.java Outdated Show resolved Hide resolved
import java.util.*;
import java.util.stream.Collectors;

public class LottoNumbersDtoGenerator {

Choose a reason for hiding this comment

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

DTO란 데이터 전송만을 담당하는 일종의 구조체와 같은 역할을 합니다.
DTO를 만드는 것에 로직이 들어간다면 이는 비즈니스 로직에 해당될 확률이 높습니다.
DTO의 역할인 데이터 전송만을 담당해보는 것이 어떨까요?

Copy link
Author

Choose a reason for hiding this comment

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

6개 숫자가 담긴 SixLottoNumbersFactory로 이름을 수정하고, return type을 Set<LottoNumber>으로 바꿔줬습니다! 아직 DTO의 개념이 헷갈려 잘 반영했는지 모르겠네요..ㅎㅎ 제 개인적인 생각으로는 DTO를 사용하게 되면

new Ticket(SixLottoNumbersFactory.createRandom())
이 코드를

new Ticket(new SixLottoNumbersDTO(SixLottoNumbersFactory.createRandom()))
이런 식으로 값을 한 번 더 감싸서 만들게 되는데, 오히려 코드를 이해하는 복잡도만 늘어나지 않나 생각이 듭니다. 혹시 제가 잘못 이해한 부분이 있다면 언제든지 다시 피드백 주세요! :)

Copy link
Author

Choose a reason for hiding this comment

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

그리고 말씀해주신 3가지 방법을 보고나니 제가 짠 방식은 2. 도메인에서 DTO를 만든다. 이네요! 다음 2단계 미션 때는 Assembler를 써서 3번째 방법으로 리팩토링해보겠습니다 :)

src/main/java/domain/LottoNumber.java Outdated Show resolved Hide resolved
src/main/java/domain/LottoNumber.java Outdated Show resolved Hide resolved
src/main/java/view/OutputView.java Outdated Show resolved Hide resolved
src/main/java/view/OutputView.java Outdated Show resolved Hide resolved
src/main/java/domain/LottoProfit.java Outdated Show resolved Hide resolved
src/main/java/view/InputView.java Show resolved Hide resolved
src/main/java/view/OutputView.java Outdated Show resolved Hide resolved
Copy link

@jaeyeonling jaeyeonling left a comment

Choose a reason for hiding this comment

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

코멘트 반영 잘 해주셨어요 💯
해당 요청은 머지하도록 하겠습니다.
2단계에서 뵙겠습니다! 고생하셨습니다 :)

@jaeyeonling jaeyeonling merged commit 910a925 into woowacourse:yebink Feb 26, 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.

2 participants