Skip to content
Merged
Show file tree
Hide file tree
Changes from 4 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
9 changes: 9 additions & 0 deletions src/main/java/Application.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
import controller.LottoController;

public class Application {

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

import model.Lotto;
import view.InputView;
import view.ResultView;

import java.util.List;
import java.util.stream.Collectors;

public class LottoController {

public void run() {
int purchaseAmount = InputView.getPurchaseAmount();

int ticketCount = Lotto.getTicketCount(purchaseAmount);

List<Lotto> tickets = Lotto.generateLottoTickets(ticketCount);

ResultView.printOrderTickets(ticketCount);
ResultView.printTickets(ticketCount, formatTickets(tickets));
}

public List<String> formatTickets(List<Lotto> tickets) {
List<String> formattedTickets = tickets.stream()
.map(lotto -> lotto.getNumbers()
.stream()
.sorted()
.map(String::valueOf)
.collect(Collectors.joining(",","[","]")))
.toList();

return formattedTickets;
}
Copy link

Choose a reason for hiding this comment

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

  1. 접근 제어자에 대한 부분을 자주 놓치고 있는 것 같아요
    접근 제어자를 지정하는 것은
    private -> protected -> public순서로 고민해보며 지정하면 좋은 습관 만들어 갈 수 있을 것 같아요

  2. 또한 stream이 중첩되어 있는 구조인데 SRP를 준수하기에는 조금 미흡하지 않나 생각이 들어요
    메서드 분리를 통해서 개선해봅시다!

Copy link
Author

Choose a reason for hiding this comment

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

  1. formatTickets()는 외부에서 직접 호출될 필요가 없는 내부 메서드이므로, public에서 private으로 변경했습니다.
    접근 제어자는 최소한의 공개 범위를 설정하는 것이 중요하다는 점을 다시 한번 확인했습니다.
    앞으로는 private → protected → public 순서로 접근 제어자를 고민하는 습관을 들이겠습니다!

  2. 로또 번호를 문자열로 변환하는 역할을 별도의 메서드(convertLottoToString())로 분리하여 SRP를 준수하도록 개선했습니다.

  • formatTickets()는 티켓 리스트를 문자열 리스트로 변환하는 역할만 수행
  • convertLottoToString()은 하나의 로또 번호를 문자열로 변환하는 역할만 수행

소중한 시간 내어 피드백 해주셔서 항상 감사드립니다!

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

import java.util.*;
import java.util.stream.Collectors;
import java.util.stream.IntStream;

public class Lotto {

// 로또 번호 관련 상수 선언
public static final int LOTTO_MIN_NUMBER = 1;
public static final int LOTTO_MAX_NUMBER = 45;
public static final int LOTTO_CREATE_SIZE = 6;
public static final int LOTTO_PRICE = 1000;
public static final List<Integer> LOTTO_NUMBER_POOL =
IntStream
.rangeClosed(LOTTO_MIN_NUMBER,LOTTO_MAX_NUMBER)
.boxed()
.collect(Collectors.toList());
Copy link

Choose a reason for hiding this comment

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

개행을 수정해봅시다!

Copy link
Author

Choose a reason for hiding this comment

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

개행을 일관성 있게 정리하여 수정하였습니다. 기존에는 IntStream을 개별 줄에 나누어 작성했지만, 가독성을 높이기 위해 한 줄에서 시작하도록 변경했습니다. 피드백 감사드립니다!

public static final List<Integer> LOTTO_NUMBER_POOL =
            IntStream.rangeClosed(LOTTO_MIN_NUMBER,LOTTO_MAX_NUMBER)
                    .boxed()
                    .collect(Collectors.toList());


private List<Integer> numbers = new ArrayList<>();

public static int getTicketCount(int purchaseAmount){
return purchaseAmount / LOTTO_PRICE;
}
Copy link

Choose a reason for hiding this comment

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

만약 1000원단위로 금액이 입력되지 못하면 어쩌죠?

Copy link
Author

Choose a reason for hiding this comment

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

기능 요구사항에서 로또1장의 가격은 1000원이기 때문에, 1000원 단위 금액이 입력되지 않은 경우는 예외 처리가 필요합니다. 또한, 1000원 미만의 구매 금액이 입력되었을 경우에도 예외처리가 필요합니다. 피드백 반영하여 리팩토링 진행하였습니다. 감사합니다!

public void validatePurchaseAmount(int purchaseAmount) {
        if (purchaseAmount < Lotto.LOTTO_PRICE) {
            throw new IllegalArgumentException("구매 금액은 1000원 이상이어야 합니다.");
        }

        if (purchaseAmount % Lotto.LOTTO_PRICE != 0) {
            throw new IllegalArgumentException("구매 금액은 1000원 단위여야 합니다.");
        }
    }

Copy link

Choose a reason for hiding this comment

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

입력값에 따른 티켓 갯수를 반환하는것은 Lotto의 책임에 어색한 느낌을 받아요
로또 1장을 관리하는 model 안에 구입할 로또 티켓의 갯수를 계산하는 것 또한 어색해보이지 않았나요?
저번 리뷰에서 말씀 드린 내용과 비슷하게 Lotto 여러장을 관리하기 위한 동적 배열(List<Lotto>)의 갯수를 Lotto에서 관리하고 있는 느낌이에요

정빈이는 어떻게 생각해요?
고민해보고 정빈이의 생각을 알려주세요~

Copy link
Author

Choose a reason for hiding this comment

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

입력값에 따른 티켓 개수를 반환하는 것 역시 Lotto의 책임이 아닌 것 같습니다.

Lotto는 한 장의 로또를 관리하는 객체여야 합니다.
Lotto 객체는 단순히 6개의 숫자를 생성하고 관리하는 역할을 수행해야 합니다. 하지만 구매 금액을 기반으로 티켓 개수를 계산하는 로직이 포함되면, 단일 책임 원칙(SRP)이 깨질 수 있을 것 같습니다.

여러 개의 로또 티켓을 관리하고 개수를 계산하는 역할은 LottoTickets 에서 수행하는 것이 적절합니다. 이를 통해 Lotto는 한 장의 로또 관리에 집중하고, LottoTickets는 여러 장을 관리하는 역할을 분리할 수 있습니다.

만약 로또 구매 방식이 변하거나 추가 규칙이 생긴다면, 티켓 개수를 계산하는 로직은 LottoTickets에서만 수정하면 됩니다. 이렇게 하면 Lotto 클래스는 불필요한 변경 없이 안정적으로 유지할 수 있습니다.

따라서, Lotto는 한 장의 로또를 관리하는 역할만 수행하고, 티켓 개수 계산은 LottoTickets에서 담당하는 것이 더 적절하다고 생각합니다. 좋은 피드백 감사합니다!


public Lotto(){
this.numbers = createLottoNumbers();
}
Comment on lines +20 to +22
Copy link

Choose a reason for hiding this comment

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

자바 스타일 가이드를 참고하면

