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

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

Conversation

yunjin1213
Copy link

@yunjin1213 yunjin1213 commented Sep 29, 2024

2주차 과제 제출합니다.

구조도는 다시 깔끔하게 작성해서 올리겠습니다!

LottoStructureFinal .pdf

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.

전반적으로 코드의 흐름이 잘 보이는 코드였습니다!
앞으로 조금씩 수정해가면서 더 성장하면 좋을 것 같습니다!! 수고했어요!

src/main/java/lotto/Lotto.java Outdated Show resolved Hide resolved
src/main/java/lotto/LottoGame.java Outdated Show resolved Hide resolved
src/main/java/lotto/Rank.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.

고생하셨습니다! 전체적으로 잘 작성된 것 같습니다.
하지만, 객체 분리가 조금 더 필요해보입니다! 화이팅입니다!

src/main/java/lotto/LottoGame.java Outdated Show resolved Hide resolved
src/main/java/lotto/LottoGame.java Outdated Show resolved Hide resolved
src/main/java/lotto/LottoGame.java Outdated Show resolved Hide resolved
src/main/java/lotto/Rank.java Outdated Show resolved Hide resolved
src/main/java/lotto/UserInterface.java Outdated Show resolved Hide resolved
src/main/java/lotto/UserInterface.java Outdated Show resolved Hide resolved
src/main/java/lotto/UserInterface.java Outdated Show resolved Hide resolved
src/main/java/lotto/UserInterface.java Outdated Show resolved Hide resolved
Copy link
Member

@TaetaetaE01 TaetaetaE01 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/lotto/LottoGame.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.

고생하셨습니다!
피드백 확인하고 적극적으로 반영해주세요

@@ -0,0 +1,52 @@
package lotto.constant;

public class Constant {
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/lotto/constant/Constant.java Outdated Show resolved Hide resolved
}

public static Rank findRank(int matchCount, boolean matchBonus) {
for (Rank rank : values()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

stream을 활용하면 가독성이 더 좋아질 것 같습니다.

Copy link
Member

Choose a reason for hiding this comment

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

stream 이걸 보면서 한번씩 적용해보세요!


public static Rank findRank(int matchCount, boolean matchBonus) {
for (Rank rank : values()) {
if (rank.matchCount == matchCount && rank.matchBonus == matchBonus) {
Copy link
Contributor

Choose a reason for hiding this comment

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

인스턴스의 필드를 직접 접근하는 건 좋지 않은 방법입니다. get 메소드를 활용해주세요

import lotto.view.LottoKiosk;
import lotto.repository.Memory;

public class KioskStart {
Copy link
Contributor

Choose a reason for hiding this comment

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

클래스 명에 동사가 들어가는 건 의미를 명확하게 전달하는 데 방해가 됩니다.
kiosk를 시작하는 주체가 클래스인데 해당 클래스명이 KioskStart라면 어색할 것 같습니다.

Suggested change
public class KioskStart {
public class KioskStarter {

위 이름은 예시인데, 명사로만 구성되는 것이 좋을 것 같습니다

Copy link
Contributor

Choose a reason for hiding this comment

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

View라는 패키지 이름을 봤을 때, 입출력을 담당하는 클래스를 집합시켜 놓은 것이라 예측할 수 있습니다.
하지만 해당 클래스는 입출력 이상의 책임을 지고 있는 것 같아, 입출력의 책임을 해당 클래스와 분리시키면 좋을 것 같습니다

}

private int requestAmount(Scanner scanner) {
while (true) {
Copy link
Contributor

Choose a reason for hiding this comment

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

변수를 사용하여 반복문을 통제하는 것이 코드의 의도를 더 쉽게 파악할 수 있을 것 같습니다

winningSystem.checkWinning(tickets, winningNumbers, bonusNumber);
}

private int requestAmount(Scanner scanner) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Scanner를 모두 매개변수로 받아 사용하고 있는데, static 멤버 변수로 선언해도 괜찮지 않을까요?

Copy link
Contributor

Choose a reason for hiding this comment

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

해당 클래스가 전혀 사용되고 있지 않은 것 같습니다.
데이터를 저장하는 용도로 사용되고 있는 것 같은데, 좋은 생각입니다.
하지만 저장을 하고 다시 사용하지 않는 것이 아쉽습니다. 다시 사용하는 코드를 생각해본다면 더욱 좋은 결과물이 되지 않을까 생각합니다.

Copy link
Contributor

Choose a reason for hiding this comment

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

Main 메소드를 가진 클래스는 보통, 해당 프로젝트의 최상단 패키지에 위치하는 것이 좋습니다.
lotto 패키지에 위치시켜주세요.

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.

전반적으로 코드의 완성도가 높아진거 같아요! 이제는 객체지향의 원칙에 대해 공부해볼 때가 된 것 같습니다.
객체 지향 설계의 5원칙 S.O.L.I.D

이를 학습하고

적용방법 이를 통해 직접 적용해보시길 바랍니다.

Comment on lines 4 to 14
import lotto.repository.Memory;

public class KioskStart {
public static void main(String[] args) {
LottoSeller lottoSeller = new LottoSeller();
Memory memory = new Memory();
WinningSystem winningSystem = new WinningSystem(memory);
LottoKiosk kiosk = new LottoKiosk(lottoSeller, winningSystem);

kiosk.start();
}
Copy link
Member

Choose a reason for hiding this comment

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

Main에서 객체를 초기화해서 WinningSystem winningSystem = new WinningSystem(memory); 처럼 의존성을 직접 주입하고 있네요!

만약 이런 의존성이 굉장히 많아진다면 어떻게 될까요? 분명 이 또한 관리하기 어려워질 것입니다.

또한 지금 Application에서는 구현체를 직접 참조하고 있기 때문에 DIP를 위반하고 있습니다. 그럼 어떻게 해야할까요?
SOLID 규칙 지키는 법

간단하게 예를 들자면 아래와 같이 적용할 수 있을거예요!
꽤나 어려울 수 있는 내용이니 꼭 이해하고 넘어가시길 바랍니다.
아래는 WinningSystem을 interface로, WinningSystemImpl을 구현체로 둔 상태입니다.

class AppConfig {

    public Memory memory() {
        return new Memory();
    }

    public WinningSystem winngSystem() {
        return new WinningSystemImpl(memory());
    }
}

Comment on lines 12 to 18
private final LottoSeller lottoSeller;
private final WinningSystem winningSystem;

public LottoKiosk(LottoSeller lottoSeller, WinningSystem winningSystem) {
this.lottoSeller = lottoSeller;
this.winningSystem = winningSystem;
}
Copy link
Member

Choose a reason for hiding this comment

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

LottoSeller와 WinningSystem은 구현체이므로 만약 요구사항이 변경된다면 이 LottoKiosk는 모두 변경되야합니다.
만약 LottoSeller와 WinningSystem가 interface라면 어떻게 될까요?
전체 프로젝트의 변동이 있지 않은 한 변경될 일이 없을겁니다.

이를 참고해서 아래에 OCP와 DIP를 지키는 방법에 대해 고민해보세요!

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