Conversation
soeun2537
left a comment
There was a problem hiding this comment.
안녕하세요, 현우님!
어느 정도의 리뷰를 원하시는지, 또 얼만큼 에너지를 쏟을 수 있으신지 정말 솔직하고 편하게 알려주시면 저도 그에 맞춰 리뷰해 볼게요. 개인 DM이든 스레드 언급이든 편한 방식으로 해주세요!
저는 답을 바로 찾기보다는 깊은 고민을 스스로 해볼 때 진정한 자산이 되는 것 같다고 생각해요. 그래서 제 질문에도 답이 있는 것이 아니라 현우님의 생각을 듣고 싶은 의도가 크기 때문에 모두 반영하려고 하기 보다는 의견을 많이 공유하는 리뷰가 되었으면 해요.
궁금한 점은 언제든지 편하게 물어봐주세요!
src/main/java/lotto/Lotto.java
Outdated
| List<String> numberString = numbers.stream() | ||
| .map(LottoNumber::toString) | ||
| .toList(); | ||
| System.out.println("[" + String.join(", ", numberString) + "]"); |
There was a problem hiding this comment.
이렇게 포맷팅하면 어떤 단점이 있을까요? 더 명시적으로 할 수 있을까요?
There was a problem hiding this comment.
음... 각 부분을 메서드로 분리해서 이름을 붙여주면 읽기 더 좋아질 것 같습니다.
다만, 지금도 이미 의미를 이해하기에는 충분하다고 생각합니다. 위의 .stream().map().toList() 부분도 그렇게 복잡하지 않다고 생각디고, 그 아래도 괜찮다고 생각합니다.
그리고 지금은 LottoNumber.toString에 formatting을 구현했는데, 아무래도 toString은 자바 기본 내장으로 있는 함수다 보니까 해당 함수가 toString을 구현하지 않았으면 사고니까요... 위에서 말한 것처럼 MVC로 바꿀 거지만, 아니라면 format() 같은 식으로 별개의 메서드를 만들면 좋겠네요.
There was a problem hiding this comment.
String.format 이라는 기능이 있어요. 저는 자주 사용하는데, 한번 찾아보시면 좋을 것 같아요.
src/main/java/lotto/Lotto.java
Outdated
|
|
||
|
|
There was a problem hiding this comment.
사소하지만 이런 공백은 일관되게 맞추는게 좋을 것 같아요. 포맷팅을 활용할 수 있을 것 같아요!
There was a problem hiding this comment.
알겠습니다! 원래 여기에 다른 클래스가 있다가 리펙토링 과정에서 옮겨가느라 놓친 것 같습니다.
Intellij 설정을 잘 만져서 자동으로 포맷팅이 되도록 해보고, 별개로 수정해서 커밋하겠습니다!
| } | ||
|
|
||
|
|
||
| public static class LottoException extends RuntimeException { |
There was a problem hiding this comment.
Exception이 Lotto 클래스 내부에 있는 이유가 궁금해요!
There was a problem hiding this comment.
Lotto 클래스에서만 throw되는 예외라서입니다. 논리적으로 완전히 종속되어 있고, 따로 LottoException.java가 있다면 어디에 속한 클래스인지 찾기 조금 더 어려워지지 않을까 싶었습니다.
별개로 위의 Lotto 클래스를 읽다가 (그런 경우는 없지만) LottoException에 종속된 논리가 있다면 바로 아래로 이동해서 읽으면 되니까 편리하지 않을까? 하는 생각도 있었습니다.
There was a problem hiding this comment.
- Lotto 클래스가 아닌 다른 도메인이나 서비스에서 던지게 된다면 예외를 이동시켜야할까요? 🤔
- IDE 환경에서도 찾기 어려울까요?
- Lotto 클래스가 예외 타입까지 정의하는 것이 책임일까요?
There was a problem hiding this comment.
Lotto 클래스가 아닌 다른 도메인이나 서비스에서 던지게 된다면 예외를 이동시켜야할까요? 🤔
일단 일련의 로또 숫자의 배열에 대한 검증의 책임은 명백하게 Lotto에 있으니 이 검증하는 함수와 예외의 위치는 지금처럼 유지될 것 같습니다. 만약 다른 도메인에서 해당 예외를 받아서 다시 던져야 한다면 해당 도메인/서비스에서 그에 맞는 다른 타입으로 감싸지 않을까요?
IDE 환경에서도 찾기 어려울까요?
IDE에서는 찾기 쉽지만, 깃허브에서 하는 코드리뷰 환경까지 생각하면 조금 더 어렵지 않을까 싶었습니다!
Lotto 클래스가 예외 타입까지 정의하는 것이 책임일까요?
이건 책임이 아닌 것 같습니다. 다시 보면 이 예외들이 Lotto라는 특정 클래스라기보다는, '로또'라는 시스템 전체에 관련된 예외인 것 같다는 생각이 들었습니다. 따라서 LottoException.java 또는 LottoFormattingException.java 등으로 바꾸면 좋을 것 같네요. 이름은 더 고민해보겠습니다.
| import java.util.HashSet; | ||
| import java.util.List; | ||
|
|
||
| public record Lotto(List<LottoNumber> numbers) { |
There was a problem hiding this comment.
record라는 존재는 원래 알고 있었지만, 이번 미션을 진행하면서 더 의미를 생각해봤던 것 같습니다.
- 기능적으로는 read-only인 객체를 생성하고, field/getter/hashCode/toString을 생성해주는 것이라 알고 있었습니다.
- 당연히 mutable한 객체는 어울리지 않습니다.
- 이번에 생각해봤을때, record면서 많은 작업을 수행하는 클래스는 어울리지 않다는 느낌을 받았습니다. 즉, 어떤 동작을 하는 객체를 지정하기보다는, '로또 종이', '로또', '로또 숫자'처럼 (비교적) 수동적인 객체에 어울린다고 생각했습니다.
- record에서
getNumbers()가 아니라numbers()처럼 생성되던게 처음에는 이해되지 않았는데, 생각해보니 오히려 자연스러웠습니다. 이 경우 클래스 이름이 의미하는 바가어떤 동작을 하는 기계가 아니라,어떤 정적인 사물/대상에 해당하기 때문에 많은 작업을 할 수 없습니다. - 결과적으로, record 클래스는 어떤 값이
Lotto처럼 의미를 갖게 하면서, 해당 값이 될 수 있는 경우를 제한하는 역할을 하기 위해 (타입을 좁히기 위해) 사용할 수 있습니다. 그리고, 해당 개체가 할 수 있어야 하는 작업들을 제한적으로 같이 붙여주는 것도 할 수 있습니다.
이 조건을 봤을 때 LottoReceipt/Lotto/LottoNumber 모두 잘 맞았기 때문에 record를 사용했습니다.
There was a problem hiding this comment.
기능적으로 좋은 면들이 있지만, 그렇다고 무작정 해당 기능을 쓰기 위해 record를 쓰는 게 바람직한지 여부는 잘 모르겠습니다. (이건 여러 관점이 있을거 같네요)
혹시 소은님은 어떤 경우/기준에 record 클래스를 사용하시나요?
There was a problem hiding this comment.
record의 설계 의도 자체가 transparent carrier for data이기 때문에, 데이터를 담고 전달하는 역할에 집중하는 객체에 적합하다고 저도 생각해요.
거기에 "많은 작업"이라는 것은 어떤 기준일까요? 조금 더 명확한 기준이 있으면 좋을 것 같아요.
충분히 고민하시고 보시면 좋을 것 같아요.
저 같은 경우는, record에 붙는 메서드가 자기 자신의 필드를 기반으로 한 파생 계산이나 검증이면 자주 사용하고, 외부 상태를 변경하거나 복잡한 비즈니스 흐름을 조율하면 record가 아닌 일반 클래스를 사용해요. 만든 이의 설계 의도 자체를 따르려고 하는 것 같아요.
말씀하시는 내용이 전반적으로 VO를 가리키는 것 같아요. 저도 VO를 사용할 때 record 클래스를 자주 사용해요. 불변, 동등성이 값 기반인지, 상속이 필요없는지의 기준을 놓고 사용하는 것 같아요. VO라는 키워드도 공부해 보시면 좋을 것 같아요.
There was a problem hiding this comment.
생각해보면 다음과 같은 차이점이 나옵니다.
일반적으로는 단일 책임 원칙에 따라 해당 클래스가 가지고 있는 값들을 처리하는 것은 해당 클래스의 책임입니다.
하지만, record는 해당 값들에 대한 주요한 책임은 사용하는 곳에 있는 것 같습니다.
의견과 자료 감사합니다. VO라는 용어 자체를 몰랐는데 더 알아보겠습니다!
추가적으로, record 내부의 값을 더 보기 쉽게 가공해줄 수는 있지만, record 내부의 값을 숨기는 게 바람직해 보이는 순간이 온다면 그게 record를 떼야 한다는 신호라고 생각합니다. (애초에 record 설계상 private으로 못하지 않기도 하고요....)
결과적으로 제가 들은 생각은 OOP의 몇가지 원칙인 encapsulation, SRP 원칙 등이 적용되지 않는 영역이라 여겼습니다.
src/main/java/lotto/Lotto.java
Outdated
|
|
||
| public record Lotto(List<LottoNumber> numbers) { | ||
| public Lotto { | ||
| if (numbers.size() != 6) { |
There was a problem hiding this comment.
이렇게 6을 선언하는 것을 매직넘버라고 하는데요, 어떤 이유로 매직넘버는 지양해야 할까요?
CS 관점에서도 한번 생각해 보시면 좋을 것 같아요!
There was a problem hiding this comment.
다른 사람들이 보면 의미를 이해하기 쉽지 않고, 다른 사람이 이 코드를 이용해서 다른 코드를 작성할 때 실수를 내기도 쉬울 것 같습니다. LottoMaker에서도 .subList(0, 6)이라는 코드가 나오는데, (그럴 일이 있을지는 모르지만) 만에 하나 복권이 5개를 뽑는 것으로 바뀌면 바꿔야 할 부분이 있을지도 알기 어려울 것 같습니다.
수정하겠습니다!
추가로, 혹시 CS 관점에서 생각해보라 하신 게 어떤 의미인지 알려주실 수 있으신가요...?
There was a problem hiding this comment.
가독성과 변경 지점을 하나로 둔다(단일 변경 지점)에서 현우님이 말슴하신 부분 저도 공감해요!
충분히 고민하시고 보시면 좋을 것 같아요.
CS 관점에서 보면, static final int LOTTO_SIZE = 6 같은 원시값 상수는 컴파일 타임에 인라인되기 때문에 매직넘버와 런타임 성능 차이가 사실상 없어요.
하지만 자료구조를 상수로 선언하는 경우에는 달라져요. 예를 들어 static final Set VALID_NUMBERS = Set.of(1, 2, ... 45) 같은 컬렉션을 매번 메서드 안에서 생성하면, 호출할 때마다 Heap에 객체가 새로 할당되고 GC 대상이 돼요. static final로 선언하면 클래스 로딩 시 딱 한 번 생성되어 재사용되니까 불필요한 객체 생성과 GC 비용을 줄일 수 있어요.
현우님이 잘하시는 것 같아서 여쭤본 거였어요. ㅎㅎ 이 부분은 그냥 이런 것이 있다는 관점에서만 이해하고 넘어가셔도 좋을 것 같아요.
| import java.util.Map; | ||
|
|
||
| public class MatchingCounts { | ||
| final Map<LottoResult, Integer> numberCounts = new EnumMap<>(LottoResult.class); |
There was a problem hiding this comment.
여기도 접근 제어가가 명시적으로 선언되어있지 않네요? 이유가 궁금해요!
There was a problem hiding this comment.
이것도 테스트 코드를 작성하느라 이렇게 됐습니다. 하지만 이미 public 메서드들로도 구현가능하기 때문에 개선하도록 하겠습니다.
| } | ||
|
|
||
| @Test | ||
| void 개수가_() { |
|
|
||
| import org.junit.jupiter.api.Test; | ||
|
|
||
| @SuppressWarnings("NonAsciiCharacters") |
There was a problem hiding this comment.
테스트 메서드 함수 이름에 한글처럼 ascii 코드가 아닌 것을 사용했을 때 생기는 warning입니다.
사실 이렇게 코드 단계에서 @SuppressWarnings를 붙이는 게 아니라 아예 Intellij의 Inspections에서 해당 항목을 비활성화하는 것도 좋을 것 같습니다.
별개로 저번 주차들동안 테스트 코드에 @DisplayName("한글") vs void 한글_메서드_이름에 대해서 생각을 해봤었습니다. 매번 @DisplayName을 쓰면 코드가 길어져서 읽기 쉽지 않을 뿐더러, 테스트 결과를 보고 해당 함수를 찾기 더 힘들어집니다. (물론 Intellij에서 해당 항목을 누르면 함수로 이동하긴 하지만...) 그래서 최소한 테스트 메서드 이름은 한글로 써도 된다고 생각했습니다.
| return receipt; | ||
| } | ||
|
|
||
| private static Lotto parseDrawnLotto(String line) { |
There was a problem hiding this comment.
이 경우도 시간적인 문제로 MVC 패턴을 구현하지 못했습니다. 다음주에도 소은님과 리뷰 진행하니 위에서 말씀드린 것과 마찬가지로 진행해도 될까요?
사실 전체적인 코드를 보다 보면 좀 생뚱맞은 느낌이 드는건 사실입니다. ... 기존에 만든 MVC 패턴을 이용해서 작성하면 딱 좋을 것 같았으나, 시간이 부족해서(페어분과 맞는 시간이 목요일이었습니다...) 엄두를 내지 못했습니다. ...
지금이 이미 토요일이라... 어설프게라도 MVC 구조로 개편하는 것은 다음주 과제 진행하면서 동시에 해도 될까요?
| @@ -0,0 +1,25 @@ | |||
| package lotto; | |||
|
|
|||
| public record LottoNumber(int number) implements Comparable<LottoNumber> { | |||
There was a problem hiding this comment.
오 Comparable을 구현하셨네요. 이 친구는 어떤 역할을 하는 친구인가요?
There was a problem hiding this comment.
이거는 처음 Collections.sort(numbers)를 구현하다가 안돼서 오류메시지를 보니 이거 구현이 안되어 있다고 해서 구현했습니다!
자바에서 정렬에는 Comparable과 Comparator이 있는데, Comparator<T>는 T라는 값끼리 비교할 수 있는 인터페이스고, Comparable<T>를 구현한 클래스의 경우 this와 other(첫번째 인수)를 비교합니다. 이 경우에는 원소 자체가 비교가능하기 때문에 별개의 Comparator을 구현할 필요가 없거나, 구현한다 해도 { a, b -> a.compareTo(b) }로 구현 가능합니다.
찾아본 것은 아니지만, 요소들끼리 비교를 할 수 있어야 하는 트리 등 collection의 경우 아마 원소 타입을 Comparable로 제한하거나, 아니면 별도의 Comparator을 받을 것 같습니다.
lhwdev
left a comment
There was a problem hiding this comment.
안녕하세요, 리뷰 정말 감사드립니다!
곧 시험기간이고 과제도 많고 하다 보니 전체적인 시간이 많지는 않지만, 그 시간 안에서 최대한으로 집중하고 몰입하고 싶습니다. 어떤 점이든 논의하고 싶거나 궁금하신 점, 이상한 부분이 있다면 말씀해주시면 감사드리겠습니다.
저도 많은 것을 얻어갈 수 있도록 계속 생각하고 노력하겠습니다.
페어분과 시간이 안맞아서 목요일에 작업을 시작했기 때문에 코드가 완전히 완성된 느낌은 못 받으셨을 것 같습니다. 하지만 최대한 열심히 해보도록 하겠습니다!
수정사항 커밋한 후 알려드리도록 하겠습니다. 감사합니다!
| import java.util.HashSet; | ||
| import java.util.List; | ||
|
|
||
| public record Lotto(List<LottoNumber> numbers) { |
There was a problem hiding this comment.
record라는 존재는 원래 알고 있었지만, 이번 미션을 진행하면서 더 의미를 생각해봤던 것 같습니다.
- 기능적으로는 read-only인 객체를 생성하고, field/getter/hashCode/toString을 생성해주는 것이라 알고 있었습니다.
- 당연히 mutable한 객체는 어울리지 않습니다.
- 이번에 생각해봤을때, record면서 많은 작업을 수행하는 클래스는 어울리지 않다는 느낌을 받았습니다. 즉, 어떤 동작을 하는 객체를 지정하기보다는, '로또 종이', '로또', '로또 숫자'처럼 (비교적) 수동적인 객체에 어울린다고 생각했습니다.
- record에서
getNumbers()가 아니라numbers()처럼 생성되던게 처음에는 이해되지 않았는데, 생각해보니 오히려 자연스러웠습니다. 이 경우 클래스 이름이 의미하는 바가어떤 동작을 하는 기계가 아니라,어떤 정적인 사물/대상에 해당하기 때문에 많은 작업을 할 수 없습니다. - 결과적으로, record 클래스는 어떤 값이
Lotto처럼 의미를 갖게 하면서, 해당 값이 될 수 있는 경우를 제한하는 역할을 하기 위해 (타입을 좁히기 위해) 사용할 수 있습니다. 그리고, 해당 개체가 할 수 있어야 하는 작업들을 제한적으로 같이 붙여주는 것도 할 수 있습니다.
이 조건을 봤을 때 LottoReceipt/Lotto/LottoNumber 모두 잘 맞았기 때문에 record를 사용했습니다.
src/main/java/lotto/Lotto.java
Outdated
|
|
||
| public record Lotto(List<LottoNumber> numbers) { | ||
| public Lotto { | ||
| if (numbers.size() != 6) { |
There was a problem hiding this comment.
다른 사람들이 보면 의미를 이해하기 쉽지 않고, 다른 사람이 이 코드를 이용해서 다른 코드를 작성할 때 실수를 내기도 쉬울 것 같습니다. LottoMaker에서도 .subList(0, 6)이라는 코드가 나오는데, (그럴 일이 있을지는 모르지만) 만에 하나 복권이 5개를 뽑는 것으로 바뀌면 바꿔야 할 부분이 있을지도 알기 어려울 것 같습니다.
수정하겠습니다!
추가로, 혹시 CS 관점에서 생각해보라 하신 게 어떤 의미인지 알려주실 수 있으신가요...?
| } | ||
| } | ||
|
|
||
| public void printToConsole() { |
There was a problem hiding this comment.
사실 전체적인 코드를 보다 보면 좀 생뚱맞은 느낌이 드는건 사실입니다. 특히 Lotto는 '뽑은 로또'나 '뽑힌 로또'처럼 사용되는 맥락을 정해둔게 아닌, 일반적으로 사용될 수 있게 해둬서 더욱 그렇습니다. 기존에 만든 MVC 패턴을 이용해서 작성하면 딱 좋을 것 같았으나, 시간이 부족해서(페어분과 맞는 시간이 목요일이었습니다...) 엄두를 내지 못했습니다.
지금이 이미 토요일이라... MVC도 배우고 있는 단계인데 구현하는 데에는 상당한 시간이 걸릴 것 같습니다. 혹시 다음주에 계속 이어서 진행해도 괜찮을까요?
별개로 이렇게 일단 급한 데드라인이 있고, 추후 수정할 시간이 있을 예정이면 이렇게 짜는 것도 나쁘지 않다고 생각합니다.
src/main/java/lotto/Lotto.java
Outdated
| List<String> numberString = numbers.stream() | ||
| .map(LottoNumber::toString) | ||
| .toList(); | ||
| System.out.println("[" + String.join(", ", numberString) + "]"); |
There was a problem hiding this comment.
음... 각 부분을 메서드로 분리해서 이름을 붙여주면 읽기 더 좋아질 것 같습니다.
다만, 지금도 이미 의미를 이해하기에는 충분하다고 생각합니다. 위의 .stream().map().toList() 부분도 그렇게 복잡하지 않다고 생각디고, 그 아래도 괜찮다고 생각합니다.
그리고 지금은 LottoNumber.toString에 formatting을 구현했는데, 아무래도 toString은 자바 기본 내장으로 있는 함수다 보니까 해당 함수가 toString을 구현하지 않았으면 사고니까요... 위에서 말한 것처럼 MVC로 바꿀 거지만, 아니라면 format() 같은 식으로 별개의 메서드를 만들면 좋겠네요.
| } | ||
|
|
||
|
|
||
| public static class LottoException extends RuntimeException { |
There was a problem hiding this comment.
Lotto 클래스에서만 throw되는 예외라서입니다. 논리적으로 완전히 종속되어 있고, 따로 LottoException.java가 있다면 어디에 속한 클래스인지 찾기 조금 더 어려워지지 않을까 싶었습니다.
별개로 위의 Lotto 클래스를 읽다가 (그런 경우는 없지만) LottoException에 종속된 논리가 있다면 바로 아래로 이동해서 읽으면 되니까 편리하지 않을까? 하는 생각도 있었습니다.
| return receipt; | ||
| } | ||
|
|
||
| private static Lotto parseDrawnLotto(String line) { |
There was a problem hiding this comment.
이 경우도 시간적인 문제로 MVC 패턴을 구현하지 못했습니다. 다음주에도 소은님과 리뷰 진행하니 위에서 말씀드린 것과 마찬가지로 진행해도 될까요?
사실 전체적인 코드를 보다 보면 좀 생뚱맞은 느낌이 드는건 사실입니다. ... 기존에 만든 MVC 패턴을 이용해서 작성하면 딱 좋을 것 같았으나, 시간이 부족해서(페어분과 맞는 시간이 목요일이었습니다...) 엄두를 내지 못했습니다. ...
지금이 이미 토요일이라... 어설프게라도 MVC 구조로 개편하는 것은 다음주 과제 진행하면서 동시에 해도 될까요?
| import java.util.Map; | ||
|
|
||
| public class MatchingCounts { | ||
| final Map<LottoResult, Integer> numberCounts = new EnumMap<>(LottoResult.class); |
There was a problem hiding this comment.
이것도 테스트 코드를 작성하느라 이렇게 됐습니다. 하지만 이미 public 메서드들로도 구현가능하기 때문에 개선하도록 하겠습니다.
| } | ||
|
|
||
| @Test | ||
| void 개수가_() { |
|
|
||
| import org.junit.jupiter.api.Test; | ||
|
|
||
| @SuppressWarnings("NonAsciiCharacters") |
There was a problem hiding this comment.
테스트 메서드 함수 이름에 한글처럼 ascii 코드가 아닌 것을 사용했을 때 생기는 warning입니다.
사실 이렇게 코드 단계에서 @SuppressWarnings를 붙이는 게 아니라 아예 Intellij의 Inspections에서 해당 항목을 비활성화하는 것도 좋을 것 같습니다.
별개로 저번 주차들동안 테스트 코드에 @DisplayName("한글") vs void 한글_메서드_이름에 대해서 생각을 해봤었습니다. 매번 @DisplayName을 쓰면 코드가 길어져서 읽기 쉽지 않을 뿐더러, 테스트 결과를 보고 해당 함수를 찾기 더 힘들어집니다. (물론 Intellij에서 해당 항목을 누르면 함수로 이동하긴 하지만...) 그래서 최소한 테스트 메서드 이름은 한글로 써도 된다고 생각했습니다.
| import java.util.HashSet; | ||
| import java.util.List; | ||
|
|
||
| public record Lotto(List<LottoNumber> numbers) { |
There was a problem hiding this comment.
기능적으로 좋은 면들이 있지만, 그렇다고 무작정 해당 기능을 쓰기 위해 record를 쓰는 게 바람직한지 여부는 잘 모르겠습니다. (이건 여러 관점이 있을거 같네요)
혹시 소은님은 어떤 경우/기준에 record 클래스를 사용하시나요?
|
수정하겠다고 한 사항들은 모두 반영했습니다! |
| checkDuplicateNumbers(numbers); | ||
| } | ||
|
|
||
| private void checkDuplicateNumbers(List<LottoNumber> numbers) { |
There was a problem hiding this comment.
List도 일급 컬렉션으로 포장할 수 있을 것 같은데, 그렇게 하면 어떤 점이 좋을까요?
There was a problem hiding this comment.
이미 Lotto가 일급 컬렉션이라 본다면 불필요하지 않을까요?
다만 Lotto()에서 checkDuplicateNumbers()를 호출할 때 this.numbers를 넘겨주는 것이 아니라 그냥 checkDuplicateNumbers에서 this.numbers를 참조하는게 좋겠네요. 이거는 수정하겠습니다!
There was a problem hiding this comment.
record class의 constructor에서는 아직 field가 초기화되지 않은 상태로 호출되네요. 몰랐습니다...😢
해당 함수를 static으로 바꾸고 Lotto를 통째로 넘기는 방법도 있겠지만 이 함수가 Lotto 자체를 검사하는 것이 아니라 numbers라는 부분을 검사하기 때문에 지금의 형태가 맞지 않을까요?
| public LottoReceipt printReceipt() { | ||
| return new LottoReceipt(lottoRows, totalPrice); | ||
| } |
There was a problem hiding this comment.
이 영수증을 프린트하는 것은 LottoPurchase의 책임이라고 판단하신 근거도 궁금해요.
There was a problem hiding this comment.
LottoPurchase는 원래 '로또 판매점에서 로또를 산다'는 행위를 클래스로 바꾼 것입니다. '로또 가맹점'이라는 개체는 당시 불필요하다고 생각해서 따로 추가하지는 않았지만, 만약 있었다면 '로또 가맹점'에서 용지를 뽑지 않았을까요. LottoVendor.printReceipt(LottoPurchase) 같은 형태였을 것 같습니다.
lhwdev
left a comment
There was a problem hiding this comment.
리뷰 정말 감사합니다! 모두 답변드렸습니다.
매번 다른 사람과의 협업을 고려하면서 짜다 보니 머리가 아프네요... 하다보면 손에 익지 않을까 싶습니다.
| import java.util.HashSet; | ||
| import java.util.List; | ||
|
|
||
| public record Lotto(List<LottoNumber> numbers) { |
There was a problem hiding this comment.
생각해보면 다음과 같은 차이점이 나옵니다.
일반적으로는 단일 책임 원칙에 따라 해당 클래스가 가지고 있는 값들을 처리하는 것은 해당 클래스의 책임입니다.
하지만, record는 해당 값들에 대한 주요한 책임은 사용하는 곳에 있는 것 같습니다.
의견과 자료 감사합니다. VO라는 용어 자체를 몰랐는데 더 알아보겠습니다!
추가적으로, record 내부의 값을 더 보기 쉽게 가공해줄 수는 있지만, record 내부의 값을 숨기는 게 바람직해 보이는 순간이 온다면 그게 record를 떼야 한다는 신호라고 생각합니다. (애초에 record 설계상 private으로 못하지 않기도 하고요....)
결과적으로 제가 들은 생각은 OOP의 몇가지 원칙인 encapsulation, SRP 원칙 등이 적용되지 않는 영역이라 여겼습니다.
| checkDuplicateNumbers(numbers); | ||
| } | ||
|
|
||
| private void checkDuplicateNumbers(List<LottoNumber> numbers) { |
There was a problem hiding this comment.
이미 Lotto가 일급 컬렉션이라 본다면 불필요하지 않을까요?
다만 Lotto()에서 checkDuplicateNumbers()를 호출할 때 this.numbers를 넘겨주는 것이 아니라 그냥 checkDuplicateNumbers에서 this.numbers를 참조하는게 좋겠네요. 이거는 수정하겠습니다!
| } | ||
| } | ||
|
|
||
| public void printToConsole() { |
There was a problem hiding this comment.
Lotto가 딱 어떤 맥락에서 쓰이기라기보단, 추상적으로 로또 번호를 나타내는 값이라 Lotto의 책임은 아니라고 생각했습니다. 그리고 LottoReceipt가 '로또 용지', '로또 영수증'을 나타내는 값이니까 LottoReceipt의 책임이 아닐까요?
| } | ||
|
|
||
|
|
||
| public static class LottoException extends RuntimeException { |
There was a problem hiding this comment.
Lotto 클래스가 아닌 다른 도메인이나 서비스에서 던지게 된다면 예외를 이동시켜야할까요? 🤔
일단 일련의 로또 숫자의 배열에 대한 검증의 책임은 명백하게 Lotto에 있으니 이 검증하는 함수와 예외의 위치는 지금처럼 유지될 것 같습니다. 만약 다른 도메인에서 해당 예외를 받아서 다시 던져야 한다면 해당 도메인/서비스에서 그에 맞는 다른 타입으로 감싸지 않을까요?
IDE 환경에서도 찾기 어려울까요?
IDE에서는 찾기 쉽지만, 깃허브에서 하는 코드리뷰 환경까지 생각하면 조금 더 어렵지 않을까 싶었습니다!
Lotto 클래스가 예외 타입까지 정의하는 것이 책임일까요?
이건 책임이 아닌 것 같습니다. 다시 보면 이 예외들이 Lotto라는 특정 클래스라기보다는, '로또'라는 시스템 전체에 관련된 예외인 것 같다는 생각이 들었습니다. 따라서 LottoException.java 또는 LottoFormattingException.java 등으로 바꾸면 좋을 것 같네요. 이름은 더 고민해보겠습니다.
| numbers.retainAll(lottoRow.numbers()); | ||
|
|
||
| int matchingCount = numbers.size(); | ||
| return switch (matchingCount) { |
There was a problem hiding this comment.
아... MatchException이란게 있는줄도 몰랐네요. 당연히 그렇게 구현하면 컴파일 오류가 날 줄 알았는데 신기하네요.
하지만 방금 찾아보니 Intellij에 SwitchStatementsWithoutDefault inspection이 있네요! 이걸 경고 대신 오류 단계로 설정해서 공유하면 괜찮지 않을까요
| default -> throw new IllegalStateException("나올 수 없는 경우입니다.: " + matchingCount); | ||
| }; | ||
| } | ||
|
|
There was a problem hiding this comment.
이것도 제 두뇌 내부의 자체적인 규칙이 이것저것 있었는데, 일단 '논리적인 구분'에 의한 공백의 경우에는 지양하는 편이 좋다고 생각했습니다. 자동화된 도구로 강제하기도 어렵고, 코드가 계속 바뀌면서 해당 논리가 적용되는 근거가 사라진 경우에도 보수적으로 공백을 놔두는 경우가 빈번할 것 같네요.
정 이런 이유로 공백을 두고 싶다면 주석으로 section을 구분하는 것이 낫지만, 주석으로 section을 구분하는 자체가 bad practice라고 알고 있고, 실제로도 그런 경우가 생기면 해당 클래스가 너무 많은 일을 하고 있다는 신호인 것 같습니다.
물론 명백하게 드러나는 'method <-> static 필드/메서드 사이 공백' 같은 규칙은 공유해도 좋을 것 같습니다. (하지만 Intellij에 이 공백만 따로 강제하는 기능이 없더라고요...)
| public LottoReceipt printReceipt() { | ||
| return new LottoReceipt(lottoRows, totalPrice); | ||
| } |
There was a problem hiding this comment.
LottoPurchase는 원래 '로또 판매점에서 로또를 산다'는 행위를 클래스로 바꾼 것입니다. '로또 가맹점'이라는 개체는 당시 불필요하다고 생각해서 따로 추가하지는 않았지만, 만약 있었다면 '로또 가맹점'에서 용지를 뽑지 않았을까요. LottoVendor.printReceipt(LottoPurchase) 같은 형태였을 것 같습니다.
| checkDuplicateNumbers(numbers); | ||
| } | ||
|
|
||
| private void checkDuplicateNumbers(List<LottoNumber> numbers) { |
There was a problem hiding this comment.
record class의 constructor에서는 아직 field가 초기화되지 않은 상태로 호출되네요. 몰랐습니다...😢
해당 함수를 static으로 바꾸고 Lotto를 통째로 넘기는 방법도 있겠지만 이 함수가 Lotto 자체를 검사하는 것이 아니라 numbers라는 부분을 검사하기 때문에 지금의 형태가 맞지 않을까요?
안녕하세요! 아시다시피 이번주는 페어 프로그래밍으로 진행했는데, 방법도 생소한 데다가 생각보다 논의해야 할 것들이 많아서 더 재미있지만 어려웠던 것 같습니다.
이번에도 기능 명세가 그렇게 구체적이지는 않았는데, 페어분(찬형님)과 같이 요구사항을 구체화해 가는 과정이 재미있었던 것 같습니다. 실제로 복권을 직접 사러 가서, 결제를 하고, 복권 용지를 받고, 결과를 확인하는 과정까지를 모두 코드로 옮겼다고 생각하시면 될 것 같습니다.
README에서 보실 수 있듯, 대략적인 구조는 다음과 같습니다.
1~2주차에는 진행도가 많지도 않고, 페어분과 소통하면서 진행했기 때문에 궁금했던 점이 막 있지는 않았습니다. 잘 부탁드립니다!