Skip to content
Open

test #132

Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions src/main/java/Application.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
import controller.LottoController;

Choose a reason for hiding this comment

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

Pending 리뷰 테스트 코멘트


import java.io.IOException;

public class Application {

public static void main(String [] args) throws IOException {
LottoController lottoController = new LottoController();
}
}
48 changes: 48 additions & 0 deletions src/main/java/controller/LottoController.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
package controller;

import model.AutoLotto;
import model.Lotto;
import model.LottoList;
import view.InputView;
import view.OutputView;

import java.util.ArrayList;
import java.util.List;

public class LottoController {
Comment on lines +10 to +12
Copy link
Author

Choose a reason for hiding this comment

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

InputViewOutputViewLottoController에서 직접 생성하고 필드로 가지고 있네요. 이렇게 되면 LottoControllerInputView, OutputView라는 특정 구현에 강하게 의존하게 됩니다. 만약 나중에 다른 방식으로 입출력을 처리하게 된다면 LottoController를 수정해야 할 수도 있습니다.

이런 경우, 의존성을 외부에서 주입하는 의존성 주입(Dependency Injection) 방식을 고려해보면 어떨까요? 예를 들어, LottoController의 생성자에서 InputViewOutputView를 전달받도록 하면, LottoController는 입출력 방식을 몰라도 되니 더 유연해질 수 있습니다. 🤔

private InputView inputView = new InputView();
private OutputView outputView = new OutputView();
private AutoLotto autoLotto;
private LottoList lottoList;


public LottoController(){
lottoStart();
}
Comment on lines +18 to +21

Choose a reason for hiding this comment

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

lottoStart() 메소드에서 inputMoney()viewLotto()를 호출하고 있습니다. lottoStart() 메소드는 로또 구매 과정을 시작하는 역할을 하는 것으로 이해됩니다. 혹시 이 메소드가 이 외에 다른 추가적인 책임을 가지고 있다면, 해당 부분을 분리하여 LottoController의 역할을 더욱 명확하게 하는 것을 고려해보면 어떨까요? 🤔

public void lottoStart() {
int price = inputMoney();
viewLotto(price);
}
Comment on lines +22 to +25

Choose a reason for hiding this comment

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

viewLotto(int price) 메소드에서 price를 받아 count를 계산하고 makeLottolist(count)를 호출하고 있네요. makeLottolist 메소드 내부의 for 루프에서 lottoList를 계속해서 새로 생성하고 마지막 lottoList만 반환하는 방식이 이전 리뷰에서도 언급되었는데, 이번에도 동일하게 유지되고 있네요. 😅 LottoList는 여러 개의 Lotto 객체를 담는 컬렉션 역할을 해야 하므로, makeLottolist 메소드 상단에서 LottoList 인스턴스를 한 번만 생성하고, 루프 안에서는 해당 LottoListLotto 객체를 추가하는 방식으로 변경하는 것이 LottoList의 역할을 더 명확하게 할 수 있을 것 같습니다. 예를 들어, LottoList lottoCollection = new LottoList();와 같이 생성한 후, 루프 안에서 lottoCollection.add(makeLotto());와 같이 사용하는 방식은 어떨까요?


public int inputMoney(){
Comment on lines +24 to +27
Copy link
Author

Choose a reason for hiding this comment

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

makeLottolist 메서드에서 lottoListnew LottoList(makeLotto())로 계속 덮어쓰고 있네요. 현재 구현으로는 마지막에 생성된 Lotto 하나만 lottoList에 담기게 됩니다. count만큼의 로또를 제대로 담으려면 lottoList 객체를 메서드 시작 전에 한 번 생성하고, 그 객체에 Lotto를 추가하는 방식으로 수정해야 할 것 같습니다. 🧐

return inputView.inputPrice();
}
Comment on lines +26 to +29

Choose a reason for hiding this comment

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

makeLotto() 메소드에서 AutoLotto 객체를 생성하고 getAutoLotto()를 호출하여 번호를 얻은 후, 바로 outputView.lottoPrint(lotto);를 호출하고 있네요. AutoLotto 객체가 번호를 생성하는 로직을 이미 가지고 있다면, getAutoLotto()를 통해 얻은 번호 목록을 바로 Lotto 객체 생성에 활용하고, 실제 출력은 LottoController의 다른 메소드에서 담당하는 것이 책임 분리 측면에서 더 좋지 않을까 생각합니다. 🤔 makeLotto 메소드는 Lotto 객체를 생성하는 역할에만 집중하는 것은 어떨까요?

public void viewLotto(int price){
int count = price/1000;
outputView.countPrint(count);
lottoList = makeLottolist(count);
}
public LottoList makeLottolist(int count){
Comment on lines +31 to +35
Copy link
Author

Choose a reason for hiding this comment

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

makeLotto 메서드에서 AutoLotto 객체를 생성하고, getAutoLotto()로 번호를 가져온 후, 다시 new Lotto(lotto)Lotto 객체를 생성하고 있네요. AutoLotto는 이미 로또 번호를 생성하는 역할을 하고 있는데, Lotto 클래스는 그 생성된 번호를 단순히 감싸는 역할만 하고 있는 것 같습니다.

AutoLotto가 생성된 로또 번호를 직접 Lotto 객체로 만들어서 반환하는 것은 어떨까요? 이렇게 하면 makeLotto 메서드의 책임이 더 명확해질 수 있습니다. 예를 들어, AutoLotto 클래스에 createLotto()와 같은 메서드를 추가하여 Lotto 객체를 반환하도록 말이죠.

for(int i=0; i<count; i++){
lottoList = new LottoList(makeLotto());
}
return lottoList;
}
public Lotto makeLotto(){
List<Integer> lotto = new ArrayList<>();
AutoLotto autoLotto = new AutoLotto();
lotto = autoLotto.getAutoLotto();
outputView.lottoPrint(lotto);
return new Lotto(lotto);
}
}
36 changes: 36 additions & 0 deletions src/main/java/model/AutoLotto.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
package model;

