[2주차]객체지향 코드 연습(tiemo0708) #8
Conversation
tiemo0708
requested review from
Hoya324,
KoSeonJe,
Gopistol and
TaetaetaE01
as code owners
There was a problem hiding this comment.
변수명, 메소드명이 어떤 역할을 하고 있는지 명시적으로 나타낼수 있도록 이름을 짓는 것이 중요합니다.
해당 메소드와 관련없는 책임을 지는 경우가 많은 것 같습니다. 메소드를 분리해줘야 할 것 같습니다.
패키지 분리를 좀더 고민해보는 것이 좋을 것 같습니다. Bank의 controller는 같은계층에 있는 것에 반해 Service와 Repository는 Bank패키지 내에 있는 것이 적절하지 않은 것 같습니다.
도메인 별로 분리를 할지, 계층별로 분리를 할지 고민해보고 한가지 방법만 사용하는 것이 좋습니다.
| public class BankingApplication { | ||
|
|
||
| public static void main(String[] args) { | ||
| BankController bankController = new BankController(new InputView(),new OutputView()); |
There was a problem hiding this comment.
의존성 주입의 책임을 지고있는 객체가 AppConfig인 것 같은데,
되도록이면 해당 책임에 대한 역할을 가진 객체에게 책임을 부여하는 게 올바르다고 생각합니다.
| import java.util.Map; | ||
|
|
||
| public class BankMemoryRepository implements BankRepository { | ||
| private static Map<String, GeneralMember> store = new HashMap<>(); |
There was a problem hiding this comment.
| private static Map<String, GeneralMember> store = new HashMap<>(); | |
| private final Map<String, GeneralMember> store = new HashMap<>(); |
하나의 인스턴스로 관리될 repository내에서 굳이 static 선언을 할 필요가 없는 것 같습니다.
static의 쓰임에 대해서 메모리와 관련하여 한번 고민해보면 좋을 것 같습니다.
또한, final키워드를 작성해줌으로써 해당 인스턴스(Map)이 변경되지 않는 다는 것을 명시해야 할 것 같습니다.
| } | ||
|
|
||
| @Override | ||
| public void addAmount(String accountNumber, BigDecimal depositAmount) { |
There was a problem hiding this comment.
이 메소드가 repository의 책임이 맞는 건지 고민해보면 좋을 것 같습니다.
| } | ||
|
|
||
| @Override | ||
| public void subtractAmount(String accountNumber, BigDecimal withdrawAmount) { |
There was a problem hiding this comment.
해당 메소드가 이 repository의 행위로써 적절한지 고민해봐야 할 것 같습니다
src/main/java/bank/BankService.java
Outdated
| GeneralMember getAccountInfo(String accountNumber); | ||
|
|
||
|
|
||
|
|
There was a problem hiding this comment.
필요없는 줄은 삭제시켜 주기 바랍니다. 코드 컨벤션을 지켜주세요!
src/main/java/util/Appconfig.java
Outdated
| } | ||
| public static BankRepository bankRepository(){ | ||
| return new BankMemoryRepository(); | ||
|
|
||
| } |
There was a problem hiding this comment.
| } | |
| public static BankRepository bankRepository(){ | |
| return new BankMemoryRepository(); | |
| } | |
| } | |
| public static BankRepository bankRepository(){ | |
| return new BankMemoryRepository(); | |
| } |
코드 컨벤션을 지켜주세요
src/main/java/util/Appconfig.java
Outdated
| public BankService bankService(){ | ||
| return new bankServiceImpl(bankRepository()); | ||
| } | ||
| public static BankRepository bankRepository(){ |
There was a problem hiding this comment.
외부로부터 접근이 너무 자유로워지는데, static을 선언하는 대신에 BankRepository를 싱글톤 패턴을 이용하는 것은 어떨까요?
src/main/java/view/OutputView.java
Outdated
| combinedInfo.forEach(System.out::println); | ||
| } | ||
|
|
||
| public void setAccountInfo() { |
src/main/java/view/OutputView.java
Outdated
| } | ||
|
|
||
| //나중에 상수처리 | ||
| public void setAccountInfo(String accountType, String bankAccountNumber, String name, BigDecimal amount, BigDecimal interestEstimated) { |
There was a problem hiding this comment.
메소드명과 메소드의 행위가 일치하지 않은 것 같습니다.
| public void setAccountInfo(String accountType, String bankAccountNumber, String name, BigDecimal amount, BigDecimal interestEstimated) { | |
| public void printAccountInfo(String accountType, String bankAccountNumber, String name, BigDecimal amount, BigDecimal interestEstimated) { |
src/main/java/view/OutputView.java
Outdated
| System.out.println("계좌번호: " + bankAccountNumber); | ||
| System.out.println("소유자: " + name); | ||
| System.out.println("잔액: " + amount); | ||
| System.out.println("예상 금액: " + interestEstimated); |
There was a problem hiding this comment.
출력의 반복은 성능을 저하시킵니다. StringBuilder를 사용하여 한번에 출력하는 방법이 더 좋을 것 같습니다.
추가 Q & A
🤔 잘 모르겠거나, 더 궁금한 내용이 있었나요?
DI(의존성 주입) 에 대해 모르는 내용이 많았습니다. Appconfig를 통해 객체 주입을 할때
controller와 view 그리고 service로 연결되는 부분을 어떻게 해야할까요?
view와 service 를 Controller로 객체주입을 하는게 맞을까요?
집중적으로 리뷰 받고싶은 것이 있나요?
현재 코드의 구조에 대한 리뷰 받고싶습니다!
특히 객체 주입에 관해서 리뷰 받고싶습니다.