[우아한테크코스] 프리코스 3주차 코드 리뷰 후기

[우아한테크코스] 프리코스 3주차 회고

 

코드 리뷰 피드백 요약

public void run() {
    int purchaseCount = setPurchaseCount();
    output.printLottoCount(purchaseCount);

    Lottos lottos = lottoService.generateLottos(purchaseCount);
    output.printLottoNumbers(lottos);

    WinningLotto winningLotto = setWinningLotto();
    Console.close();

    ResultCalculator resultCalculator = lottoService.calculateResult(lottos, winningLotto);

    output.printWinningDetails(resultCalculator);
    output.printYield(resultCalculator.calculateRate(purchaseCount * PURCHASE_UNIT));
}

run() 메서드의 코드를 로또 발매 로직과 당첨 확인 로직으로 세분할 수 있으므로, 각각의 로직을 별개의 메서드로 다시 분리할 수 있을 것 같습니다.

 

나는 별다른 추가 로직없이 메서드들로만 이루어진 메서드는 좀 별로라고 생각했었기 때문에 공백 라인을 통해 로직이 달라질 때를 분리해주었다. 딱 15라인이어서 그냥 냅두었긴한데 만약 라인이 더 길어졌으면 메서드 분리를 하는게 맞았을 것 같긴 하다.

---

private WinningLotto setWinningLotto() {
    return validateInput(() -> lottoService.createWinningLotto(input.getWinningNumber(), input.getBonusNumber()));
}

이렇게 하면 만약 당첨 번호는 정상적으로 처리 됐는데 보너스 번호에서 에러가 나도 당첨 번호부터 입력받아야 되지 않나요??

 

이건... 제출 전까지 계속 리팰토링하다가 실수한 부분이다.........ㅠㅠㅠㅠㅠ....... 왜 그랬지..................

리팩토링하면서 더 이상 사용하지 않는 메서드들도 그래도 남겨두었다........ 마지막 주차 미션은 절대 그러지 말아야지........

---

검증을 LottoUtils 처럼 유틸 클래스로 관리하는 것과 이렇게 도메인 내부에서 관리하는 것을 선택하는 기준이 궁금합니다!

 

1에서 45 사이의 숫자인지 확인하는 것처럼 여러 곳에서 반복적으로 사용되는 `Lotto` 관련 검증 메서드는 `LottoUtil`에 넣었고, 반면 도메인 내부에서만 사용되는 검증 메서드는 도메인 내부에서 관리하도록 하였다.

---

private void validateNoDuplicates(List<Integer> numbers) {
    Set<Integer> uniqueNumbers = new HashSet<>(numbers);
    if (uniqueNumbers.size() != numbers.size()) {
        throw new IllegalArgumentException(DUPLICATE_LOTTO_NUMBER.getMessage());
    }
}

중복 검사를 수행할 때 stream의 distinct를 활용하는 것도 가능합니다!

 

이건 다른 사람들 코드를 보면서 기록해두었었던 부분이었다. 

private void validateDuplication(List<Integer> numbers) {
    if (numbers.size() != numbers.stream().distinct().count()) {
        throw new LottoException(INVALID_LOTTO_DUPLICATE);
    }
}

 

위에처럼 `stream().distinct().count()`를 하면 굳이 `set`을 만들지 않아도 됐다.

---

public List<Lotto> getLottos() {
    return lottos;
}

내부 값을 그대로 반환하면 외부에서 수정을 할 위험이 있어 조심해야할거 같습니다! final로 하더라도 컬렉션 내부 값은 수정 가능합니다! 그렇기에 복사를 하여 내보는게 좋을거같습니다.

 

`final`하더라도 내부 값의 수정이 가능하다는 걸 처음 알았다... 

 

다른 사람들의 코드를 보니 `Collections.unmodifiableList(lottos)`나 `ArrayList<>(lottos)`를 통해 값을 반환하던데 나도 다음주부터는 이렇게 해야겠다.

---

private void validateBonusNumberUnique() {
    if (lotto.getNumbers().contains(bonusNumber)) {
        throw new IllegalArgumentException(DUPLICATE_WITH_WINNING_NUMBER.getMessage());
    }
}

 

이 코드에 대해서 많은 분들이 의견을 냈다. 나는 로또 당첨 번호와 보너스 번호가 들어있는 `WinningNumber`에 이 메서드를 넣었었다. 왜냐하면 당연히 `bonusNumber`가 중복됐는지 검증하는 부분은 `Lotto` 클래스와는 상관없고 `bonusNumber`를 몰라도 된다고 생각했었기 때문이다.

이 부분을 여기서 하는 게 아닌 Lotto에서 할 수 있도록 하면 Lotto의 데이터도 캡슐화가 되고 Lotto 객체의 자율성을 지켜줄 수 있을 것 같아요. 어떻게 생각하시나요??

저는 보너스번호가 로또 번호에 들어가 있는지 확인하는 코드이기 때문에 해당 클래스에 있는 것도 좋다고 생각합니다. 로또 클래스에서 보너스 번호를 검증하는 로직이 있는 건 역할이 아니라고 생각합니다

저도 lotto 클래스 안에 있는 numbers 라는 상태를 이용해서 bonus 번호가 있는지 검증하는 로직이기 때문에 getter를 사용해서 가져와 처리하는 것보다는 bonus 번호를 lotto 클래스 내부 로직으로 보내서 처리하는게 맞다고 생각해요. :)

 

많은 분들이 의견을 내주셨고 나는 캡슐화와 자율성을 더 중시한다면 `Lotto` 내부 로직으로 처리하고, 역할을 분명히 분리하고 싶으면 나처럼 `WinningLotto`에서 처리하면 된다고 생각했다.

 

그래서 그냥 취향 차이라고 생각했는데 공통 피드백으로 이 내용이 딱 등장했다.

 

나는 지금까지 클래스의 필드를 변경할 때는 setter을 사용하기보다 클래스 내부의 메서드를 통해 변경하는 것이 좋다는 점을 알고 있었다.  하지만 getter 메서드에 대해서는 크게 신경쓰지 않았고 최대한 사용하지 않는 것이 좋지만 필요한 경우 어쩔 수 없이 써야 한다고 생각했었다. 이번 피드백을 통해 getter에 대해 좀 더 명확히 생각을 정리할 수 있게 되었다.

 

객체에서 데이터를 꺼내(get) 사용하기보다는, 데이터가 가지고 있는 객체가 스스로 처리할 수 있도록 메시지를 던지게 하자!

---

INVALID_AMOUNT_UNIT("[ERROR] 금액은 1,0000원 단위여야 합니다.")

요구 사항에서 구매 단위가 1,000에서 다른 금액으로 바뀔 경우도 고려해서 문자열 format의 사용은 어떨까요?

 

맞는 말인 것 같다. 실제로 생성할 로또 수량을 구하기 위해 1000으로 나누는 부분은 상수화했는데 이 부분도 상수화하는게 더 적절했을 것 같다.

---

private final String message;

ExceptionMessage(String message) {
    this.message = message;
}

public String getMessage() {
    return this.message;
}

"[ERROR]"를 메세지 안에 넣지 않고, getMessage할 때 return "[ERROR] " + this.message;를 해주셔도 좋을 것 같아요!
그렇게 되면 만약 에러메세지의 출력형식이 [ERROR]이 아닌 등으로 바뀐다 해도 쉽게 수정할 수 있을 것 같아요 ㅎㅎ

 

이 부분은 나도 고민했던 부분인데 딱히 어떻게 할 방법이 생각나지 않아 우선 넘기고 그대로 제출했던 부분이다. 그런데 이런 방법이 있었다니 정말 좋은 것 같다!

 

이렇게 하면 될 것 같다!

private final static String PREFIX = "[ERROR] ";

ErrorMessage(String message) {
    this.message = PREFIX + message;
}

---

private static int validateAndParse(String input) {
    InputUtil.validateEmptyInput(input);
    String trimmedInput = input.trim();
    if (trimmedInput.startsWith(POSITIVE_SIGN)) {
        throw new IllegalArgumentException(POSITIVE_SIGN_INPUT.getMessage());
    }
    InputUtil.validatePositiveInteger(input);

    long money = Long.parseLong(trimmedInput);
    if (money > MAX_PURCHASE_AMOUNT) {
        throw new IllegalArgumentException(EXCEEDS_MAX_AMOUNT.getMessageWithMaxAmount(MAX_PURCHASE_AMOUNT));
    }

        return Integer.parseInt(trimmedInput);
}

하나의 메서드 안에서 여러 검증이 이루어지고 있어서 좀 더 메서드 분리가 이루어질 수 있을 것 같아요!👍

 

어차피 검증에 대한 메서드이고 만약 메서드 분리를 한다면 2라인으로 끝나는 메서드여서 굳이...? 분리를 할 필요가 있을까 생각했는데 지금 보니 코드가 깔끔하지 않아서 분리를 하는 게 더 좋았을 것 같긴하다.