  1. 중첩 클래스
  2. 필드(멤버 변수)
  3. 생성자
  4. 메서드

순서를 제시하고 있어요 생성자의 순서가 조금 어색하죠? 수정해봅시다ㅎㅎ

Copy link
Author

Choose a reason for hiding this comment

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

초기화는 생성자에서 이루어지는 것이 적절하다고 생각합니다.

멤버 변수(numbers)는 생성자에서 초기화됩니다.

Lotto 클래스에서 numbers는 객체가 생성될 때마다 새로운 로또 번호를 가져야 하므로, 생성자 내부에서 this.numbers = createLottoNumbers(); 와 같이 초기화하는 것이 적절합니다.
초기화 로직은 별도의 메서드에서 처리하는 것이 가독성과 유지보수 측면에서 유리합니다.

createLottoNumbers() 메서드를 통해 초기화 로직을 분리하면, 생성자 내부가 간결해지고 코드의 역할이 명확해집니다.

불필요한 초기화를 제거하고 생성자에서 멤버 변수를 초기화하는 것이 자바 스타일 가이드에도 적합하며, 유지보수성과 가독성을 높일 수 있는 방법이라고 생각합니다.

좋은 피드백 감사합니다!


private List<Integer> createLottoNumbers(){
List<Integer> shuffledNumbers = new ArrayList<>(LOTTO_NUMBER_POOL);
Collections.shuffle(shuffledNumbers);
numbers = shuffledNumbers.subList(0, LOTTO_CREATE_SIZE);

return numbers;
}

public static List<Lotto> generateLottoTickets(int ticketCount){
List<Lotto> tickets = new ArrayList<>();

for (int i = 0; i < ticketCount; i++){
tickets.add(new Lotto());
}

return tickets;
}
Copy link

@woogym woogym Feb 21, 2025

Choose a reason for hiding this comment

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

  1. 해당 메서드는 Lotto 객체의 책임이 벗어나 있어요
  • generateLottoTickets()Lotto객체의 생성과 컬렉션 리스트 관리까지의 책임을 담당하고 있네요
    Lotto라는 객체만을 생각했을때는 로또 번호만을 생성하는 역할만 가져야 자연스럽다고 생각해요
    여러개의 로또를 관리하는 것이 Lotto라는 객체의 역할과 부합해보이지 않아요

  • Lotto 객체가 자기 자신을 여러개 생성하는 구조
    만약 객체가 자신을 여러개 생성하는 구조라면 외부에서 직접 관리하기 어려워지고, 객체 생성 방식이 변경되면 그에 따른 Lotto클래스의 내부도 수정이 필연적입니다

이 부분에 있어서는 많이 찾아보시고 공부해보시고 고민해봅시다!

