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주차] 객체지향 코드 연습(starshape7) #1

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

Conversation

starshape7
Copy link

@starshape7 starshape7 commented Mar 31, 2024

추가 Q & A

🤔 잘 모르겠거나, 더 궁금한 내용이 있었나요?

Interest Calculater 부분을 어떻게 구현해야할지 잘 모르겠습니다.

집중적으로 리뷰 받고싶은 것이 있나요?

구조나 코드 컨벤션에 대해 리뷰해주시면 감사하겠습니다.

@starshape7 starshape7 changed the title [2주차] 객체지향 코드 연습(starshape7) [2주차] 객체지향 코드 연습(starshape77) Mar 31, 2024
@starshape7 starshape7 changed the title [2주차] 객체지향 코드 연습(starshape77) [2주차] 객체지향 코드 연습(starshape7) Mar 31, 2024
Copy link

@Erichong7 Erichong7 left a comment

Choose a reason for hiding this comment

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

MVC패턴, SOLID 원칙에 대해 많은 고민이 필요해보입니다. 또한, 패키지 구조가 전혀 분리되어 있지 않습니다. 현재와 같은 상태로 코드 파일이 수천개가 되면 관리하기 정말 어려울 것입니다. layered architecture에 대해 공부하여 패키지 구조를 나눠봅시다.

src/Account.java Outdated
public BigDecimal getAmount() {return amount;}
public void setAmount(BigDecimal amount) {this.amount = amount;}
public boolean isActivated() {return activated;}
public void setActivated(boolean activated) {this.activated = activated;}

Choose a reason for hiding this comment

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

코드 컨벤션에 대하여 알아보고 적용하여 읽기 쉽고 관리하기 쉬운 코드에 대해 고민해보아요

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

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

getter와 setter를 남발하는 것은 좋지 않습니다. 객체지향의 장점은 캡슐화에 있습니다. 즉, 외부에 객체의 구체적인 정보를 은닉하는 것에 중점을 둬야합니다. 특히 객체 내부에서 setter를 사용하고있는데 외부가 아닌 내부에서 변경되는 값인데 setter를 사용하는 이유가 무엇인가요? 접근 제어자의 역할에 대해서도 고민해봅시다.