---

그 외 알게 된 점

`assertThatExceptionOfType(IllegalArgumentException.class)` 대신 `assertThatIllegalArgumentException()`을 사용할 수 있다.

 

enum 클래스를 key로 사용하는 경우, HashMap 대신 EnumMap을 사용할 수도 있다.

---

다른 사람들의 코드에 대한 후기

이번에는 다른 사람들의 코드 리뷰에서 토론한 내용을 보고, 그들이 코드를 어떻게 작성했을지 궁금해져 여러 부분을 따라가며 살펴본 부분이 많다. 

 

메서드 이름

나는 `controller`에서 사용자로부터 입력 값을 받고 검증하는 메서드 이름을 `set`으로 시작하도록 했다. 그런데 어떤 분은`buyLotto`와 같이 좀 더 상품을 구매하는 관점에서 메서드 이름을 짓기도 했다. 저렇게 사용자 관점에서 이름을 정하는 것도 나쁘지 않을 것 같다는 생각이 들었다.

 

EnumMap

이건 사람들 코드 구경하다가 본 `EnumMap`이다.

private final EnumMap<Rank, Integer> results;


private LottoResult(List<Rank> ranks) {
        this.results = new EnumMap<>(Rank.class);
        ranks.forEach(rank -> results.put(rank, results.getOrDefault(rank, 0) + 1));
}

`EnumMap`을 사용하면 `Integer`을 따로 안 넣어도 된다!

 

BonusNumber 클래스 분리

숫자 하나이기 때문에 나는 그냥 `int`로 받아 계속 다루었다. 이러면 클래스가 너무 많이 생기는 게 아닌가 싶기도 했는데 다른 사람들의 코드를 보니 내가 객체로 만들지 않았던 부분들을 domain으로 만들어둔 사람들이 많았다. 난 이런 점이 많이 부족한 것 같다. 도메인 객체를 어떻게 분리하는 게 좋은지에 대해 좀 더 공부하고 생각해보아야할 것 같다. domain이 되기 위한 조건이 무엇인지, 결합도를 낮추기 위해서는 무엇이 좋은지 등...

 

상수화

어디까지 상수화를 진행해야할까? 코드를 보다보니 "원", "일치" 같이 너무 자세한 부분을 상수화한 사람도 있었다. 근데 저런 부분은 바뀔일이 없을 것 같은데 안해도 되지 않을까?

 

 

원시값 포장

int purchaseAmount = inputHandler.getPurchaseAmount();

int 같은 원시값 포장해보면 좋을 것 같아요.

 

이렇게 원시값은 포장을 하면 좋을 것 같다는 말이 있었다. `int`와 `Integer`의 차이에 대해서 어느 정도 알고는 있다. `null`이 될 수 있는지 없는지, 참조값을 사용하는 지 등.... 근데 직접 코드를 작성할 때는 어떤 경우에 무엇을 많이 적용하는지 잘 모르겠다. 프리코스가 끝나면 공부해야할 게 많을 것 같다.

 

 

자원 관리

나는 그냥 입력을 다 받으면 `Console.close();`를 맨 끝에 넣어주었는데 이 분은 `try-finally`를 통해 자원 관리를 했다. 근데 코드 리뷰에서 어떤 분이 `try-finally` 대신  `try-with-resources` 구문을 사용하는 것이 좋다는 말과 함께 이 링크를 참조해주셨다.

 

 

Provider

package lotto.provider;

import java.util.List;

@FunctionalInterface
public interface NumbersProvider {

    List<Integer> getNumbers();

}

package lotto.provider;

import camp.nextstep.edu.missionutils.Randoms;

import java.util.List;

import static lotto.model.LottoOption.*;

public class RandomUniqueNumbersProvider implements NumbersProvider {

    @Override
    public List<Integer> getNumbers() {
        return Randoms.pickUniqueNumbersInRange(MIN_NUMBER_OF_RANGE.value(), MAX_NUMBER_OF_RANGE.value(), TOTAL_ELEMENT_COUNT.value());
    }

}

 

이 코드는 `@FunctionalInterface` 이 애노테이션을 처음 보아서 나중에 공부해보아야지 하고 기록해두었다!

 

 

 

`Collections.unmodifiableList`와 `List.copyOf`

제기억엔 저번주차엔 Collections.unmodifiableList를 사용해서 불변을 보장하셨던 것 같은데, 이번엔 copyOf를 사용하신 이유가 있을까요??

Collections.unmodifiableList는 개발자가 수정을 가하려고 할 때 예외가 발생합니다! 개발자의 실수로 예외를 던져 프로그램이 종료되는거 보다는 차라리 개발할때 복사본을 줘서 지장이 없게 하자고 생각해서 이번에는 copyof를 사용하였습니다. 하지만 이 부분에 대해서는 저도 저만의 기준을 아직 잡지 못한거 같네요! 수정을 못하게 예외를 터트려 개발자들에게 확실히 알리는게 좋을지 아니면 내부적으로 지장이 없게 개발할때 복사본을 주는게 맞는지 고민을 계속하게됩니다!

오오..! 좋은 방식이네요!
음.. 지금 생각해보면 이부분은 이 리스트를 어떻게 확장해서 사용할수있을지에 따라 달라질 것 같아요!getLottos()의 경우에는 로또 리스트인데, 음... 이를 복사해서 새로운 비즈니스로직을 만들 가능성도 있다고 봐요.가령, 로또들 중 중복 로또가 있으면 제거하는 그런 로직..? ㅋㅋ 좀 무리순가요뭐 아무튼 Lottos를 활용해서 로직을 확장하는 방법도 있지 않을까.. 그래서 copyOf도 적합하지 않을까?? 생각이 듭니당 ㅎㅎ
근데 만약 Lotto 객체에서 getLotto()를 썼다면 고민을 좀 더 할 것 같아요!로또번호라는 게 뽑은 6자리가 건드려지지 않고 유지되는 게 굉장히 중요하기 때문에, 이를 함부로 수정해서 다른 곳에 쓰지 못하게 할 것 같아요!
그런데 쓰다보니 이를 get해서 수정하려는 시도 자체가 막.. 바람직한 것은 아닌 것 같기도 해요 ㅎㅎ getter로 가져온 값을 조회 이상의 목적으로 쓰는 걸 지양하다보니깐요.그래서 상황에 따라 다르겠지만, getter를 다른 목적으로 쓰지마라! 라는 의미를 담아 unmodifiableList도 괜찮지 않을까? 싶습니다 ㅎㅎ

`Collections.unmodifiableList`을 이번에 처음 보았고, 나도 둘의 차이가 궁금해서 찾아보았다.

 

`Collections.unmodifiableList`는 원본 리스트에 대한 변경 가능성을 막은 읽기 전용 뷰를 반환해서 수정할 수 없게 한다. 원본 리스트가 변경되면 이 읽기 전용 뷰도 그 변화를 반영한다. `List.copyOf`는 원본 리스트의 독립적인 복사본을 불변 리스트로 반환해서 원본 리스트가 변해도 반환된 리스트는 변하지 않는다.

 

따라서 원본 리스트의 상태가 변경될 가능성이 있으면 후자를, 단순히 외부에서 수정하지 못하게 할 목적이라면 전자를 사용하면 될 것 같다!

 

 

 

DTO 반환

Model에서 DTO를 반환하는건 View를 의존하는것과 비슷하다고 생각합니다!
View의 변경 → DTO의 변경 → Model의 변경으로 View의 변경이 Model에 영향을 줄 수 있을 것 같습니다

 

이번 과제에서는 DTO를 사용안했는데, 4주차 때부터 사용할 예정이어서 가져와봤다.

 

 

Config

package lotto;

import lotto.controller.LottoController;
import lotto.model.lotto.lottoNumber.RandomNumberPicker;
import lotto.model.lotto.winningResult.rank.DefaultRankDeterminer;
import lotto.service.winningNumber.DefaultNumberGenerator;
import lotto.service.winningNumber.NumberGenerator;
import lotto.service.lotto.DefaultLottoMachine;
import lotto.service.lotto.LottoMachine;
import lotto.view.input.InputView;
import lotto.view.output.OutputView;

public class DependencyConfig {

    public LottoController lottoController() {
        return new LottoController(
                new InputView(), new OutputView(), lottoMachine(), numberGenerator()
        );
    }

    public LottoMachine lottoMachine() {
        return new DefaultLottoMachine(new DefaultRankDeterminer(), new RandomNumberPicker());
    }

    public NumberGenerator numberGenerator() {
        return new DefaultNumberGenerator();
    }
}

의존성을 이런 식으로 관리한 사람도 있었다. 근데 다른 사람이 코드 리뷰로 너무 많은 게 엮인 게 아닌지하는 사람도 있어서 조금 더 알아보고 내 코드에도 써봐야겠다.

 

 

 

Exceptions

public enum Exceptions {
    EMPTY_VALUE("값을 입력해주세요.", Type.GENERAL),

