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

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

Conversation

fuirian
Copy link

@fuirian fuirian commented Apr 2, 2024

추가 Q & A

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

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

@fuirian fuirian changed the title [2주차]객체지향 코드(fuirian) [2주차]객체지향 코드 연습(fuirian) Apr 2, 2024
Copy link
Member

@Hoya324 Hoya324 left a comment

Choose a reason for hiding this comment

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

클래스명에 조금더 신경써주시고, 불필요한 bin과 같은 파일은 .ignore 파일에 넣어서 관리해주세요! 전반적으로 1주차라 그런지 조금 아쉬운 부분이 보입니다! 각 클래스가 어떤 역할과 책임을 갖고 메서드를 통해 협력관계를 이룰 수 있을지 한번 고민해보시고 어딘가에 정리해서 그걸 바탕으로 코드를 다시 작성해보시는게 큰 도움이 될거라고 생각합니다!

Comment on lines 5 to 12
static Account[] accounts = new Account[100];
static int index = 0;

static void accountList() {
for (int i = 0; i < index; i++) {
accounts[i].printAccounts();
}
}
Copy link
Member

Choose a reason for hiding this comment

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

AccountList는 어떤 역할을 가진 클래스일까요? 또한 100을 배열의 limit으로 두신 이유와 컬렉션이 아닌 배열로 두신 이유가 궁금합니다.! 또한 매직넘버를 한번 처리해주세요!

Comment on lines 14 to 36
do {
System.out.println("----------------");
System.out.println("1.credit 2.devit");
System.out.println("----------------");
System.out.print("number> ");
int code = Integer.parseInt(scanner.nextLine());

switch (code) {
case 1:
credit = new Credit();
credit.credit(); // CreateAccount 클래스의 createAccount 메서드 호출
break;
case 2:
devit = new Devit();
devit.devit();
default:
System.out.println("error");
break;
}

} while(!run);
System.out.println("exit");
}
Copy link
Member

Choose a reason for hiding this comment

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

기본 계좌에서 입력을 이루고 있는것 같은데 해당 클래스명이 이 역할과 잘 어울리는지 한번 고민해보시면 좋을거 같습니다!


import java.util.Scanner;

