우아한테크코스 로또 미션을 진행하며 배운 내용과 함께 피드백 내용을 정리한다.
1단계 - 로또(자동)
리뷰어: 닉
고민한 내용
WinningNumbers 라고 하는 당첨 번호 리스트를 관리하는 일급 컬렉션을 두고, 정적 팩토리 메소드를 적용하여 아래와 같은 코드를 작성하였다.
private WinningNumbers(List<Integer> normalWinningNumbers, Integer bonusWinningNumber) {
validateNormalWinningNumbers(normalWinningNumbers);
validateDuplication(normalWinningNumbers, bonusWinningNumber);
this.winningNumbers = getWinningNumbers(normalWinningNumbers, bonusWinningNumber);
}
...
public static WinningNumbers create(List<String> normalWinningValues, String bonusWinningValue) {
List<Integer> normalWinningNumbers = translateIntegerList(normalWinningValues);
Integer bonusWinningNumber = translateInteger(bonusWinningValue);
return new WinningNumbers(normalWinningNumbers, bonusWinningNumber);
}이 때, 생성자에서 검증에 대한 로직을 두고, 정적 팩토리 메소드에서 변환을 수행하다 보니 도메인 자체가 너무 무거워지는 것 아닌가 하는 고민이 들었다.
책임의 측면에서 보면 검증에 대한 로직은 해당 검증이 필요한 도메인에 두는 것이 맞다고 생각하는데, 변환과 같은 부분은 별도로 둘 수 있지 않을까 하는 생각이 들었다.
그렇다면 해당 로직은 입력을 받는 View로 이동시키는게 적절할까?
고민에 대한 답변
이에 대한 나의 생각은 다음과 같다.
"로또 번호" 라고 하는 도메인에 종속적인 조건은 숫자가 1~45 사이여야 한다는 점이다. 따라서 해당 입력이 1과 45 사이의 숫자인가에 대한 검증은 LottoNumber 라고 하는 VO객체에서 이루어질 수 있지만, 해당 입력이 숫자인지를 검증하고 이를 변환해주는 역할은 view에서 할 수 있을 것이다.
즉, view에서 최소한의 사용자 입력 유효성을 검사해주는 것이다.
피드백 내용
- VO(Value Object)는
eq & hc를 오버라이딩 해줘야한다. 왜일까?
우선 VO객체는값, 그 자체를 표현하는 객체를 말하며 도메인의 일종이고,객체의 불변성을 보장한다.
따라서 이름이 다르다고 하더라도모든 속성 값이 같으면같은 인스턴스라고 이야기 할 수 있어야 한다.
결국 VO에서는 최상위 클래스인 Object 클래스의 equals() 와 hashCode()를 재정의하여 속성 값(필드 값)이 같으면 같은 객체라고 판단하도록 한다.
Q. 상수는 어디서 관리해야할까?
로또 미션에서 '6'이라는 숫자는 LottoTicket에서 관리되면 자연스럽지 않을까?
A. 당시 코드에서는 LottoTicketGenerator가 '6'자리의 티켓 하나를 생성해준다는 점에서 LottoTicketGenerator 그리고 LottoTicket에서 6자리의 숫자인지를 검증해주기 위해서 두 곳 모두에서 '6'이라는 매직넘버를 상수로 관리해주고 있다.
물론 피드백 주신 내용처럼 '6'이라는 매직넘버는 LottoTicket에서 관리하는게 자연스럽지만 그렇게 되면 각각의 번호에 대한 생성 책임이 LottoTicket으로 넘어가게 된다. 라고 생각하였다.
하지만 만약 로또 게임의 규칙이 변경되어 7자리의 숫자로 변경되면 두 곳 모두에서 변경이 일어나게 된다.
그리고 각각의 번호에 대한 생성 책임이 LottoTicket으로 넘어가게 된다. 라고 생각하였는데, 단순히 상수만 public으로 열어두면 되지 않나? 그리고 LottoTicketGenerator 에서 LottoTicket.LOTTO_TICKET_SIZE와 같이 사용하면 된다.
상수는 private으로 '왜' 감쌌던 걸까? 그 이유는 상수를 사용하는 곳이 해당 클래스로 제한되어 있었기 때문이다. 나는 이에 대한 깊은 고민을 하면서 상수에 대해서 private 접근제어자를 부여한 것이 아니라 습관적으로 아무런 생각없이 private 접근제어자를 부여하고 있었기 때문에, 이러한 고정관념으로 상수를 public으로 열면되는데, 그러한 생각을 하지 못하고 그 상수를 이용하는 코드 자체를 가져오려고 생각한 것이었다.
-
Stream 제대로 사용하기
return winningNumbers.stream() .filter(lottoTicket::isSame) .collect(toList()) .size();위와 같은 코드는 불필요한 리스트를 생성하게 된다.
아래와 같이 stream을 제대로 사용하면 불필요한 리스트 생성을 피할 수 있을 뿐 아니라, 코드의 가독성 또한 높아지게 된다.return winningNumbers.stream() .filter(lottoTicket::isSame) .count();또한 단순히 다음과 같은 enhanced for loop 보다는 stream을 사용하는 것이 코드의 가독성 뿐 아니라 무엇(what)에 집중할 수 있고, side-effect가 발생하지 않는다.
int totalPrizeMoney = 0; for (Rank rank : ranks.keySet()) { totalPrizeMoney += rank.getPrizeMoney(); }int totalPrizeMoney = ranks.keySet() .stream() .mapToInt(Rank::getPrizeMoney) .sum(); -
@SuppressWarnings("NonAsciiCharacters") 는 좋은가?
해당 어노테이션을 테스트 코드에 사용함으로써 우리는 단위 테스트 메소드 명을 한글로 지음으로써 발생하는 IDE의 경고를 방지할 수 있다. 그리고 이를 통해서 테스트 코드 자체가 하나의 좋은 문서가 되는데 이바지 할 수 있다.
하지만 이는 각 테스트 클래스를 만들 때마다 적용해주어야 한다는 단점이 존재한다.
심지어 테스트 클래스를 만들면서 빠트리고 나중에서야 수정을 하게 되는 경험을 여러 번 하였다.
따라서 다음과 같은 두 가지 방안을 생각할 수 있다.
첫번재로는 DisplayName이 있으니 메소드 이름은 영어로 짓는다.
두번째로는 IntelliJ 설정으로 해결한다.
그런데 두번째 방법의 경우, 다른 사람과 함께 코딩한다고 생각했을 때 IDE 설정이 안되어 있는 동료도 있을 수 있으므로 영어로 된 메소드명을 가지는 것이 좋다고 생각한다. -
공백을 제거해야할 때는 trim() 사용을 먼저 고려해보자. replace(" ", "")와 같이 사용할 수도 있지만 이는 코드를 읽는 사람 입장에서는 코드에 대한 해석을 필요로 한다는 점에서 trim()을 이용해보는 것도 좋은 방법이다.
List<String> strings = Arrays.asList(inputWinningNumbers.replace(" ", "").split(SEPARATOR));보다는 아래의 코드가 보다 가독성 있다.
String[] splitInput = inputWinningNumbers.split(SEPARATOR); List<String> result = stream(splitInput) .map(String::trim) .collect(toList()); -
추상화 정도를 비슷하게 맞추면 보다 가독성 있는 코드가 될 수 있다.
public double calculateYield(LottoPurchaseMoney lottoPurchaseMoney) { int price = lottoPurchaseMoney.getPrice(); int totalPrizeMoney = ranks.keySet() .stream() .mapToInt(Rank::getPrizeMoney) .sum(); return Math.floor((double) totalPrizeMoney / price * 100) / 100.0;위와 같은 코드의 문제점은 무엇일까? price를 구하는 로직과 totalPrizeMoney를 구하는 로직의 추상화 정도가 일치하지 않는다.
totalMoney 를 구하는 부분을 별도의 메소드로 추상화 시키게 된다면 다음과 같은 간결한 코드를 얻을 수 있고, 이 과정을 통해서 우리는 보다 명확하게 코드의 의도를 파악할 수 있다.public double calculateYield(LottoPurchaseMoney lottoPurchaseMoney) { int price = lottoPurchaseMoney.getPrice(); double totalPrizeMoney = getTotalPrizeMoney(); return totalPrizeMoney / price; } -
Stringbuilder와 StringBuffer 그리고 String의 차이점은 무엇일까?
ticketsInfo = ticketsInfo + LOTTO_PREFIX; for (LottoNumber lottoNumber : lottoNumbers) { ticketsInfo = ticketsInfo + lottoNumber.getNumber() + SEPARATOR; } ticketsInfo = ticketsInfo.substring(0, ticketsInfo.length()-2); ticketsInfo = ticketsInfo + LOTTO_SUFFIX;위와 같은 코드는 현재 ticketsInfo라고 하는 String에서 문자열 합산이 빈번하게 일어나고 있다.
이러한 경우 StringBuilder를 사용하면 좋은데 StringBuilder는 무엇일까?
String은 Immutable한 객체이다. 즉, 불변이기 때문에 String + String 연산을 수행하게 되면 기존의 객체가 변경되는 것이 아니라 새로운 String 객체가 더하는 시점에 메모리에 할당되게 된다. 하지만 StringBuilder는 String과 다르게 기존 데이터에 새로운 데이터를 더하는 방식을 취하기 때문에 이렇게문자열 합산이 잦은 경우 혹은 긴 문자열을 더하는 경우, 저장공간이나 속도 측면에서 이점을 얻을 수 있다.
이와 비슷한 StringBuffer도 존재하는데 차이점은 쓰레드 환경에서 안전한지 여부이다. StringBuffer의 경우에 동기화가 가능하여 thread-safe하지만 속도가 느리기 때문에 String 대신 StringBuffer를 사용해서 얻는 이점이 별로 없다.(이를 사용하는 목적과 어긋남) 따라서 StringBuffer 보다는 StringBuilder를 고려하자.
2단계 - 로또(수동)
리뷰어: 닉
고민한 내용
수동이라는 기능이 추가되면서 책임을 어느쪽에 부여하는게 적절한가 하는 부분이 가장 크게 고민되었다.
우선 수동과 자동 로또 구매 개수를 계산하고 검증하는 책임은 LottoPurchaseMoney 쪽에 부여하였다. 이러한 과정에서 calculate() 라는 메소드의 기능이 단순 totalCount 계산에서 자동으로 구매한 로또 티켓의 개수를 계산하도록 변경하게 되었다.
다음으로는 LottoMachine에서 purchase() 할 때 LottoPurchaseMoney 뿐 아니라 수동으로 생성한 List 을 함께 받도록 수정하게 되었다.
(제대로 요구사항을 반영한것인지 의문..)
따라서 테스트 코드도 다음과 같이 변경하게 되었는데,
@DisplayName("로또 티켓의 갯수를 반환한다.")
@Test
void checkLottoCount() {
// given
int lottoCount = 14;
LottoTicketGenerator lottoTicketGenerator = new AutoLottoTicketGenerator();
// when
LottoTickets lottoTickets = new LottoTickets(lottoCount, new ArrayList<>(0), lottoTicketGenerator);
// then
assertThat(lottoTickets.totalCount()).isEqualTo(lottoCount);
}이 때 LottoTickets 의 생성자로 현재 new ArrayList<>(0) 을 전달해주고 있는데 이것이 적절한지는 의문이 들었다. 내가 생각할 때 현재 LottoTickets의 생성자로 직접 null이 전달되는 경우는 없기 때문에 null에 대한 검증을 하는 코드를 두지 않았는데, 테스트 코드에서 new ArrayList<>(0) 을 전달하는 부분이 마음에 걸린다.
또한 1단계 마지막 피드백에서 말씀해주신 내용이 잘 이해가지 않았다.
이 부분에서 고민이 필요해 보여요.
일단 1 ~ 45의 로또 숫자를 미리 생성해서 캐싱해놓는다는 관점에서는 잘 하셨어요 👍
다만 그렇다면 다음 사항들을 추가로 고려해서 리팩토링 해보면 좋을 듯 해요.
LottoNumber를 생성하려는 경우, 이미 생성되어 캐싱된 인스턴스가 있을 경우 새로 만들지 않고 기존 인스턴스를 반환하도록 한다.
혹은 LottoNumber 내부에서가 아니라, LottoNumber 생성을 전담하는 Pool을 별도로 만들어도 좋다.
캐싱하는 자료구조가 List인데, 더 나은 자료구조는 없을까?LottoNumber를 생성하려는 경우, 이미 생성되어 캐싱된 인스턴스가 있을 경우 새로 만들지 않고 기존 인스턴스
를 반환하도록 한다. -> 싱글톤 패턴의 적용??
하지만 LottoNumber 생성시 인자로 int값을 받고 이를 상태로 가진다는 점에서 어떻게 싱글톤을 적용해야할지 모르겠다.
LottoNumber 내부에서가 아니라, LottoNumber 생성을 전담하는 Pool을 별도로 만들어도 좋다는 내용이 이해가 가지 않는다.
첫번째 질문에 대한 답변
수동 티켓 없이 totalCount()를 검증하고 싶다면 위와 같이 진행하면 되고, 수동 티켓도 포함해서 totalCount()를 검증하고 싶다면 티켓을 생성해서 넣어주면 된다.
참고로 ArrayList의 생성자로 넣어주는 0은 List의 capacity에 해당하며 new ArrayList<>()와 같이 생성할 경우 default는 10이다.
(위의 코드에서 생성자로 0을 준 이유는 initialCapacity가 0임을 명시적으로 하고 싶어서 이다.)
두번째 질문에 대한 답변
아이구 제가 다른 분들 피드백과 같이 드리다보니 너무 집약적으로 이야기를 했나봅니다. 미안해요!
Pool이니 뭐니 이런 용어들은 일단 제외하고 본질적인 부분만 먼저 이야기를 하도록 할게요.
현재는 new LottoNumber(1) 하면 매번 새로운 인스턴스를 만들어주고 있어요.
그런데 과연 매번 새로운 인스턴스를 만들 필요가 있을까요?
LottoNumber number1 = new LottoNumber(1);
LottoNumber number2 = new LottoNumber(1);number1과 number2가 의미하는 것은 같은 로또숫자 1번인데, 인스턴스는 각각 생성되고 있어요.
그렇다면 한번 생성된 인스턴스를 어딘가에 저장해두고, 다음번에 동일한 요청이 왔을 때 기존에 저장해놓은 인스턴스를 반환해주면 어떨까? 라는 질문이에요.
이를 캐싱이라고 표현할게요. (싱글턴과는 의미가 달라요 ㅎㅎ)
캐싱한 인스턴스를 어디에 보관하면 좋을까요? LottoNumber 자체에 static으로 보관해도 좋고, 별도의 클래스로 빼도 좋아요.
생성자는 말그대로 인스턴스를 만들어주는 도구라서 생성자 자체에서 해당 기능을 제공하기는 어려울 것이고, 정적 팩토리 메서드를 사용해 보세요.
고민해보시고 잘 모르겠으면 DM으로 말씀주세요. 추가 설명 드릴게요 :)
피드백 내용
-
given-when-then
@DisplayName("로또 티켓의 갯수를 반환한다.") @Test void checkLottoCount() { // given int lottoCount = 14; LottoTicketGenerator lottoTicketGenerator = new AutoLottoTicketGenerator(); // when LottoTickets lottoTickets = new LottoTickets(lottoCount, new ArrayList<>(0), lottoTicketGenerator); // then assertThat(lottoTickets.totalCount()).isEqualTo(lottoCount); }검증의 대상이 되는 행위가 when절에 위치해야한다.(따라서 when절은 한 줄로 끝나는 경우가 대부분)
따라서 위와 같은 코드에서 검증의 대상이 되는 lottoTickets.totalCount()가 when절에 해당하는 것이 적절하고, 아래와 같이 수정할 수 있다.@DisplayName("로또 티켓의 갯수를 반환한다.") @Test void checkLottoCount() { // given int lottoCount = 14; LottoTicketGenerator lottoTicketGenerator = new AutoLottoTicketGenerator(); LottoTickets lottoTickets = new LottoTickets(lottoCount, new ArrayList<>(0), lottoTicketGenerator); // when & then assertThat(lottoTickets.totalCount()).isEqualTo(lottoCount); } -
getInstance()는 보통 실글턴 패턴에서 유일한 인스턴스를 반환할 때 사용하는 네이밍이다.
LottoNumber는 싱글턴이 아니니 적당한 정적 팩토리 메소드로 생각해서 네이밍해볼 수 있겠다. (ex.of())public static LottoNumber getInstance(int number) { ... }이펙티브 자바를 확인해보니 getInstance 라는 네이밍은
매개변수로 명시한 인스턴스를 반환하지만 같은 인스턴스임을 보장하지 않는다.라고 명시되어 있다. 'of' 는여러 인스턴스를 받아 적합한 인스턴스를 반환하는 집계 메소드라고 명시되어 있다. 따라서 위와 같은 경우매개변수를 하나 받아서 해당 타입의 인스턴스를 반환하는 형변환 메소드를 의미하는 from이 가장 적절하다고 생각하였다.
Q. 로또 숫자를 의미하는 number가 List의 index역할도 하고 있다. 의미를 추가적으로 해석해야할 뿐 아니라, 로또 숫자 리스트가 불연속적이거나 형태가 변경된다면 굉장히 위험한 코드이다. 어떻게 변경하면 좋을까? (더 적절한 자료구조는 무엇일까?)
public static LottoNumber getInstance(int number) {
validateNumberBoundary(number);
LottoNumber lottoNumber = LOTTO_TOTAL_NUMBERS.get(number - 1);
return Objects.requireNonNullElseGet(lottoNumber, () -> new LottoNumber(number));
}A. Map 자료구조를 활용해볼 수 있다. 그렇게 된다면 key값을 통해서 value를 구해서 이를 반환해줄 수 있게되고, 이렇게 되면 저장순서가 불연속적인지에 무관하게 된다. 그리고 이 경우 1~45라는 숫자에 대항하는 자료구조 이므로 Map<Integer, LottoNumber>와 같이 구성하여 LottoNumber와 동일한 Integer 값을 Key로 사용할 수 있을 것이다. 즉, 다음과 같이 코드가 개선된다.
private static final Map<Integer, LottoNumber> LOTTO_TOTAL_NUMBERS = IntStream.rangeClosed(MINIMUM_LOTTO_NUMBER, MAXIMUM_LOTTO_NUMBER)
.boxed()
.collect(toMap(identity(), LottoNumber::new));
public static LottoNumber from(int number) {
validateNumberBoundary(number);
LottoNumber lottoNumber = LOTTO_TOTAL_NUMBERS.get(number);
return Objects.requireNonNullElseGet(lottoNumber, () -> new LottoNumber(number));
}- isEqualTo() 메소드와 isSameAs() 메소드 차이알기
isEqualsTo는 실제 값이 지정된 값과 같은지를 검증한다. 따라서 동일 인스턴스인지에 대한 검증에는 적절하게 사용되지 못한다.
isSameAs와 isNotSameAs를 사용하면 동일 인스턴스인지 동일인스턴스가 아닌지를 확인할 수 있다.
(Verifies that the actual value is the same as the given one, ie using == comparison.)
즉, isSameAs는 주소값을 비교하는 메소드이고, isEqualTo는 대상의 내용자체를 비교한다.
