로또 2단계 미션 제출합니다. #39
로또 2단계 미션 제출합니다. #39
Conversation
hyunssooo
left a comment
hyunssooo
left a comment
There was a problem hiding this comment.
안녕하세요. :)
몇 가지 코멘트 남겼습니다. 확인 부탁드립니다.
| public Lotto get(int ix) { | ||
| return values.get(ix); | ||
| } | ||
|
|
There was a problem hiding this comment.
이 메서드는 view에서 사용하고 있는데요. 웹 환경이라면 해당 메서드는 호출 가능한가요?
getter를 사용하지 말라는 말은 도메인 모델끼리 비즈니스 요구 사항을 풀어내는 경우를 말합니다.
view에서는 getter를 사용해 데이터를 출력하는 하면 됩니다.
There was a problem hiding this comment.
이 코멘트는 무슨 의미인지 이해하지 못했습니다.. 🤔
get메소드를 view에서만 사용하기 때문에 웹에서는 사용하지 않게 되어 사라질 메소드라 지양하는 의미로 받아들였는데 마지막 줄의 getter를 사용하여 데이터를 출력하라는게 무슨 의미인지 모르겠습니다.
There was a problem hiding this comment.
https://edu.nextstep.camp/s/MCLQmhAp/ls/DO2AJZZW
젤 아래에 getter 메소드 없이 구현 가능한가? 를 읽어보시면 좋을 거 같아요.
출력을 위해 DTO를 만드는 경우 getter를 사용하고 혹은 view에 도메인 객체가 나간다면 getter를 통해서 데이터를 그대로 꺼내면 된다는 의미 입니다.
hyunssooo
left a comment
hyunssooo
left a comment
There was a problem hiding this comment.
안녕하세요.
몇 가지 추가로 코멘트 남겼습니다. 확인 부탁드려요!
| public Lotto get(int ix) { | ||
| return values.get(ix); | ||
| } | ||
|
|
There was a problem hiding this comment.
https://edu.nextstep.camp/s/MCLQmhAp/ls/DO2AJZZW
젤 아래에 getter 메소드 없이 구현 가능한가? 를 읽어보시면 좋을 거 같아요.
출력을 위해 DTO를 만드는 경우 getter를 사용하고 혹은 view에 도메인 객체가 나간다면 getter를 통해서 데이터를 그대로 꺼내면 된다는 의미 입니다.
| Lotto lotto = lottoSystem.convertNumbersToLotto(manualNumbers); | ||
| allLottos.add(lotto); |
There was a problem hiding this comment.
lottoSystem의 역할은 무엇인가요? 메서드들을 보니 책임이 하나인 것 같지는 않아서요.
There was a problem hiding this comment.
로또 플레이할 때 필요한 기능들을 모아둔 클래스입니다. 어떻게 쪼개야할 지 몰라서 한 클래스에 모두 들어간 느낌이네요..
|
지금까지 너무 구조화가 잘 안돼있던 감이 있어서 좀 어려웠네요.. 추가로 코멘트 달아주시면 확인해보겠습니다! |
hyunssooo
left a comment
hyunssooo
left a comment
There was a problem hiding this comment.
안녕하세요.
몇 가지 가벼운 코멘트 남겼습니다.
확인 부탁드립니다!
함께 고민해 보고 싶은 부분은 코멘트나 DM 으로 남겨주세요!
| public Lotto convertNumbersToLotto(List<Integer> numbers) { | ||
| ManualLottoGenerator generator = new ManualLottoGenerator(numbers); | ||
| return generator.generateLotto(); | ||
| } | ||
|
|
||
| private List<Lotto> generateAutoLottos(long count) { | ||
| RandomLottoGenerator generator = RandomLottoGenerator.getInstance(); | ||
| return generator.generateAutoLottos(count); | ||
| } |
There was a problem hiding this comment.
이 부분은 제가 의도한거랑은 조금 차이가 있는데요. 🙄
List<LottoGenerator> lottoGenerators = List.of(
new RandomLottoGenerator(),
new RandomLottoGenerator(),
new ManualLottoGenerator(List.of(1, 2, 3, 4, 5, 6)),
new ManualLottoGenerator(List.of(1, 2, 3, 4, 5, 6))
);
new Lottos(
lottoGenerators.stream()
.map(it -> it.generate())
.collect(Collectors.toList())
);
이런 느낌을 기대하긴 했어요. 작성해 주신 구조라면 다형성이 크게 의미는 없을 것 같아서요.
| public Result scoreLottos(WinningNumber winningNumber) { | ||
| Result result = new Result(); | ||
| for (Lotto lotto : values) { | ||
| result.scoreLotto(lotto, winningNumber); | ||
| } | ||
| return result; | ||
| } |
There was a problem hiding this comment.
public Result scoreLottos(WinningNumber winningNumber) {
return new Result(
values.stream()
.map(it -> winningNumber.match(it))
.collect(Collectors.groupingBy(it -> it, Collectors.counting()))
);
}이런 방법도 있을 것 같아요.
이 경우 테스트는 더 유연해 지지 않을까요? 테스트 코드는 어떻게 변경될 수 있을까요?
There was a problem hiding this comment.
Result.reward는 필드에 꼭 필요할까요? 내부 Map 에서 계산할 수 있지 않을까요?
| private static RandomLottoGenerator instance; | ||
| private final List<Ball> ballPool = IntStream.rangeClosed(MIN_NUMBER, MAX_NUMBER) | ||
| .mapToObj(Ball::new) | ||
| .collect(Collectors.toList()); | ||
|
|
||
| private RandomLottoGenerator() { | ||
| } | ||
|
|
||
| public static RandomLottoGenerator getInstance() { | ||
| if (instance == null) { | ||
| synchronized (RandomLottoGenerator.class) { | ||
| if (instance == null) { | ||
| instance = new RandomLottoGenerator(); | ||
| } | ||
| } | ||
| } | ||
| return instance; | ||
| } |
There was a problem hiding this comment.
private static final RandomLottoGenerator instance = new RandomLottoGenerator();으로 사용해도 괜찮지 않을까요?
There was a problem hiding this comment.
이렇게 사용하면 동시성이슈는 없나요? 인터넷에 검색했을 땐 동시성이슈때문에 이렇게 구현하라는 말을 들어서요.
2 participants
안녕하세요!
이번 미션 쉽지 않네요. 수동 로또가 추가된 것으로 많은 수정이 필요했어서 확장에 약하다는 것을 느꼈습니다. 😭