{
if(account instanceof SavingAccount)
{
if(((SavingAccount) account).compareCAGA(account))

Choose a reason for hiding this comment

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

3번 이상 nesting된 코드는 좋지 않습니다. 함수로 분리하는 방법, Early Return을 사용하는 방법 등.. 여러가지 해결 방안이 있으니 찾아보고 내 코드에 가장 적합한 방안을 적용해봅시다.

{
account.withDraw(amount);
}
else { System.out.println("계좌 금액이 목표 금액보다 낮습니다. 인출이 거부됩니다."); }

Choose a reason for hiding this comment

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

매직넘버에 대해 알아보고 적용해봅시다.
https://jake-seo-dev.tistory.com/155

Choose a reason for hiding this comment

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

인터페이스를 사용하는 이유는 무엇일까요?
현재는 인터페이스의 장점을 사용했다고 보기 어렵고 이 인터페이스의 존재 이유도 잘 모르겠습니다.
https://gofnrk.tistory.com/22

BigDecimal overTenM= new BigDecimal("10000000"); // 천만
BigDecimal overFiveM = new BigDecimal("5000000"); // 오백만
BigDecimal overM = new BigDecimal("1000000"); // 백만
BigDecimal overTenT = new BigDecimal("10000"); // 만

Choose a reason for hiding this comment

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

함수 안에서 객체를 정의하여 할당하고 있습니다. 이렇게 되면 함수를 호출할 때마다 객체가 다시 정의되어 비효율적입니다. 함수 내부가 아닌 클래스 내부에 정의하는 것이 좋아보입니다.

// 이자를 계산하고 출력
public void printAccountInfoNInterestRate(String accountNum)
{
for (Account account : accounts)

Choose a reason for hiding this comment

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

java stream 사용해봅시다!

return null;
}

// 이자를 계산하고 출력

Choose a reason for hiding this comment

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

이자를 계산하고 / 출력... 두가지 역할을 하나의 함수에서 하고있습니다. 하나의 함수는 하나의 역할만 하도록 함수를 분리하여 단일책임원칙을 지키도록 코드를 작성해주세요

src/Main.java Outdated
{
Scanner s = new Scanner(System.in);
Main m = new Main();
CentralBank c = new CentralBank();

Choose a reason for hiding this comment

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

의미있는 변수명을 사용해주세요

src/Main.java Outdated

while(true)
{
int i = m.showMenu(s);
Copy link

@Erichong7 Erichong7 Apr 1, 2024

Choose a reason for hiding this comment

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

여기도 selectedNumber와 같이 의미있는 변수명을 가졌으면 좋겠습니다.
코딩의 꽃은 협업입니다.

src/Account.java Outdated
{
System.out.println("Failed WithDraw!");
}
}

Choose a reason for hiding this comment

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

출금, 입금, 송금 등의 책임이 Account 객체보다는 AccountService에 있다고 보는 것이 더 좋을 것 같습니다. Account는 자신의 계좌의 출금할 만큼 돈이 있는 지 등과 같은 역할을 하는 것이 좋을 것 같습니다. 즉, 객체의 책임에 대하여 고민해보고 분리해봅시다.
어떤 객체가 해야할 일을 다른 객체가 대신 해주고 있는데 객체의 구조가 변경된다면 그 객체의 일을 대신 해주는 모든 코드를 수정해야 합니다. 책임 분리합시다.

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.

모든 메소드가 하나의 기능만을 수행할수는 없겠지만, 추상화를 최소화해서 하나의 기능, 하나의 책임만을 가지도록 노력해야 합니다.

책임을 지는 객체, 기능을 가진 메소드 들은 책임을 최소화 할수록 기능을 최소화 할수록 유지보수가 쉬워집니다.
객체 분리, 단일책임 원칙에 대해 더 고민해보셔야 할 것 같습니다.

코드 컨벤션은 같이 프로젝트하는 동료를 위한 것입니다. 통일된 규칙으로 작성하여 가독성이 좋은 코드가 될 수 있습니다. 꼭 지켜주세요

src/Main.java Outdated
public class Main
{
Pattern pattern = Pattern.compile("\\d{6}-\\d{2}-\\d{6}");
public int showMenu(Scanner s)
Copy link
Contributor

Choose a reason for hiding this comment

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

화면을 출력해주는 책임을 가진 객체를 따로 분리하시는 것이 좋을 것 같습니다

src/Main.java Outdated
public class Main
{
Pattern pattern = Pattern.compile("\\d{6}-\\d{2}-\\d{6}");
public int showMenu(Scanner s)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
public int showMenu(Scanner s)
private int showMenu(Scanner s)

외부에 드러내도 되는 메소드인가, 외부에서 사용될 메소드인가를 고민하셔서 접근제어자를 설정해야합니다.

src/Main.java Outdated
import java.util.regex.Matcher;
import java.util.regex.Pattern;

public class Main
Copy link
Contributor

Choose a reason for hiding this comment

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

Main Class는 전체적인 어플리케이션을 실행시키는 역할인데, 어떤 어플리케이션인지 고민하면서 클래스 이름도 변경해보는 것도 좋을 것 같습니다.

src/Main.java Outdated
import java.util.regex.Matcher;
import java.util.regex.Pattern;

public class Main
Copy link
Contributor

Choose a reason for hiding this comment

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

Main 클래스가 너무 많은 책임을 지고 있지 않나 고민해보셔야 할 것 같습니다.
현재 Main클래스는 흐름제어, 입력, 출력, Account, validate 등 관련된 여러 가지 책임을 맡고 있는 것 같습니다.
흐름제어만을 남기고 따로 객체를 분리해주세요.

src/Main.java Outdated
aNum = getAccountNum(s);
amount = getAmount(s);

if(type.equals("S"))
Copy link
Contributor

Choose a reason for hiding this comment

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

S가 의미하는 바는 무엇인가요? 어떤 다른 사람이 봐도 이해할 수 있게 구체적으로 작성해야 합니다.
따로 변수를 선언해주세요

BigDecimal interestRate = calculator.getInterest(account.getAmount());
BigDecimal interestAmount = account.getAmount().multiply(interestRate);

account.setAmount(account.getAmount().add(interestAmount));
Copy link
Contributor

Choose a reason for hiding this comment

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

account의 amount를 추가하는 로직은 Account클래스의 책임이지 않을까요!?

"Goal Asset : ₩" + goalAmount;
}

public boolean compareCAGA(Account account)
Copy link
Contributor

Choose a reason for hiding this comment

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

CAGA는 무엇인가요? 메소드명을 구체적이게 적어주세요

@Override
public BigDecimal getInterest(BigDecimal amount)
{
BigDecimal overTenM= new BigDecimal("10000000"); // 천만
Copy link
Contributor

Choose a reason for hiding this comment

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

값을 이용하여 변수명을 짓는 것은 좋지 않습니다.

public class AccountInterestRate implements InterestCalculator
{
@Override
public BigDecimal getInterest(BigDecimal amount)
Copy link
Contributor

Choose a reason for hiding this comment

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

한 개의 메소드에서 여러 개 책임을 갖고 있는 것 같습니다.
인스턴스 생성, 비교 및 검증, 값 반환 의 책임을 지고 있는 것 같네요. 분리해주는 게 좋을 것 같습니다

src/Main.java Outdated
System.out.print("원하시는 작업을 선택해주세요 : ");
return s.nextInt();
}
public Account getAccount(Scanner s)
Copy link
Contributor

Choose a reason for hiding this comment

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

getAccount의 메소드가 단지 account를 얻는 책임만을 가지고 있는지 고민해봐야 할 것 같습니다.
이름 생성, 입력, 출력, 계좌 종류 체크, 인스턴스 생성 의 책임을 가지고 있습니다.
너무 많은 책임을 지고 있는 것은 유지보수에 좋지 않습니다. 책임을 분리해야합니다

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.

코드컨벤션을 지키려는 노력이 많이 보인 것 같습니다.
패키지명은 소문자만 사용해야 합니다. 또한, 하나의 명사로만을 작성하는 것을 지향합니다.
메소드 분리가 더 필요할 것 같습니다. 스스로 해당 메소드가 어떤 책임을 가지고 있을 까 고민하면서 분리하는 고민을 하면 좋을 것 같습니다.

열심히 잘하고 계신 것 같아서 좋습니다. 꾸준히 고민해주세요!

@@ -0,0 +1,12 @@
import bankService.Message;

public class StartBank {
Copy link
Contributor

Choose a reason for hiding this comment

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

클래스 명은 명사로 지어주는 것이 좋습니다.
현재 클래스 명은 동사 + 명사 입니다.
동사가 섞인 이름은 메소드에 적합할 것 같습니다

this.owner = owner;
this.amount = amount;
this.activated = true;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

사소하지만 매개변수와 주입시켜주는 순서도 일치하면 가독성이 좋아질 것 같습니다

}

@Override
public boolean withdraw(BigDecimal withdrawAmount) { //출금
Copy link
Contributor

Choose a reason for hiding this comment

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

도메인과 비즈니스 로직을 분리해주셨으면 합니다.
도메인 내의 로직은 오로지 도메인의 상태변화와 관련된 로직만 좋겠다는 생각입니다.

System.out.println("Deposite Finish! Your Amount : ₩" + amount);
return true;
}
private boolean isValid(BigDecimal withdrawAmount) { // 예산 > 출금 금액인 경우
Copy link
Contributor

Choose a reason for hiding this comment

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

어떤 유효성을 검사하고 있는 것인지 메소드명을 구체적으로 작성해주세요

Comment on lines 20 to 21
}
@Override
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
}
@Override
}
@Override

코드컨벤션 지켜주세요

import java.math.BigDecimal;
import java.util.Scanner;

public class Message {
Copy link
Contributor

Choose a reason for hiding this comment

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

메시지를 가지고 있는 책임, 출력 책임, 입력 책임만 가지고 있어도 충분하다고 생각합니다.

Comment on lines 8 to 9
private CentralBank centralBank = new CentralBank();
public boolean askAction(){
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
private CentralBank centralBank = new CentralBank();
public boolean askAction(){
private CentralBank centralBank = new CentralBank();
public boolean askAction(){

코드 컨벤션 지켜주세요

import java.math.BigDecimal;

public abstract class Account {
protected String accountType;
Copy link
Contributor

Choose a reason for hiding this comment

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

Enum을 활용해보는 것이 더 좋을 것 같습니다


import java.math.BigDecimal;

public abstract class InterestCalculator {
Copy link
Contributor

Choose a reason for hiding this comment

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

상수 값을 직접 변수명에 적용하는 것은 좋지 않습니다. 알고 계시면 좋을 것 같습니다.
하지만, 지금 이 부분은 도메인에 대한 이해가 필요한 부분인 것 같아 어쩔 수 없는 부분인 것 같습니다.

public class AccountInterestRate extends InterestCalculator {

@Override
public BigDecimal calculateInterest(BigDecimal amount) {
Copy link
Contributor

Choose a reason for hiding this comment

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

else if 문을 반복하는 것은 좋지 않습니다.
enum을 활용해 적용해보는 것이 적절해보입니다.

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