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

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

Conversation

Suehyun666
Copy link

환경문제로 테스트를 수행하지 못해 일단 메소드라도 만져봤습니다..

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.

전반적인 로또 로직에 대해서는 잘 짜신 거 같습니다!
하지만� LottoGame 객체에서 모든 로또와 관련된 작업을 진행하고 있는 상황에 역할에 따른 분리가 필요해 보입니다. 역할을 분리 할 때 해당 클래스가 어떤 책임을 갖게 되는지, 왜 이렇게 분리했는지 등 고민을 하시면 더 좋은 코드가 될 거 같습니다.

고생하셨습니다:)


public class LottoGame {
//
private static final int LOTTO_PRICE = 1000;
Copy link
Member

Choose a reason for hiding this comment

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

매직넘버로 변수를 관리하는 것은 가독성인 면에서 아주 좋습니다!
매직변수가 많아졌을 때 변수가 어떤 것을 의미하는지 네이밍에 대해서도 신경써주시면 좋을 거 같습니다

String input = Console.readLine();
return validateMoney(input);
}
//잘못된 입금 시 에러 처리
Copy link
Member

Choose a reason for hiding this comment

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

메소드간 간격 유지 해주시기 바랍니다.

//,로 파싱 => indentation 수정 예정
private List<Integer> parseWinningNumbers(String input) {
String[] tokens = input.split(",");
if (tokens.length != 6) {
Copy link
Member

Choose a reason for hiding this comment

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

여기서 사용한 6이라는 변수가 무엇을 의미하는지 모르겠습니다!
이 또한 매직변수를 통해 변수의 의미를 지정하여 사용하는 것이 좋습니다!

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.

고생하셨습니다!
객체 분리에 많은 고민해주셔야 할 것 같습니다!


public class Lotto {
private final List<Integer> numbers;
private final int bonusNumber;
Copy link
Contributor

Choose a reason for hiding this comment

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

과제 요구사항 중 Lotto클래스 내부에 필드를 추가할 수 없다고 적혀있습니다!
요구사항을 지켜주세요

그래도 bonusNumber를 Lotto내에 한곳에 관리한다는 접근은 굉장히 좋았던 것 같습니다.

private final int bonusNumber;

// 번호를 저장할 수 있는 리스트 생성(ALL)
public Lotto() {
Copy link
Contributor

Choose a reason for hiding this comment

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

매개변수가 없는 생성자를 추가할 수 없다는 요구사항이 있습니다. 요구사항 지켜주세요

}

// 보너스 번호 생성 (Lotto )
private int generateBonusNumber() {
Copy link
Contributor

Choose a reason for hiding this comment

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

코드 컨벤션을 지켜주세요.
private 메소드는 public 메소드 아래에 위치해야 합니다.

//일반 번호 6개 생성 (Lotto)
public void generateRandomNumbers() {
numbers.clear();
numbers.addAll(Randoms.pickUniqueNumbersInRange(1, 45, 6));
Copy link
Contributor

Choose a reason for hiding this comment

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

1, 45, 6이 어떤 의미를 가지는지 이름을 지어주세요.
코드 내에 숫자를 그대로 작성하는 것은 가독성을 해칠 뿐만 아니라 유지보수성도 떨어뜨립니다.
매직넘버를 상수로 바꿔주세요.
아래에 관련 링크 남깁니다!

https://velog.io/@jeongbeom4693/JAVA-%EC%83%81%EC%88%98-%EB%A7%A4%EC%A7%81-%EB%84%98%EB%B2%84Magic-Number

return bonusNumber;
}

//validate
private void validate(List<Integer> numbers) {
Copy link
Contributor

Choose a reason for hiding this comment

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

어떤 검사를 하고있는지, 메소드 명을 구체적으로 변경하여도 좋을 것 같습니다.

}
}

// TODO: 추가 기능 구현
private boolean hasDuplicate(List<Integer> numbers) {
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 boolean hasDuplicate(List<Integer> numbers) {
private boolean isDuplicated(List<Integer> numbers) {

메소드 명은 개인이 짓는 것이지만, 의견 하나 내보고 갑니다!

import java.util.List;
import java.util.Set;

public class LottoGame {
Copy link
Contributor

Choose a reason for hiding this comment

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

이 클래스에 너무 많은 책임이 부여되어 있습니다.

  1. 입출력의 책임
  2. 로또를 생성하는 책임
  3. 입력한 String을 관리하기 쉽게 parsing해주는 역할
  4. 비즈니스 로직과 관련된 계산하는 로직
  5. 결과를 출력하기 위한 로직
  6. 입력한 값이 필요한 비즈니스 로직에 넘겨주는 책임

이외에도 여러 책임이 있고 조금 분리해야할 필요성이 보입니다.

return validateMoney(input);
}
//잘못된 입금 시 에러 처리
private int validateMoney(String input) {
Copy link
Contributor

Choose a reason for hiding this comment

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

무언가를 검사하는 메소드에서 String타입을 int타입의 Money를 반환하는 것을 올바르지 않은 것 같습니다.
메소드를 분리해주세요

Suggested change
private int validateMoney(String input) {
private void validateMoney(String input) {

// generate lotto (manually)
private List<Lotto> buyLottos(int count) {
List<Lotto> lottos = new ArrayList<>();
for (int i = 0; i < count; i++) {
Copy link
Contributor

Choose a reason for hiding this comment

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

stream API를 사용하여 가독성을 높일 수 있습니다

return numbers;
}

private boolean HasDuplicate(List<Integer> numbers) {
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 boolean HasDuplicate(List<Integer> numbers) {
private boolean hasDuplicate(List<Integer> numbers) {

Suehyun666

This comment was marked as abuse.

Suehyun666

This comment was marked as abuse.

Suehyun666

This comment was marked as spam.

입력받는 역할만 수행하는 클래스를 만들었습니다.
매직넘버를 상수로 변경하였습니다.
변경사항 없습니다.
1. 입력받는 역할을 수행하는 클래스로 분리했습니다.
2. 매직넘버를 상수로 변경했씁니다.
3. 에러도 메소드로 처리했습니다.
로또 숫자를 저장하는 클래스입니다.
Lotto클래스 내부에 다른 필드 삭제했습니다.
로또 추첨 결과를 처리하는 클래스입니다.
로또를 랜덤으로 생성하는 클래스입니다.
1. streamapi이용하여 간단하게 했습니다.
2. 매직넘버를 상수로 변환했씁니다.
전체적인 비즈니스 로직을 관리하는 클래스입니다.
 출력을 수행하는 클래스입니다.
1. 상금을 상수로 변환했습니다.
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.

고생하셨습니다.
객체 분리 부분에서 조금 더 고민해보시면 좋을 것 같습니다.

// TODO: 프로그램 구현
LottoGame lottoGame = new LottoGame();
lottoGame.start();

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

줄바꿈 신경써주세요.

Comment on lines 14 to 19
public LottoGame() {
this.outputHandler = new OutputHandler();
this.inputHandler = new InputHandler(outputHandler);
this.lottoGenerator = new LottoGenerator();
this.resultCalculator = new ResultCalculator();
}
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

Choose a reason for hiding this comment

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

LottoGame이 자신이 사용할 객체를 직접 고르면 LottoGame은 이 객체들의 변동사항에 대해 밀접하게 영향을 받게됩니다. 그러면 어떻게 해결해야할까요?

private final ResultCalculator resultCalculator;
private final OutputHandler outputHandler;

public static final int LOTTO_PRICE = 1000;
Copy link
Contributor

Choose a reason for hiding this comment

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

접근제어자를 public으로 선언한 이유가 있을까요? 내부에서만 사용하는 것이 좋아보이고, 외부에서 사용하면 안된다고 생각합니다. 전체 클래스에서 공통적으로 사용하려면, 상수를 관리할 객체를 생성해주세요


public List<Lotto> generateLottos(int count) {
return IntStream.range(0, count)
.mapToObj(i -> new Lotto(Randoms.pickUniqueNumbersInRange(LOTTO_NUMBER_MIN, LOTTO_NUMBER_MAX, LOTTO_NUMBER_COUNT)))
Copy link
Contributor

Choose a reason for hiding this comment

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

로또를 생성할 때, 랜덤으로 번호를 생성하는 것을 Lotto 객체 외부에서 할 필요가 없을 것 같습니다. Lotto 내부에서 생성해도 괜찮을 것 같네요.
팩토리 메소드를 알아보시고 활용해보세요

Comment on lines 4 to 8
private static final int PRICE_THREE = 5000;
private static final int PRICE_FOUR = 50000;
private static final int PRICE_FIVE = 1500000;
private static final int PRICE_FIVE_BONUS = 30000000;
private static final int PRICE_SIX = 2000000000;
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 final int PRICE_FIVE_BONUS = 30000000;
private static final int PRICE_SIX = 2000000000;

public void printLottos(java.util.List<Lotto> lottos) {
Copy link
Contributor

Choose a reason for hiding this comment

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

import가 안된 것 같습니다.

Suggested change
public void printLottos(java.util.List<Lotto> lottos) {
public void printLottos(List<Lotto> lottos) {

Comment on lines 13 to 14
}
public void printInputPrompt(String message) {
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 void printInputPrompt(String message) {
}
public void printInputPrompt(String message) {

컨벤션 지켜주세요

return (int) numbers.stream().filter(winningNumbers::contains).count();
}

private int calculateResultIndex(int matchCount, boolean bonusMatch) {
Copy link
Contributor

Choose a reason for hiding this comment

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

private 메소드 전부 public 메소드 아래에 위치하도록 해주세요!

Comment on lines 23 to 24
if (matchCount == MATCH_SIX) {return MATCH_SIX; // 6개 일치
} if (matchCount == MATCH_FIVE && bonusMatch) {return MATCH_FIVE_BONUS; // 5개 + 보너스 일치
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
if (matchCount == MATCH_SIX) {return MATCH_SIX; // 6개 일치
} if (matchCount == MATCH_FIVE && bonusMatch) {return MATCH_FIVE_BONUS; // 5개 + 보너스 일치
if (matchCount == MATCH_SIX) {
return MATCH_SIX; // 6개 일치
}
if (matchCount == MATCH_FIVE && bonusMatch) {
return MATCH_FIVE_BONUS; // 5개 + 보너스 일치

Comment on lines 32 to 43
public int[] calculateResults(List<Lotto> boughtLottos, List<Integer> winningNumbers, int bonusNumber) {
int[] matchCounts = new int[8];

for (Lotto lotto : boughtLottos) {
int matchCount = getMatchCount(lotto.getNumbers(), winningNumbers);
boolean bonusMatch = lotto.getNumbers().contains(bonusNumber);

int resultIndex = calculateResultIndex(matchCount, bonusMatch);
incrementCount(matchCounts, resultIndex);
}
return matchCounts;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

matchCount를 int 로 관리하는 것보다, 객체를 생성하여 객체 내부의 상태로 관리하는 것이 좋습니다.
재사용성이나 유지보수성에서 더 좋습니다.

Copy link

@0702Yoon 0702Yoon left a comment

Choose a reason for hiding this comment

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

일단 입출력에서의 책임을 조금 더 덜면 좋을 것 같아요.
계산하는 부분이 조금 가독성이나 더 좋게 만들 수 있을 것 같아요.
특히 enum을 사용하면 좋을 것 같아요.
또 조금 더 나아가서 외부에서 의존성을 주는 게 무슨 말이지에 대해서 아는 것도 좋을 것 같아요.
객체를 조금 더 잘 쪼개보세요

start 메서드안에 이제 단계별로 잘 분리를 한 것 같아요.
수현님은 더 좋게 코드를 짤 수 있다고 생각합니다!

Comment on lines 14 to 19
public LottoGame() {
this.outputHandler = new OutputHandler();
this.inputHandler = new InputHandler(outputHandler);
this.lottoGenerator = new LottoGenerator();
this.resultCalculator = new ResultCalculator();
}
Copy link

Choose a reason for hiding this comment

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

LottoGame이 자신이 사용할 객체를 직접 고르면 LottoGame은 이 객체들의 변동사항에 대해 밀접하게 영향을 받게됩니다. 그러면 어떻게 해결해야할까요?

}

private void validateMoney(String input) {
if (!input.matches("\\d+") || Integer.parseInt(input) < LottoGame.LOTTO_PRICE) {
Copy link

Choose a reason for hiding this comment

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

노리신거라면 대단하신데 Integer.parseInt를 하기 전에 이렇게 숫자인 지 확인함으로써 NumberFormatException이 안발생하게 한거 좋은 것 같아요.
근데 Console.readLine();과 이 input.matches("\d+")를 합쳐서 readInt() 이런 메서드를 만드는 것도 좋아보여요. Console.readLine을 그대로 쓰는 것보다 그걸 가공한 메서드를 만들면 이 if문이 좀 짧아질 것 같아요. 제 의도가 전달됐나요?

import java.util.List;
import java.util.Set;

public class InputHandler {
Copy link

Choose a reason for hiding this comment

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

public 메서드가 private 메서드보다 위에 위치시켜주세요. 라고 제가 알고있었는 데 제가 다시 찾아보니 기능별로 응집하는 것도 좋다는 말도 있네요. 저도 개발하다보면 public 메서드들이 위에 많아져서 그 안에서 호출하는 private 메서드들이 많아져서 리팩토링할 때 조금 힘들었습니다.
하지만 다르게 보면 하나의 클래스의 역할이 너무 많아서 public private찾기가 힘들다고 볼 수 있겠네요. 이거에 대해서 고민해보세용. (private이 전부 다 위에 있는 건 장점이 없는 것 같아요.)
https://random-topic.tistory.com/159

}
}

private void validateWinningNumbersCount(String[] tokens) {
Copy link

Choose a reason for hiding this comment

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

이 입력받는 클래스는 입력값에 대한 형태에 대한 예외처리를 처리하도록 하는 건 괜찮은 데 이제 비즈니스 적인 에외처리도 하고 있죠.
뭔가 분리하면 더 이 입력받는 객체의 역할이 줄어들지 않을까요?

Comment on lines 22 to 38
// 구입 금액 입력
int money = inputHandler.getMoney();
int lottoCount = money / LOTTO_PRICE;

// 로또 번호 자동 생성
List<Lotto> boughtLottos = lottoGenerator.generateLottos(lottoCount);

// 구매한 로또 출력
outputHandler.printLottos(boughtLottos);

// 당첨 번호와 보너스 번호 입력
List<Integer> winningNumbers = inputHandler.getWinningNumbers();
int bonusNumber = inputHandler.getBonusNumber(winningNumbers);

// 당첨 결과 계산 및 출력
int[] matchCounts = resultCalculator.calculateResults(boughtLottos, winningNumbers, bonusNumber);
outputHandler.printStatistics(matchCounts, lottoCount * LOTTO_PRICE);
Copy link

Choose a reason for hiding this comment

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

주석 처리는 최종 결과물에선 없애주세요. 없어도 읽히는 코드를 원합니다!

}

public int[] calculateResults(List<Lotto> boughtLottos, List<Integer> winningNumbers, int bonusNumber) {
int[] matchCounts = new int[8];
Copy link

Choose a reason for hiding this comment

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

이 8이 무슨 의미인지 모르겠어요.
매직넘버 도입하면 알것같아요.

Comment on lines 19 to 23
System.out.println("3개 일치 (5,000원) - " + matchCounts[3] + "개");
System.out.println("4개 일치 (50,000원) - " + matchCounts[4] + "개");
System.out.println("5개 일치 (1,500,000원) - " + matchCounts[5] + "개");
System.out.println("5개 일치, 보너스 볼 일치 (30,000,000원) - " + matchCounts[7] + "개");
System.out.println("6개 일치 (2,000,000,000원) - " + matchCounts[6] + "개");
Copy link

Choose a reason for hiding this comment

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

위에 선제멘토님이 말한대로 matchCounts[] 라는 int[]을 사용하니까 앞으로의 유지보수가 좋아보이지 않아요.
그리고 enum이라는 열거형 타입 문법에 대해 알아보는 거 어떨까요. 5000원, 3개, 5등 뭔가 연관성 있어 보이지 않나요?
묶어서 관리해봅시다

1. lottogmae에 의존성을 외부에서 주입하기위해서 여기서 생성했습니다.
constants이용해서 문자열 안보이게 했습니다.
validator 클래스 만들어서 비즈니스 로직만 수행하도록 했습니다.
constants만들어서 상수처리했습니다.
입력 시 오류를 처리하는 클래스입니다.
로또 생성하는 클래스와 합쳤습니다.
로또 상수처리 위해 constants만들었습니다.
로또 구매 정보를 저장하는 클래스입니다.
결과계산 수행하는 클래스입니다.
상수처리 위해 constants 만들었습니다
매치카운트 정보를 저장하는 클래스도 만들었습니다.
-11 for문 수정예정입니다..
constants입니다. 관련있는 정보를 한데 묶었습니다.
매치카운트를 저장하는 클래스입니다.
아직 손볼 곳이 있어서 constants를 만들지 않았습니다.
상수처리, import, 관련있는 정보 모아서 가독성있게 만들었습니다.
의존성을 외부에서 주입하고 주석없앴습니다.
필드이용하지 않고 정보를 저장하는 클래스를 만들었습니다.
-클래스이름을 좀 더 이해가기 쉽게 만들고 싶은데 추천부탁드립니다..
당첨번호를 저장하는 클래스입니다.
패키지로 묶었습니다.
어플리케이션입니다.
generator도 생성했서 주입했습니다.
바뀐 곳은 별로 없습니다.
String input = console.realine이 반복돼서 줄일까 합니다.
에러의 상수까지 넣었습니다.
상수처리했습니다.
생성하는 메서드는 다른 킅래스로 빼서 오직 값만 저장하도록 했습니다.
바뀐 곳은 없습니다.
구매한 만큼의 로또를 저장하는 클래스입니다. lottogenerator를 통해 로또를 생성받아 값을 저장하는 역할을 수행합니다.
입력받은 로또의 정보를 저장하는 클래스입니다.
결과를 연산하는 클래스입니다.
상수처리했습니다.
바뀐 곳은 별로 없습니다.
상수를 담고있는 costants입니다.
로또를 구매한만큼 생성하는 클래스입니다.
상수처리했고 달라진 곳은 별로 없습니다.
constants입니다.
application에서 의존성을 주입받아 전체적인 게임의 로직을 관리합니다.
생성한 로또와 당첨번호가 일치하는 개수를 저장합니다.
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