    NOT_POSITIVE_INTEGER("양의 정수로만 입력해주세요.", Type.INTEGER),
    OUT_OF_INTEGER_RANGE("10자리 이하의 금액을 입력해주세요.", Type.INTEGER),

    NOT_DIVISIBLE_BY_LOTTO_PRICE("1000 단위의 금액을 입력해주세요.", Type.LOTTERY),
    WRONG_LOTTERY_NUMBER_SIZE("로또 번호는 6개여야 합니다.", Type.LOTTERY),
    DUPLICATED_LOTTERY_NUMBER("중복되지 않은 숫자를 입력해주세요.", Type.LOTTERY),
    OUT_OF_LOTTERY_NUMBER_RANGE("1 이상 45 이하의 정수를 입력해주세요.", Type.LOTTERY),
    DUPLICATED_BONUS_NUMBER("보너스 번호는 당첨 번호와 중복되지 않는 값입니다.", Type.LOTTERY),
    ;

    private final String message;
    private final Type type;

    Exceptions(String message, Type type) {
        this.message = message;
        this.type = type;
    }

    public String getMessage() {
        return String.format("[ERROR] %s", message);
    }

    private enum Type {
        GENERAL,
        INTEGER,
        LOTTERY,
    }
}

 

exceptions 클래스 안에 enum으로 Type을 만들어서 어디에서 발생되는 에러인지 나타내는 게 좀 신박? 신기? 했다. 

 

그 외

• 메서드 파라미터에도 `final`을 꼼꼼히 선언한 사람을 보았다.
• assert랑 assertj 차이
• controller가 아니라 LottoGame을 그냥 클래스로 만들어서 사용한 사람도 있다
• 나는 MVC 패턴만 쓸 줄 아는데 다른 계층 구조를 사용한 사람이 많았는데 신기했다...
• Player 클래스를 만들어, lotto, rankcount, wallet을 관리한 사람도 있었다. 나는 사람 클래스를 만들 생각을 아예 안했었는데...
• Payment 클래스를 따로 만들어 여기서 1000원 단위로 나누어지는지 확인하는 메서드를 넣는 경우도 있었다.

 

 

`write`와 `flush` 를 처음 봐서 나중에 공부할려고 기록!

public void result(ResultResponse response) {
    write(INIT_RESULT.getMessage());

    Rank.values(Collections.reverseOrder()).stream()
        .filter(rankPrice -> !rankPrice.equals(Rank.NONE))
        .forEach(rank -> write(RANK_RESULT.getMessage(rank,
            response.getRankCount(rank.getRank()))));

    write(RETURN_RESULT.getMessage(response.gain() * 100));

    flush();
}

 

코드 깔끔해서 참고용

public static String notationFrom(WinningResult result) {
    return Arrays.stream(values())
        .filter(rank -> rank != NONE)
        .map(rank -> rank.notation(result))
        .collect(Collectors.joining());
}

 

포맷

 private String convertPrizeFormat(Integer prize) {
    return new DecimalFormat(PRIZE_NUMBER_FORMAT).format(prize);
}

돈을 포맷할 생각을 안했는데 좋은 생각인 것 같아서 기록했다.

 

---

인터페이스

이번 주차에는 `service` 계층을 도입하는 것 뿐만아니라 인터페이스를 도입하고 싶어서 인터페이스도 view 페이지에 사용하였다. 이걸 사용하려면 뭘 고려하고 뭐가 좋고인지는 깊게 생각하지도 않고 그냥 새로운 걸 넣고 싶다는 생각으로 인해서...

 

그리고 받은 코드 리뷰 2개

인터페이스를 돌입한 이유가 궁금합니다!

인터페이스를 왜 input에 정의했는지 궁금해요.input은 문제의 요구사항에 아주 의존적이라고 생각하는데 인터페이스를 도입함으로써 만약 다른 input을 정의할 때, 요구사항을 만족하는 구현체를 인터페이스의 규칙에 맞추는 것이 더 유지보수를 떨어뜨리는 작업 아닐까라고 생각합니다.

 

대답을 못했다... ㅎㅎ........... 그냥 새로운 걸 쓰고 싶다고 어떻게 대답을 하냐....

 

그리고 다짐했다....... 남에게 설명하지 못하는 코드는 쓰지 말자!!!!

 

 

 

 

 

 

 

 

 

이거............ 다 적고 잘려고 했는데....... 얘가 임시 저장을 자동으로 안해서................ 날리고 다시 썼다........... 울고 싶었다......... 새벽에 잤다.........