Skip to content

feat: step2 구현 #4188

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

Open
wants to merge 1 commit into
base: sundongkim-dev
Choose a base branch
from

Conversation

sundongkim-dev
Copy link

안녕하세요.
시간이 없어서 인공지능의 힘을 빌렸,,, VIBE 코딩 어마어마하네요;;
피드백 부탁드립니다.

Copy link

@pci2676 pci2676 left a comment

Choose a reason for hiding this comment

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

리뷰를 확인해주세요.


import java.util.List;

public class LottoGame {
Copy link

Choose a reason for hiding this comment

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

ai를 이용해 바이브 코딩을 하셨으면 TDD를 하기 더 쉬웠을것 같은데요. 전체적인 클래스에 비해 테스트 코드가 빈약한것 같습니다. 😢

ai를 활용하는 것은 좋지만 지금 교육 프로그램에 참여하신 목적에 더 알맞게 객체지향적으로 TDD 사이클을 이용해 생각하는 연습을 하시면 더 도움이 되지 않을까 싶습니다.

지금은 너무 시간에 쫓겨 미션을 수행하기 위해 급히 바이브 코딩을 하신것 같은데 그러면 이 과정에서 얻어가시는게 너무 적지 않으실까 싶습니다.

Comment on lines +24 to +30
public static LottoNumbers fromText(String text) {
List<Integer> numbers = Arrays.stream(text.split(","))
.map(String::trim)
.map(Integer::parseInt)
.collect(Collectors.toList());
return from(numbers);
}
Copy link

Choose a reason for hiding this comment

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

view와 밀접한 로직인것 같습니다.

return from(numbers);
}

private void validate(List<LottoNumber> numbers) {
Copy link

Choose a reason for hiding this comment

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

호출되는 위치에 가깝게 정의해주면 좋을것 같습니다.


public LottoRank lottoRank(LottoNumbers winningNumbers) {
Set<LottoNumber> winningSet = new HashSet<>(winningNumbers.numbers());
long matchCount = numbers.stream().filter(winningSet::contains).count();
Copy link

Choose a reason for hiding this comment

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

한줄에 하나의 .만 찍어서 메서드 체이닝을 해주세요.

Comment on lines +11 to +15
static {
for (int i = MIN_NUMBER; i <= MAX_NUMBER; i++) {
CACHE.put(i, new LottoNumber(i));
}
}
Copy link

Choose a reason for hiding this comment

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

cache 좋네요~! 👍

Comment on lines +16 to +26
public LottoWinningRecord result() {
Map<LottoRank, Integer> rankMap = new EnumMap<>(LottoRank.class);
for (LottoRank rank : LottoRank.values()) {
rankMap.put(rank, 0);
}
purchased.forEach(ticket -> {
LottoRank rank = ticket.lottoRank(winningNumbers);
rankMap.put(rank, rankMap.get(rank) + 1);
});
return new LottoWinningRecord(rankMap);
}
Copy link

Choose a reason for hiding this comment

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

아무런 매개변수 없는 메서드를 이용하는 것이라면 matcher라는 클래스가 반드시 필요할까요?

Comment on lines +15 to +16
private static final Map<Integer, LottoRank> MATCH_MAP = Arrays.stream(values())
.collect(Collectors.toMap(rank -> rank.match, Function.identity()));
Copy link

Choose a reason for hiding this comment

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

map으로 별도의 구현체를 만들어두신 이유가 있나요?

long totalPrize = rankMap.entrySet().stream()
.mapToLong(e -> e.getKey().money() * e.getValue())
.sum();
return totalSpent == 0 ? 0 : (double) totalPrize / totalSpent;
Copy link

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 +50
@RepeatedTest(5)
@DisplayName("여러 번 실행해도 로또 번호가 매번 섞여 나온다")
void generate_returnsShuffledNumbers() {
LottoNumbers first = LottoNumberGenerator.generate();
LottoNumbers second = LottoNumberGenerator.generate();

List<Integer> firstList = first.numbers()
.stream()
.map(n -> n.getNumber())
.collect(Collectors.toList());
List<Integer> secondList = second.numbers()
.stream()
.map(n -> n.getNumber())
.collect(Collectors.toList());

assertThat(firstList).isNotEqualTo(secondList);
Copy link

Choose a reason for hiding this comment

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

repeatedtest 할 필요는 없어보입니다~

import java.util.List;

public class LottoGame {
public static void main(String[] args) {
Copy link

Choose a reason for hiding this comment

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

LottoGame자체는 테스트가 불가능할까요?

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