  1. 정적 메서드가 인스턴스의 상태를 제어하고 있어요
    정적 메서드는 클래스 레벨에서 실행되는 메서드에요, 즉 특정 객체에 속하지 않죠 그렇다면 정적 메서드가 인스턴스의 상태를 제어하거나 생성하는 역할은 정적 메서드가 해서는 안될 역할이라고 볼 수 있어요
    이유는 해당 리뷰가 도움 될 수 있을 것 같아요~

  2. 마찬가지로 외부에서 List<Lotto> lottos = getTicketCount(6) 이러한 형태로
    오용될 가능성도 가지고 있네요

찾아보고, 고민해보고, 개선해봅시다!

Copy link
Author

@JeongBeanHyun JeongBeanHyun Feb 24, 2025

Choose a reason for hiding this comment

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

  1. Lotto가 직접 여러 개의 객체를 생성하면, 로또 번호 생성로또 리스트 관리라는 두 가지 역할을 동시에 수행하는 것이 되겠군요..! 이렇게 되면 객체의 책임이 모호해지고 유지보수가 어려워질 것 같습니다.

Lotto는 하나의 로또를 생성하고 저장해야하는데, Lotto 클래스 자체를 로또자판기로 생각하면서 작성했었습니다. 그래서 객체의 역할이 명확해지도록 클래스를 나눌 필요가 있을 것 같습니다. LottogenerateLottoTickets() 메서드는 여러 개의 Lotto를 생성하고 리스트로 관리하는 역할까지 포함하고 있었습니다. 그래서 이 역할은 LottoTickets 클래스로 분류하였습니다. Lotto는 로또 번호 6개를 생성하는 역할만 담당하도록 수정했습니다.

  1. 기존 코드에서는 generateLottoTickets()가 정적(static) 메서드로 정의되어 있었고, 내부에서 new Lotto()를 호출하여 객체를 직접 생성했습니다. 이렇게 하면 정적 메서드가 인스턴스를 관리하는 역할을 하게 되어 객체지향 원칙에 어긋난다는 것을 알게되었습니다.
    static 메서드였던 generateLottoTickets()를 삭제하고, 대신 LottoTickets에서 객체를 생성하도록 변경했습니다.

  2. getTicketCount()는 원래 로또 개수를 계산하는 메서드인데, 마치 여러 개의 로또를 생성하는 메서드처럼 보일 수도 있었습니다. getTicketCount()는 그대로 두고, 여러 개의 로또를 생성하는 기능은 LottoTickets 클래스에서 담당하도록 변경했습니다.

좋은 피드백 감사합니다!


public List<Integer> getNumbers() {
List<Integer> sortedNumbers = new ArrayList<>(numbers);
Collections.sort(sortedNumbers);

return sortedNumbers;
}
Copy link

Choose a reason for hiding this comment

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

getNumbers()의 이름말고도 더 많은 역할을 수행하고 있는 것 같아요~
네이밍은 자세할수록 좋아요~

Copy link
Author

Choose a reason for hiding this comment

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

네 현재 getNumbers()는 정렬된 리스트를 반환해주는 역할을 하고 있습니다. 보다 명확한 의미를 전달하기 위해getSortedNumbers() 로 수정하였습니다. 피드백 감사합니다!

public List<Integer> getSortedNumbers() {
        List<Integer> sortedNumbers = new ArrayList<>(numbers);
        Collections.sort(sortedNumbers);

        return sortedNumbers;
    }

}
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 static final Scanner scanner = new Scanner(System.in);

public static int getPurchaseAmount(){
System.out.println("구입금액을 입력해 주세요.");
int purchaseAmount = scanner.nextInt();
scanner.close();
Copy link

Choose a reason for hiding this comment

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

입력이 끝난 시점에서 바로 닫아주는 거 좋아용👍


return purchaseAmount;
}
}
16 changes: 16 additions & 0 deletions src/main/java/view/ResultView.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
package view;
import java.util.List;

public class ResultView {

public static void printOrderTickets(int ticketCount){
System.out.println();
System.out.println(ticketCount + "개를 구매했습니다.");
}

public static void printTickets(int ticketCount ,List<String> formattedTickets){
Copy link

Choose a reason for hiding this comment

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

구입한 티켓을 추력하는 것으로 보이는데, 좀 더 자세하게 네이밍 해봅시다
요구사항에는 네이밍을 축약하지 않는다고 되어 있으니 더 준수해봅시다!
네이밍은 자세할수록 좋습니다!

Copy link
Author

Choose a reason for hiding this comment

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

네이밍을 좀더 자세하게 작성할 필요가 있을 것 같습니다! 앞으로의 과제에서도 명심하겠습니다.
구입한 로또 티켓을 출력한다는 의미를 명확히 표현하기 위해 printPurchasedLottoTickets()로 수정하였습니다.
좋은 피드백 감사합니다!

for (String ticket : formattedTickets) {
System.out.println(ticket);
}
}
Copy link

Choose a reason for hiding this comment

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

mvc 패턴에 대한 이해도가 높이진게 티가 나네요👍

}