[2주차]객체지향 코드 연습(0711kc) #13
Conversation
0711kc
requested review from
Hoya324,
KoSeonJe,
Gopistol and
TaetaetaE01
as code owners
There was a problem hiding this comment.
main application에 view 코드와 비지니스 로직들이 많이 들어가 있습니다. 객체로 관심분리가 필요해보입니다. 구조가 가지는 책임 그러고 validate를 해야되는 시점에 대해 한번 고민 후에 코드 작성하면 조금 수훨하게 하실 수 있을 거 같습니다!
Q : 여기서는 굳이 Optional을 사용하지 않고 그냥 if문을 이용하는게 더 좋을까요??
A : Optional은 null이 되는 객체를 감싸는 wrapper로 NullPointerException을 방지하고 null을 다루는데 제일 명확한 방법입니다. if문으로 null을 제어할 수 있지만 확실하게 Optional을 사용하는 것이 저는 더 좋다고 생각합니다
| for(EMenu menu : EMenu.values()) | ||
| menuStringBuilder.append("\n").append(menu.ordinal()+1).append(". ").append(menu.getMenuText()); |
There was a problem hiding this comment.
반복문의 내용이 한 줄이더라도 가독성을 위해 컨벤션을 지켜주세요!!
| menuStringBuilder.append("\n").append(menu.ordinal()+1).append(". ").append(menu.getMenuText()); | ||
| this.menu = menuStringBuilder.toString(); | ||
| } | ||
| public void run() { |
There was a problem hiding this comment.
뱅킹 시스템의 일련의 과정들이 모두 한 객체에서 이루어지고 있습니다. 객체를 통환 책임의 분리가 필요해보입니다. 기능과 속성에 따른 고민을 조금 더 해보시는 것을 추천드립니다
| if(inputInt < 0) System.out.println("0���� ū ���ڸ� �Է����ּ���."); | ||
| else validData = true; |
| else | ||
| System.out.println("���� ������ �����Ͽ����ϴ�"); |
| */ | ||
| private void withdrawal() { | ||
| String accountNumber = getAccountNumber("����� "); | ||
| if(accountNumber == null) return; |
There was a problem hiding this comment.
계좌번호가 생성되었을 때 비활성화 하는 기능을 만들어야 할 같습니다! 또한 string 객체로 null이 아닌 다른 방법으로 비활성화/활성화 기능을 체크하는 메소드를 고민해보세요
| * �۱� | ||
| */ | ||
| private void remittance() { | ||
| String withdrawalAccountNumber = getAccountNumber("����� "); |
There was a problem hiding this comment.
각 account 기능 마다 format을 이렇게 가져가기 보단 하나의 객체가 print를 담당하는 것이 편해보입니다
| @Override | ||
| public BigDecimal getInterest(BigDecimal balance) { | ||
| BigDecimal interest = null; | ||
| if(balance.compareTo(getValueOf(10000000)) >= 0) { |
There was a problem hiding this comment.
매직넘버를 도입해주세요. 이자율 정책이 변경될 경우 BasicAccountInerest 코드를 변경해야하며, 무엇보다 상수만 봤을 때 어떤 의미를 가지는 지 알 수가 없어요.
CentralBank.java 는 Controller, Service, Repository로 분리하였다. BankingApplication.java 는 AppConfig, BankingManager, BankingFunction, FrontController, View로 분리하였다.
| StringBuilder menuStringBuilder = new StringBuilder(); | ||
| menuStringBuilder.append("[�� ����]"); | ||
| for(EMenu menu : EMenu.values()) | ||
| menuStringBuilder.append("\n").append(menu.ordinal()+1).append(". ").append(menu.getMenuText()); |
There was a problem hiding this comment.
enum의 ordinal() 사용을 추천하지 않습니다.
현재는 출력을 위해서 사용했기에 큰 영향은 없지만, 순서를 기반으로한 로직 같은 경우에는 순서가 바뀌거나, 중간에 추가가 될 때 복잡해지는 경우도 있기에 필드 하나를 추가하는 것을 추천드립니다!
저도 최교수님이 써서 알아보다가 우연히 발견했어요.
| private int getInputInt(String printMsg) { | ||
| System.out.println(printMsg); | ||
| boolean validData = false; | ||
| int inputInt = 0; | ||
| do { | ||
| try { | ||
| String input = scanner.next(); | ||
| inputInt = Integer.parseInt(input); | ||
| if(inputInt < 0) System.out.println("0���� ū ���ڸ� �Է����ּ���."); | ||
| else validData = true; | ||
| } catch (NumberFormatException e) { | ||
| System.out.println("���ڸ� �Է����ּ���."); | ||
| } | ||
| } while(!validData); | ||
| return inputInt; | ||
| } |
There was a problem hiding this comment.
입출력에 대한 책임을 BankingApplicaiton이 져야할까요??
| HashMap<EParamType, Object> params = | ||
| accountType.getAccountMaker().makeAccount(inputView, outputView); |
There was a problem hiding this comment.
params보단 조금 더 의미있는 변수명을 사용하면 좋을 것 같아요.
| accountTypeBuilder.append("[계좌 종류 선택]"); | ||
| for(AccountType accountType : AccountType.values()){ | ||
| accountTypeBuilder.append("\n") | ||
| .append(accountType.ordinal() + AccountType.MIN_INDEX) |
There was a problem hiding this comment.
저번에 enum의 ordinal() 말고 인덱스를 직접 정의하는 것을 지향하라는 조언이 있었는 데,
그렇게 해야하는 이유중에 하나가 이것이 될 수 있겠어요.
이러한 로직이 여러군데 존재하는 데 매번 AccountType.MIN_INDEX를 더하는 게 좋아보이지 않아요.
| return eMenu; | ||
| } | ||
| } | ||
| throw new IndexOutOfRangeException(Message.IndexOutOfRange, EMenu.MIN_INDEX, EMenu.MAX_INDEX, index); |
There was a problem hiding this comment.
한번 더 예외처리한 이유가 있나요?
물어본 이유가 위에 if문으로 예외 처리를 이미 했다고 생각해서 그렇습니다.
| public FrontController() { | ||
| controllerMap.put(EMenu.eMakeAccount, new CreateAccountController()); | ||
| controllerMap.put(EMenu.eDeleteAccount, new DeleteAccountController()); | ||
| controllerMap.put(EMenu.eAccountInfo, new GetAccountInfoController()); | ||
| controllerMap.put(EMenu.eActivateAccount, new ActivateAccountController()); | ||
| controllerMap.put(EMenu.eDeactivateAccount, new DeactivateAccountController()); | ||
| controllerMap.put(EMenu.eWithdrawal, new WithdrawalController()); | ||
| controllerMap.put(EMenu.eDeposit, new DepositController()); | ||
| controllerMap.put(EMenu.eRemittance, new RemittanceController()); | ||
| controllerMap.put(EMenu.eInterest, new GetInterestController()); | ||
| } |
There was a problem hiding this comment.
지금 구조를 보면서 하나가 아쉬웠다가 이것을 보고 떠오른 게 있습니다.
일단 BankingView의 책임이 너무나도 많은 것 같아요.
그 이유가 모든 은행 업무을 다 책임지고 있어서 그런 것 같았거든요.
그래서 제 생각엔 이것은 메뉴를 받는 객체로만 정의하고, 각 도메인 별로의 사용자 인터페이스, 컨트롤러, 서비스를 분리시켰으면 좀 좋았지 않았을 까라는 생각도 듭니다.
그래서 여기 FrontController가 선택된 메뉴에 따라서 부서의 MVC라든지, 그런식으로 했으면 조금 더 분리가 되었지 않을 까 싶어요.
이건 개인적인 생각입니다..!
구조도입니다.
