피드백) 1-2(로또) 코드리뷰

📜 제목으로 보기

PR- 로또 미션 피드백(제이)
- 구조
- 네이밍
다른 크루들 PR 참고
- 구조
- docs
- 네이밍
- 캐슁
- map
- Factory
- test
- .copyOf()
- for vs stream

PR- 로또 미션 피드백(제이)

구조

Collections.unmodifiableList(issuedLotto); / issuedLotto인스턴스변수를 넘겨줌
- getter or 메서드 return으로 넘겨주는 list가issuedLotto라는걸 인스턴스변수로 사용한다면, 객체의 어떤 메서드로 issuedLotto를 조작하게되면 반환된 issuedLotto값이 바뀔거에요.
  - 생성자에서 계산후 생성될 때부터 초기화되는 변수가 아니면 인스턴스 변수로 가지지 마라. 메서드에 의해 바뀐다
  - my) Collections.unmodifiableList() 대신 List.of() 불변 복사 / new ArraysList<>()가변 변 복사를 사용하자.
null처럼 아예 입력이 들어오지 않는 것은 view가 아니라 도메인에서 검증
- nullOrEmtpy와 format정도의 공통검증만 inputView로 옮기겠습니다!
일급의 getter에서는 외부에서 내부 포장변수 수정 못하도록 수정불가 타입으로 응답하기
- 방어적 복해서 넘기기
누적합 + 카운트는 stream으로 depth줄이기
도메인 내부에서 toDto 쓰지말 것
- 컨트롤러에서 from도메인 , to도메인 등을 써서 변환해야한다.
서비스에서 필요에 의해 만들어진 특정 도메인을 관련 로직들은 결국 사용되는 변수를 포장하도록 클래스를 파고( 사용하는 스태틱 클래스를 파도 됨) 그 내부로 로직을 가져간다
로직의 결과로 나온 데이터가 추가 계산이 필요할 경우 클래스로 파서 포장하는 대신 그 생성과정은 생성자에 넣어라
메인로직을 담당하는 service에서는 인스턴스변수를 두지말고 input -> output의 스태틱 형태를 유지해라. 중요하지 않은 컨트롤러에서 도메인 변수를 생성후 -> 서비스에 input으로 줘도 되니까
컨트롤러에서는 outputView를 메인로직과 같이 묵지말고, 들어가는 input들이 서비스 메서드에 확실히 필요한 것들인지 한눈에 파악하게 작성
Dto는 민감한 데이터, 불필요한 데이터를 짤라 넘기는 것인데, 작은 프로젝트들에서는 다 쓰인다. 넘겨도 된다~
- dto를 항상 만들라고 하지말자~
빈 List가 가능한 자리에는
- early return + Collections.emptyList() 활용
서비스가 비대해진다면, 도메인 로직을 포함하고 있는지 점검하기
- 도메인 로직은 도메인 (없으면 추출해서) 클래스에서 처리하기
다른 도메인과 추가 검증, 비교로직이 필요할 경우, 이미 포장된 도메인이라도 한번더 포장할 수 있다.
- 입력받은 당첨Lotto + 보너스볼은 같이 다니며 + 검증도 요구된다
- 추가 로직 없이 단순히 같이 다니는 것들을 포장해서 2개의 상태값으로 보관할 수 있다.`
  - 블랙잭에서 dealer + gamblers -> Players -> this.dealer + this.gamblers
  - 같이 다녀서 포장 -> 필요할 땐, 각각 getter로 바로 줘도 된다.

네이밍

final priavate -> 접근제한자 final 순으로…
상수 <-> 변수 개행 -> class 생성후 개행도 해주기
도메인Factory는 정펙매가 너무 커서 static클래스로 뺀 것

다른 크루들 PR 참고

베루스 - 앨런
리차드 - 루피
케이 - 던
후니 - 엘런

구조

Lotto내 상수 1_000이 Lottos, Money도 알아야한다?

public 상수로 ? (X)

여러 도메인이 아는 상수라도 각 도메인패키지내에 도메인상수들을 class로 관리하는 것도 좋은 방법이라고 생각해요

  1_000원 이라는 것이 로또와 금액, Lottos가 모두 알아야 한다면 1_000원이라는 금액이 Lotto 에 있는 것이 맞는 것인가? 어쩔 수 없이 각 객체별로 공유해야하는 상수가 있다면 그 값은 누가 갖는게 맞는가? 에 대해서도 고민해보면 좋을 것 같아요. 지금처럼 로또가 같는 것도 방법이고, 도메인패키지내에 도메인상수들을 관리하는 것도 좋은 방법이라고 생각해요🙂

포맷팅(변환)은 view(입력)에 따라 달라지니 중요하지 않은 controller에서!

convert를 도메인에서하면, 입력달라질 시 -> 도메인도 같이 수정되어야한다.

formating, convert는 자주 바뀌는 view에 따라 달라지니, 중요하지 않은 controller에서

  포맷팅은 분명 현재 로또게임에서 중요한 로직이지만 입력과 도메인을 분리하기 위해서는 저는 Controller 패키지에서 담당하는 것이 맞다는 생각이 들어요🙂 추가로 DTO와 같은 객체들을 통해서 각 패키지나 레이어에서 필요로 하는 포맷을 재정의할 수도 있습니다.

controller에서는 상태값을 가지지 말 것
- 다른 사용자는 다른 controller 인스턴스 객체를 가질 것이라고 생각해서 문제가 없다고 생각했어요.
  - 다른 사용자라도 여러 사용자가 같은 인스턴스 객체사용 가능 -> 상태값 오염 가능
```
  `synchronized`의 잠금을 통해 멀티쓰레드 환경에서 안정성을 보장할 수 있지만, 현재는 인스턴스변수를 가지지 않는 것으로 충분할 것 같아요.
