-
Notifications
You must be signed in to change notification settings - Fork 5
[1주차] 객체지향 코드 연습(박진현) #3
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: tiemo0708
Are you sure you want to change the base?
Conversation
baaamk
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
저도 헥사고날 아키텍쳐에 대해서 공부해봐야겠음을 느꼈습니다..
고생하셨습니다!!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
제가 핵사고날 아키텍쳐를 몰라서,,
mvc패턴 관점으로 볼때 비즈니스 로직과 파라메터 타입이 적절하지 않은 것 같습니다..!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
도메인 객체가 컨트롤러에서 생성되는게 어색하다는 얘기이실까요? 그렇다면 헥사고날에서는
도메인 객체의 생성 책임을 어댑터(Controller)가 맡을 수 있습니다
| private void validate(List<Integer> numbers) { | ||
| if (numbers.size() != 6) { | ||
| throw new IllegalArgumentException(ErrorMessages.INVALID_LOTTO_NUMBER_COUNT); | ||
| } | ||
| } | ||
| private void validateNumberRange(List<Integer> numbers) { | ||
| if (numbers.stream().anyMatch(num -> num < 1 || num > 45)) { | ||
| throw new IllegalArgumentException(ErrorMessages.INVALID_LOTTO_NUMBER_RANGE); | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
간단하지만 매직넘버 해소도 필요해보입니다!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
여기를 빠뜨렸네요! 감사합니다
| void purchase(int amount); | ||
| LottoTickets getLottoTickets(); | ||
|
|
||
| LottoResultDTO checkWinning(LottoTickets tickets, WinningLotto winningLotto, int amount); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DTO로 사용해주신 이유가 무었인가요???
KoSeonJe
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
고생하셨습니다!
현재 PR 제목에 작성하신 것과 같이 객체지향 코드 연습을 목적으로 코드를 작성하신다면,
적절한 책임 분리와 책임을 가질 객체를 생성하는 것에 집중하는 것이 좋아보입니다!
아키텍처를 공부하고 연습해본다는 의미에서 헥사고날 아키텍처를 적용해볼 수 있지만, 저희는 객체지향을 공부하고 있으니 객체 분리에 더 집중해보시면 좋을 것 같아요!
전체적으로 책임이 분산되고 있어 응집도가 떨어지는 것 같아요!
어떤 장단점이 있을까? 고민해보고 어떤 기준으로 응집도를 포기하고 책임을 분산시킬지, 응집도를 챙길지를 정해보면 좋을 것 같아요!
| private static final String PURCHASE_AMOUNT_PROMPT = "구입 금액을 입력해 주세요."; | ||
| private static final String WINNING_NUMBERS_PROMPT = "\n당첨 번호를 입력해 주세요 (쉼표로 구분)."; | ||
| private static final String BONUS_NUMBER_PROMPT = "\n보너스 번호를 입력해 주세요."; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
상수로 분리한 이유가 있을까요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
단순한 문자열이 아니라, 무엇을 출력하는지 명시하기위해 사용했습니다
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
그렇다면 이 문자열을 사용하는 클래스 내부에 선언해도 될텐데, 따로 클래스를 생성하신 이유가 있나요?
어떤 문제가 있나요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
지금 해당클래스내에서 사용하고 있는거 아닌가요..?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
아 그렇네요 ㅎㅎ 잘못 봤네요
| - [ ] **로또 구입 금액 입력 기능** | ||
| - [ ] 사용자가 로또 구입 금액을 입력한다 | ||
| - [ ] 구입 금액이 1,000 단위로 나누어 떨어져야한다 | ||
| - [ ] **로또 번호 생성 기능** | ||
| - [ ] 사용자가 입력한 금액에 따라 로또 티켓 수량을 계산한다. | ||
| - [ ] 각 로또 티켓마다 1~45 사이의 중복되지 않는 6개의 번호를 생성한다. | ||
| - [ ] 로또 번호는 오름차순으로 정렬하여 생성한다. | ||
| - [ ] **로또 티켓 발행 및 출력 기능** | ||
| - [ ] 생성된 로또 티켓의 번호를 화면에 출력한다 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
아직 구현하지 못한 기능일까요? 체크가 누락된 것 같아요!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
앗 그렇네요 감사합니다
| private final ConsoleInputView inputView; | ||
| private final ConsoleOutputView outputView; | ||
| private final LottoUseCase lottoUseCase; | ||
| private final NumberValidation<String> amountValidator; | ||
| private final NumberListValidation<String> numberListValidator; | ||
| private final NumberValidation<String> bonusValidator; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
인스턴스 필드가 많다는 것은 무엇을 의미할까요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
해당 컨트롤러는 객체들을 조립하고 흐름을 제어하고 있기에 행당 인스턴스 필드들이 존재합니다
다른 개선 방안이 있을까요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
저의 질문이 인스턴스 필드가 많은 이유에 대해 묻는 것은 아니었습니다!
컨트롤러는 6개의 객체에 의존하고 있는데, 문제점을 생각해보면 좋을 것 같아요!
inputView, outputView를 묶을 수도 있고, Validator들을 모두 묶을 수도 있겠네요!
결합을 약화시켜보는 게 좋을 것 같아요
| private final NumberValidation<String> amountValidator; | ||
| private final NumberListValidation<String> numberListValidator; | ||
| private final NumberValidation<String> bonusValidator; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Validator는 좋은 객체일까요? 좋은 객체는 무엇일까요?
각 독립적인 객체가 자신의 검증 로직을 가지지않고, 검증 로직을 한곳에 모아둔다면 어떤 단점이 있을까요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
해당 Validator는 입력검증에 관한것입니다. 도메인 객체는 도메인 객체 검증이 내부에있습니다
즉 사용자의 입력값 형식이 유효한가는 입력검증의 책임이라고 생각합니다
여기서는 숫자를 입력받아야하는데 문자가 입력된다면 입력 검증에서 걸러줬습니다.
도메인검증에서는 입력값이 비즈니스 규칙에 맞는지 확인하는거라 생각합니다.
여기서는 1~45범위, 중복등을 해당 도메인 객체에서 처리했습니다!
저는 이 검증의 시점이 다르다고 생각 합니다.
입력검증은 사용자 입력즉시, 그리고 도메인 검증은 객체 생성시 라고 생각합니다!
또한 입력검증의 경우 재사용할 수 있기때문에 인터페이스-구현 객체 구조로 설계하였습니다
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
단순한 입력 검증이라면, util성일 경우가 강할 것 같은데 무엇을 입력받는 지에 따라 도메인 이름을 포함한 검증 클래스를 생성하고 인터페이스를 생성하는 것은 과하다고 생각이 들어요!
구현체가 도메인 이름을 포함하고 있다면, 도메인 로직과 관련이 있다고 생각하게 되는데, 그렇다면 입력 검증의 책임이 맞을까요?
도메인 로직과 관련이 없고 단순히 입력 검증이라면 Util 클래스로 정적 메서드를 사용하지 않고, 인스턴스를 생성해서 사용하는 이유는 있나요??
사용자 입력 즉시 검증 받아야한다면, 입력 클래스에서 검증이 이루어져야 하지 않을까요? 컨트롤러에서 입력과 관련된 검증을 하신 이유가 있나요?
|
|
||
| public void run() { | ||
| try { | ||
| int amount = amountValidator.validate(inputView.inputPurchaseAmount()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
amountValidator의 검증 로직은 amount의 책임 아닐까요?
분리해놓으면 어떤 단점이 있나요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
해당 amountValidator는 입력검증만 해줍니다. 명시적으로 amountInputValidator로 변경하는게 좋아보입니다!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
amount와 관계 없이 단순히 숫자인지 검증하고 싶다면, amount라는 이름을 제거해도 괜찮지 않을까요?
이것이 amount이기 때문에 숫자인지 검증해야한다는 것이라면, 이 책임은 amount의 책임 아닌가요?
|
|
||
| for (Lotto ticket : tickets.getTickets()) { | ||
| int matchCount = countMatchingNumbers(ticket.getNumbers(), winningLotto.getWinningNumbers()); | ||
| boolean bonusMatch = ticket.getNumbers().contains(winningLotto.getBonusNumber()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이 로직은 ticket의 책임 아닐까요?
| private int countMatchingNumbers(List<Integer> userNumbers, List<Integer> winningNumbers) { | ||
| return (int) userNumbers.stream() | ||
| .filter(winningNumbers::contains) | ||
| .count(); | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lotto의 책임 아닐까요?
| private void validateWinningNumbers(List<Integer> numbers) { | ||
| if (numbers.size() != LottoConstants.LOTTO_NUMBER_COUNT) { | ||
| throw new IllegalArgumentException(ErrorMessages.INVALID_LOTTO_NUMBER_COUNT); | ||
| } | ||
| if (new HashSet<>(numbers).size() != LottoConstants.LOTTO_NUMBER_COUNT) { | ||
| throw new IllegalArgumentException(ErrorMessages.DUPLICATE_LOTTO_NUMBER); | ||
| } | ||
| if (numbers.stream().anyMatch(n -> n < LottoConstants.MIN_LOTTO_NUMBER || n > LottoConstants.MAX_LOTTO_NUMBER)) { | ||
| throw new IllegalArgumentException(ErrorMessages.INVALID_LOTTO_NUMBER_RANGE); | ||
| } | ||
| } | ||
|
|
||
| private void validateBonusNumber(List<Integer> winningNumbers, int bonus) { | ||
| if (bonus < LottoConstants.MIN_LOTTO_NUMBER || bonus > LottoConstants.MAX_LOTTO_NUMBER) { | ||
| throw new IllegalArgumentException(ErrorMessages.INVALID_LOTTO_NUMBER_RANGE); | ||
| } | ||
| if (winningNumbers.contains(bonus)) { | ||
| throw new IllegalArgumentException(ErrorMessages.DUPLICATE_BONUS_NUMBER); | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
중복되는 로직이 있는 것 같은데, 메서드를 분리해보면 중복도 제거되고 가독성도 높아질 것 같아요
| void purchase(int amount); | ||
| LottoTickets getLottoTickets(); | ||
|
|
||
| LottoResultDTO checkWinning(LottoTickets tickets, WinningLotto winningLotto, int amount); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| void purchase(int amount); | |
| LottoTickets getLottoTickets(); | |
| LottoResultDTO checkWinning(LottoTickets tickets, WinningLotto winningLotto, int amount); | |
| void purchase(int amount); | |
| LottoTickets getLottoTickets(); | |
| LottoResultDTO checkWinning(LottoTickets tickets, WinningLotto winningLotto, int amount); |
사소한 줄바꿈도 지켜주면 좋을 것 같아요
| private static final String ERROR_PREFIX = "[ERROR] "; | ||
|
|
||
| public static final String INVALID_LOTTO_NUMBER_COUNT = ERROR_PREFIX + "로또 번호는 6개여야 합니다."; | ||
| public static final String DUPLICATE_LOTTO_NUMBER = ERROR_PREFIX + "로또 번호는 중복될 수 없습니다."; | ||
| public static final String INVALID_LOTTO_NUMBER_RANGE = ERROR_PREFIX + "로또 번호는 1부터 45 사이의 숫자여야 합니다."; | ||
| public static final String INVALID_PURCHASE_AMOUNT = ERROR_PREFIX + "금액은 1,000원 단위여야 합니다."; | ||
| public static final String EMPTY_LOTTO_TICKET_LIST = ERROR_PREFIX + "구매한 로또 티켓 목록은 비어 있을 수 없습니다."; | ||
| public static final String INVALID_AMOUNT_FORMAT = ERROR_PREFIX + "구입 금액은 숫자여야 합니다."; | ||
| public static final String DUPLICATE_BONUS_NUMBER = ERROR_PREFIX + "보너스 번호는 당첨 번호와 중복될 수 없습니다."; | ||
| public static final String INVALID_NUMBER_INPUT = ERROR_PREFIX + "입력값은 숫자여야 합니다."; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
커스텀 예외 클래스를 생성해서 중복을 줄여보는 것은 어떤가요?
Seooooo24
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
고생 많으셨습니다!
제 시점에서 보이는 의문점들은 이미 다 말씀해주셔서 따로 코멘트 달 부분은 없는 것 같습니다... 프로그램 구조가 객체지향적으로 매우 잘 짜여졌다고 생각합니다!
덕분에 헥사고날 패턴에 대해 공부할 수 있었습니다. 감사합니다.
KoSeonJe
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
코멘트 답글 확인해주세요!
한번 객체의 책임을 분산시키지 않고, 객체에 온전히 부여해보면 좋을 것 같아요. 적용하면서 장점과 단점을 느끼고 단점을 어떻게 해소시킬 수 있을까 또 고민해나가보면 좋을 것 같습니다!
| private final ConsoleInputView inputView; | ||
| private final ConsoleOutputView outputView; | ||
| private final LottoUseCase lottoUseCase; | ||
| private final NumberValidation<String> amountValidator; | ||
| private final NumberListValidation<String> numberListValidator; | ||
| private final NumberValidation<String> bonusValidator; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
저의 질문이 인스턴스 필드가 많은 이유에 대해 묻는 것은 아니었습니다!
컨트롤러는 6개의 객체에 의존하고 있는데, 문제점을 생각해보면 좋을 것 같아요!
inputView, outputView를 묶을 수도 있고, Validator들을 모두 묶을 수도 있겠네요!
결합을 약화시켜보는 게 좋을 것 같아요
| private static final String PURCHASE_AMOUNT_PROMPT = "구입 금액을 입력해 주세요."; | ||
| private static final String WINNING_NUMBERS_PROMPT = "\n당첨 번호를 입력해 주세요 (쉼표로 구분)."; | ||
| private static final String BONUS_NUMBER_PROMPT = "\n보너스 번호를 입력해 주세요."; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
아 그렇네요 ㅎㅎ 잘못 봤네요
| private final NumberValidation<String> amountValidator; | ||
| private final NumberListValidation<String> numberListValidator; | ||
| private final NumberValidation<String> bonusValidator; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
단순한 입력 검증이라면, util성일 경우가 강할 것 같은데 무엇을 입력받는 지에 따라 도메인 이름을 포함한 검증 클래스를 생성하고 인터페이스를 생성하는 것은 과하다고 생각이 들어요!
구현체가 도메인 이름을 포함하고 있다면, 도메인 로직과 관련이 있다고 생각하게 되는데, 그렇다면 입력 검증의 책임이 맞을까요?
도메인 로직과 관련이 없고 단순히 입력 검증이라면 Util 클래스로 정적 메서드를 사용하지 않고, 인스턴스를 생성해서 사용하는 이유는 있나요??
사용자 입력 즉시 검증 받아야한다면, 입력 클래스에서 검증이 이루어져야 하지 않을까요? 컨트롤러에서 입력과 관련된 검증을 하신 이유가 있나요?
|
|
||
| public void run() { | ||
| try { | ||
| int amount = amountValidator.validate(inputView.inputPurchaseAmount()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
amount와 관계 없이 단순히 숫자인지 검증하고 싶다면, amount라는 이름을 제거해도 괜찮지 않을까요?
이것이 amount이기 때문에 숫자인지 검증해야한다는 것이라면, 이 책임은 amount의 책임 아닌가요?
| WinningLotto winningLotto = new WinningLotto(winningNumbers, bonusNumber); | ||
|
|
||
| LottoResultDTO resultDTO = lottoUseCase.checkWinning(tickets, winningLotto, amount); | ||
| outputView.printStatistics(resultDTO.getResult(), resultDTO.getProfitRate()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DTO가 애플리케이션의 응답 객체라 하였는데, 응답은 클라이언트의 요청에 따른 응답을 말하는 것 아닌가요?
출력형식에 최적화되어 있지 않다면, 어떤 것에 최적화되어있는지 궁금해요
| public class BonusNumberValidator implements NumberValidation<String> { | ||
| @Override | ||
| public int validate(String input) { | ||
| return parseNumber(input); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
파싱이 되는가?만 검증으로 마치면 괜찮다고 생각하는데, 아예 파싱의 책임을 갖고 있는 것 같아 어색해보였습니다.
| @Override | ||
| public List<Integer> validate(String input) { | ||
| return parseNumbers(input); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
검증 = 파싱이라는 말이 어색하지는 않나요?
파싱과정에 검증 로직이 포함된다고 생각해요.
|
|
||
| import java.util.List; | ||
|
|
||
| public interface NumberListValidation<T> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
입력값을 검증하기 위한 객체라 하였는데, 문자열 외 다른 입력값이 들어올 수 있나요?
그리고 제네릭 장점 설명할 때 타입이라는 키워드를 사용해주시면 좋을 것 같아요
| import java.util.List; | ||
| import java.util.Map; | ||
|
|
||
| public class ResultCalculator { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
로또 결과를 계산하는 책임을 LottoResult 내부에서 처리하지 않고 따로 분리하신 이유가 있있나요?
헥사고날 패턴을 적용하려 노력해 보았습니다.
최대한 기능 별로 분리해보려 했으나 아직 분리가 덜 된 상태인것 같습니다!
테스트코드는 리팩토링 하면서 추가할 예정입니다
부족하지만 잘 부탁드립니다!!
참고
기존 설정된 jdk 11->jdk 17로 변경해서 사용하여 우테코 라이브러리에서 로그 메시지가 뜨는 이슈가 있습니다!