[완두콩] 한지수 로또 미션 제출합니다. - #204
Conversation
There was a problem hiding this comment.
안녕하세요 지수님! 미션 수고많으셨어요~
객체도 잘 분리하고 코드를 잘 작성해주셨더라구요
몇가지 세부 리뷰와, 로직을 작동하는 데 있어 추가 구현이 필요한 부분을 작성해두었으니 반영해주세요!
이번 미션도 파이팅입니다 🔥
코드 전반적으로 예외처리가 부족해요 🥲
아래 몇가지 적어보았는데, 이외의 케이스들도 고민 후 수정해주세요!
-
구입금액 입력
- 문자를 입력한 경우 에러 처리
- 공백 혹은 아무것도 입력하지 않은 경우 에러 처리
- 숫자 + 공백 을 입력한 경우 에러 처리
-
구입할 수 있는 로또의 수 보다, 더 많은 로또를 구매한 경우
구입 가능한 로또가 하나도 없는데, 수동 로또 1개 구매 입력 시 정상적으로 로직이 돌아가는 부분이 어색해보였습니다. 사용자가 느끼기에 정상적으로 구매 된건가? 하는 착각을 불러올 수 있을 것 같아요.
-
로또 번호 중복 예외 처리
수동 로또와, 정답 로또 모두 중복된 숫자 입력이 가능하네요. (예: 1,1,2,3,4,5)
보너스 숫자 역시 다른 중복된 숫자를 입력하지 못하도록 예외처리가 필요해보입니다.
| } | ||
|
|
||
| private void validateNumber(int number) { | ||
| if (number < 1 || number > 46) { |
There was a problem hiding this comment.
범위가 잘못된 것 같아요! number > 45 로 수정이 필요해 보입니다
| public List<Integer> setWinningNumber(String enteredWinningNumber) { | ||
| List<Integer> winningNumber = new ArrayList<>(); | ||
| String[] item = enteredWinningNumber.split(","); | ||
| for (int i = 0; i < item.length; i++) { | ||
| item[i] = item[i].trim(); | ||
| winningNumber.add(Integer.parseInt(item[i])); | ||
| } | ||
| Collections.sort(winningNumber); | ||
|
|
||
| return winningNumber; | ||
| } |
There was a problem hiding this comment.
당첨 숫자들은 Lotto Number 검증을 거치지 않고 있는 것 같아요🥲
| public List<Integer> getLottoNumbers() { | ||
| return lottoNumbers; | ||
| } |
There was a problem hiding this comment.
getter로 lottoNumbers를 그대로 넘겨주면 어떤 문제가 발생할 수 있을까요?
추가로 방어적 복사 에 대해 들어보셨나요?
학습 후 그 내용을 작성해주세요!
There was a problem hiding this comment.
final 키워드는 lottoNumbers 변수의 재할당만 막아줄 뿐, 참조하는 내부 요소를 변경하는 것은 막지 못합니다. 따라서 외부에서 getLottoNumbers를 호출한뒤 .add(), .remove(), clear() 같은 메서드를 호출해서 수정이 가능하다는 문제가 있습니다. LottoNumber 객체의 내부 검증을 완벽히 통과해서 생성되었더라도, 외부에서 상태가 오염되는 위험이 생깁니다.
방어적 복사란 내부의 객체를 반환 할 때, 객체의 복사본을 만들어서 반환하는 것입니다.
방어적 복사를 사용하는 방법에는
- new ArrayList<>()로 새로운 List를 생성해서 반환하는 방법
- List.copyOf()로 Collection 반환
- 복사 생성자 + Collections.unmodifiableList로 Collection 반환
new ArrayList<>() 로 복사한 리스트는 변경이 가능하기 때문에, List.copyOf()를 적용하였습니다. 방어적 복사를 통해 외부와의 참조를 끊어내는 동시에, 반환된 리스트 역시 변경 불가능한 상태로 만들어 객체의 불변성과 캡슐화를 확보하려 했습니다.
| System.out.println("5개 일치 (1500000원)-" + winningStatistics.getWinningStatistics().get(Rank.SECOND_PLACE).getWinnerNum()); | ||
| System.out.println("5개 일치, 보너스 볼 일치(30000000원)-" + winningStatistics.getWinningStatistics().get(Rank.SECOND_PLACE_BONUS).getWinnerNum()); | ||
| System.out.println("6개 일치 (2000000000원)-" + winningStatistics.getWinningStatistics().get(Rank.FIRST_PLACE).getWinnerNum()); | ||
| System.out.println("총 수익률은 " + profitRate.getProfitRate() + "입니다."); |
There was a problem hiding this comment.
수익률 계산 시 % 단위로 나타내려면 100을 곱해야할 것 같은데, 미션 내용을 확인해보지 못해서 😅...
현재의 출력 방법이 맞나요??
| int price = 1000; | ||
| List<Integer> winninggLotto = List.of(1, 2, 3, 4, 5, 6); | ||
| int testBonusNum = 7; | ||
|
|
||
| Lottos testLottos = new Lottos(List.of( | ||
| createLotto(1, 2, 3, 4, 5, 6), //1등 | ||
| createLotto(1, 2, 3, 4, 5, 7), //보너스 2등 | ||
| createLotto(1, 2, 3, 4, 5, 8), //2등 | ||
| createLotto(8, 9, 10, 11, 12, 13) //MISS | ||
| )); |
There was a problem hiding this comment.
천원을 지불했는데, 로또를 4개 구매한 테스트 케이스가 부자연스러운 것 같습니다
| private int matchBallNum; | ||
| private int prize; | ||
| private boolean hasBonusBall; |
There was a problem hiding this comment.
해당 필드들은 변하지 않는 값이네요! 어떤 키워드를 붙일 수 있을까요?
| Rank rank = Rank.getRank(matchCount, hasBonus); | ||
| assertThat(rank).isEqualTo(expectedRank); | ||
| } | ||
| } No newline at end of file |
There was a problem hiding this comment.
파일 가장 아래 금지판 표시 보이시나요?
이는 각 파일 마다 개행을 추가하지 않으면 발생하는 개행 문자 표시입니다. 이게 있으면 컴파일 에러가 발생할 수 있어서요
EOF 방지
블로그 읽어보시고 설정해두시면 편할 것 같아요~
There was a problem hiding this comment.
하나의 메서드에 쭉 로직이 작성되어있네요 🥲
메서드 분리를 통해 가독성도 높이고, 책임별로 로직을 묶어볼까요?
|
안녕하세요 리뷰어님! 수동 로또 번호 입력 시 중복 검증 및 예외 처리 흐름에 대해 고민이 있어 질문드립니다. 도메인 객체 검증 시 발생한 예외를 잡아 사용자에게 재입력을 받는 복구 로직을 깔끔하게 구조화하려면 어떤 방향으로 개선하면 좋을지 조언 부탁드립니다! |
There was a problem hiding this comment.
안녕하세요 지수님!
먼저 여러 예외 케이스를 꼼꼼하게 고려하고, 객체를 적극적으로 분리해주신 점이 좋았습니다. 👍
다만 객체가 많아진 만큼 전체 구조가 다소 복잡해진 것 같아요. 구조를 조금 더 명확하게 정리하기 위해 아래 두 가지를 고민해보면 좋겠습니다.
-
상수와 객체를 어떤 기준으로 구분하고 있나요?
어떤 값은 단순한 상수로 두고, 어떤 값은 객체로 만들어야 하는지 기준을 정리해보세요.
또한 객체로 분리할 필요가 없는데 객체로 만든 부분이나, 반대로 객체가 책임져야 할 값을 상수로만 관리하고 있는 부분은 없는지 확인해보면 좋겠습니다. -
각 클래스의 역할과 책임을 정리해주세요.
현재 여러 클래스가 존재하는데, 각 클래스가 어떤 역할을 담당하는지 직접 작성해보세요.
역할을 정리하다 보면 특정 메서드가 현재 클래스가 아니라 다른 클래스에 위치하는 것이 더 자연스럽지는 않은지, 하나의 클래스가 너무 많은 책임을 가지고 있지는 않은지 판단하는 데 도움이 될 것 같습니다.
아래 코멘트에도 남겼지만, 객체는 자신의 상태와 관련된 책임을 스스로 수행해야 한다는 점을 고려해 수정해보세요.
main 메서드의 코드가 수정된다면 test코드 또한 수정되어야합니다.
수정 범위가 조금 클 수 있지만, 천천히 학습하며 반영하면 좋을 것 같아요. 파이팅입니다! 👊👊👊
| public static int getPurchaseAmount() { | ||
| try { | ||
| System.out.println("구입금액을 입력해 주세요."); | ||
| int purchaseAmount = Integer.parseInt(scanner.nextLine()); | ||
| return purchaseAmount; | ||
| } catch(NumberFormatException e) { // 문자, 공백, 숫자+문자, 숫자+공백 등 입력시 | ||
| System.out.println("숫자만 입력 가능합니다. 다시 입력해주세요."); | ||
| return getPurchaseAmount(); | ||
| } | ||
| } |
There was a problem hiding this comment.
-1등의 음수는 입력 가능하네요. 검증 로직이 부족합니다!
추가로 입력값에 대한 검증을 할 수 있는 방법은 여러가지가 있습니다.
- InputView에서의 검증하는 방법
- Application.java 파일 내부에서 검증하는 방법
- 입력받은 값을 단순히 상수나 원시값으로 들고 있기보다, 해당 값을 의미 있는 객체로 캡슐화하고 그 객체 내부에서 검증하는 방법
지수님은 어느 위치에서 검증하는 것이 적절하다 판단하셨나요?
만일 현재의 방법을 유지하기로 판단하셨거나, 셋중 한가지 방법으로 수정하신다면 그 이유에대해서 설명해주세요!
There was a problem hiding this comment.
3번으로 검증하는 것이 적절한 것 같습니다. 객체가 생성되는 시점에서 생성자 내부에서 스스로 유효성을 검증함으로써, 생성된 객체가 올바른 상태라는 것을 보장할 수 있기 때문입니다.
따라서 PurchaseAmount라는 구입금액 정보를 가지고 있는 객체를 만들었습니다. 그리고 인풋에서 정수 변환 가능 여부같은 형식검증만 진행하고 비즈니스 규칙 검증은 도메인 객체가 담당하도록 하였습니다!
|
|
||
| private void validateManualCount(int manualCount) { | ||
| if (totalCount < manualCount || manualCount < 0) { | ||
| System.out.println("0개 이상" + " " + totalCount + "개 이하로 입력해주세요"); |
There was a problem hiding this comment.
출력문이 도메인에 섞여있네요!🚨
추가로 48번째 줄에도
String input = InputView.getManualPurchasedLottos();
직접 View 계층을 도메인이 호출하고 있어요
현재 미션에서 mvc 패턴을 다 이해하실 필요는 없지만, 코드 구조상 지수님은 domain 계층과, view 계층, main 파일로 프로젝트를 분리하신 것 같아요
각 계층 별 역할을 무엇이라 생각하고 구현하셨나요?
There was a problem hiding this comment.
저는 main은 프로그램 실행 흐름을 제어하고 도메인들을 이어주는 역할, 뷰는 입력 데이터를 받아오거나 결과를 화면에 출력하는 역할, 도메인은 각 객체만의 일을 수행하는 역할로 생각하고 구현했습니다.
기존에는 mvc 패턴에 대해서 잘 몰라서 도메인 내부에서 InputView를 직접 호출하거나 출력문이 포함되어 있어 의존성 분리가 미흡했습니다. 😞
도메인은 뷰를 몰라야 한다는 원칙을 적용하여. 도메인 내부의 뷰 호출 및 출력 로직을 모두 제거했습니다.
대신 Main에서 View로부터 받은 입력값을 도메인 객체로 전달하는 구조로 리팩토링했습니다.
| public class LottoNumber { | ||
| private static final List<Integer> NUMBERS = new ArrayList<>(); | ||
| static { | ||
| for (int i = 1; i <= 45; i++) { | ||
| NUMBERS.add(i); | ||
| } | ||
| } | ||
|
|
||
| private final List<Integer> lottoNumbers; |
There was a problem hiding this comment.
정말 사소하지만, lottoNumber이라는 이름의 클래스가 lottoNumbers를 필드로 가지고 있는 것이 어색해 보일 수 있을 것 같아요.
수정이 꼭 필요한 부분은 아니지만 앞으로 클래스명, 필드명 등을 생성할 때 주의하시면 좋을 것 같습니다!
| @@ -0,0 +1,54 @@ | |||
| package domain; | |||
|
|
|||
| import java.util.*; | |||
There was a problem hiding this comment.
import문에 "*" 와일드카드가 들어가있네요!
와일드카드는 보통 지양하라고 하는데요, 왜 지양해야할까요?
There was a problem hiding this comment.
남겨주신 피드백을 보고 찾아보았습니다.
핵심 이유는 크게 두 가지 이유가 있습니다.
먼저 서로 다른 패키지에 동일한 이름을 가진 클래스가 존재할 경우 의도치 않은 클래스가 매핑되거나 충돌 오류가 일어날 수 있습니다.
두 번째는 코드 가독성이 저하됩니다. 코드 내에서 사용하는 클래스가 정확히 어떤 외부 클래스들을 참조하고 있는지 한눈에 알기 어렵습니다.
앞으로는 필요한 클래스만 명시적으로 import하여 작성하도록 신경 쓰겠습니다!
| String[] items = input.split(","); | ||
| for (String item : items) { | ||
| lottoNumbers.add(Integer.parseInt(item.trim())); | ||
| } |
There was a problem hiding this comment.
for (String value : input.split(",")) {
numbers.add(Integer.parseInt(value.trim()));
}
이렇게 줄일 수 있겠군요!
추가로 LottoParser 클래스의 역할이 무엇인가요?
단순히 문자열 파싱을 한다기에는, Integer list로 변환도 하고, sort도 해주어 많은 일을 하고있는 것 같아서요 !
There was a problem hiding this comment.
해당 클래스의 역할에 대해서도 쭉 리스트업해보면 좋을 것같아요.
PurchaseManager가 하고있는 역할이 많은 것 같아요!
오히려 Lotto List를 필드로 가지고있는 Lottos가 가져야할 역할도 이 클래스가 가지고 있는 것 같은데 일단 리스트업 후 다른 여러 클래스들로 적절히 분리해보시죵
There was a problem hiding this comment.
당시에는 당첨 번호와 비교하여 일치 개수를 세는 기능이 비중있게 느껴져서 LottoParser 처럼 단일 역할을 하는 객체로 분리했습니다. 하지만 각 클래스의 역할과 책임을 정리해보니 이미 당첨번호를 필드값으로 가지고있는 당첨번호 객체에서 충분히 처리할 수 있는 로직이었고, 굳이 별도으 클래스로 분리할 필요까지는 없었던 것 같습니다. 해당 클래스는 제거하는 방향으로 개선했습니다!
| Rank rank = Rank.getRank(matchCount.getCount(), matchCount.hasBonusNumber(lottoNumber.getLottoNumbers(), bonusNumber)); | ||
| winningStatistics.get(rank).increase(); | ||
| } | ||
| } |
There was a problem hiding this comment.
구조가 조금 어색해보입니다ㅠㅠ
전체 코멘트에도 작성했지만 객체를 getter로 가져와서 계산은 다른 곳에서 하는 부분이 잘못된 책임 분리라고 느껴졌어요
상수를 상수로 두지 않고 객체로 감싸는 이유가 무엇인가요? 전체 코멘트에 질문을 남기긴했는데 먼저 답변드리자면 간단하게는 캡슐화를 위함이라고 생각합니다.
캡슐화를 하는 것 까지는 잘 해주셨는데, 이 부분 뿐 만 아니라 다른 코드 전반에도 getter가 많이 사용되고 있어요🥲
getter를 남용하면 사실 캡슐화로 필드를 보호하는 목적이 옅어집니다.
그래서 객체로 잘 감싸고, 해당 객체의 동작은 해당 클래스 내에 구현하는 것이 좋아요!
공부할 부분을 염두하고 대략적으로 작성해보았는데, 구체적으로 학습해보시면 도움이 될 것 같아요!
학습하신 내용 역시 코멘트에 달아 공부한 바를 설명하는 습관을 들이면 좋을 것 같습니다👍
|
안녕하세요 해윤님! 꼼꼼한 피드백 감사합니다. 🙇♂️
좋은 질문들을 던져주신 덕분에 단순히 작동하는 코드를 넘어 객체지향적인 설계에 대해 많이 배울 수 있었습니다. 감사합니다! 😊 |
안녕하세요 해윤님! 초록스터디 완두콩에 참여하고 있는 한지수 입니다.
미션을 진행하며 작성한 코드를 제출합니다. 잘 부탁드립니다. 😊
중점적으로 봐주셨으면 하는 부분은,
수동 로또 입력 처리
처음에는 InputView 내부에서 구매 수량만큼 for문을 돌려 모든 수동 번호를 String[] 배열로 받아온 뒤 한 번에 도메인으로 넘기는 방식을 고민했습니다. 하지만 이 방식은 문자열 배열을 다시 순회하며 Lotto 객체로 변환하고 추가해야 해서 데이터 전달 단계가 번거로워진다고 느꼈습니다. 그래서 현재는 InputView는 단 1장의 수동 로또 입력만 받도록하고 main에서 입력 수량만큼 루프를 돌며 받아온 단일 입력을 즉시 객체로 변환하여 리스트에 추가하는 방식으로 변경했습니다. InputView가 루프를 제어하여 반환하는 방식이 좀 더 직관적이라고 생각 되어서, 혹시 이런 입축력 흐름을 더 깔끔하게 다룰 수 있는 다른 설계적 대안이 있다면 조언 부탁드립니다.
Enum(Rank)과 Map 기반의 통계/수익률 구조 설계
등수와 상금, 당첨 조건 및 꽝(MISS) 상태를 Rank Enum으로 일관되게 관리하고, WinningStatistics에서 해쉬맵을 활용해 당첨 수량을 집계하도록했습니다.
ResultView에서의 출력 데이터 접근 방식 (. 연쇄 호출 문제)
결과를 출력하는 과정에서
winningStatistics.getWinningStatistics().get(rank).getWinnerNum()과 같이 점(.) 호출이 다소 길어지는 현상이 발생했습니다. 뷰(View)에 데이터를 전달하는 과정에서 객체 캡슐화가 깨지고 있는 것은 아닌지 궁금합니다.MatchCount 클래스의 설계 및 상태 관리 방식 검토
로도 번호와 당첨 번호를 비교하는 MatchCount 클래스를 작성하면서 설계상 아쉬움과 불편함을 많이 느꼈습니다. 클래스 내부에서 int count 필드를 상태값으로 관리하고 매번 초기화하여 사용하는 방식 등 구조가 다소 어색하게 느껴졌습니다. 이 부분을 어떤 방식으로 개선해야 할지 감이 잘 잡히지않아 조언을 부탁드립니다..!
시간이 부족하여 5단계 리팩터링 및 예외처리를 완료하지 못한 상태로 제출하게 되었습니다. 😭
완성되지 않은 부분은 빠르게 보완하여 반영하겠습니다. 바쁘시겠지만 시간 내어 리뷰해 주셔서 감사합니다! 잘 부탁드립니다. 😊