```

controller에서 도메인 객체(domain) 생성 메서드 <-> 출력(view)메서드을 1개 메소드로 묶지말 것

controller에서 전체 로직에 대한 행위들이 프로그램 전체를 나눠서 설명할 수 있게 끔만 나눌 것

controller전체를 너무 간결하게 나누지 말것
실행과 출력도 나눌 것

  비즈니스로직을 실행하는 행위와 출력하는 행위는 분리되는 것이 관심사가 분리되는 것이라 생각이 되어요.
        
  두 가지의 역할이 하나의 메소드에 들어가는 것 보단, 각각 관리될 때 역할이 하나기 때문에 유지보수하기 용이한 코드가 아닐까 라는 생각을 하고 있습니다.🙂

일급컬렉션도 컬렉션이라서 add 메서드 추가해도 된다!
- 가변 컬렉션, 불변 컬렉션 따로 있는 듯.

(베)

inputView에서 커스텀Exception을 쓰는 순간 -> view에서 백엔드의 Exception을 의존하게 된다 -> 쓰지말 것
List<일급>도 일급컬렉션으로 포장할 것

뭔가 인스턴스 변수를 인자없이 기본생성자초기화해놓고 -> FOR문돌면서 add의 가변값처럼 사용하지마라. -> **일급 가변 생성을 정펙매를 통해 생성과 동시에 + 값할당기능을 제공할 수 있다. **

  // 잘못된 예
  Statistic statistic = new Statistic(inputMoney);
  for (Lotto lotto : lottoes) {
      statistic.addRank(getRankBy(lotto));
  }
  return statistic;

  Statistic를 생성해놓고, 값을 추가하고있는데요. 객체가 가변값처럼 사용되고 있는 것 같아요.
  Statistic에서 정잭팩토리를 제공해서, 생성과 동시에 값을 할당하는형태로 만드는건 어떨까요?

list를 합칠 때 -> 빈 리스트(가변값)에 add를 대신하는 stream

기존list.stream() + 새list.stream() -> Stream.concat() -> collect( collectingAndThen( toList(), 포장생성자::new )

2stream 을 1개 list로 변환해서 모으고 -> 포장까지 할 수 있다.!!

  public Lottoes combine(Lottoes other) {
      List<Lotto> result = new ArrayList<>();
      result.addAll(lottoes);
      result.addAll(other.lottoes);
      return new Lottoes(result);
  }
            
  // 변환후
  public Lottoes combine(Lottoes other) {
      return Stream.concat(lottoes.stream(), other.lottoes.stream())
          .collect(collectingAndThen(toList(), Lottoes::new));
  }

(리)

controller에서 input받을때의 안내문 -> InputView.printException(exception);
미래를 예측해서 코드를 짤 필요 없다? (확장가능성을 미리 X)

콘솔 출력만 하는데 InputView 로 분류하신 이유가 있으실까요 ?

  사용자와 직접 커뮤니케이션
  사용자의 요청을 컨트롤러로 전달
  컨트롤러의 응답을 최종적으로 사용자에게 전달
        
  콘솔 출력은 사용자에게 오류 메시지 혹은 결과를 전달하는 과정이기 때문에
  View의 역할이라고 생각했습니다!
        
  혹시 제가 조금 더 생각해봐야할 지점이 있을까요?! 😅

(후)

컨트롤러의 코드는 시나리오로서 글처럼 읽을 수 있게 == input/output명확하게는

  사용자 시나리오로 글처럼 읽을 수 있는 코드를 만들려고 노력하고있는데요.
  이 부분은 돈을 생성하는게 아닌, 돈을 입력받는 부분이 아닐까요?

일급이 중복을 허용하지 않는다 -> List대신 Set을 상태값으로 가져도
- 중복검사가 제거된다 -> 갯수검사만 하면 됨.
- LinkedHashSet을 쓰면 정렬도

도메인 검증은 도메인 기본 생성자에서

  1~45의 숫자인게 도메인 규칙인데요. 이 부분이 '생성하는 곳'에서 검증되어야한다고 생각

inputview에서 반복해서 입력받기 가능.

그러나 stream 작성하면 디버깅이 어려울 수도 있음.

  public static List<List<Integer>> inputManualLottos(final int counts) {
      System.out.println(INPUT_MANUAL_LOTTO_NUMBER);
      return IntStream.range(0, counts)
          .mapToObj(index -> inputManualLotto())
          .collect(Collectors.toList());
  }

(중복) 가변으로 list 추가로 받기
- 기존과 들어올 list 모두 stream으로 concat하여 불변형태로 합치기
스트림내 스트림은 가독성 -> 메서드 분리

(후)

LottoNumber 정적메서드만 있는 static Factory 클래스로 싱글톤(service, cahce)를 대체하자.
- 블로그

LottoResult -> Map을 초기화시켜놓고, 거기에 하나씩 put하는 형태인데요. 집계함수(groupingBy)도 고려해볼 수 있을 것 같아요. 🙂

map을 put이 아니라 replace로? ㄴㄴ

  //수정 전
  final Map<Rank, Integer> rankResults = Rank.initResultMap();
  for (Lotto lotto : lottos.getLottos()) {
      Rank rank = winLotto.matchResult(lotto);
      rankResults.replace(rank, rankResults.get(rank) + 1);
  }
  return new LottoResult(rankResults);

            
  replace를 사용함으로써 initialize한 key값 외 예외적인 값이 들어올 상황을 방지
  개인적으로 방어적인 코드보다, 빠른 피드백을 받을 수 있도록 예외를 던지는게 더 좋다고 생각하고있어요.
            
  근본적으로 Rank.initResultMap();로 씨드를 만든 Map으로 put해주는 구조가 조금 특이하긴 하네요..🤔

map을 거꾸로 출력도 view에서!!해야함 - > values().stream -> view의 책임이라 inputView에서

  toReverseList 는 뷰에 의존적인 코드라고 생각해요. 거꾸로 순서가 필요한 건 뷰의 책임이라고 생각하는데, 뒤집어주는 건 뷰에서 처리하는게 좋을 것 같아요.

toString()활용보다는 그냥 getter

NEW_LINE = System.lineSeparator() 도 상수화

역순으로 출력안하고 title만 하드코딩 + 개별요소만 get하는 sense

  public class ResultView {
      public static void printResult(LottoResult lottoResult) {
          System.out.print(System.lineSeparator());
          System.out.println("당첨 통계");
          System.out.println("---------");
          System.out.println("3개 일치 (5000원) - " + lottoResult.getLottoResultByRank(FIFTH).getCount() + "개");
          System.out.println("4개 일치 (50000원) - " + lottoResult.getLottoResultByRank(FOURTH).getCount() + "개");
          System.out.println("5개 일치 (1500000원) - " + lottoResult.getLottoResultByRank(THIRD).getCount() + "개");
          System.out.println("5개 일치 보너스 볼 일치(30000000원) - " + lottoResult.getLottoResultByRank(SECOND).getCount() + "개");
          System.out.println("6개 일치 (2000000000원) - " + lottoResult.getLottoResultByRank(FIRST).getCount() + "개");
          System.out.println(
              "총 수익률은 " + lottoResult.getRateOfReturn() + "입니다.(기준이 1이기 때문에 결과적으로 " +
              getWinOrLoseByLottoResultDto(lottoResult) + "라는 의미임)");
      }
    
      private static String getWinOrLoseByLottoResultDto(LottoResult lottoResult) {
          if (lottoResult.getRateOfReturn() > 1) {
              return "이익";
          }
    
          return "손해";
      }
  }

만약 역순 출력한다면?

map을 돌고 있으니, 각 요소들이 comparable 상속 중이면 .sorted(Comparator.reverseOrder())

  If your stream elements implements Comparable then the solution becomes simpler:
        
   ...stream()
   .sorted(Comparator.reverseOrder())

values()만 역순으로 하고 싶다면.. list로 바꿔서..

  List<Rank> ranks = Arrays.asList(Rank.values());
  Collections.reverse(ranks);

docs

어떠한 프로그램이다(요구사항), 혹은 실행결과 등도 같이 정의

  문서에 이 프로그램이 그래서 어떠한 프로그램이다, 혹은 실행결과 등을 같이 보여준다면 프로그램을 훨씬 빠르게 이해할 수 있을 것 같아요.

예시

  ## 요구 사항
        
  가상으로 사용자가 로또를 구매하고 지난주 당첨 번호를 직접 입력하여 당첨이 얼마나 되는지 수익률이 몇인지 알 수 있는
  프로그램입니다.
  - java로만 구현된 애플리케이션(jdk11버전)
  - 터미널에서 사용자가 구입금액, 지난주 당첨 번호를 입력하고 터미널에서 결과를 확인할 수 있다.
  - 로또는 1 ~ 45 중 랜덤으로 6개의 번호를 가진 로또를 발급한다.
  - 로또는 1장당 1000원이다.
  - 당첨기준은 실제 로또와 같다.
        
        
  ## 실행 결과
  ```text
  구입금액을 입력해 주세요.(1000원~20억원)
  14000

