Skip to content
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

[2주차] 객체지향 코드 연습 (win9-tech) #6

Open
wants to merge 15 commits into
base: main
Choose a base branch
from

Conversation

win9-tech
Copy link
Member

No description provided.

Copy link

@Gopistol Gopistol left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

도메인 간 역할과 책임에 대해 정말 고민을 많이 하신 것 같네요!
예외처리에 대해서 각각의 예외사항마다 세부적으로 분리해서 커스텀 예외를 만드셨는데, 예외를 처리할 사항이 점점 많아진다면 이를 관리하는 것 역시 많은 시간과 노력이 필요할 거 같습니다.
저라면 자바에서 기본으로 제공해주는 예외 클래스들을 최대한 활용하면서, 그 안의 메시지를 다르게 넣는 식으로 에러 코드와 메시지를 enum으로 관리할 것 같습니다.

src/main/java/person/Person.java Outdated Show resolved Hide resolved
src/main/java/person/Person.java Outdated Show resolved Hide resolved
src/main/java/account/SavingAccount.java Outdated Show resolved Hide resolved
src/main/java/account/DepositAccount.java Outdated Show resolved Hide resolved
Copy link
Contributor

@KoSeonJe KoSeonJe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

객체 분리는 잘하신 것 같습니다.
하지만 전체적으로 코드 컨벤션(띄어쓰기, 들여쓰기, 매직넘버 처리 등)이 너무 지켜지지 않고 있습니다. 코드 컨벤션을 지키지 않으면 가독성이 매우 떨어져, 같이 개발하는 동료들이 불편할 수 있습니다. 아래 링크를 참고해주세요.
https://sihyung92.oopy.io/af26a1f6-b327-45a6-a72b-c6fcb754e219

화면 입출력을 담당하는 객체가 있었으면 좋았을 것 같다는 생각이 듭니다. 여러 객체에서 입력을 받고 출력을 하고 있는 것은 좋지 않다고 저는 생각합니다.
이미 작성된 코드가 많아서 힘들 수도 있는데, 고민해보는 것만으로 좋을 것 같습니다

src/main/java/AppConfig.java Outdated Show resolved Hide resolved
src/main/java/AppConfig.java Outdated Show resolved Hide resolved
src/main/java/AppConfig.java Outdated Show resolved Hide resolved
src/main/java/AppConfig.java Outdated Show resolved Hide resolved
src/main/java/bank/CentralBank.java Show resolved Hide resolved
src/main/java/bank/clerk/ChangeStatusClerk.java Outdated Show resolved Hide resolved
src/main/java/bank/clerk/CreateAccountClerk.java Outdated Show resolved Hide resolved
src/main/java/bank/clerk/RemittanceClerk.java Outdated Show resolved Hide resolved
src/main/java/person/Person.java Show resolved Hide resolved
src/main/java/person/Person.java Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants