[LBP] 권지후 로또 미션 1단계 제출합니다. #82
Conversation
src/main/java/model/Lotto.java
Outdated
| .toList(); | ||
| } | ||
|
|
||
| private static List<Integer> getTempLottoNumbers() { |
There was a problem hiding this comment.
getTempLottoNumbers()메서드는 static으로 작성한 이유에 대해서 지후의 생각이 궁금해요
이유가 있어보이지만 지후의 생각을 들어보고 싶어요~
There was a problem hiding this comment.
이 리뷰를 보고 든 첫번째 생각은
제가 static의 사용법을 제대로 모르고 차용해왔다는 생각이 먼저 드는 것 같습니다
반성의 의미로 static에 대한 고찰을 해봅니다...
본인이 기존의 알고 있었던 static :
- 불필요한 객체 생성을 막는다
- 여기서 객체 생성이 필요한 기준은 객체마다 다른 값을 유지하는 경우,
- 상태 변경 메서드를 가져야 하는 경우
"특정 상태(멤버 변수)를 가지지 않는 경우엔 static을 써서 불필요한 객체 생성을 막는다." 까지밖에 알지 못했습니다.
솔직히 저 코드에서도 static을 쓴 저만의 이유가 없었던 것 같습니다.. 😢
공부해본 static :
static을 사용하려면 그 사용 목적이 명확해야 하고 특정 상태를 가지지 않거나, 여러 객체 간에 상태를 공유해야 할 때 적절히 활용하는 것이 좋다고 느꼈습니다. 그 이유는
static은 명시적으로 생성 시점을 제어할 수 없고, 메모리 해제도 개발자가 직접 할 수 없음thread-safe하지 않음static메서드는 오버라이딩 불가 (다형성 x)static은 객체 지향이 아닌 절차 지향 특성을 띄움
저도 이 과정에서 static의 사용에 대해 다시 한 번 되새기게 되었고, 향후에는 static을 남발하지 않게
좀 더 신중하게 사용하겠습니다.
결론 :
도메인 객체의 상태에 접근하지 않는 선에서, 재사용성이 높은 경우에
static을 활용하는 접근으로 나아가보겠습니다 !
(+)
개인적으로 페어 진행하면서 요구사항의 실행결과값과 똑같이 나오면 구현완료했다! 라는 잘못된 습관을
아직도 못 고친것 같습니다. 이 부분을 반성합니다.
더 세세히 보고 성급하게 완료됐다는 판단을 고쳐나가보도록 하겠습니다.
날카로운 피드백 정말 감사드립니다 🙇♀️🙇♀️
혹시 이해가 잘못되었으면 답변 부탁드립니다 감사합니다 !
There was a problem hiding this comment.
반성하고 고찰할 필요는 전혀 없어요ㅎㅎ
그러지말고 생각해본적이 없네? 이번 기회에 생각해보자~ 하고 새로운 기회가 생겼네~ 하고 생각해주셨음 좋겠어요
전혀 혼내거나 그럴 의도나 그럴 계획이 없어요ㅠㅠ
잘 고민해보고 유리한 조건을 채택할 수 있게 되가고 있는 것 같아서 부럽기도하고 칭찬해드리고 싶어요
| private List<Integer> generateTempLottoNumbers() { | ||
| List<Integer> tempLottoNumbers = getTempLottoNumbers(); | ||
|
|
||
| Collections.shuffle(tempLottoNumbers); | ||
|
|
||
| return tempLottoNumbers.stream() | ||
| .limit(LOTTO_NUMBERS_SIZE) | ||
| .toList(); | ||
| } |
src/main/java/view/InputView.java
Outdated
|
|
||
| public class InputView { | ||
|
|
||
| private static final Scanner scanner = new Scanner(System.in); |
There was a problem hiding this comment.
Scanner를 static으로 선언하신 지후의 의도가 궁금해요~!
view를 static메서드로 선언하여 입출력을 담당하는 함수로서의 동작을 의도한 것 같아요👍
그에 따라서 Scanner는 static으로서 선언되어야 동작이 가능하겠지요
Scanner가 전역적인 상태를 공유하면 따라오는 단점도 있습니다 (공부해봅시다 ㅎㅎ)
프로그램의 구조와 요구사항에 맞추어서 적절한 선택이 필요하겠지요
지후는 고민하고 공부해봤을때 어떻게 선택할지 궁금하기도 하구요!
There was a problem hiding this comment.
보통 Scanner 사용할 때 static final로 선언하는 버릇 때문에
이 부분 또한 static에 대한 저의 견문이 좁아서 쓴 것 같습니다ㅜㅜ 😢
Scanner를 static으로 선언하면 ?
"static을 사용하면 여러 클래스에서 Scanner를 공유하게 된다."
즉 입력 스트림이 동시 접근될때 문제가 생길 수 있습니다.
scanner는 입력을 받을 때만 필요한 객체인데 static으로 남용하게 된다면
입력을 받지 않는 클래스에서도 scanner가 공유되어서 생명주기 관리가
제대로 되지 않는 설계라는 것을 알게되었습니다.
public class InputView {
private final Scanner scanner = new Scanner(System.in);
public String purchaseAmountTitle() {
System.out.println("구입금액을 입력해 주세요.\n");
String input = scanner.nextLine();
scanner.close();
return input;
}
}static을 제거하고, Controller에서 InputView 객체를 생성한 뒤
purchaseAmountTitle()을 호출하도록 리펙토링 하였습니다. 감사합니다 !
src/main/java/view/ResultView.java
Outdated
| public static void printLottoResult(List<Lotto> tickets) { | ||
| System.out.println(tickets.size() + "개를 구매했습니다."); | ||
| for (Lotto ticket : tickets) { | ||
| System.out.println(ticket.getSortedLottoNumbers()); | ||
| } | ||
| } |
There was a problem hiding this comment.
-
printLottoResult()에서는 List를 받아오고 있어요! (추가로Lotto.getSortedLottoNumbers()를 호출도 하고 있네요) 결합도 ⬆️
이는 결국view가model을 알고 있다는 점이 중요해요, 알고 있다는 건 결국 의존하고 있다는 뜻이에요
model이 변경되면view의 수정은 필연적으로 발생하게 됩니다 이는 결국 MVC의 역할 분리를 위배하고 있어요
고민해보고 개선해봅시다! -
Lotto.getSortedLottoNumbers()를 호출 이것이 정말 맹점이에요! - 결합도⬆️
현재printLottoResult()는static으로 관리되고 있는 정적 메서드에요, 이 정적 메서드는ticket.getSortedLottoNumbers()를 통해서 인스턴스 변수의 상태를 변경하고 있어요.
이는 정말 위험한 설계가 될 수 있어요
- OCP, SRP
객체의 상태는 객체 내부에서 관리되어야 하는것이 객체지향 프로그래밍의 원칙중 하나이죠
정적 메서드가 특정 객체의 상태를 변경하면 객체의 역할이 불분명해질 수 있어요 즉 객체의 데이터를 변경하는 책임이 객체 자신에게 있어야 하는데, 외부의 정적 메서드가 이를 조작하면 응집도가 낮아지는 설계가 됩니다 - 동시성 문제
여러 스레드가 동일한 정적 메서드를 호출하여 인스턴스 변수의 상태를 변경하면, 예측할 수 없는 동작이 발생할 수 있는 가능성이 있어요
public class SharedResource {
private static int count = 0; // 공유 자원
public static void increment() {
count++; // 동시 접근 시 문제 발생 가능
}
}
여러 스레드가 increment()를 호출하면 count의 값이 정상적인 증가의 동작을 하지 못하게 되겠죠?
- 테스트가 용이성⬇️, 유지보수성⬇️
정적 메서드가 객체의 상태를 변경하게 되면 객체의 예상 동작을 테스트하기 어려운 설계가 됩니다
예를 들자면 단위 테스트에서는 객체를 초기 상태로 유지하려면 매번new Lotto()를 생성하게 될 거에요
위와 같은 이유로 해당 부분 리팩토링 해봅시다 ( + 객체의 결합도는 낮추고 응집도를 높혀봅시다)
저도 위 부분에 대해서 정말 많은 고민의 시간을 보냈던 기억이 있습니다 지후만의 정답을 찾아가는 과정인 만큼 진득하게 고민해보고 최대한 많은 부분 얻어갔으면 좋겠어요
고민해보고 개선해봅시다!
There was a problem hiding this comment.
MVC 각 계층에 대한 책임을 더 명확히 인식한 후에
코드에 적용시켜보도록 더 노력하겠습니다
친절하게 설명해주신 덕분에 view 계층에 대한 책임이 명확해졌습니다
말씀해주신대로 현재 저 코드에서 도메인 객체를 접근하는 printLottoResult메서드가 static이면
외부에서 변경 가능한 위험한 설계라는 것을 알게되었습니다... 😢
또 view가 model에 의존하게 되면 이는 mvc패턴에 위배한다는 것도
정적 메서드가 객체의 내부 상태를 변경하면, 객체가 담당해야 할 책임이 외부로 나가게 되고
OCP(Open/Closed Principle) 또한 위반한다는 것도 다시금 명심하겠습니다.
말씀해주신 리뷰 토대로 전체적으로 코드 리펙토링 진행하였습니다! 확인해주시면 너무 너무 감사하겠습니다
public void printLottoResult(List<String> tickets) {
System.out.println(tickets.size() + "개를 구매했습니다.");
for (String ticket : tickets) {
System.out.println(ticket);
}
}(+)
객체의 상태를 관리하는 책임이 객체 자신에게 있어야 한다
view는 model을 몰라야 한다.
Model은 Controller와 View에 의존하지 않아야 한다.
이 기준을 바탕으로 코드를 수정하였는데, 혹시 어긋나는 부분이 있으면 답변 주시면 감사하겠습니다!
앞으로 mvc 패턴을 사용하여 구현할 때 각 계층의 책임이 무엇인 지를 꼭 머릿속에 집어넣도록 하겠습니다.
또 OCP에 대한 공부도 해보겠습니다
자세하고 친절한 코드리뷰 덕분에 또 배워갑니다 너무너무 감사드립니다 🙏
There was a problem hiding this comment.
귀찮기도 하고 고민하는 동안 힘들었을텐데 잘 해주셨네요 좋아요 좋습니다! 굿굿
src/main/java/model/Lotto.java
Outdated
|
|
||
| private static List<Integer> getTempLottoNumbers() { | ||
| List<Integer> tempLottoNumbers = new ArrayList<>(); | ||
| for(int i = 1; i <= 45; i++) { |
There was a problem hiding this comment.
- 45는 어떤 동작을 위한 정수?
- i는 어떤 역할을 담당하고 있나요?
각자 어떤 역할을 수행하는지 명확해지면 좋을거 같아요
There was a problem hiding this comment.
페어 프로그래밍 진행했을 때 for문에 대한 상수화는 굳이 필요없다고 판단하였는데,
곰곰히 생각 해보니 상수화 시키는 게 코드를 보았을 때 이해가 더 빠를 것 같다는 생각에
리팩토링 진행하였습니다 감사합니다!
private List<Integer> getTempLottoNumbers() {
List<Integer> tempLottoNumbers = new ArrayList<>();
for (int number = LOTTO_MINIMUM_NUMBER; number <= LOTTO_MAXIMUM_NUMBER; number++) {
tempLottoNumbers.add(number);
}
return tempLottoNumbers;
}Q :
보통 i라고 통용되게 쓰이는 반복문 인덱스 문자도 상수화를 시켜주는 지 궁금합니다..!
There was a problem hiding this comment.
반복문 인덱스 까지 상수화를 시켜보자 까지는 아니였는데
이렇게 하니 코드가 더 잘 읽히는 느낌이에요, 저도 배워가네요 ㅎㅎ
| public void lottoRun() { | ||
| int purchaseAmount = InputView.purchaseAmountTitle(); | ||
|
|
||
| int lottoCount = purchaseAmount / TICKET_PRICE; |
There was a problem hiding this comment.
넵 ! 아래와 같은 예외처리 리펙토링 진행하였습니다
감사합니다
예외 처리 리팩토링
- 구매 금액이 1000원으로 나누어 떨어지지 않을 경우
- 구매 금액이 양수가 아닌 수일 경우
- 구매 금액이 숫자가 아닌 경우
private int parsePurchaseAmount(String input) {
try {
return Integer.parseInt(String.valueOf(input));
} catch (NumberFormatException e) {
throw new IllegalArgumentException("구매 금액은 숫자여야 합니다.");
}
}
private void validatePurchaseAmount(int purchaseAmount) {
if (purchaseAmount <= 0) {
throw new IllegalArgumentException("구매 금액은 양수여야 합니다.");
}
if (purchaseAmount % TICKET_PRICE != 0) {
throw new IllegalArgumentException("구매 금액은 1000원 단위로 입력되어야 합니다.");
}
}
There was a problem hiding this comment.
리팩토링 고생 많으셨어요 지후!
전반적으로 코드의 완성도가 좋아보여요
MVC나 SRP를 많이 고민한 것 같아서 코드가 잘 읽혔던 것 같아요!
추가로 좀 더 자신감을 가지고 부족하다 이런 생각 안했으면 좋겠어요!
매번 저희가 옳은게 아니고 지후의 생각이 더 적절할 수도 있는데 지후가 너무 위축되어있으면 지후의 성장기회를 지후가 잃어버리는 게 되어요,
위축되지 말고 자신있게 생각하고 질문해줬으면 좋겠어요!
지후의 성장을 위해서라도!
코멘트 남겨두었으니 확인부탁해요~~
| private int parsePurchaseAmount(String input) { | ||
| try { | ||
| return Integer.parseInt(String.valueOf(input)); | ||
| } catch (NumberFormatException e) { | ||
| throw new IllegalArgumentException("구매 금액은 숫자여야 합니다."); | ||
| } | ||
| } | ||
|
|
||
| private void validatePurchaseAmount(int purchaseAmount) { | ||
| if (purchaseAmount <= 0) { | ||
| throw new IllegalArgumentException("구매 금액은 양수여야 합니다."); | ||
| } | ||
| if (purchaseAmount % TICKET_PRICE != 0) { | ||
| throw new IllegalArgumentException("구매 금액은 1000원 단위로 입력되어야 합니다."); | ||
| } | ||
| } |
There was a problem hiding this comment.
입력값 검증에 대한 위치는 어디가 제일 어울릴까요?
저는 정말 많이 고민했었기도 하고 사람마다 다들 각각 의견이 다른 부분이에요
지후가 생각하기에 입력값에 대한 검증은 어디서 이루어지는게 가장 적절할 것 같나요?
There was a problem hiding this comment.
처음에는 입력값을 검증하는 부분이 input 값을 받는 view 레이어에서 이루어지는 것이 좋다고 생각했는데
단순 UI를 제공하는 view에서 입력값 검증은 아닌 것 같다는 생각이 들게되었고
게임의 흐름을 관리하는 controller에서 입력값을 검증하는 것이 더 맞다고 생각해서,
현재 제 코드에서는 입력값 검증의 책임을 controller로 이양했습니다.
음 .. 지금 제가 생각하기엔 controlller에서 입력값 검증이 적절하다고 생각이 되는데
Q :
혹시 리뷰어님께서는 입력값 검증을 어느 레이어에서 수행하는 것이 적절하다고 생각하시나요..? 궁금합니다 !
| List<Lotto> lottoTickets = generateLottoTickets(lottoCount); | ||
| List<String> lottoTicketList = getLottoTicketStrings(lottoTickets); |
There was a problem hiding this comment.
얼핏보기에 컬렉션의 자료형만 다른데
| List<Lotto> lottoTickets = generateLottoTickets(lottoCount); | |
| List<String> lottoTicketList = getLottoTicketStrings(lottoTickets); | |
| List<Lotto> lottoTicketList = generateLottoTickets(lottoCount); | |
| List<String> lottoTickets = getLottoTicketStrings(lottoTickets); | |
변수명이 이렇게 바뀌어도 이상하지 않을 것 같아요
더 의미있게 네이밍을 수정해봅시다
There was a problem hiding this comment.
이번 스터디 통해서 네이밍 스킬을 더 길러보겠습니다 !
말씀하신대로의 리펙토링을 진행하였습니다 감사합니다 !
src/main/java/model/Lotto.java
Outdated
| @Override | ||
| public String toString() { | ||
| List<Integer> sortedLottoNumbers = getSortedLottoNumbers(); | ||
| return sortedLottoNumbers.toString(); | ||
| } |
There was a problem hiding this comment.
Object객체 오버라이딩이 인상깊어요
디버깅과 로깅에 있어서 추적에 용의한 장점도 있어요
하지만 "toString()이라는 메소드명에 부합한 기능을 하고 있는가?" 를 생각해보면 좋을 것 같아요
현재 toString()은 출력 형식 결정이라는 책임이 추가되어 있어요 - SRP
또한 출력 형식이 변경된다면 서비스 전체에 영향을 미칠 가능성을 가지고 있어요
가장 중요한건 toString()을 호출할때마다 정렬이 수행된다는 점도 있어보이구요
지후는 어떻게 생각해요?
There was a problem hiding this comment.
곰곰히 생각해보니 toString()는 이미 객체의 문자열 표현을
단순히 반환하는 역할이 정의되어 있는 메서드이기 때문에
toString() 오버라이딩 메서드 대신 로또 번호의 정렬 및 출력 형식 결정은
toStringLottoTickets 메서드로 정의해서 리펙토링 진행하였습니다 감사합니다 !
public String toStringLottoTickets() {
List<Integer> sortedLottoNumbers = getSortedLottoNumbers();
return sortedLottoNumbers.toString();
}
리뷰어 : 김우진님
리뷰이 : 권지후
페어 : 김태우님
<1단계를 구현하며 느낀 점>
로또 1단계 과제 제출합니다 !
저번주에
MVC과제를 해봐서이번 과제 또한
MVC로 나눠서 미션 진행하였습니다.로또 1단계 구현하면서 느낀 점은
역시 네이밍과 계층 별 책임에 대해서 아직도 조금 미숙하다고 느꼈습니다
구현하면서도 특히 네이밍,
mvc에 대해 페어랑 이게 맞나..? 라는 물음표를 갖고 계속 진행했습니다계층 별 책임이나 네이밍에 대해서 잘 봐주시면 감사하겠습니다
미숙한 코드이지만 잘부탁드립니다.....
🎰 로또 자동 생성 미션
Lotto객체를 활용하여 로또 게임을 진행하고, 입력과 출력을 조율 (controller)❓ 질문
Q1.
요구사항대로 메서드 10줄을 넘기지 않는 한에서 SRP를 지키고자 했는데
혹시 SRP를 제대로 준수했는지 궁금합니다
Q2.
저번 과제로 마찬가지로
현재의 클래스들이
MVC의 역할을 올바르게 수행하고 있는지 궁금합니다.구조적으로 개선할 점이 있을까요??
Q3.
MVC에서 컨트롤러의 책임이 아직 헷갈립니다..
admin이라고 이해하였는데 제 코드에서는 컨트롤러에서Lotto객체를 직접 생성하는 게컨트롤러의 책임이 맞는 지가 헷갈리는 것 같습니다.
리뷰어님은
mvc의 각 계층별 책임을 어떻게 이해하고 계신지 궁금합니다..감사합니다!