import java.util.*;

public class AutoLotto {

private static final int MAX_LOTTO_NUMBER = 45;
private static final int CNT_LOTTO_NUMBER = 6;

private static List<Integer> lottoNums = new ArrayList<>();
public AutoLotto(){
createAutoLotto();
Comment on lines +10 to +12
Copy link
Author

Choose a reason for hiding this comment

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

lottoNumsstatic으로 선언하고 createAutoLotto 메서드에서 초기화하고 있네요. static 변수는 여러 인스턴스가 공유하기 때문에, 여러 로또를 구매하는 상황에서 예상치 못한 동작을 유발할 수 있습니다. 예를 들어, AutoLotto 객체를 여러 번 생성하더라도 lottoNums는 계속 같은 리스트를 참조하게 되어, 각 로또가 동일한 번호를 가지게 될 수도 있습니다.

로또 번호는 각 로또 객체마다 독립적인 값을 가져야 하므로, static 대신 인스턴스 변수로 관리하는 것이 더 적절해 보입니다. 👍 createAutoLotto 메서드도 AutoLotto 객체가 생성될 때마다 독립적인 번호를 생성하도록 수정하면 좋을 것 같습니다.

}
public void createAutoLotto(){
Random random = new Random();
Set<Integer> uniqueNumbers = new TreeSet<>(); // 중복되지 않는 숫자를 보장하는 TreeSet 사용

// 로또 번호 생성
while (uniqueNumbers.size() < CNT_LOTTO_NUMBER) {
int num = random.nextInt(MAX_LOTTO_NUMBER) + 1;

Choose a reason for hiding this comment

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

lottoNums 필드가 static으로 선언되어 있고, 생성자에서 createAutoLotto()를 호출하여 이 static 필드를 갱신하고 있습니다. 이전 리뷰에서도 이 부분에 대해 말씀드렸었는데, 혹시 static 필드를 유지하기로 결정하신 특별한 이유가 있으신가요? 만약 여러 개의 AutoLotto 객체를 생성하게 된다면, static 필드는 모든 인스턴스가 공유하기 때문에 의도치 않은 동작이 발생할 수 있습니다. 각 AutoLotto 객체는 독립적인 로또 번호 목록을 가져야 할 것 같은데요. 이 lottoNums 필드를 인스턴스 변수로 변경하는 것을 다시 한번 고려해보시는 건 어떨까요? 이렇게 하면 각 AutoLotto 인스턴스가 자신만의 번호 목록을 가지게 되어 더욱 안정적인 코드가 될 것입니다.

uniqueNumbers.add(num);
}

lottoNums = new ArrayList<>(uniqueNumbers);

}
public List<Integer> sortLotto(List<Integer> lottoNums){
Comment on lines +20 to +27
Copy link
Author

Choose a reason for hiding this comment

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

createAutoLotto 메서드에서 TreeSet을 사용하여 고유한 번호를 생성한 후, lottoNumsArrayList로 변환하여 저장하고 있습니다. TreeSet은 이미 정렬된 상태를 유지하는데, lottoNums에 저장된 번호들은 getAutoLotto 메서드를 통해 그대로 반환되고 있습니다.

만약 로또 번호를 항상 정렬된 상태로 사용해야 한다면, TreeSet에서 바로 반환하거나 ArrayList로 변환할 때 정렬을 보장하는 방식을 고려해볼 수 있습니다. 현재 sortLotto 메서드가 있지만, getAutoLotto에서 반환되는 lottoNums가 이미 정렬된 상태라면 sortLotto를 별도로 호출하지 않아도 될 수도 있습니다. 이 부분에 대해 어떻게 생각하시나요? 🤔

Comment on lines +23 to +27

Choose a reason for hiding this comment

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

createAutoLotto 메소드에서 TreeSet을 사용하여 중복을 제거하고 번호를 생성하는 것은 매우 좋은 접근입니다! 👍 TreeSet은 이미 정렬된 상태를 유지하는 자료구조인데, lottoNumsTreeSetArrayList로 변환하여 할당한 후, sortLotto 메소드에서 다시 정렬을 수행하고 있습니다. TreeSet의 특성을 고려했을 때, 이미 정렬된 TreeSetArrayList로 변환하는 것만으로도 충분하지 않을까요? sortLotto 메소드의 역할이 TreeSet에서 얻은 정렬된 목록을 다시 정렬하는 것이라면, 이 부분에서 좀 더 효율적인 방법을 고민해볼 수 있을 것 같습니다.

List<Integer> sortLottoList = new ArrayList<>(lottoNums);
sortLottoList.sort(Comparator.naturalOrder());
return sortLottoList;

Choose a reason for hiding this comment

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

getAutoLotto() 메소드가 lottoNums 필드를 반환하고 있는데, 만약 lottoNums 필드가 static이라면 이 메소드 또한 static으로 선언하는 것이 더 자연스러울 수 있습니다. 혹은 lottoNums 필드를 인스턴스 변수로 변경한다면 static이 아닌 일반 메소드로 두는 것이 맞을 것 같습니다. 이 부분에 대한 고민을 좀 더 해보면 좋을 것 같아요.

}
public List<Integer> getAutoLotto(){
return lottoNums;
Comment on lines +29 to +33
Copy link
Author

Choose a reason for hiding this comment

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

sortLotto 메서드를 별도로 분리한 것은 좋지만, AutoLotto 클래스의 주된 책임이 로또 번호를 생성하는 것이라면, 생성된 번호를 정렬하는 것까지 AutoLotto가 담당하는 것이 자연스러울까요? 아니면 로또 번호를 사용하는 곳에서 정렬하는 것이 더 적절할까요? 이 부분에 대한 고민을 해보면 좋을 것 같습니다.

}

}
16 changes: 16 additions & 0 deletions src/main/java/model/Lotto.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
package model;