public class CreateAccount {
Copy link
Member

Choose a reason for hiding this comment

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

클래스명에 대해 한번 더 고민해주세요! 제가 생각하기에는 해당 클래스가 계좌를 생성하고 있는 것 같아 은행원과 같은 역할을 한번 부여하시는게 어떤지 여쭤보고싶어요!

Comment on lines 11 to 13
public Credit() {

}
Copy link
Member

Choose a reason for hiding this comment

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

코드 스타일에 신경써주세요!


import java.util.Scanner;

public class Devit {
Copy link
Member

Choose a reason for hiding this comment

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

구글에 쳐서 나오지 않는 단어는 다른 개발자로 하여금 큰 혼란을 줄 수 있습니다!

스크린샷 2024-04-03 09 48 46


import java.util.Scanner;

public class NAccount {
Copy link
Member

Choose a reason for hiding this comment

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

조금더 의미있는 클래스명을 사용해주세요!

public class NAccount {

static Scanner scanner = new Scanner(System.in);
static Account[] accounts = new Account[100];
Copy link
Member

Choose a reason for hiding this comment

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

static으로 하신 이유가 있을까요??

Comment on lines 1 to 9
/**
*
*/
/**
* @author kmjkh
*
*/
module Bank {
}
Copy link
Member

Choose a reason for hiding this comment

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

이건 뭘까요??

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.

코드 컨벤션에 대하여 알아보고 내 코드에 적용하여 봅시다. 현재 코드에서는 static가 남발되어 있는데 이는 좋지 않습니다. ( https://kellis.tistory.com/127 ) static에 대하여 공부하여 봅시다 + 자바 접근 제어자에 대하여도 공부해봅시다. 그리고 패키지 구조가 분리가 전혀되어 있지 않은데 계층화 아키텍쳐에 대하여 알아봅시다.

Comment on lines 27 to 28
Account account = new Account();
accounts[index++] = account;

Choose a reason for hiding this comment

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

아무런 매개변수가 없는 생성자를 통해 값 변경 목적으로 접근하는 것은 좋지 않다고 생각합니다.
아무런 매개변수가 없는 생성자가 꼭 필요한지 고민해보아요

Comment on lines 26 to 28
case 2:
devit = new Devit();
devit.devit();

Choose a reason for hiding this comment

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

break; 문이 누락된 거 같은데 맞나요?

Comment on lines 34 to 36
} while(!run);
System.out.println("exit");
}

Choose a reason for hiding this comment

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

while문을 빠져나오는 로직이 없는 걸로 보이는데 맞나요?

Choose a reason for hiding this comment

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

static으로 변수들을 선언하는 이유가 무엇인가요? 자바의 접근제어자와 static에 대해 고민해봅시다


}
public void credit() {
Scanner scanner = new Scanner(System.in);

Choose a reason for hiding this comment

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

위에 static으로 Scanner을 선언해두었음에도 아래에서 또 선언을 하는데 이유가 무엇인가요?
Scanner는 자바의 가비지컬렉션 제외 대상입니다. 따라서 scanner 사용이 끝났다면 close()를 통하여 메모리를 해제 해주어야 하며 위에 선언을 해두고 다시 함수 안에서 선언하는 것은 고치도록 합시다

private SAccount sAccount;
static Scanner scanner = new Scanner(System.in);

public BankApplication() {

Choose a reason for hiding this comment

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

생성자에 run에 대한 책임을 부여하는 것 보다는 run이라는 함수를 BankApplication에 두는 것이 좋지 않을까요?

Comment on lines 34 to 38
for(int i = 0; i < index; i++) {
if(accounts[i].getAccountNumber().equals(accountNumber)) {
return accounts[i];
}
}

Choose a reason for hiding this comment

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

stream을 통하여 더욱 간결하게 표현할 수 있습니다.


Account account = findAccountByAccountNumber(accountNumber);
if(account == null) {
System.out.println("계좌번호를 정확히 입력해주세요.");

Choose a reason for hiding this comment

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

매직넘버에 대하여 공부하여 적용해봅시다

Comment on lines 17 to 21
Account account = findAccountByAccountNumber(accountNumber);
if(account == null) {
System.out.println("계좌번호를 정확히 입력해주세요.");
return;
}

Choose a reason for hiding this comment

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

여러 클래스에서 반복되는 로직인 거 같은데 어떻게 반복을 줄일 수 있을 지 고민해봅시다.
함수 분리나 상속을 이용할 수 있지 않을까요?

Comment on lines 30 to 37
private Account findAccountByAccountNumber(String accountNumber) {
for(int i = 0; i < index; i++) {
if(accounts[i].getAccountNumber().equals(accountNumber)) {
return accounts[i];
}
}
return null;
}

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.

선호하진 않지만 이럴 때야 말로 static을 사용하여 반복을 줄일 수는 있습니다. 다른 것은 static으로 선언하였는데 이건 static을 사용하지 않은 이유가 궁금하네요

@fuirian
Copy link
Author

fuirian commented Apr 7, 2024

eclipse를 사용하여 만든 파일이라 그런지 소스코드 외 복잡한 파일이 많은 것 같습니다. 어떻게 해결해야하나요?

@Hoya324
Copy link
Member

Hoya324 commented Apr 8, 2024

eclipse를 사용하여 만든 파일이라 그런지 소스코드 외 복잡한 파일이 많은 것 같습니다. 어떻게 해결해야하나요?

.gitignore 파일에 추가해주세요!

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.

Account를 관리하는 곳이 여러개인 것이 가장 큰 문제인 것 같습니다.
한 곳에서 관리하도록 해야할 것 같습니다.
클래스 역할에 따른 패키지 분리가 필요해보입니다.

아직 구현한 기능들이 너무 적은 것 같아요.

BankApplication bankApplication = new BankApplication();

}
}
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
}
}

EOL(End Of Line)에 대해 알고 적용하면 좋을 것 같습니다.

https://velog.io/@bjy991018/EOLEndOfLine

Copy link
Contributor

Choose a reason for hiding this comment

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

Intellij를 사용하면 개발에 있어서 훨씬 더 편해질 것 같습니다


public class Main {

public Main() {
Copy link
Contributor

Choose a reason for hiding this comment

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

기본 생성자는 이처럼 명시하지 않아도 다른 생성자를 생성하지 않는 한 기본적으로 생성되어있습니다.

@@ -0,0 +1,19 @@
package Bank;

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 보다 더 의미있는 클래스명으로 바꾸면 좋을 것 같습니다.


}

public void run() {
Copy link
Contributor

Choose a reason for hiding this comment

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

프로그램을 실행하는 main 클래스에는 꼭 필요한 경우가 아니라면 추가적인 메소드를 구현하지 않는 게 좋습니다


public class Credit {
private static Scanner scanner = new Scanner(System.in);
private static Account[] accounts = new Account[100];
Copy link
Contributor

Choose a reason for hiding this comment

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

계좌를 한곳에서 관리해주세요

import java.util.Arrays;
import java.util.Scanner;

public class NAccount {
Copy link
Contributor

Choose a reason for hiding this comment

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

클래스명을 구체적이게 작성해주세요

private static int index = 0;

// N 계좌 거래 수행 메서드
public void nAccount() {
Copy link
Contributor

Choose a reason for hiding this comment

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

메소드명은 해당 객체가 수행할 행위를 뜻하는 것입니다. nAccount라는 행위는 의미가 없는 것 같습니다. 의미있게 작성해주세요

import java.util.Arrays;
import java.util.Scanner;

public class SAccount {
Copy link
Contributor

Choose a reason for hiding this comment

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

클래스명 더 의미있게 바꿔주세요

import java.util.Arrays;
import java.util.Scanner;

public class Credit {
Copy link
Contributor

Choose a reason for hiding this comment

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

NAccount, SAccount, Credit 세 클래스의 차이는 무엇인가요? 모두 똑같은 행위를 하고 있는 것 같습니다.

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.

4 participants