Conversation
qwejiung
requested review from
Hoya324,
KoSeonJe,
Gopistol and
TaetaetaE01
as code owners
There was a problem hiding this comment.
전체적으로 책임의 분리가 필요해보입니다. 하나의 책임만을 가진 객체를 생성하고 그 객체와 비슷한 역할을 하고 있는 객체끼리 묶어 패키지를 생성하는 것도 필요해보입니다.
예를 들어 account 패키지를 만들어 account 관련 클래스를 한 곳에서 관리하는 것입니다.
그리고 사용자의 입력할 수 있는 부분이 구현되지 않았습니다. 구현해주시면 좋을 것 같습니다.
변수명, 메소드명을 짓는 것에 많은 고민을 하고 해당 변수가 어떤일을 하는지 알 수 있게 명확한 단어로 선언해야 합니다. 또한 가독성을 높이기 위해 아래 링크를 보고 코드 컨벤션을 지키면 좋을 것 같습니다.
https://sihyung92.oopy.io/af26a1f6-b327-45a6-a72b-c6fcb754e219
| // 내부에서 입력을 받는 액션? | ||
| // 적금 계좌는 | ||
| public class Bank { | ||
| private ArrayList<Account> accountArray; |
There was a problem hiding this comment.
자료구조를 직접 변수명으로 만드는 것은 좋지 않습니다. 아래와 같이 변경하는 것은 어떨까요?
| private ArrayList<Account> accountArray; | |
| private ArrayList<Account> accounts; | |
| // 내부에서 입력을 받는 액션? | ||
| // 적금 계좌는 | ||
| public class Bank { | ||
| private ArrayList<Account> accountArray; |
There was a problem hiding this comment.
| private ArrayList<Account> accountArray; | |
| private final ArrayList<Account> accountArray = new ArrayList<>(); | |
가독성과 코드 중복 방지를 위해 필드에서 초기화 하는 것이 좋습니다.
예를 들어 여러 개의 생성자를 만들 경우, 각 생성자 마다 초기화 해야하는 코드 중복이 발생합니다
final 키워드를 사용해 변경되지 않을 인스턴스라는 것을 명시해야 합니다.
|
|
||
| // 계좌생성 | ||
| // 한 메소드안에 NORMAL과 SAVING을 같이 넣을려했지만 매개변수 이슈로 나눔 | ||
| public void createAccount(AccountType accountType, String accountNumber, String owner, |
There was a problem hiding this comment.
메소드명을 더 구체적으로 작성해야 할 것 같습니다.
어떤 Account를 생성하는지 명시해줘야 합니다.
| public void createAccount(AccountType accountType, String accountNumber, String owner, | |
| public void createBasicAccount(AccountType accountType, String accountNumber, String owner, | |
Week2/BankingApplication.java
Outdated
| bank.deposit("123123",BigDecimal.valueOf(10)); | ||
| bank.getAccountInfo("123123"); | ||
|
|
||
|
|
|
|
||
| if( balance.compareTo(BigDecimal.valueOf(1000)) >= 0 ) | ||
| return balance.multiply(BigDecimal.valueOf(0.5)); | ||
| else if ( balance.compareTo(BigDecimal.valueOf(500)) >= 0) |
There was a problem hiding this comment.
else if 를 반복해서 사용하는 것은 매우 좋지 않은 방법입니다.
Switch문을 사용해보세요. Enum클래스를 활용하는 방법도 있습니다!
| // 1000만원이상은 이자율 50%, 500만원 이상은 7%, 100만원 이상은 4%, 1만원 이상은 2%, 그 외에는 1% | ||
| public BigDecimal getInterest(BigDecimal balance){ // 계좌 잔액에 대한 이자금액 반환 | ||
|
|
||
| if( balance.compareTo(BigDecimal.valueOf(1000)) >= 0 ) |
There was a problem hiding this comment.
1000은 어떤 숫자를 의미하는 것인가요? 또한 0은??
상수를 구체적으로 설명해주는 변수 선언이 필요할 것 같습니다
| public void withdraw(BigDecimal value) { | ||
| BigDecimal target = new BigDecimal(getTargetAmount()); | ||
| int temp = getBalance().compareTo(target.multiply(BigDecimal.valueOf(0.01))); | ||
| if(temp>0) { |
There was a problem hiding this comment.
| if(temp>0) { | |
| if(temp > 0) { | |
| import java.math.BigDecimal; | ||
|
|
||
| public class SavingAccount extends BasicAccount{ | ||
| private String targetAmount; |
There was a problem hiding this comment.
targetAmount도 Bigdecimal로 생성하는 것이 좋지 않을까요?
There was a problem hiding this comment.
전체적으로 객체 분리는 어느정도 잘 하신 것 같습니다.
하지만 객체 분리만 되어있고 메소드의 책임 분리가 안되어 있습니다.
50줄이 넘어가고 100줄이 넘어갑니다. 메소드가 20줄이 넘어가지 않게 해주세요.
그리고 고쳐야 할 점
- 중첩된 반복문, 조건문 사용않고 모두 고치는 것이 가능합니다. 되도록이면 사용하지 말아주세요
- 매직넘버가 너무 많습니다. 없애기 위해 공부해보세요
- 줄바꿈,띄어쓰기, 변수명 등 코드 컨벤션이 많이 지켜지지 않고 있습니다. 고쳐주세요
- else, else if문은 되도록이면 거의 사용을 안하시는게 좋습니다. early return을 사용하여 고쳐주세요.
- 패키지 명은 소문자로만 구성해야 합니다. 패키지 안의 클래스를 추상화 시킨 이름으로 사용하는 것이기 때문에 하나의 명사로 작성하는 것이 가능하실 겁니다.
- 사용하지 않은 메소드, 클래스는 삭제해주세요.
|
|
||
| import java.math.BigDecimal; | ||
|
|
||
| public interface Account { // |
| void eraseStringBuilder(); | ||
| boolean isActivated(); | ||
|
|
||
| } |
There was a problem hiding this comment.
| } | |
| } | |
| // public BasicAccount(AccountType accountType, String accountNumber, String owner, BigDecimal balance, boolean isActivated) { | ||
| // this.stringBuilder = new StringBuilder(); | ||
| // this.accountType = accountType; | ||
| // this.accountNumber = accountNumber; | ||
| // this.owner = owner; | ||
| // this.balance = balance; | ||
| // this.isActivated = isActivated; | ||
| // } |
| this.balance=this.balance.add(value); | ||
| } | ||
|
|
||
|
|
| this.stringBuilder.setLength(0); | ||
| } | ||
|
|
||
| public void setBalance(BigDecimal balance) { this.balance = balance;} |
There was a problem hiding this comment.
| public void setBalance(BigDecimal balance) { this.balance = balance;} | |
| public void setBalance(BigDecimal balance) { | |
| this.balance = balance; | |
| } | |
컨벤션!
| import java.util.Scanner; | ||
|
|
||
|
|
||
| public class UI extends Bank { |
| // 1. 송금 2. 출금 3. 계좌찾기 이런식으로해서 해야할듯 | ||
| // 3누르면 계좌번호 입력하라고뜨고 그 계좌번호를 가지고있는 account찾아서 정보 출력 | ||
|
|
||
| public void userView() { |
| } | ||
| } | ||
|
|
||
| switch (number) { |
| } | ||
| } | ||
|
|
||
| switch (number) { |
| import java.util.*; | ||
| import java.math.BigDecimal; | ||
|
|
||
| public class Bank extends InputAccount { |
There was a problem hiding this comment.
Bank가 inputAccount를 상속받는데, 이것은 적절해보이지 않습니다
추가 Q & A
디테일한거는 아직 완성 못 해서 피드백 받으면서 보완하고자 먼저 올립니다