import java.util.List;

public class Lotto {
private static final int BONUS_NUMBER_INDEX = 6;
private final List<Integer> numbers;

Choose a reason for hiding this comment

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

BONUS_NUMBER_INDEX 상수가 static final로 선언되어 있지만, 현재 Lotto 클래스 내부에서는 이 상수를 활용하는 로직이 보이지 않습니다. 이 상수가 Lotto 객체 자체의 번호와 직접적인 관련이 없다면, 다른 클래스로 이동시키거나 혹은 보너스 번호를 포함하는 로직이 있다면 그 의미를 명확히 하는 것이 좋겠습니다.


public Lotto(List<Integer> numbers) {
this.numbers = numbers;
Comment on lines +7 to +10
Copy link
Author

Choose a reason for hiding this comment

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

Lotto 클래스에서 numbersfinal로 선언한 것은 좋은 점입니다. 하지만 numbers 필드에 static 키워드가 붙어있네요. Lotto 객체는 각각 고유한 번호들을 가져야 하는데, static으로 선언되면 모든 Lotto 인스턴스가 동일한 numbers 리스트를 공유하게 됩니다. 😥

각 로또는 자신만의 번호 리스트를 가져야 하므로, static 키워드를 제거하는 것이 맞습니다.

}

public List<Integer>getNumbers(){
return numbers;
}
Comment on lines +13 to +15
Copy link
Author

Choose a reason for hiding this comment

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

getNumbers() 메서드에서 numbers 리스트를 그대로 반환하고 있습니다. 만약 Lotto 객체의 불변성을 유지하고 싶다면, 리스트의 복사본을 반환하는 것이 더 안전할 수 있습니다. 이렇게 하면 Lotto 외부에서 numbers 리스트를 수정하는 것을 방지할 수 있습니다. 예를 들어, return new ArrayList<>(numbers); 와 같이 말이죠.

}
22 changes: 22 additions & 0 deletions src/main/java/model/LottoList.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
package model;

