[SCG] 이찬형 로또 3-4단계 제출합니다.#183
Conversation
| @@ -9,7 +10,7 @@ public class Main { | |||
|
|
|||
| public static void main(String[] args) { | |||
There was a problem hiding this comment.
함수(또는 메서드)의 길이가 10라인을 넘어가지 않도록 구현한다.
요구 사항을 지켜볼까요?! 상당히 줄이 길면 비즈니스 흐름을 파악하기 어려워요!! 😭
|
|
||
| LottoDraw draw = new LottoDraw(drawnLotto, receipt); | ||
| System.out.println("보너스 볼을 입력해 주세요."); | ||
| int bonusNumber = Integer.parseInt(scanner.nextLine()); |
There was a problem hiding this comment.
NumberFormatException 이전과 동일하게 에러가 발생하지 않을까요?! 🤔
| return reward; | ||
| } | ||
| } | ||
| } No newline at end of file |
There was a problem hiding this comment.
지난 리뷰때 소개해 드렸던, 자동 정렬을 사용하면 마지막 줄 개행을 자동 추가해줘요!
| private final MatchingCounts counts; | ||
|
|
||
| public LottoDraw(Lotto drawnLotto, LottoReceipt receipt) { | ||
| public LottoDraw(Lotto drawnLotto, int bonusNumber, LottoReceipt receipt) { |
There was a problem hiding this comment.
int bonusNumber 원시값 포장을 할 수 있는 객체가 충분히 있어 보이는데요? 🤔
There was a problem hiding this comment.
그러네요..? 기껏 왜 LottoNumber를 만들어놓고 쓰지 않았을까요.. 수정했습니다.
| boolean matchBonus = lottoRow.numbers().stream() | ||
| .anyMatch(num -> num.number() == bonusNumber); |
There was a problem hiding this comment.
bonusNumber 원시값 포장하게 된다면 이 부분도 개선할 수 있어요. (Hint. contains)
- 어떻게 개선할 수 있을지?
- 개선한 코드가 되는 이유를 작성해보세요!
There was a problem hiding this comment.
boolean matchBonus = lottoRow.numbers().contains(bonusNumber); 로 바꿔봤습니다.
lottoRow.numbers()는 List를 반환할 것이고, List에 특정 숫자가 포함되어 있는지를 확인하는 contains 메소드를 활용해서 가독성 측면에서 더욱 간결하게 작성할 수 있을 것 같습니다.
조금 더 찾아보니, anymatch와 contains는 둘 다 선형으로 탐색하다 탐색을 성공하면 바로 return을 하는 것은 똑같지만,
anymatch는 내부에서 함수 호출 (위에서는 lambda식)을 진행하기에 contains에 비해 약간의 비용이 더 발생하고, stream()을 추가로 호출하기 때문에 이에 대한 비용도 추가됩니다. 다만 조건식을 활용하기에 이에 대한 범용성은 더 높을 수 있지만, 단순 비교에 그친 지금의 경우에는 해당 사항이 없습니다.
contains는 함수를 호출하지 않으니 비용(시간) 면에서 약간 더 빠를 수 있고, equals 메소드를 오버라이딩해서 사용하면 값을 직접 꺼내오지 않고 다양한 연산을 활용할 수 있기에 더욱 개선된다고 볼 수 있을 것 같습니다.
| return (float) sumOfReturn / receipt.totalPrice(); | ||
| } | ||
| } | ||
| } No newline at end of file |
| public static LottoResult valueOf(int matchingCount, boolean matchBonus) { | ||
| if (matchingCount == 6) { | ||
| return SIX; | ||
| } | ||
| // 3개 미만은 모두 NONE으로 처리 | ||
| if (count >= 0 && count < 3) { | ||
| if (matchingCount == 5 && matchBonus) { | ||
| return BONUS; | ||
| } | ||
| if (matchingCount == 5) { | ||
| return FIVE; | ||
| } | ||
| if (matchingCount == 4) { | ||
| return FOUR; | ||
| } | ||
| if (matchingCount == 3) { | ||
| return THREE; | ||
| } | ||
| if (matchingCount < 3 && matchingCount >= 0) { | ||
| return NONE; | ||
| } | ||
| throw new IllegalArgumentException("유효하지 않은 당첨 개수입니다: " + count); | ||
| throw new IllegalArgumentException("유효하지 않은 당첨 개수입니다: " + matchingCount); | ||
| } |
There was a problem hiding this comment.
Enum 으로 옮겼지만 요구 사항의 "함수(또는 메서드)의 길이가 10라인을 넘어가지 않도록 구현한다." 준수해 볼까요?
이 부분 리뷰가 3번째 이긴한데, 어렵게 느껴지시면 DM 주셔도 좋아요! 🙂 대신!! 충분히 고민 후!! 왜냐면 이미 사용한 곳이 있어서 고민하시면 풀리실 거 같아서요!
There was a problem hiding this comment.
이전에 썼던 게 for문을 활용한 반복문이었던 것 같은데, code depth가 2 이상이면 안된다는 요구 사항도 있어서 이를 지키려다 자꾸 저런 식으로 짜네요..!
또 다른 방법이 있을까요? Enum 자체 값 mapping을 1, 2도 만들어서 사용하는 방법도 있을 것 같긴 한데, 어차피 결과가 똑같은 걸 중복해서 생성하니 괜히 더 복잡해지는 게 아닐지.. 싶습니다.
There was a problem hiding this comment.
code depth가 '2' 넘지 말라는 요구 사항이 왜 있었을까요?! 🤔 그걸 해결하기 위한 방법이 무엇이 있을 지 생각해보면, 메서드 분리가 좋은 방향이 될 수 있습니다. 그 뿐만 아니라 다른 방식으로도 개선해볼 수 있죠!
public static LottoResult of(int count, boolean matchBonus) {
if (count == 5 && matchBonus) {
return BONUS;
}
return Arrays.stream(values())
.filter(result -> result != BONUS && result != NONE)
.filter(result -> result.matchingCount == count)
.findFirst()
.orElse(NONE);
}There was a problem hiding this comment.
Stream 활용을 제시해드린 이유는 다른 코드에서 이미 Stream 사용한 사례가 있어서 이 질문도 했었고, 예시를 전달드렸습니다!
There was a problem hiding this comment.
stream에 대한 것을 좀 찾아보니 되게 많은 상황에서 사용될 수 있겠네요..! stream이 사실 mapping이나 그런 쪽에서만 활용될 수 있을 줄 알았는데 문서를 좀 더 찾아보니까 제가 생각한 것보다 훨씬 많은 방향에 활용될 수 있네요.
메서드 분리에 대한 건 생각을 안 해본 건 아닌데, 그럼 지금 존재하는 메소드의 역할이 좀 애매해지는 것 같아서 그 요구사항 때문에 괜히 객체를 하나 더 생성하는 게 아닌가 싶은 생각이 들었습니다. 제공해주신 예시가 전 가장 좋아보이네요.
There was a problem hiding this comment.
시간이 괜찮다면 이번에 테스트 한 객체마다 '단위 테스트' 작성해보는건 어때요?! 🤔
There was a problem hiding this comment.
시험공부하면서 중간중간 생각나는 것들 하나씩 추가해볼게요..! 리뷰 달때마다 한 2-3개 정도씩 추가하면 단위 테스트가 되지 않을까요..?
taek2222
left a comment
There was a problem hiding this comment.
찬형님 많이 바쁘시죠? 😭 시험 기간이라 정신 없으실 것 같아요. 그래도 지금 이 미션을 경험하는 게 나중에 더 큰 성장으로 이어질 수 있다고 생각합니다. 🔥
시험 기간이 끝난 후, 한 번 로또를 다시 구현해보는 건 어떨까요? 😊
절대 코드가 이상해서 드리는 말씀은 아니고요!
제가 과거에 가장 크게 성장했다고 느꼈던 순간은,
잘못된 설계가 눈에 보이기 시작하고
“이 부분은 이렇게 개선해야겠다”,
“이 구조면 나중에 객체 간 문제가 생기겠는데?”
라는 걸 초반에 인지해 개선하면 더 넓은 관점에서 새로운 문제를 마주할 때였습니다.
그래서 저는 같은 미션을 혼자 3번씩 다시 구현해보기도 했어요. 🙂
그 과정에서 “예전엔 이런 설계 때문에 객체들이 서로 힘들었구나” 라는 걸 깨닫게 되었고, 점점 설계 기준이나 코딩 요령도 생기더라고요.
그래서 이 방법을 한 번 추천드리고 싶었습니다. 🙂 스터디 기간이 아니더라도, 나중에 다시 구현하시다가 리뷰가 필요하시면 언제든지 디스코드 DM 주세요! 같이 고민하고 풀어보면서 함께 성장할 기회를 이어가보도록 하죠! 하하 ~
| } | ||
|
|
||
| private static void displayRank(LottoResult res, int count) { | ||
| String label = res == LottoResult.BONUS ? "5개 일치, 보너스 볼 일치" : res.getMatchingCount() + "개 일치"; |
There was a problem hiding this comment.
'3항 연산자' 사용을 지양하는 요구 사항이 있었습니다! 🙂
그리고 왜 '3항 연산자' 사용을 지양할까요?! 찬형님 의견이 궁금해요.
There was a problem hiding this comment.
전 코드를 짤 때 자주 애용하는 편이라 사실 쓰지 말라고 하는 방향이 그리 익숙하진 않습니다..! 다만 사용하게 된다면 if-else 같은 코드 블록이 나뉘어져 있는 것들에 비해 코드 가독성이 떨어지는 것은 사실이고, 구분자가 확실히 보이지 않아 한 눈에 안 들어올 것 같습니다.
이외에도 다른 이유가 더 있나요?
| int price = inputPrice(); | ||
| if (price < 1000) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
이 부분은 메서드 분리를 적용 안한 이유를 알 수 있을까요?! 또한, 에러 메시지 표현도 아닌 return;로 프로그램을 종료한 이유도 궁금해요 🤔
There was a problem hiding this comment.
inputPrice라는 함수에서 사용자에게 필요한 정보를 전달했고, 돈을 필요한 만큼 내지 않았으니 inputPrice에서 메시지를 return하고, 해당 코드는 main을 종료하기 위한 용도로 사용했는데, 제대로 된 금액을 받을 때까지 재시도하게 만드는 것이 훨씬 바람직해 보입니다.
수정했습니다!
|
|
||
| System.out.println("구입금액을 입력해 주세요. (ex. 1000) (숫자가 아닌 경우 0으로 간주)"); | ||
| LottoPurchase purchase = buyLottos(price); | ||
| LottoReceipt receipt = purchase.printReceipt(); |
There was a problem hiding this comment.
print 단어를 보면 출력하는 행위로 보이는데요! 막상 LottoReceipt 객체를 반환하는데, 코드를 보는 다른 사람에게 혼란을 줄 수 있어요! 🤔
|
|
||
| System.out.println("\n당첨 통계\n---------"); | ||
| for (LottoResult res : LottoResult.values()) { | ||
| if (res == LottoResult.NONE) { |
There was a problem hiding this comment.
개인적으로 Enum도 행위를 충분히 가질 수 있는 존재라 생각합니다.
Enum 서로를 비교하는 행위가 외부에 드러나는 것보다 내부에서 'isDisplayable()'의 행위를 정의해 서로 메시지를 주고 받는 형태는 어때요?! 🙂
There was a problem hiding this comment.
Enum이 C/C++에선 단순 자료 열거로 사용되는 경우를 많이 봤어서 enum이 행위를 가진다는 게 익숙하진 않네요. 보다 보면 좀 익숙해지지 않을까 싶습니다..!
수정했습니다!
| return new LottoNumber(bonusValue); | ||
| } catch (NumberFormatException e) { | ||
| System.out.println("숫자를 입력해 주세요."); | ||
| return bonusBall(); |
There was a problem hiding this comment.
bonusBall() 함수를 호출하는 재귀 형태이군요?! 🤔
재귀로 호출하면 어떤 장단점이 있을까요? 🙂 (Hint. Runtime 시점)
There was a problem hiding this comment.
장점으로는 우선 코드가 단순해지고, 복잡한 경우에 있어서 변수를 여러 개 할당할 필요 없이 지속적으로 변수를 유지해 사용할 수 있다는 것 정도가 있겠네요.
단점으로는 위와 같은 경우, 숫자가 아닌 것이 계속 들어오게 된다면 메모리에 함수가 계속 호출되게 되어 결국 프로그램이 터져버린다거나, 너무 많은 메모리가 pile되어 성능 저하도 발생할 수 있을 것 같습니다.
코드 컨벤션..때문에 재귀를 쓰긴 했는데, 메소드 분리를 통해 반복문으로 구현하는 방식으로 바꾸는 게 나을까요?
There was a problem hiding this comment.
정답은 없지만, 재귀의 위험 부담감을 없애는 방향이 더 좋아보여요! 🙂
| public class Main { | ||
| private static final LottoMaker LOTTO_MAKER = new LottoMaker(); | ||
| private static final LottoParser LOTTO_PARSER = new LottoParser(); | ||
| private static final Scanner SC = new Scanner(System.in); |
There was a problem hiding this comment.
변수를 봤을 때 익숙하지 않다면 명확하게 알 수 없는 게 단점이라 생각합니다.
일반적으로 저렇게 많이 쓰길래 그냥 썼는데, 사실 이유 없이 그냥 쓴다는 게 좋을 수는 없기 때문에 수정하는 게 바람직해 보이네요.
수정했습니다!
There was a problem hiding this comment.
private 접근 제어자로 설정된 메서드도 순서가 중요할 수 있어요! 찬형님이 다른 사람의 코드를 보았을 때, 어떤식으로 순서가 정렬되어 있다면 보기 편할지? 상상해볼까요?!
There was a problem hiding this comment.
메서드가 main의 실행 로직 순서대로 배치되어 있으면 읽을 때 조금 더 빠르게 확인할 수 있을 것 같네요. 변수나 메소드나 중요한 것을 위에 두고, 아래로 갈수록 중요도가 떨어지는 것을 배치하라고 했는데, 지금은 거의 다 필수 로직에 해당하는 것 같아서 프로그램 동작 흐름별로 배치해 봤습니다.
taek2222
left a comment
There was a problem hiding this comment.
리뷰가 늦어 죄송해요!! 😭 자 계속해서 발전해서 로또 만들어 봅시다! 🔥
| if (res == LottoResult.BONUS) { | ||
| label = "5개 일치, 보너스 볼 일치"; | ||
| } |
There was a problem hiding this comment.
2등의 관한 로직이 Main 에서 진행되고 있어요. 메시지를 주고 받는 형태로 변경해 보는 건 어때요?! 🤔
There was a problem hiding this comment.
메소드를 분리하면서 자연스럽게 main에선 떨어져 나가긴 했는데, 이걸 메시지로 주고 받는 형태라 해야할 진 모르겠네요.
메시지를 주는 주체가 res가 되어야 할까요?
| int price = 0; | ||
| while (price < 1000) { | ||
| price = inputPrice(); | ||
| } | ||
|
|
||
| LottoPurchase purchase = buyLottos(price); | ||
| LottoReceipt receipt = purchase.getReceipt(); | ||
| displayReceiptInfo(receipt, purchase.getChange()); | ||
|
|
||
| System.out.println("구입금액을 입력해 주세요. (ex. 1000) (숫자가 아닌 경우 0으로 간주)"); | ||
| displayResult(runDraw(receipt)); | ||
| } | ||
|
|
||
| private static int inputPrice() { | ||
| System.out.println("구입금액을 입력해 주세요. (ex. 1000)"); | ||
| try { | ||
| totalPrice = Integer.parseInt(scanner.nextLine()); | ||
| int price = Integer.parseInt(SCANNER.nextLine()); | ||
| if (price < 1000) { | ||
| System.out.println("1000원 이상 입력해야 합니다."); | ||
| } | ||
| return price; | ||
| } catch (NumberFormatException e) { | ||
| totalPrice = 0; | ||
| return 0; | ||
| } | ||
| } |
There was a problem hiding this comment.
private static int inputPrice() {
while (true) {
int price = tryParsePrice();
if (price >= 1000) {
return price;
}
System.out.println("1000원 이상 입력해야 합니다.");
}
}
private static int tryParsePrice() {
System.out.println("구입금액을 입력해 주세요. (ex. 1000)");
try {
return Integer.parseInt(SCANNER.nextLine());
} catch (NumberFormatException e) {
System.out.println("숫자를 입력해 주세요.");
return 0;
}
}이런 비슷한 형태로 리팩토링 해보는 건 어때요? main() 비즈니스 흐름을 더 잘 보이도록 개편해 봅시다! 🙂
제가 구성한 코드는 출력이 이상합니다 ^__^ 그대로.. 말고 수정해 보세요!
제가 구성한 코드도 depth 2를 넘는 걸 볼 수 있어요. 물론 메서드 분리하면 개선할 수 있겠지만, depth 2 를 넘기지 않는 프로그래밍 요구 사항이 왜 있었을까요?! 🤔 try-catch 부분에서도 이를 적용하면 오히려 과한 메서드 분리로 가독성을 좋지 못하게 하지 않을까요?
There was a problem hiding this comment.
depth 2를 넘기지 말라는 게 너무 과도한 반복문이나 조건문을 활용해서 코드 가독성이 떨어지고, 자바같이 네이밍이 긴 언어 같은 경우엔 한 줄에 코드가 다 안 담길 수 있다는 문제가 있기 때문이지 않을까 싶습니다.
사실 2까지는 괜찮을 거라 생각해요. 저도 과도한 메서드 분리와 코드 depth 사이에서 계속 고민해 보고 있는데, 이게 '코드'가 기준이 되면 안되고 로직이 기준이 되어야 할 것 같습니다. 특정 처리 안에서 너무 분기가 생기게 된다면 메서드 분리가 더욱 효과적이고, 그렇지 않다면 반복문이 메서드보다 훨씬 직관적으로 보일테니까요.
일단 그래도 컨벤션 요구사항에 있었으니 해당 방향에 맞게 수정해보겠습니다.
There was a problem hiding this comment.
main 클래스가 100줄이 넘어갔습니다. 현재 로또의 요구 사항에 비해 상당한 코드 라인이 존재해요! 어떤 부분을 분리해볼 수 있을까요? 🤔
There was a problem hiding this comment.
main의 메소드들을 입력, 출력, 처리를 담당하는 객체로 따로 분리해서 각각을 호출할 수 있게끔 하면 좋을 것 같습니다. 기존의 메소드는 일부 public으로 변경되겠지만, 가독성이나 책임 측면에서 훨씬 좋을 것 같긴 합니다.
| assertThatThrownBy(() -> new Lotto(duplicateNumbers)) | ||
| .isInstanceOf(Lotto.LottoException.DuplicateNumber.class) | ||
| .hasMessage("중복된 숫자가 있습니다."); | ||
| .isInstanceOf(RuntimeException.class); |
There was a problem hiding this comment.
RuntimeException.class 중복된 숫자로 발생한 예외인지 알 수 있을까요?!
| ))); | ||
| LottoMaker maker = new LottoMaker(); | ||
|
|
||
| Assertions.assertThrows(IllegalArgumentException.class, |
There was a problem hiding this comment.
assertThatThrownBy
Assertions.assertThrows
찬형님은 두 사용 기준이 무엇인가요?! 🤔
There was a problem hiding this comment.
아직까진 이것저것 손에 잡히는 대로 써보고 있긴 합니다.. 특별히 기준이랄 건 따로 없는 것 같아요.
둘의 특별한 차이점이 따로 있나요?
| Assertions.assertThrows(IllegalArgumentException.class, | ||
| () -> new LottoPurchase(totalPrice, manualLottos, maker)); |
There was a problem hiding this comment.
예외 메시지를 검증하는 테스트도 있고, 아닌 테스트도 있어요. 차이의 기준이 무엇인가요?!
There was a problem hiding this comment.
예외 메시지를 throw하게 되어 있는 상황에 대해선 최대한 검증을 해 보려고 하고, 이외의 경우에는 예외 메시지가 아니라 그냥 로직이 제대로 잘 돌아가는지에 대해 검증해 보기 위한 테스트라고 생각합니다.
다만 테스트를 한 번에 작성한 게 아니고 계속 추가하고 하다보니 일관성이 그리 높진 않은 것 같네요..!
이걸 위해 QA가 있지 않나 싶습니다. 게임을 예시로 들자면 테스트를 위한 전담 팀인 QA팀이 따로 존재하고, QA 팀에서도 미처 발견하지 못한 경우에 있어서 이를 파악하기 위해 베타 테스트를 진행하는 것처럼 테스트를 여러 번 시행해 보며 최대한 버그를 많이 잡아내려고 하는 것이 일종의 조치라 생각합니다. 최대한 다양한 엣지 케이스를 경험해 보며, 이를 수정하지만 모든 상황에 대처가 가능한 완벽한 소프트웨어는 실질적으로 없을 것 같습니다. 그렇기에 유지보수가 소프트웨어에서 굉장히 중요한 부분이고, 더욱 용이한 유지 보수를 위해 코드 가독성을 신경쓰고, 생명 주기를 올리는 데에 힘을 쓰려고 하는 거겠죠. 다만, 내가 짠 코드가 아닌 남이 짠 코드를 더욱 빠르게 이해하고 이를 유지보수하는 것은 쉽지 않은 것 같습니다.. 이번 로또 미션만 하더라도 당장 1-2단계 코드 베이스가 제가 짠 비중이 크지 않다보니 이에 대한 유지보수 또한 시간이 꽤 걸린 편이었구요. 아직 JAVA에 대한 이해도가 높지 않은 것도 있어서 그런 것 같지만, 항상 새로운 기술은 계속해서 나오기 마련이니 경험을 많이 쌓아보는 게 필요할 것 같습니다. |



안녕하세요, SCG에서 활동하는 이찬형입니다.
지난 1-2단계 미션에 이어, 3-4단계 미션 완료 후 제출합니다.
1-2단계 미션에서 짠 코드를 이어받아 그대로 사용하다보니, 어느 정도 기본 구현이 되어 있어서 코드를 짜는 데에 그렇게 오랜 시간이 걸리진 않았는데, 특정 기능을 수정하면서 내부에서 같이 호출되는 객체에 대한 수정이 필요해서 이 흐름을 따라가는 게 조금 힘들었던 것 같습니다.
기능 하나를 추가했더니 객체 3-4개를 바꿔야 하는 상황, 좋은 코드는 아니라고 생각이 들긴 하는데, 처리 로직이 객체 3-4개에 걸쳐 있어서 필수적으로 수정해야 했기 때문에 맞는 방향인건지, 아니면 코드를 잘못 짜서 이런 식의 문제가 발생한 건지에 대한 확신이 제대로 서지 않네요.
검토 부탁드립니다! 감사합니다!