Final #2101
Final #2101
Conversation
DTO를 사용하지 않을 계획이기에 LottoResult가 직접 당첨금 정보를 가지고 있어야 한다.
| import lotto.domain.lotto.entity.Lottos; | ||
| import lotto.view.dto.LottoResultDTO; | ||
| import lotto.view.io.Printer; | ||
|
|
There was a problem hiding this comment.
문제 요구사항에 따라 변동성이 있겠지만,
만약 로또 미션처럼 출력문이
[~~, ~~, ~~, ~~, ~~]
[~~, ~~, ~~, ~~, ~~]
[~~, ~~, ~~, ~~, ~~]
구조이고 + 리스트를 활용할 수 있는 구조라면 리스트의 기본 출력 형식이 [~~, ~~, ~~, ~~]이기 때문에
FORMAT_LOTTO_NUMBER와 같이 상수로 뽑지 않고, String.join 같은 것만 잘 활용해서 진행하시면 시간 단축에 미약하게나마 도움 되실 것 같습니다!
There was a problem hiding this comment.
맞습니다!
사실 이 부분은 처음에는 말씀처럼 리스트의 toString()의 기본형식을 사용했다가, 이후에 리팩토링할 시간이 조금 생겨 바꾸어 준 부분입니다!
아마 최종 코테에서도 같은 형식이라면 그냥 toString() 기본 형식을 먼저 사용할 것 같습니다!
| public static final String TITLE_RESULT = "당첨 통계"; | ||
| public static final String MESSAGE_SEPERATOR = "---"; | ||
| private final Printer printer = new Printer(); | ||
|
|
There was a problem hiding this comment.
Domain 객체를 출력문에서 활용하느냐 안하느냐 개인적으로 정--말 큰 고민이네요 ㅎㅎ..
There was a problem hiding this comment.
정말로요....ㅎㅎ
저는 "기본적으로 domain 객체를 출력에서 직접 사용하는 것에 대한 사이드 이펙트가 크지 않기에 그대로 사용한다" 라는 의견을 고수해 왔는데요.
로또 미션에서 등수를 출력할 때 "보너스 볼 일치" 부분을 처리하는 것 때문에 고민하다가 DTO를 사용하는 쪽으로 리팩토링이 진행되었습니다.
하지만 이 부분은 지금은 각자 손에 익은 방법을 사용하는 것이 정답이지 않을까 하고 생각합니다 ㅎㅎ
| package lotto.view.dto; | ||
|
|
||
| import lotto.domain.lotto.entity.LottoResult; | ||
|
|
There was a problem hiding this comment.
DTO가 View의 책임도 갖고 있는 것 같습니다! 아니면 출력 형식 정도는 DTO가 가져도 된다고 생각하시는걸까요? DTO의 책임은 어디까지인지 참 모호한 경계에 있는 것 같기도 하네요!
시간관리 + 구현의 편리성을 염두하여 구현하신 것 같아 리뷰가 조금 조심스럽지만 그래도 코멘트 드려봅니다 😊
There was a problem hiding this comment.
어라, 생각해보니 DTO가 출력 형식과는 크게 상관이 있는 클래스가 아니군요!
이 부분은 완전한 실책이네요 ㅋㅋㅋ....
처음엔 엔티티가 직접 출력 형식을 관리하고 있었습니다. 그러다 출력의 책임을 분리해 주고자 "DTO"를 만들었는데 이 마저 어색했었군요.
하지만 이제 저희에겐 고정된 출력에 구멍만 뚫어 렌더링 하는 치트키가 생겼기 때문에 크게 걱정이 되진 않습니다 ㅎㅎ
| String input = Console.readLine(); | ||
| return parseInt(input); | ||
| } | ||
|
|
There was a problem hiding this comment.
Console 입력을 인터페이스로 분리 vs 직접 참조하는 부분도 개인적으로 고민인 사항인데,
입력값에 대한 테스트는 시간관계상 힘들 것으로 판단하여, 직접 참조하여 구현하신걸까요?
There was a problem hiding this comment.
네, 그렇습니다!
우선, 인터페이스를 분리하는 것은 "입력 방법이 변경되는 상황은 비즈니스 요구사항의 변경 가능성보다 작다" 라고 생각해 그냥 구현클래스에서 직접 사용할 예정입니다.
또, 테스트에 대해선 저는 입력 값에서의 예외 상황은 "숫자 입력이 필요할 때 숫자 형식이 아닌 입력", "구분자로 시작하거나 끝나는 입력" 정도만 검증해 주며 나머지는 모두 도메인 생성시 검증을 하는 편인데요.
그렇기에 미리 만들어 두었던, 안전하다고 생각되는 클래스를 그대로 가져가 사용하고 테스트는 별도로 진행하지 않을 생각입니다!
| public BigDecimal getPrize() { | ||
| return prize; | ||
| } | ||
|
|
There was a problem hiding this comment.
이렇게 하면, 플레이어 최종 상금을 계산할 때 getter를 안쓸 수 있겠군요! 👍
| return Collections.unmodifiableMap(this.lottoResultCount); | ||
| } | ||
|
|
||
|
|
There was a problem hiding this comment.
아마 이런식으로도 리팩터링 해볼 수 있을 것 같습니다!
List<BigDecimal> list = this.lottoResultCount.keySet().stream()
.map(lottoResult -> lottoResult.getTotalPrize(rankMatchCounts.get(lottoResult)))
.toList();
// or
List<BigDecimal> list = this.lottoResultCount.keySet().stream()
.map(lottoResult -> {
int count = lottoResultCount.get(lottoResult);
return lottoResult.getTotalPrize(count);
})
.toList();저는 entrySet보다 keySet이 조금 가독성이 괜찮은 것 같더라구요. 취향 차이 같아서 가볍게 보시면 좋을 듯 해요 😊
There was a problem hiding this comment.
아하 내부에서 다시 Map의 값을 가져오는군요?
저 역시 Entry 자체를 스트림에서 사용하면 람다식이 너무 뚱뚱해져 별로 좋아하지 않았는데, 좋은 의견이네요!!
감사합니다 👍
|
|
||
| public class Lotto { | ||
| private final List<LottoNumber> numbers; | ||
|
|
There was a problem hiding this comment.
stream ~ toList를 메서드로 분리하면 조금 더 깔끔해질 것 같아요!!
| } | ||
| } | ||
|
|
||
| // TODO: 추가 기능 구현 |
There was a problem hiding this comment.
getXXX 메서드 네이밍은
추후에 만약 getter 을 걷어내는 리팩터링을 수행한다면, 여러 메서드가 뒤섞여 추적하기 어려울 수도 있겠다는 생각이 들더라구요!
하지만 이를 대체할 수 있는 calculateXXX 는 메서드 네이밍이 너무 길어지는 것 같기도해서, 혁수님 생각이 궁금해요!
There was a problem hiding this comment.
이 부분에 대해 이전에도 피드백을 받은 적이 있는데요.
아직 저만의 기준이 잡히진 않았고 저 역시 고민중인 주제입니다.
getter를 사용하는 부분은 내부 필드 변수를 가져오는 상황에 가장 맞는 네이밍인 것 같기도 해요.
다만 ~~한 정보를 가져온다. 라고 할 때 get을 제외하고 그 뉘앙스를 잘 살려줄 수 있는 다른 네이밍을 찾지는 못했기에 일단 임시방편으로 사용하는 중입니다.
상황에 따라 create~~와 같은 네이밍을 사용하거나, 조금 더 동작에 집중해 matchingNumbers와 같은 네이밍을 사용할 수도 있겠네요.
추가적으로 상황을 잘 설명하기 위함이라면 너~무 길지 않은 메서드 명은 오히려 좋다고 생각하는 편입니다!
There was a problem hiding this comment.
평균적으로 10~16자 정도의 변수 및 메소드 명을 사용하면 디버깅하기 쉽다고 알고 있습니다.
구글 코드 베이스도 변수명도 평균 10자 정도로 기억해요.
이런 경향은 언어에 따라 달라지기도 해서 Go언어 같은 경우는 this를 아예 클래스명의 첫글자로 축약해버립니다.
Person이면 p.name 이런식으로요.
Java는 간결한 것보다 명시적인 변수, 메소드명을 선호한다고 알고 있어서 calculateXXX 정도는 조금 길더라도 명시적이고 이해하기 어렵지 않은 이름이라고 생각됩니다.
말씀해주신대로 getter와 헷갈릴 여지도 줄일 수 있을 것 같아요.
너무 길다면 calcXXX 정도로 줄일 수도 있을 것 같습니다.
min, max, temp 등의 관례적으로 줄여서 쓰는 이름은 팀 간의 소통만 잘 하고 사용하면 문제가 없다고 봤는데, calc도 여기 해당되지 않나 싶어요.
지금은 개인 코드라 편한대로 사용해도 좋을 것 같네요.
| public class LottoNumber implements Comparable<LottoNumber> { | ||
|
|
||
| public static final int LOTTO_RANGE_MAX = 45; | ||
| public static final int LOTTO_RANGE_MIN = 1; |
There was a problem hiding this comment.
MIN이 prefix로 붙는 것이 좀 더 관례에 맞고 명시적이라는 느낌이 드는데 큰 문제는 없어 보입니다.
There was a problem hiding this comment.
아 그런가요? 공통된 PREFIX를 사용해 값을 찾는데 더 도움이 되었으면 하는 생각으로 지은 네이밍이긴 합니다..!
|
|
||
| import lotto.domain.lotto.entity.LottoResult; | ||
|
|
||
| public class LottoResultDTO { |
There was a problem hiding this comment.
DTO가 Data Transfer Object의 축약어여서 대문자로 표기하신 것으로 보입니다.
저도 얼마 전까지 축약어를 대문자로 표기했었고, 좀 더 축약어임을 명시적으로 보여줘서 더 좋다고 생각했었는데,
사용하다 보니 단점도 깨닫게 되어 제 경험을 공유하고자 남깁니다.
축약어를 모두 대문자로 표기할 경우, 예를 들어 api와 id는 모두 축약어기 때문에 변수명 my_api_id를 CamelCase로 표기하면, myAPIID가 됩니다.
물론 변수명은 lower camel case이기 때문에 조금 나은 편이지만,
만약 클래스명인 경우, MyAPIID가 됩니다.
이 경우, 네이밍 컨벤션으로 인해 upper snake case인 상수와 헷갈릴 여지가 주어진다고 생각됩니다.
그래서 저는 개인적으로 축약어라도 그냥 Dto와 같이 표기하는 것을 선호하게 되었는데,
결국 나중에 팀에서 정한 규칙을 따르는 것이 최우선일 것이고, 이런 문제가 발생할 수도 있다고 인지만 해둬도 좋을 것 같습니다.
There was a problem hiding this comment.
글을 쓰다가 생각났는데 Java의 UUID는 대문자고 requestURL도 축약어를 대문자로 표기하는데 HttpRequest는 또 HTTPRequest로 표기하진 않아서 더 헷갈렸던 것 같습니다.
There was a problem hiding this comment.
오, 생각지 못했던 부분이네요!
말씀 듣고 보니 저는 무조건 첫 글자만 대문자로 작성하는 쪽으로 기준을 세울 것 같네요
There was a problem hiding this comment.
언어가 java가 아니라 dart기는 한데 구글에서 작성한 문서라 신뢰성이 보장되는 문서 링크를 하나 남깁니다. 이 부분에 대해 고민하다가 우연히 찾게 되어 도움을 받았던지라 다른 분들도 한 번 보시면 좋을 것 같아요.😊
그리고 OAuth는 이 규칙에서 예외적으로 Oauth가 아닌 OAuth가 보편적으로 사용되는 것 같습니다.
아직 저도 많이 보지는 못했는데, 이런 예외적인 케이스도 공부하다보면 더 보게될 것 같아요.
| void run() { | ||
| //로또 구입 | ||
| Money purchaseMoney = getPurchaseMoney(); | ||
| Lottos lottos = purchaseLotto(purchaseMoney); | ||
| printPurchasedLotto(lottos); | ||
|
|
||
| //정답 생성 | ||
| LottoAnswer answer = getLottoAnswer(); | ||
|
|
||
| //결과 출력 | ||
| LottoResultCount results = lottos.getResults(answer); | ||
| printResult(results); | ||
| printRevenu(purchaseMoney, results); | ||
| } |
| LOSE(new BigDecimal(0), 0, MatchBonusNumber.IGNORE), | ||
| FIFTH(new BigDecimal(5_000), 3, MatchBonusNumber.IGNORE), | ||
| FOURTH(new BigDecimal(50_000), 4, MatchBonusNumber.IGNORE), | ||
| THIRD(new BigDecimal(1_500_000), 5, MatchBonusNumber.NOT_MATCH), | ||
| SECOND(new BigDecimal(30_000_000), 5, MatchBonusNumber.MATCH), | ||
| FIRST(new BigDecimal(2_000_000_000), 6, MatchBonusNumber.IGNORE), |
There was a problem hiding this comment.
@zangsu
수식 계산의 안정성과 BigDecimal가 제공하는 기능을 위해서는 사용하는 게 맞다고 보는데, new로 생성하는 부분을 개선할 수 없을까요?
특성상 하나의 값들이 부여되어야 한다는 것을 머리로는 알지만, 여전히 고민 중인 부분이기에 코멘트 달아봤습니다. 🥲
There was a problem hiding this comment.
어떤 점 때문에 new로 인스턴스를 생성하는 부분을 개선하시고 싶으신건가요?
고민하고 계신 부분을 조금 더 설명해 주시면 저도 좋은 고민거리가 될 수 있을 것 같습니다!
There was a problem hiding this comment.
@zangsu
예를 들면 테스트와 런타임 시 를 말씀드릴 수 있을 것 같아요.
제가 코드를 작성하며 하나의 인스턴스를 강조하는 이유는 불변성의 이유도 있지만 new를 통한 인스턴스 생성 비용을 줄이고자 하는 의도도 있습니다.
현재까지는 수식을 사용하는 데 BigDecimal를 사용하는 것이 최적이라는 생각을 하고 있는데, 위의 코드에서의 생성 비용과 테스트에서 도메인 객체를 단독으로 생성해야 하는 경우의 비용이 반복되면 결과적으로 느린 테스트와 실행시간이라는 이슈가 발생하지 않을까 하는 고민을 하고 있어요.
물론 그렇게 크지 않은 비용일지도 모르지만, 줄일 수 있다면 최적화에 도움이 되지 않을까 하는 마음에 코멘드 남겼습니다. 😄
아래는 참고를 위해 첨부한 구입한 로또 테스트에 사용되는 provider로직이에요.
// List<LottoDto> 에 담아서 전달하기 위함.
public static Stream<Arguments> provideForGenerateTest() {
return Stream.of(
Arguments.of(
List.of(
new LottoDto(List.of("1", "2", "3", "4", "5", "6"))
), 1
),
Arguments.of(
List.of(
new LottoDto(List.of("1", "2", "3", "4", "5", "6")),
new LottoDto(List.of("1", "2", "3", "4", "5", "10")),
new LottoDto(List.of("1", "2", "3", "6", "7", "15")),
new LottoDto(List.of("45", "44", "43", "42", "41", "40"))
), 4
),
Arguments.of(
List.of(
new LottoDto(List.of("1", "2", "3", "4", "5", "6")),
new LottoDto(List.of("1", "2", "3", "4", "5", "10")),
new LottoDto(List.of("1", "2", "3", "6", "7", "15")),
new LottoDto(List.of("45", "44", "43", "42", "41", "40")),
new LottoDto(List.of("1", "2", "3", "4", "5", "6")),
new LottoDto(List.of("1", "2", "3", "4", "5", "10")),
new LottoDto(List.of("1", "2", "3", "6", "7", "15")),
new LottoDto(List.of("45", "44", "43", "42", "41", "40"))
), 8
)
);
}There was a problem hiding this comment.
아하, 말씀해 주신 부분이 BigDecimal 자체를 static 상수로 가지고 있게 된다면 여러 군데에서 이미 생성되어 있는 상수를 가져와 사용할 수 있기에 조금 더 성능을 높일 수 있다고 이해를 하면 괜찮을까요?
여러 불변 클래스들이 자주 사용될 가능성이 높은 값들을 상수로 정의해 두고 이를 가져오기 위한 정적 팩토리 메서드를 제공하는 것과 느낌이 비슷할 것 같네요!
정말 좋습니다!!!
There was a problem hiding this comment.
@zangsu
조금 다릅니다. 단독으로 생성되어야 하는 로직의 경우 static으로 가지고 있을 수 없어요.
정적으로 선언된 경우 새로운 매개변수가 들어오면 새로운 값으로 초기화되어 버리기 때문인데, 여기서 오는 딜레마로 고민이 깊어졌습니다. 😅
현시점에서서 엄청 중요한 내용은 아니고, 앞으로 알아가면 되는 부분이기에 너무 깊게 고민하지는 않으셨으면 해요.
| public static LottoResult getResult(LottoAnswer lottoAnswer, Lotto lotto) { | ||
| return Arrays.stream(LottoResult.values()) | ||
| .filter(result -> result.matchesSameNumberCount(lottoAnswer, lotto)) | ||
| .filter(result -> result.checkBonusNumber(lottoAnswer, lotto)) | ||
| .findFirst() | ||
| .orElse(LOSE); | ||
| } |
There was a problem hiding this comment.
.orElse()을 사용하셨네요. 저는 이제 학습한 부분을 적용하신 모습이 정말 멋집니다. 🥲
| public BigDecimal getTotalPrize() { | ||
| BigDecimal result = BigDecimal.ZERO; | ||
| List<BigDecimal> list = this.lottoResultCount.entrySet().stream() | ||
| .map(entry -> entry.getKey().getTotalPrize(entry.getValue())) | ||
| .toList(); | ||
| for (BigDecimal decimal : list) { | ||
| result = result.add(decimal); | ||
| } | ||
| return result; | ||
| } |
There was a problem hiding this comment.
음, 분리를 한다면
- 각 등수별 상금을 받아오는 부분과
- 등수별 상금들을 더하는 부분
정도로 나눌 수 있을 것 같기는 하네요.
이렇게 나눌 경우 메서드를 추출하는 쪽이 좋을 것 같고요.
이 단계가 메서드를 분리하기엔 저의 기준에선 하나의 작은 기능정도의 느낌을 가지고 있어 잘은 모르겠습니다...ㅎㅎ
코멘트 주신 부분이 마침 제가 아쉬운 점이 있었던 부분이라 공유 드리면, for문에 해당하는 부분을 사실은 Stream.sum() 또는 Stream.reduce()와 같은 메서드를 사용해 구현할 수 있지 않을까 라는 생각을 했었는데요.
결과적으론 BigDecimal이 불변객체이기에 값을 변경하기 위해선 새로운 인스턴스를 생성해야 했고, 적절한 방법을 찾지는 못했습니다.
There was a problem hiding this comment.
@zangsu
이미 분기의 기준은 잘 잡으신 것 같습니다.
저는 주로 반복문을 통해 메서드 분기점을 잡는 편인데, 이런 방법은 어떨까요? 별도의 메서드에서 조합하는 방식으로 직관성을 높이고자 했고, 핵심 로직을 은닉화 할 수 있다는 장점이 있어 보입니다.
public BigDecimal getTotalPrize() {
final List<BigDecimal> totalPrizeStatus = getTotalPrizeStatus();
return makeTotalPrize(totalPrizeStatus);
}
private List<BigDecimal> getTotalPrizeStatus() {
return this.lottoResultCount.entrySet()
.stream()
.map(entry -> entry.getKey().getTotalPrize(entry.getValue()))
.toList();
}
private BigDecimal makeTotalPrize(final List<BigDecimal> totalPrizeStatus) {
BigDecimal result = BigDecimal.ZERO;
for (BigDecimal decimal : totalPrizeStatus) {
result = result.add(decimal);
}
return result;
}There was a problem hiding this comment.
제가 이해한 부분이 맞았던 것 같네요!
확실히 여러 단계로 구성되는 로직을 한번 더 메서드 추출을 한다면 전체 로직을 메서드 이름만으로 쉽게 파악할 수 있기에 가독성 측면에서 큰 도움이 되는 것 같습니다.
No description provided.