import java.util.ArrayList;
import java.util.List;

Choose a reason for hiding this comment

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

LottoList 클래스에서 lottoList 필드를 List<Lotto> lottoList = new ArrayList<>();로 선언하고, 생성자에서 Lotto lotto를 인자로 받아 lottoList.add(lotto);만 수행하고 있습니다. 이전 리뷰에서도 언급했듯이, LottoList가 여러 개의 Lotto 객체를 담는 역할을 하려면 생성자나 다른 메소드를 통해 Lotto 객체들을 추가할 수 있어야 합니다. 현재 LottoController에서 lottoList = new LottoList(makeLotto());와 같이 사용되는 방식이라면, LottoList가 단 하나의 Lotto 객체만 담게 될 가능성이 있습니다. 여러 개의 Lotto를 담을 수 있도록 LottoList 클래스를 개선해보는 것은 어떨까요? 예를 들어, addLotto(Lotto lotto)와 같은 메소드를 추가하는 것을 고려해볼 수 있습니다.

public class LottoList {
List<Lotto> lottoList = new ArrayList<>();
private int totalPrice;

public LottoList(Lotto lotto){
Comment on lines +7 to +10
Copy link
Author

Choose a reason for hiding this comment

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

LottoList 클래스가 Lotto 객체를 하나씩 추가하는 생성자를 가지고 있네요. 하지만 LottoList는 여러 개의 Lotto를 담는 역할을 해야 하는데, 현재 생성자는 Lotto 하나만 받아 lottoList에 추가하는 방식입니다.

LottoList의 생성자에서 List<Lotto> 전체를 받거나, Lotto 객체를 추가하는 addLotto(Lotto lotto)와 같은 메서드를 제공하여 lottoList를 관리하는 것이 더 일반적인 일급 컬렉션의 형태일 것 같습니다.

lottoList.add(lotto);
}
public List<Lotto> getLottoList(){
return lottoList;
}
Comment on lines +14 to +15
Copy link
Author

Choose a reason for hiding this comment

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

totalPrice 필드에 setTotalPrice 메서드를 통해 값을 누적하고 있는데, 이 totalPrice가 무엇을 의미하는지 네이밍만으로는 명확하게 파악하기 어렵습니다. 🤔 예를 들어, 로또 구매 총액인지, 당첨금 총액인지 등 구체적인 의미를 알 수 있도록 네이밍을 좀 더 명확하게 하는 것이 좋겠습니다.

Choose a reason for hiding this comment

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

LottoList 클래스에서 totalPrice 필드를 가지고 있으며, setTotalPricegetTotalPrice 메소드를 통해 총 가격을 관리하고 있습니다. LottoList가 로또 목록을 관리하는 역할과 더불어 구매 금액까지 관리하는 것은 책임이 다소 분산되는 느낌이 있습니다. LottoList는 단순히 Lotto 객체들의 컬렉션 역할에 집중하고, 총 가격 계산과 같은 로직은 별도의 클래스나 메소드에서 처리하는 것이 LottoList 클래스의 책임을 더 명확하게 분리하는 데 도움이 될 것 같습니다. 예를 들어, LottoPurchase와 같은 클래스에서 구매 금액과 로또 목록을 함께 관리하도록 설계하는 것을 고려해볼 수 있습니다.

public void setTotalPrice(int reward){
this.totalPrice+=reward;
}
public int getTotalPrice(){
return totalPrice;
}
}
16 changes: 16 additions & 0 deletions src/main/java/view/InputView.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
package view;

