[코드 리뷰 정리] Level1. 로또
2021-03-01글
1단계 피드백
객체인 Controller
LottoFactory를 인스턴스 변수로 두지 않았던 이유는
Controller는 객체가 아니라 데이터를 가지면 안된다고 생각하고 있었다.
당연히 Domain만 객체라고 생각했었다. (모르는건 부끄러운게 아니다 🥲)
하지만 생각해보니 Controller도 도메인과 뷰를 이어주는 동시에, 객체를 생성하는 책임이 있다.
인스턴스 변수로 가지고 있게 된다면 getLottoTickets 메소드에서 매번 객체를 생성할 필요가 없다.
일단 이 코드에서 중요한 점은 어차피 지금 로또를 생성하는 책임을 컨트롤러에서 하고 있었으니
"불필요한 객체 생성을 방지하도록 인스턴스 변수로만들어서 재활용하자"가 목적이다.
반복되는 상수 처리
미션 데드라인을 맞추기 위해 좀 더 꼼꼼히 확인을 못했던 부분인데,
로또 티켓을 만드는 LottoTicketFactory에 있는 상수 값들이 LottoNumber나 LottoTicket등 여러 곳에서 중복되어 쓰이고 있었다.
또 심지어 로또의 MAX 번호는 45인데, 49까지로 입력해 놓았다 🙃
덕분에 상수값 하나라도 꼼꼼히 확인해야겠다는 깨달음을 얻었다.
중복되는 상수를 막으며, 각 객체가 알고 있어야 하는 값대로 상수를 두고
이를 import 해서 사용하도록 수정했다.
LottoNumber
public class LottoNumber implements Comparable<LottoNumber> {
public static final int MIN_LOTTO_NUMBER = 1;
public static final int MAX_LOTTO_NUMBER = 45;
private static final Pattern NUMBER_PATTERN = Pattern.compile("^[0-9]*$");LottoTicket
public class LottoTicket {
public static final int LOTTO_TICKET_SIZE = 6;LottoFactory
import static lotto.domain.LottoNumber.MAX_LOTTO_NUMBER;
import static lotto.domain.LottoNumber.MIN_LOTTO_NUMBER;
import static lotto.domain.LottoTicket.LOTTO_TICKET_SIZE;
import static lotto.domain.Money.LOTTO_PRICE;
public class LottoTicketFactory {
private static final int START_INDEX = 0;구매 금액 계산 법
리팩토링 전 로직으로는 구매 금액을 입력 받을 때 만약 14500원이 들어오면 사용한 금액은
14장을 사는데 14000원만 들게 하고 500원이 남는 것은 예외라고 생각하지 않고 진행했다.
그런데 이 로직은 구매한 금액이 아닌 지불한 14500원으로 수익률을 계산하고 있다.
때문에 입력 금액과 구매 금액의 미일치 오류가 발생한다.
피드백을 통해 lottoResult에서 구할 수 있는 티켓 개수와 상수 변수인 로또의 구매장수로
지불 금액을 구할 수 있음을 깨닫고 showResult() 에서 money를 매개변수로 받을 필요가 없음을 깨달았다.
덕분에 money와의 연관관계를 하나 끊을 수 있고,
수익금 계산 시 입력 금액과 구매 금액 미일치로 인한 수익률 에러를 방지할 수 있다.
Prize에서의 static 메서드 사용
리팩토링 전 Prize
public static Prize getPrizeType(int matchCount, boolean isBonusBall) {
if (isMatchCountEqualsPivot(matchCount) && isBonusBall) {
return SECOND_PRIZE;
}
return Arrays.stream(values())
.filter(s -> s.matchCount == matchCount)
.findFirst()
.orElse(NO_PRIZE);
}
private static boolean isMatchCountEqualsPivot(int matchCount) {
return matchCount == BONUS_CHECK_PIVOT;
}
public static double calculatePrizeMoneySum(List<Prize> lottoResults, Money money) {
Money moneySum = Money.ZERO;
for (Prize prize : Prize.values()) {
Money perPrizeMoneySum = prize.prizeMoney
.multiple(getCountByPrizeType(lottoResults, prize));
moneySum = moneySum.plus(perPrizeMoneySum);
}
return moneySum.getRate(money);
}
public static int getCountByPrizeType(List<Prize> lottoResults, Prize prize) {
return (int) lottoResults.stream()
.filter(p -> p.equals(prize))
.count();
}
리팩토링 전 LottoResult
public class LottoResult {
private final List<Prize> lottoResults;
public LottoResult(List<Prize> lottoResults) {
this.lottoResults = new ArrayList<>(lottoResults);
}
public double calculateProfitRate(Money money) {
return Prize.calculatePrizeMoneySum(lottoResults, money);
}
public int getCountPerPrizeType(Prize prize) {
return Prize.getCountByPrizeType(lottoResults,prize);
}
}LottoResult에서 calculateProfitRate()을 하기 위해 Prize의 도움을 받았다.
lottoResults가 Prize의 도움을 받지 않고 List<Prize> lottoResults
각각의 prize에게 메시지를 보내 스스로 계산할 수 있도록 코드를 다음과 같이 수정했었다.
LottoResult
public Money getTotalProfit() {
Money totalProfit = Money.ZERO;
for (Prize prize : lottoResults) {
totalProfit = totalProfit.plus(prize.getPrizeMoney());
}
return totalProfit;
}하지만 리팩토링 후에도 아직 객체 스스로 할 수 있는일들이 남아있었다.
또 이때까지 로또 티켓과 당첨 티켓을 비교해 결과를 반환하는 책임을
lott. Tickets가 가지고 있었다.
곰곰히 생각해보니 로또 티켓과 당첨 티켓을 비교해 결과를 반환하는 책임은 winningLotto가 맡는 것이 더 적합하다고 판단이 되었고,
winningLotto에서 prize를 체크하도록 책임을 이동하였다.
최종적으로 LottoResult를 반환하는 것은 winnigLotto에서 담당하며,
또 Prize의 도움을 받지 않고 스스로 prize를 체크하도록 하였다.
리팩토링한 WinnigLotto의 메소드 일부
public Prize matchPrize(LottoTicket lottoTicket) {
int matchCount = getMatchingCount(lottoTicket);
boolean isBonusNumber = isContainBonusNumber(lottoTicket);
return Prize.findPrize(matchCount, isBonusNumber);
}
private int getMatchingCount(LottoTicket lottoTicket) {
return (int) lottoTicket.lottoTicket().stream()
.filter(winningTicket.lottoTicket()::contains)
.count();
}
private boolean isContainBonusNumber(LottoTicket lottoTicket) {
return lottoTicket.lottoTicket()
.stream()
.anyMatch(lottoNumber -> lottoNumber.equals(bonusNumber));
}리팩토링 후 Prize에서 줄어든 static 메소드들
public static Prize findPrize(int matchCount, boolean isBonusNumber) {
if (isMatchCountEqualsPivot(matchCount) && isBonusNumber) {
return SECOND_PRIZE;
}
return Arrays.stream(values())
.filter(s -> s.matchCount == matchCount)
.findFirst()
.orElse(NO_PRIZE);
}
private static boolean isMatchCountEqualsPivot(int matchCount) {
return matchCount == BONUS_CHECK_PIVOT;
}
public Money getPrizeMoney() {
return prizeMoney;
}
public int getMatchCount() {
return matchCount;
}collectingAndThen
리뷰어님께서 수정해주신 코드는 collectingAndThen메소드를 사용했는데,
처음보는 메소드라 찾아보니 collecting을 진행한 후 그 결과로 메소드를 호출할 수 있는 메서드 였다.
stream map을 써서 객체 하나하나를 생성하고 이를 리스트로 바꿔,
또 객체를 생성하는 나의 로직에서 이 메서드를 사용하면
좀 더 간결하게 표현할 수 있었다.
사용법을 배웠으니, 앞으로 자주 사용할 것이다 👀
또 Collections의 다양한 API를 살펴보아야겠다.
2단계 초기 리팩토링
LottoCount 객체 추가
이번 리팩토링에서 수동 로또 개수를 입력 받고, 이 갯수 만큼 로또를 생성하고 출력해야했다.
원시값 포장의 의미도 담으면서 입력 받는 수동 로또 갯수에 대한 검증하고,
이에 대한 상태와 행위를 한 곳에서 관리 하기 위해 LottoCount 객체를 만들게 되었다.
public class LottoCount {
private final int manualCount;
private final int autoCount;
public LottoCount(int manualCount, int totalCount) {
validateLottoCount(manualCount, totalCount);
this.manualCount = manualCount;
this.autoCount = totalCount - this.manualCount;
}
private void validateLottoCount(int manualCount, int totalCount) {
if (manualCount > totalCount || manualCount < 0) {
throw new IllegalArgumentException("[ERROR] 구매 가능한 로또 개수 범위가 아닙니다.");
}
}
public int getManualCount() {
return manualCount;
}
public int getAutoCount() {
return autoCount;
}
}Exception
화요일 강의인 Exception이 내게는 많은 배움을 주어서,
이번 요구사항인 사용자 입력값에 대한 예외처리를 신경썼다.
이전에는 Custom Exception을 만들어서 각 객체마다 예외 처리를 해주었으나,
- 리팩토링 전 Custom Class
package lotto.exception;import lotto.view.ErrorView;public class IllegalMoneyException extends IllegalArgumentException { public IllegalMoneyException() { ErrorView.printIllegalMoneyErrorMessage(); }}각 Custom Exception 클래스가 따로 처리하는 일이 없고,
메세지도 super에게 전달하는 것이 아닌 ErrorView라는 객체를 통해 출력하고 있기 때문에
필요성을 못느끼게 되어 자바의 기본 예외를 사용하도록 리팩토링을 진행했다.
또 객체 단위로 묶어서 에러 메세지를 보내던 부분은
각 예외 사항에 맞게 에러 메시지를 보내도록 수정했다.
LottoTicketFactory -> AutoNumbersFactory
요구사항에 따라 리팩토링을 진행하면서 LottoTickets를 생성해내던 LottoTicketfactory의 역할에 대한 의문이 들었다.
처음 리팩토링에서는 객체를 많이 변경하지 않기 위해 수동 로또 번호들을 받고 자동 로또 갯수를 받아서LottoTicketFactory에서 자동 로또 숫자들을 만들고 LottoTickets를 생성하는 역할로 진행했는데,
로또 티켓들 객체를 생성하기 위해 또 다른 LottoTicketFactory를 생성하는 것이 좋지 못한 방안이라는 생각이 들었다.
때문에 LottoTickets 객체를 생성할 때 로또 번호들의 리스트들을 받아서
(List<List<LottoNumber>> lottoNumbersGroup) LottoTickets 안에서 로또 티켓들을 생성하는 방식으로 변경했다.
그리고 기존 LottoTicketFactory는 AutoNumbersFactory로 변경하여
자동 로또 숫자 리스트를 만들어내는 역할로 변경해보았다 !
LottoNumber
0223 수업 때 로또 넘버의 범위에 제한이 있으니, 이를 미리 캐싱해서 사용하는 방법을 생각해보라고 하셨다.
생각을 해보고 캐싱하는 방법을 찾아보니 HashMap을 이용해서 캐싱을 구현할 수 있음을 깨달았다.
이로 생성자를 private로 막아 불필요한 객체 생성을 막고,static block에 의해 미리 캐싱된 LottoNumber에서 키값으로 접근해 해당 로또넘버를 가져오는 형식으로 리팩토링을 진행했다.
- LottoNumber 캐시 적용 부분
static { IntStream.range(MIN_LOTTO_NUMBER, MAX_LOTTO_NUMBER + 1) .forEach(i -> CACHE_LOTTO_NUMBERS.put(i, new LottoNumber(i)));}public static LottoNumber valueOf(int index) { validateLottoNumber(index); return CACHE_LOTTO_NUMBERS.get(index);}2단계 피드백
정적 팩터리 메소드
정적 팩터리 메서드에 대해 정리는 했었으나,
사실 언제 써야하고 언제 쓰지 말아야할지 아직 감이 안왔다 해야하나 🥲
그런데 크루들이랑 얘기를 해보니,
정답은 없으나 자신이 쓰는 명분을 가지는게 좋을 것 같다는 결론을 내렸다.
사실 이때까지 정적 팩터리 메서드의 사용성을 제대로 인지하지 못하고 있었던 것 같다.
static이라는 명분 때문에 객체를 공유한다고 생각했으나,
정적 팩터리 메서드 내에서 새로운 객체를 생성해 반환한다면
상태를 공유하지 않는 것이다.
왜 static이라는 명목 때문에 정적 팩터리 메서드가 무조건
공유되는 객체를 만든다고만 생각했을까 🥲
때문에 이번 리팩토링에서는 정적 팩터리 메서드를 적극 사용해 보았다.
많은 책임을 지고 있던 LottoController
이 당시 내가 구현한 LottoController는
private LottoTickets buyLottoTickets(LottoCount lottoCount) {
List<List<LottoNumber>> lottoNumbersGroup = new ArrayList<>();
lottoNumbersGroup.addAll(createAllManualLottoTicket(lottoCount.getManualLottoCount()));
lottoNumbersGroup.addAll(createAutoNumbers(lottoCount.getAutoLottoCount()));
return new LottoTickets(lottoNumbersGroup);
}이런식으로 LottoTicket에 해당하는 List<LottoNumber>도 Controller에서 만들고,
이를 가지고 LottoTickets까지 만들어 낸다.
또 보다싶이 List<List<LottoNumber>>를 만들어 내기 위해 복잡한 코드를 가지고 있었다.
private List<List<LottoNumber>> createAllManualLottoTicket(int manualLottoCount) {
OutputView.printInputManualLottoNumbers();
return IntStream.range(0, manualLottoCount)
.mapToObj(i -> InputView.inputNumbers()
.stream()
.map(input -> LottoNumber.valueOf(ParseUtil.parseInt(input)))
.collect(Collectors.toList()))
.collect(Collectors.toList());
}이에 대해 남겨주신 피드백은 다음과 같다.
이로 인해 전반적으로 많은 수정이 일어났다.
LottoTicket에 auto(), manual() 정적 팩터리 메서드를 만들고,
LottoTickets에도 auto() 정적 팩터리 메서드를 만들어
LottoController에서 생성하고 있던 책임을 분배했다.
LottoTicket - auto(), manual()
public static LottoTicket auto() { return new LottoTicket(LottoNumbersFactory.generateAutoLottoNumbers());}public static LottoTicket manual(List<String> lottoNumberStrings) { return lottoNumberStrings.stream() .map(i -> LottoNumber.valueOf(ParseUtil.parseInt(i))) .collect(collectingAndThen(toList(), LottoTicket::new));}LottoTicket에서 List<String>을 받아 수동 로또 티켓을 생성하도록 리팩토링 해보았다 !
LottoTickets - auto()
public static LottoTickets auto(int count) { return Stream.generate(LottoTicket::auto) .limit(count) .collect(collectingAndThen(toList(), LottoTickets::new));}덕분에 LottoContoller에 있던 많은 책임이 분배되었다고 생각하고(?),
각 메서드 코드도 간결해졌다고 느낀다.
LottoController 일부
private LottoTickets buyLottoTickets(LottoCount lottoCount) { LottoTickets lottoTickets = createManualLottoTickets(lottoCount.getManualCount()); lottoTickets.combine(LottoTickets.auto(lottoCount.getAutoCount())); return lottoTickets;}private LottoTickets createManualLottoTickets(int manualCount) { OutputView.printInputManualLottoNumbers(); return Stream.generate(() -> LottoTicket.manual(InputView.inputNumbers())) .limit(manualCount) .collect(collectingAndThen(toList(), LottoTickets::new));}LottoTickets
피드백을 듣고 생각해보니 LottoTickets를 나는 단순히 LottoTicket의 모음이라고만 생각했었다.
(약간 Repository로 생각했던 것 같다.)
리펙토링 전 LottoTickets
private final List<LottoTicket> lottoTickets;
public LottoTickets(List<List<LottoNumber>> lottoNumbersGroup) {
this.lottoTickets = new ArrayList<>(createLottoTickets(lottoNumbersGroup));
}
public List<LottoTicket> lottoTickets() {
return Collections.unmodifiableList(lottoTickets);
}
private List<LottoTicket> createLottoTickets(List<List<LottoNumber>> lottoNumbersGroup) {
return lottoNumbersGroup.stream()
.map(LottoTicket::new)
.collect(Collectors.toList());
}고민해보니 이전에 아래와 같이 WinningTicket에서 당첨 결과를 산출했었는데,
리펙토링 전 WinnigLotto
public LottoResult checkPrizes(LottoTickets lottoTickets) {
return lottoTickets.lottoTickets().stream()
.map(this::matchPrize)
.collect(collectingAndThen(toList(), LottoResult::new));
}
public Prize matchPrize(LottoTicket lottoTicket) {
int matchCount = getMatchingCount(lottoTicket);
boolean isBonusNumber = isContainBonusNumber(lottoTicket);
return Prize.findPrize(matchCount, isBonusNumber);
}
private int getMatchingCount(LottoTicket lottoTicket) {
return (int) lottoTicket.lottoTicket().stream()
.filter(winningTicket.lottoTicket()::contains)
.count();
}
private boolean isContainBonusNumber(LottoTicket lottoTicket) {
return lottoTicket.lottoTicket()
.stream()
.anyMatch(lottoNumber -> lottoNumber.equals(bonusNumber));
}이렇게 모든 메서드에서 LottoTicket의 값을 꺼내오고 있었다.
때문에 LottoTickets에 WinnigLotto를 주어 각 LottoTicket에게 비교하라는 메시지를 주는것이 더 옳다고 생각해서
LottoTickets에서 결과를 비교하는 역할을 부여하게 되었다.
또 Controller에서 로또 티켓을 생성하는 역할을 덜기 위해 메서드들이 바로 LottoTickets를 반환하도록 하면서ManualLottoTickets와 LottoTickets.auto를생성하는데
생성된 이 두 LottoTickets들을 합치는 역할도 필요하다고 생각해서combine()할 수 있는 기능을 추가했다.
그런데 WinnigLotto에 있던 것들을 LottoTickets로 분배하니 WinningLotto의
역할이 다 빼앗겨 버린건 아닐까 고민이 되었지만,
리뷰어 분께서 괜찮다고 말씀해주셨다.
미션을 진행하면 할수록 아직 배울 것이 많고 나의 부족함을 느낀다.
배울 것이 많다는 것은 느낄 성취감이 많다는 것이니 앞으로 화이팅하자 🏃♂️