## 페어프로그래밍 컨벤션도 유지

  ## 페어 프로그래밍 컨벤션
        
  - 10분 간격으로 역할을 바꾼다.
  - BDD 테스트 코드 작성 패턴으로 작성한다.
  - 테스트만 통과할 수 있을 정도로 TDD를 구현한다.
  - 커밋 단위를 README 기준으로 한다.

네이밍

메서드 입력요구는 request (getMoney -> requestMoney)??
- 내부에서 print 로직을 포함하지 말 것
- print는 controller에서 뿌려주면 됨.

상수는 재사용 뿐만 아니라 의미의 명확화의 관점도 포함된다.

  맞습니다. 깔끔하다면 계속 사용하셔도 됩니다!
    
  다만 우리가 상수화를 할 때 단순히 재사용을 위해서 하는 것 뿐만아니라 의미를 알 수 없는 값에 이름을 부여해서 의미를 드러낸다라는 관점도 있다는 점을 알고 가시면 좋을 것 같아요!

사소하더라도 한 줄에는 . 1개 -> stream은 한 줄씩 모두 띄워주는게 가독성이 좋은 것 같아요. 그 역할이 조금 더 뚜렷하게 보이는 것 같아서요.

  public Lotto(List<Integer> numbers) {
      validateNumbers(numbers);
      this.numbers = numbers.stream().map(Number::new).collect(Collectors.toSet());

디미터의 법칙도 객체에게 메시지를 보내라!의 관점
아래는 1줄에 . 1개이지만, 가독성을 위해 stream은 줄 띄우기
```
  map(Number::new).collect(Collectors.toSet());
```

  numbers.stream() // 스트림을 열고
          .map(Number::new) // 가공하여
          .collect(Collectors.toSet()); // 결과를 셋으로 만든다.

Money가 VO가 아니라 검증이 추가되었다면 대명사 Money(X) -> 로또에서만 사용되는 1_000단위의 금액 -> LottoMoney로서 대명사 VO가 아니라 도메인임을 명시해줄 것

   지금 Money는 로또를 구매하기 위한 금액이기 때문에 1000으로 나누어떨어져야한다는 제약사항이 있어요. 순수한 Money는 아니게 되는거죠.
    
  그렇기때문에 개인적으로는 LottoMoney / PurchaseMoney 와 같이 이 금액은 구매를 위한 금액이고 제약사항이 있다라는 것을 명시할 수 있는 이름을 사용할 것 같아요.

특정 프로그램 패키지 안에 있더라도 클래스명에 명시

  작은 프로그램이지만 앞으로 프로그램이 커질 때 각 클래스들의 패키지를 잘 안보게 되기도 하고, 코드 자체에서 드러나지 않기 때문에 놓치는 경우가 많더라구요.
  이런 부분을 지양하고자 저는 후자를 선호합니다🙂 다만 네이밍이 길어지는 단점도 분명 존재하는 것 같습니다.

앞에 검증 등 포장되어있다면, 앞에 도메인명을 붙이자.! Number -> LottoNumber
포장되는 내부 인스턴스 변수를 클래스 그대로 따오지 말고 -> value -> getValue()
예외 던질때 메세지 피드백을 상세하게
- 어떤 값때문에 예외가 발생했는지 추가

캐슁

캐슁 작업을 Lotto 생성자에서 해주다가 복잡해지니 정적 팩토리 메서드->클래스 LottoFactory로 뺐다.

 객체생성이 복잡한 경우 팩토리라는 키워드로 조금 더 공부해보시면 좋을 것 같아요.
 맞습니다. 정적팩토리메소드를 통해 자동로또를 만들어낸다라는 의미를 잘 드러낼 수 있어요.
 추후에 생성이 더 복잡해진다면 생성을 담당하는 LottoFactory와 같은 형태로 리팩토링하는 것도 방법인 것 같아요.

처음부터 미리 만들지말고, 생성할때마다 CACHE맵에서 검사후 없으면 반환

  private static final Map<Integer, LottoNumber> CACHE = new HashMap<>();
    
  private final int number;
    
  private LottoNumber(int lottoNumber) {
      this.number = lottoNumber;
  }
    
  public static LottoNumber getInstance(int lottoNumber) {
      validateRange(lottoNumber);
    
      if (CACHE.containsKey(lottoNumber)) {
          return CACHE.get(lottoNumber);
      }
    
      LottoNumber newInstance = new LottoNumber(lottoNumber);
      CACHE.put(lottoNumber, newInstance);
      return newInstance;
  }
    
  private static void validateRange(int number) {
      if (isOutOfRange(number)) {
          throw new IllegalArgumentException(ERROR_MESSAGE_FOR_OUT_OF_RANGE_NUMBER);
      }
  }
    
  private static boolean isOutOfRange(int number) {
      return number < MINIMUM_LOTTO_NUMBER || MAXIMUM_LOTTO_NUMBER < number;
  }
    

캐슁은 맵을이용해라 IntStream -> toMap( number -> number, 그걸로 new 객체 생성자() )

이펙티브 자바, 아이템 28. 배열보다는 리스트를 사용하라를 한번 찾아보시는 것도 좋을 것 같네요 🙂

  LOTTO_NUMBER_CACHE = IntStream.rangeClosed(MIN_LOTTO_NUMBER, MAX_LOTTO_NUMBER)
      .boxed()
      .collect(Collectors.toMap(
          number -> number, 
          LottoNumber::new)
      );

블로그

map

map 돌기: map.entrySet().stream().map( entry -> entry.getKey(), entry.getValue() ) + getKey(enum)에 메서드 정의하여 추가로직 -> sum()

 public long getTotalProfit() {
     return result.entrySet()
         .stream()
         .mapToLong(entry -> entry.getKey().multiply(entry.getValue()))
         .sum();
 }

entry는 getKey()할 수 밖에 없으며, 하나의 맾핑된 key객체로서 .메서드( ) 로 메세지를 던진다. -> .multiply( )

메서드를 어캐 정의했나

  public long multiply(int count) {
      return (long) price * count;
  }

  public double getRateOfProfit(Money money) {
      long totalMoney = 0L;
      for (Map.Entry<LottoRanking, Integer> entry : result.entrySet()) {
            
            
          //요렇게도 사용할 수 있을 것 같네요🙂
            
          public double getRateOfProfit(Money money) {
              long totalMoney = result.entrySet()
                  .stream()
                  .mapToLong(entry -> entry.getKey().multiply(entry.getValue()))
                  .sum();
              return money.getRateOfProfit(totalMoney);
          }

enumMap: HashMap보다 성능상에서 이점이 많은 것 같습니다.

Null값을 key로 가질 수 없다.
hashcode 충돌이 존재하지 않는다.
단순 배열의 구조를 가진다.
Enum의 순서를 유지할 수 있다.

 개인적으로 현재 EnumMap을 사용해서 위의 장점을 얻을 수 있다 라는 접근보단, 어떠한 코드를 구현하는데 있어서 조금 더 적절한 (그것을 위한 형태)가 있다면 사용해보는 것도 좋다라는 생각

findAny가 스트림이 직렬이면 findFirst와 동일하게 작동하는군요!

(베)

개인코드 -> map초기화시 toMap() + Function.identity(), r->0이용

  private Map<Rank, Integer> emptyRankMap() {
      return Stream.of(Rank.values())
          .collect(toMap(Function.identity(), r -> 0));
  }

(리)

비교 결과를 알아서 그룹별,갯수의 map에 담아주는 stream

  private static Map<Rank, LongSummaryStatistics> getLottoResult(WinningLotto winningLotto,
                                                                 Lottos lottos) {
      Objects.requireNonNull(winningLotto, ERROR_MESSAGE_FOR_NULL_WINNING_LOTTO);
      Objects.requireNonNull(lottos, ERROR_MESSAGE_FOR_NULL_LOTTOS);
        
      return lottos.getLottos()
          .stream()
          .map(winningLotto::getRankByLotto)
          .collect(groupingBy(identity(),
                              () -> new EnumMap<>(Rank.class),
                              summarizingLong(Rank::getPrize)));
  }

map의 values()값만 sum by LongSummaryStatistics

  private static long getTotalReturnByLottoResult(Map<Rank, LongSummaryStatistics> lottoResult) {
      return lottoResult.values()
          .stream()
          .mapToLong(LongSummaryStatistics::getSum)
          .sum();
  }

(후)

누적합을 IntSream으로 변환해서 .sum() 하는게 > reduce(0부터, 변환하면서 누적) 하는 것보다 깔끔하다.

  IntStream 스트림을 사용하면 sum() 메서드 활용이 가능해서, 더 읽기좋을 것 같아요.
  그리고 메서드체이닝(.)을 행 단위로 나눠주시면 더 읽기 좋습니다 👍

  //수정전
  return rankResults.entrySet().stream().map(Entry::getValue).reduce(0, Integer::sum)
      * Lotto.LOTTO_PURCHASE_MONEY;
        
  //수정후
  return rankResults.entrySet().stream()
      .mapToInt(Entry::getValue)
      .sum() * Lotto.LOTTO_PURCHASE_MONEY;

Factory

개인적으로는 객체생성의 롤을 객체에게 전담하도록 하는 편이고, Lotto 와 같이 생성이 복잡한 일부분에 대해서만 팩토리를 사용하는 편이에요. 참고만 부탁드립니다
- 생성과 팩토리를 통한 생성 두가지가 모두 가능할 때, 이 코드를 유지보수하는 사람이 이 두가지를 적절하게 잘 이용하기 어렵다는 단점도 있을 것 같아요.
  - 객체 생성에 대한 역할을 두 곳에서 관리하고 있기 때문에

test

Nested로 의미가 잘 드러나는 것 같아요🙂

여기서는 가독성이라는 측면이 더 중요한 것 같아요. 이 더 가독성이 좋다면 사용하시면 될 것 같습니다.
똑같은 상태값 공유하는 것들만 다르게 setUp하는 용도로 쓰자.

  저 개인적으로는 상태값을 공유하고 그 의미가 비슷한 것들은 @nested로 작성하고 이외에 독립적인 것들은 클래스내에 메소드로 작성하는 것 같아요🙂
      - 페어와 팀원과 적절한 룰을 만들어가며 장단을 비교해가면 좋을 것 같습니다.

(베)

테스트는 오히려 먼저 변수 선언 (or초기화)후 -> 나중 할당하는 @beforeEach에서 초기화하기 위해 이용한 가변으로 선언

  //LottoesTest.java
  private static final Lotto[] LOTTOES = new Lotto[]{
      Lotto.create(List.of(1,2,3,4,5,6)),
      Lotto.create(List.of(11,12,13,14,15,16)),
      Lotto.create(List.of(21,22,23,24,25,26)),
      Lotto.create(List.of(31,32,33,34,35,36))
  };
    
  테스트 픽스쳐를 만들어주셨는데요. 지금은 큰 문제가 없지만 이 부분도 가변이 될 수 있어서요.
    
  특정테스트에서 값이 변경되어 디버깅이 굉장히 어려워질 수 있습니다. 이 부분을 @BeforeEach로 빼보시는걸 추천해요🙂

.copyOf()

(리)

읽기전용을 의미하는 Collections.unmodifiableList()이라도, 원본이 바뀌면 따라 바뀐다. 할당받은 변수만 조작불가 -> 원본과는 연결됨

생성자에서 검증에서 파라미터 원본값의 오염가능성 -> 포장이 가진 내부 상태값은 연결고리를 끊고난 것을 주자. -> this.value = List.copyOf()

  //수정 전
  validateSize(lottoNumbers);
  validateDuplicate(lottoNumbers);
  this.lottoNumbers = Collections.unmodifiableList(lottoNumbers);
        
  //이 생성자를 사용하는 곳에서 생성자가 실행된 이후에 전달된 파라미터의 원본 값(지역변수이자 원본 값)에 list 를 조작한 뒤 디버깅을 해보시면 좋을 것 같아요
        
  //수정 후
  public Lotto(List<LottoNumber> lottoNumbers) {
      Objects.requireNonNull(lottoNumbers, ERROR_MESSAGE_FOR_NULL_LOTTO_NUMBERS);
        
      validateSize(lottoNumbers);
      validateDuplicate(lottoNumbers);
      this.lottoNumbers = List.copyOf(lottoNumbers); // 상태값에는 연결고리를 파라미터와 연결고리륵
  }

unmodifiableList(anotherList) 로 선언한 참조변수, 즉 복사본 컬렉션에다가 요소를 추가거나 삭제하려고 할 경우, UnsupportedOperationException이 발생
- 그러나 원본 컬렉션에 요소를 추가 / 삭제할 경우, 복사본 컬렉션에서도 그대로 적용
- 요약
  1. 원본 컬렉션과 주소값이 다르며 컬렉션 내부 요소들의 주소값은 동일하다.
  2. 원본 컬렉션에서 요소 추가, 삭제가 일어날 경우, 복사본 컬렉션에서도 적용된다.
  3. 복사본 컬렉션에서 요소 추가, 삭제를 시도할 경우 UOE 이 발생되며 실패한다.
List.copyOf(anotherList);를 통한 방어적 복사를 하려면
- 복사본 컬렉션에 대한 요소 추가 / 삭제 시도 시 UOE가 발생한다는 점은 unmodifiableList와 동일
- 그러나 원본 컬렉션에서 요소 추가 / 삭제 가 일어나도, 영향을 받지 않습니다
- 그럼에도 불구하고 copyOf()로 만들어낸 컬렉션이 완전한 불변이라고 볼 수는 없습니다. 왜냐하면 copyOf의 반환값으로 전달된 unmodifiable list에서 객체를 꺼내서, 그 객체 내부의 값을 변경할 경우, 아니면 외부에서 그 객체의 값을 변경할 경우에는 요소 까지 깊은 복사를 하지는 않았기 때문에 값이 변경됨.
  - 원소들도 불변이여야지 완벽한 불변

  new ArrayList<>(anotherList), 
  new ArrayList<>() + addAll() 과 동일합니다.
        
  컬렉션의 주소값은 달라지지만, 요소들의 주소값은 동일합니다.
  따라서, 컬렉션 레벨의 요소 추가 / 삭제 는 공유되지 않지만,
  요소 레벨의 요소 값 수정 등은 공유됩니다.new ArrayList<>(anotherList), new ArrayList<>() + addAll() 과 동일합니다.
        
  컬렉션의 주소값은 달라지지만, 요소들의 주소값은 동일합니다.
  따라서, 컬렉션 레벨의 요소 추가 / 삭제 는 공유되지 않지만,
  요소 레벨의 요소 값 수정 등은 공유됩니다.

공부용 코드

  package lotto;
            
  import java.util.ArrayList;
  import java.util.Arrays;
  import java.util.Collections;
  import java.util.List;
  import org.junit.jupiter.api.Test;
            
  public class ListTest {
      @Test
      void modify_UOE() {
          String[] stringArray = {"a", "b"}; // 배열
          final List<String> stringList = Arrays.asList(stringArray); // 가변 스트링
          final List<String> unmodifiableStringList = Collections.unmodifiableList(stringList);
            
          System.out.printf("unmodifiableStringList={}", unmodifiableStringList);
            
          //        unmodifiableStringList.add("c");
            
          System.out.printf("unmodifiableStringList={}", unmodifiableStringList);
      }
            
      @Test
      void wrap_and_modify_original_and_get_unmodifiable() {
          String[] stringArray = {"a", "b"}; // 배열
          List<String> stringList = new ArrayList<>(Arrays.asList(stringArray)); // 가변 스트링 list
          //final List<String> unmodifiableStringList = Collections.unmodifiableList(stringList);
          //원본 List<String>을 포장해서 들고 있으면서, 반환만 Collections.unmodifiableList();시키면
          UnmodifiableList unmodifiableList = new UnmodifiableList(stringList);
          List<String> returnedUnmodifiableList = unmodifiableList.getValue();
            
          // a, b
          System.out.printf("returnedUnmodifiableList={\%\s}%n", returnedUnmodifiableList.toString());
            
          //add할때는 원본 list에 add -> 이게 객체내부의 value에도 영향을 미칠까?
          stringList.add("c");
            
          // a, b, c -> 원본을 고쳤는데, unmodifiableList만 못건들였지, 같이 수정됨.
          // -> 쓰기불가지만, 컬렉션 주소를 공유
          System.out.printf("returnedUnmodifiableList={\%\s}%n", returnedUnmodifiableList.toString());
      }
            
      @Test
      void wrap_and_modify_original_and_get_copyOf() {
          String[] stringArray = {"a", "b"}; // 배열
          List<String> stringList = new ArrayList<>(Arrays.asList(stringArray)); // 가변 스트링 list
            
          // 마찬가지로 원본을 들고 있으면서, 반환만 copyOf로 해주는 클래스 생성
          final CopyOfList copyOfList = new CopyOfList(stringList);
          final List<String> returnedCopyOfList = copyOfList.getValue();
            
          // a, b
          System.out.printf("returnedCopyOfList={\%\s}%n", returnedCopyOfList.toString());
            
          //add할때는 원본 list에 add -> 이게 객체내부의 value에도 영향을 미칠까?
          stringList.add("c");
            
          // a, b  -> 원본을 고쳤는데도, 콜렉션주소가 공유되지 않아, returnedCopyOfList는 불변
          System.out.printf("returnedCopyOfList={\%\s}%n", returnedCopyOfList.toString());
      }
            
      private class UnmodifiableList {
          private List<String> value;
            
          public UnmodifiableList(final List<String> value) {
              this.value = value;
          }
            
          public List<String> getValue() {
              return Collections.unmodifiableList(value);
          }
      }
            
      private class CopyOfList {
          private final List<String> value;
            
          public CopyOfList(final List<String> value) {
              this.value = value;
          }
            
          public List<String> getValue() {
              return List.copyOf(value);
          }
      }
  }
            

(후)

페어 덕분에 copyOf에 대해 알게됐는데요, unmodifiable을 이용해서 반환하는 것만으로도 충분히 값 변환에 있어서 안정성을 보장할 수 있을 것 같은데 앨런은 Java 11에서 왜 Collections를 새로 생성해서 반환하는 copyOf를 만들었다고 생각하시나요?

  copyOf도 unmodifiableList를 반환하지 않던가요? 😅 List.of()와 List.copyOf()에 어떤 차이가 있을까요?
    
  둘다 불변을 보장하지만, 매개변수 받아주는 것 차이
  - List.of 와 List.copyOf는 둘다 unmodifiableList를 반환하는 것은 같지만 전자는 리스트의 요소가되는 여러 매개변수를 받고 후자는 존재하는 리스트를 받아옵니다!
    
  copyOf를 사용하면 새로운 리스트를 만들어 unmodifiable로 반환
  -> 도메인에서 직접 unmodifiable을 사용하여 반환하는 것보다도 더 안전하게 리스트를 사용
    
  코멘트 감사합니다! �업무에서 List.of()를 사용하고 있었는데, 조사해준덕분에 알게됐군요! copyOf를 활용

for vs stream

횟수가 적을 땐 for -> 그 자체를 여러번 돌릴 땐 stream -> 스레드 세이프라면 .parallel()도 선언해서..하면 더 빠르다.

stream으로 전체 중 일부를 뽑는 랜덤을 구현

전체list -> shuffle -> stream + .limit()+ sorted() + 캐슁getInstance + tolist or .limit()대신 list상태에서 .subList()

  private static List<LottoNumber> getAutoLottoNumbers() {
      List<Integer> lottoNumbersForNewLotto = new ArrayList<>(lottoNumbers);
      Collections.shuffle(lottoNumbersForNewLotto);
    
      return lottoNumbersForNewLotto.stream()
          .limit(LOTTO_NUMBERS_SIZE)
          .sorted()
          .map(LottoNumber::getInstance)
          .collect(toList());
  }