import java.util.Scanner;

public class InputView {

private final static String INPUT_PRICE = "구입금액을 입력해 주세요.";
private static final Scanner scanner = new Scanner(System.in);


public int inputPrice(){
System.out.println(INPUT_PRICE);
int price = scanner.nextInt();
return price;
Comment on lines +11 to +14
Copy link
Author

Choose a reason for hiding this comment

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

scanner 필드를 static으로 선언하여 사용하고 있네요. Scanner는 시스템 자원을 사용하기 때문에, 여러 인스턴스에서 공유하는 것은 좋지만, 프로그램 종료 시 close() 메서드를 호출하여 자원을 해제해주는 것이 중요합니다. 현재 코드에서는 Scanner를 닫는 로직이 없어 프로그램 종료 시 자원 누수가 발생할 수 있습니다.

Application 클래스에서 main 메서드가 종료될 때 Scanner를 닫아주거나, InputView 클래스 자체에서 Scanner를 관리하고 종료 시 자동으로 닫히도록 설계하는 것을 고려해볼 수 있습니다.

Choose a reason for hiding this comment

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

inputPrice() 메소드에서 scanner.nextInt()를 사용하여 금액을 입력받고 있습니다. 만약 사용자가 숫자가 아닌 값을 입력하는 경우 InputMismatchException이 발생할 수 있습니다. 이 예외를 처리하거나, 입력 값을 문자열로 받은 후 숫자로 변환하는 방식을 사용하면 좀 더 견고한 입력 처리가 가능할 것 같습니다.

}
}
16 changes: 16 additions & 0 deletions src/main/java/view/OutputView.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
package view;

import java.util.List;

public class OutputView {

private final static String BUY_MESSAGE = "개를 구매했습니다.";

public void countPrint (int count){
System.out.println("\n" + count + BUY_MESSAGE);

}
public void lottoPrint(List<Integer> numbers) {
Comment on lines +11 to +13
Copy link
Author

Choose a reason for hiding this comment

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

countPrint 메서드에서 구입한 로또 수를 출력하는 부분은 깔끔하게 잘 구현되었습니다. 👍

Choose a reason for hiding this comment

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

lottoPrint 메소드에서 System.out.println(numbers);를 사용하여 List<Integer>를 직접 출력하고 있습니다. 실행 결과 예시를 보면 로또 번호 목록이 [8, 21, 23, 41, 42, 43]과 같이 출력되는 것을 볼 수 있는데, 이는 List의 기본 toString() 메소드 결과입니다. 사용자에게 좀 더 보기 좋게 번호를 출력하려면, Lotto 객체 내부의 번호 목록을 포맷에 맞게 변환하여 출력하는 로직을 추가하거나, Lotto 객체 자체를 출력하는 방식으로 변경하는 것을 고려해보면 좋을 것 같습니다.

System.out.println(numbers);
}
Comment on lines +14 to +15
Copy link
Author

Choose a reason for hiding this comment

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

lottoPrint 메서드에서 List<Integer>를 그대로 출력하고 있습니다. 이 부분은 Lotto 클래스에서 getNumbers()를 호출하여 얻은 결과일 텐데요, Lotto 클래스 자체에 toString() 메서드를 오버라이드하여 "[1, 2, 3, 4, 5, 6]"과 같은 형식으로 자체적으로 출력하도록 구현하는 것이 더 객체지향적인 설계일 수 있습니다. 이렇게 하면 OutputViewLotto 객체를 받아 그대로 출력하기만 하면 되니, 책임이 더 명확해질 것입니다.

}