[SCG] 박초은 로또 미션 1-2 단계 제출합니다!#166
Conversation
There was a problem hiding this comment.
특히 컨트롤러가 어느 정도의 역할을 해야 할지 감이 잘 안 잡힙니다.
컨트롤러라고 명시하신 이유와, 초은님께서 생각하시는 컨트롤러의 의미를 말씀해주시면 그에 대한 제 생각도 말씀드릴게요.
WinningLotto 부분에서도 일급컬렉션을 사용할 수 있을지도 궁금합니다!
제 생각은, WinningLotto도 같은 Lotto 아닌가? 라는 생각이 들고있어요 ㅋㅋㅎ
그래서 번호 비교를 Lotto 안에서 해도 될 것 같은 느낌입니다.
제 생각이니 참고차 봐주시면 감사하겠습니다~
또한 예외처리 로직의 위치에 대한 고민이 많았어요. 처음에는 View에서 입력값을 바로 검증했다가, 컨트롤러에서 했다가, 이것저것 찾아본 결과 도메인에서 메소드로 처리하기로 결정했습니다..!!
전 보통 나눠서 진행합니다. 도메인 규칙이라고 판단(로또는 1부터 45의 숫자를 가진다.)된다면 도메인 안에 넣고, 그게 아닌 범용적인 예외(숫자가 아니다.)는 view에서 처리할 것 같아요.
초은님이 이번 미션에서 얻어가실 게 요구사항에 나와있지만, 제 욕심에 한가지 말씀드리고 싶은 것이 있어요.
특정 행위, 예시로
- 로또 번호를 비교한다.
- 비교한 결과로 특정 로또의 등수를 확인한다.
등등 특정 요구사항은 어떤 객체가 행해야하는 지 고민해보셨으면 좋겠어요.
즉 역할의 의미를 알아가셨으면 좋겠습니다. 고생하셨어요!
저도 리뷰어가 이번에 처음이라, 제가 미쳐 생각하지 못한 부분이나 초은님께서 중점적으로 봐주셨으면 하는 부분이 있다면 편하게 말씀해주셔도 좋습니다!
| InputView inputView = new InputView(); | ||
| ResultView resultView = new ResultView(); | ||
| ErrorView errorView = new ErrorView(); |
There was a problem hiding this comment.
접근 제어자를 default로 열어두신 다른 의도가 있으실까요 ?!
There was a problem hiding this comment.
아직 접근 제어자 붙이는 게 습관이 안되어서.. 빼먹어 버렸네요.. 메소드나 변수에는 그래도 꼬박꼬박 붙이려고 하는데 생성자는 뭔가 뺴먹게 되네요ㅠ
클래스 내에서 사용하고, 재할당되지 않으니 private final 으로 수정했습니다!
| ErrorView errorView = new ErrorView(); | ||
|
|
||
| public void run() { | ||
| int purchaseAmount = inputView.getPurchaseAmount(); |
There was a problem hiding this comment.
구매금액에 있어서도 모든 원시 값과 문자열을 포장한다. 를 지켜볼까요 ?
There was a problem hiding this comment.
원시값을 포장한다.. 라는게 정말 이해하는 데 오래걸려서 최대한 파일의 모든 원시값을 포장해보았습니다.. 잘 한 건지 모르겠어서 피드백 주시면 너무 감사하겠습니닷!!
| private Lotto getValidWinningLotto() { | ||
| try { | ||
| List<Integer> numbers = inputView.getWinningNumbers(); | ||
| return new Lotto(numbers); | ||
| } catch (IllegalArgumentException e) { | ||
| errorView.printErrorMessage(e.getMessage()); | ||
| return getValidWinningLotto(); | ||
| } | ||
| } |
There was a problem hiding this comment.
Lotto, Lottos 에서는 예외가 발생하지 않나요 ? errorView를 통해 예외가 출력되지 않을 것 같네용
There was a problem hiding this comment.
InputView 에서 재입력을 받는 식으로 코드를 작성해 두어서 컨트롤러에서는 따로 예외처리를 하지 않았어요!
There was a problem hiding this comment.
그럼 에러 및 예외를 처리하는 것이 분산되어 있네요 !
- SRP를 잘 지켰다고 볼 수 있을까요 ?
- try-catch가 반복적으로 나타나고 있어요. 없앨 수 있을까요 ? 보일러 플레이트 코드처럼 보이네요!
src/main/java/domain/Lotto.java
Outdated
| public List<Integer> getNumbers() { | ||
| return numbers; | ||
| } |
src/main/java/domain/Lottos.java
Outdated
| private static final int LOTTO_UPPER_BOUND = 45; | ||
| private final List<Lotto> lottos; | ||
|
|
||
| public Lottos(final int purchaseAmount) { |
There was a problem hiding this comment.
다른 메서드에서 파라미터로 받는 HashMap, List 들과 달리 여기서 넘어온 구매 금액은 재할당되지 않기 때문이에요..!!
src/main/java/domain/Lottos.java
Outdated
| private ArrayList<Integer> getSingleLotto() { | ||
| ArrayList<Integer> lottoNumbers = generateLottoNumbersArray(); | ||
| Collections.shuffle(lottoNumbers); | ||
| List<Integer> subNumbers = lottoNumbers.subList(0, 6); | ||
| Collections.sort(subNumbers); | ||
| return new ArrayList<>(subNumbers); | ||
| } | ||
|
|
||
| private ArrayList<Integer> generateLottoNumbersArray() { | ||
| ArrayList<Integer> lottoNumbers = new ArrayList<>(LOTTO_UPPER_BOUND); | ||
| for(int i = 0 ; i < LOTTO_UPPER_BOUND ; i++) { | ||
| lottoNumbers.add(i, i + 1); | ||
| } | ||
| return lottoNumbers; | ||
| } |
There was a problem hiding this comment.
Lottos 에서 결국 모든 로또가 들어있는 이중 리스트를 만들건데 singleLotto 만들어진 것을 새 리스트에 추가하는 과정이 Lottos 안에 담겨 있어야 singleLotto 를 가져다 쓰기가 편해서 그렇게 작성하게 되었어요!
There was a problem hiding this comment.
그럼 로또 하나의 변경 방식이 바뀐다면 Lottos 를 변경해야하는 걸까요 ?!
| public HashMap<MatchResult, Integer> getMatchResult(List<Lotto> allLottos, List<Integer> winningNumbers) { | ||
|
|
||
| HashMap<MatchResult, Integer> resultMap = new HashMap<>(); | ||
|
|
||
| for(MatchResult result: MatchResult.values()) { | ||
| resultMap.put(result, 0); | ||
| } | ||
|
|
||
| for(Lotto singleLotto: allLottos) { | ||
| int singleEqualCount = 0; | ||
| for (Integer number : winningNumbers) { | ||
| if (singleLotto.getNumbers().contains(number)) { | ||
| singleEqualCount += 1; | ||
| } | ||
| } | ||
|
|
||
| for(MatchResult result: MatchResult.values()) { | ||
| if(singleEqualCount == result.getMatchCount()) { | ||
| resultMap.put(result, resultMap.get(result) + 1); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| return resultMap; | ||
| } |
There was a problem hiding this comment.
결과를 뽑는 것이 따로 분리가 되어있네요! 고민의 흔적이 많이 보이는 것 같아요.
제가 개발을 했다면,
- 당첨 번호도 하나의
Lotto로 만든다. MatchResult객체 안에서Lottos,Lotto를 받는 메서드를 작성한다.Lotto안의public메서드로 비교하는 메서드를 받아 두 로또의 동일 갯수를 반환하는 메서드를 만든다.MathResult안에서 결과를 정리한다.
이렇게 했을 것 같아요. 현재 초은님께서 작성하신 방법과, 제가 말씀드린 방법의 차이가 어떤 것인지 정리 부탁드려도 될까요 ?
저도 저만의 객체지향의 방식이 있는거라, 초은님께서 틀렸다는 게 아닌 다른 거라고 생각해요. 편하게 저와 말씀 나눠주시면 감사하겠습니다~!
There was a problem hiding this comment.
저는 WinningLotto가 모든 비교 책임을 가지고 Lotto는 단순히 숫자 목록을 보관하는 역할만 하고 있네욥.. MatchResult는 상수만 보관하고 있어서 뭔가 데이터와 로직? 행위가 따로 노는 것 같아요.
진영님은 Lotto 안에서 다른 로또를 비교하고, MatchResult 가 결과 집계까지 하고 있어서 WinningLotto가 필요하지 않아 보여요..!
사실 이번 미션 진행하면서 계속 했던 생각이 제 코드가 객체지향과 항상 거리가 있는 것 같다.. 였습니다ㅜㅜ
한번 수정해볼게요!!
[be35ad7]
| class LottoTest { | ||
| } No newline at end of file |
There was a problem hiding this comment.
테스트 코드를 작성해 보려다 "어떤 것을 테스트 해야하지..?" 부터 팀원과 막혀버렸습니닷..ㅜㅜ
그때가 예외처리를 작성하기 전이라 "그럼 일단 예외처리를 구현하자!" 하고,
다 구현하고 나니 그러면 main을 실행시킨 다음에 예외 상황을 의도한 다음, 구현한 대로 예외가 잘 던져지는 것을 확인하고 나니
다시 첫 의문으로 돌아가게 되었어요..
테스트는 메소드 하나하나? 클래스 하나? 어느 정도의 범위나, 어떤 상황을 만드는 것이 좋을까요..?
There was a problem hiding this comment.
괜찮습니다 당연한 고민이에요. 저도 겪었던 경험이에요.
해당 부분에서는 초은님께서 직접 답을 찾으셔야 합니다. 모르면 작성하지 않는 것이 맞아요.
현재 고민해야할 지점은 '무엇을 위해 테스트를 작성해야하는가?' 가 더 맞아보이는 것 같아요.
고민 후 다음 단계 본문에 남겨주시면 감사하겠습니다!
전 위의 질문에 대해 저만의 답을 정한 뒤, 블라디미르 코리코프의 책인 단위 테스트를 읽었더니 엄청 좋았어요. 단순 추천이니 넘어가셔도 좋습니다.
| # 로또 자동 구매 요구사항 | ||
|
|
||
| - [x] 로또 구입 금액을 입력하면 해당하는 로또 발급 | ||
| - [x] 1장 가격은 1000원 | ||
| - [x] 1부터 45까지 한장에 6개 숫자 | ||
| - [x] 한 로또에 중복되는 숫자 없어야 한다 | ||
|
|
||
| - [X] 로또 당첨 번호를 받아 일치한 번호 수에 따라 당첨 결과를 보여준다. | ||
|
|
There was a problem hiding this comment.
요구사항이 모든 예외가 담겨있지 않은 것 같아요.
살아있는 문서를 작성하시지 않은 이유를 말씀해주세요!
There was a problem hiding this comment.
기능 구현에 급급하다 보니 저희끼리 노트같은 곳에 설계하고, 그러다보니 리뷰어를 배려하지 않은 문서가 되었네요 죄송합니다!!ㅠ
[84ceb58]
컨트롤러는 뷰 와 도메인 부분을 연결해주고, 그 둘이 서로 최소한의 의존성만을 가질 수 있도록 중개해주며, 프로그램의 핵심 로직을 알아볼 수 있는 부분이라고 이해했어요! 그래서 예전에 js (next, express,,) 로 개발할 때도 controller 폴더 안에 페이지별, 기능별 컨트롤러를 만들었던 기억이 있는데 간단한 프로그램이다 보니 컨트롤러가 하나만 있어도 될 것 같아 클래스 하나에 코드를 작성했습니다.
기존 Lotto 에서 발급한 로또를 활용해 당첨 번호와 비교하는 거니까 일리가 있네요!! 저는 뭔가 "당첨 로또" 라는 것이 새로 생기고, 이를 중심으로 비교한다는 생각에 분리하게 되었어요! 또한 최근 MVC 패턴과 책임 분리 원칙에 집착하다 보니 최대한 기능별로 나누려는 습성을 보였네요ㅎㅎ!
입력을 받거나, 비교를 하는 등 작성한 코드에서 예외 상황에 예상된다면 바로 그 위치에 예외처리를 구현하신다는 말씀인가요? 생각해보니 굳이 예외처리 로직을 한곳에 몰아버릴,.. 필요는 없는 것 같네요!
네 감사합니다!! |
- Lotto 안에서 일치 개수 비교 - MatchResult 에서 결과 집계 -> WinningLotto 사용 X
There was a problem hiding this comment.
컨트롤러는 뷰 와 도메인 부분을 연결해주고, 그 둘이 서로 최소한의 의존성만을 가질 수 있도록 중개해주며, 프로그램의 핵심 로직을 알아볼 수 있는 부분이라고 이해했어요! 그래서 예전에 js (next, express,,) 로 개발할 때도 controller 폴더 안에 페이지별, 기능별 컨트롤러를 만들었던 기억이 있는데 간단한 프로그램이다 보니 컨트롤러가 하나만 있어도 될 것 같아 클래스 하나에 코드를 작성했습니다.
저도 초은님과 비슷한 것 같아요~ 다만 핵심 로직이 담겨서는 안되는 것 같아요.
그렇다면 현재 구조에서 정말 controller는 외부와 연결하는 역할만 한다면, LottoGame 이라는 객체를 만들어 볼 수 있겠네요!
입력을 받거나, 비교를 하는 등 작성한 코드에서 예외 상황에 예상된다면 바로 그 위치에 예외처리를 구현하신다는 말씀인가요? 생각해보니 굳이 예외처리 로직을 한곳에 몰아버릴,.. 필요는 없는 것 같네요!
맞아요. 상황에 맞는 위치에 예외를 두고, 그 예외 메시지를 출력하는 것을 중앙 집중화하면 좋은 코드라고 생각해요!
컨트롤러는 뷰 와 도메인 부분을 연결해주고, 그 둘이 서로 최소한의 의존성만을 가질 수 있도록 중개해주며, 프로그램의 핵심 로직을 알아볼 수 있는 부분이라고 이해했어요! 그래서 예전에 js (next, express,,) 로 개발할 때도 controller 폴더 안에 페이지별, 기능별 컨트롤러를 만들었던 기억이 있는데 간단한 프로그램이다 보니 컨트롤러가 하나만 있어도 될 것 같아 클래스 하나에 코드를 작성했습니다.
저도 초은님과 비슷한 것 같아요~ 다만 핵심 로직이 담겨서는 안되는 것 같아요.
그렇다면 현재 구조에서 정말 controller는 외부와 연결하는 역할만 한다면, LottoGame 이라는 객체를 만들어 볼 수 있겠네요!
입력을 받거나, 비교를 하는 등 작성한 코드에서 예외 상황에 예상된다면 바로 그 위치에 예외처리를 구현하신다는 말씀인가요? 생각해보니 굳이 예외처리 로직을 한곳에 몰아버릴,.. 필요는 없는 것 같네요!
맞아요. 상황에 맞는 위치에 예외를 두고, 그 예외 메시지를 출력하는 것을 중앙 집중화하면 좋은 코드라고 생각해요!
현재 코멘트들은 다음 단계에서 진행해주시면 될 것 같습니다 👍 고생하셨어요! 다음 리뷰때 뵈어요!
| public class LottoNumber { | ||
| private static final int MIN = 1; | ||
| private static final int MAX = 45; |
| @Override | ||
| public boolean equals(Object o) { | ||
| if (this == o) return true; | ||
| if (o == null || getClass() != o.getClass()) return false; | ||
| LottoNumber that = (LottoNumber) o; | ||
| return number == that.number; | ||
| } | ||
|
|
||
| @Override | ||
| public int hashCode() { | ||
| return Integer.hashCode(number); | ||
| } |
| } | ||
| return lottoNumbers; | ||
| } | ||
| } No newline at end of file |
There was a problem hiding this comment.
EOF 가 뜨네요 ! 수정부탁합니다. 이거 템플릿으로 정의해놓으면 자동화 되는 걸루 알고 있어요 검색해보시죠!
| private void validate(int number) { | ||
| if (number < MIN || number > MAX) { | ||
| throw new IllegalArgumentException("로또 번호는 1~45 사이여야 합니다."); | ||
| } | ||
| } |
There was a problem hiding this comment.
검증 로직을 메서드 네이밍만 보고 판단할 수 있게 만들어보는 건 어떤가요 ?
| private final int number; | ||
|
|
||
| public LottoNumber(int number) { | ||
| validate(number); | ||
| this.number = number; | ||
| } |
There was a problem hiding this comment.
int vs Integer 왜 원시형을 썼는지 말씀해주시면 감사하겠습니다!
| public class PurchaseAmount { | ||
| private static final int LOTTO_PRICE = 1000; | ||
|
|
||
| private final int amount; |
| public int getLottoCount() { | ||
| return amount / LOTTO_PRICE; | ||
| } |
There was a problem hiding this comment.
저도 고민이 되는 포인트인데, 로또 갯수를 반환하는 역할은 누가 가져야 합리적일까요?
안녕하세요 진영님! SCG 박초은 입니다!
이번 스터디로 본격적인 자바 공부는 처음이라 아직 많이 어색하지만 그래도 점점 익숙해지는 중이에요!
이번에는 페어프로그래밍이라는 새로운 방식으로 미션을 하느라 시행착오가 많았지만 확실히 혼자할 때보다 소통이나 지식 부분에서 도움이 많이 되었던 것 같아요!
어려웠던 부분
MVC 패턴에 아직 익숙하지 않다 보니 각각의 역할이나 참조 방향 등이 헷갈려서 잘 적용한 것인지 모르겠네요.. 특히 컨트롤러가 어느 정도의 역할을 해야 할지 감이 잘 안 잡힙니다.
일급컬렉션이라는 개념을 이번 미션에서 처음 접하게 되었는데, 생각보다 코드에 적용하기가 어렵지는 않았지만.. 저도 모르게 자꾸 함수형 프로그래밍을 하게 되어서 적절히 사용하는 법은 아직 제대로 익히지 못했어요ㅜㅜ 일단 Lotto, Lottos 를 일급컬렉션으로 분리한 상태인데 WinningLotto 부분에서도 일급컬렉션을 사용할 수 있을지도 궁금합니다!
또한 예외처리 로직의 위치에 대한 고민이 많았어요. 처음에는 View에서 입력값을 바로 검증했다가, 컨트롤러에서 했다가, 이것저것 찾아본 결과 도메인에서 메소드로 처리하기로 결정했습니다..!!
특히 신경 쓴 부분
로또 생성 까지는 큰 어려움 없이 리스트들을 순회하며 구현할 수 있었는데,
당첨번호와 비교한 뒤 "O개 일치 (가격) - O개" 부분에서 3,4,5,6 개마다 속성을 한번에 관리하고 싶어서 여러 방법 (하드코딩, 리스트..)들을 시도해보다가 enum 과 HashMap 을 도입해 최종 코드를 작성하게 되었어요..!!
그리고 SRP 원칙에 따라 최대한 메소드를 분리하고, public 을 최소화하려고 노력해보았습니다!
긴글 읽어주셔서 감사합니다! 잘 부탁드려요~!