-
Notifications
You must be signed in to change notification settings - Fork 21
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주차]객체지향 코드 연습(usernamejunhyuk) #28
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
개인적으로 GPT 사용한 부분 다 지우시고 직접 모자라도 짜셔주시길 바랍니다. 어느정도 자신의 코드로 완성한 다음 GPT를 쓰는 것으로 뭐라하지 않습니다. 저도 GPT에서 자주 물어보고, 조언을 구하는 편이니까요. 그렇지만 적어도 과제이고, 공부를 목적으로 하신다면 // Step x, 주석 등 GPT가 짜준 코드를 그냥 복사 붙여넣기하는건 예의가 아니라고 생각합니다. 전혀 다른 이야기란 말입니다.
결정은 준혁님 마음이지만 사실대로 말한다고 모든 것이 올바른 선택은 아닙니다.
GPT를 쓰셨다면 해당 코드에 대해 하나하나 직접 대면에서 설명을 요구드리겠습니다.
앞으로 GPT 쓰실 생각이시라면 저는 그 부분에 대해 리뷰하지 않을 생각입니다.
그렇다고 멘토로써 질문이나 어려움을 묵인할 생각은 없습니다. 다른 동기분들은 모르는 것을 질문하고 새로 배워가며 성장하십니다. 준혁님도 할 수 있다고 믿습니다. 응원합니다.
감사합니다.
src/main/java/lotto/LottoRank.java
Outdated
@@ -0,0 +1,47 @@ | |||
package lotto; | |||
|
|||
public enum LottoRank { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Enum을 사용해서 상태를 관리하는 것도 GPT가 알아서 해준건가요? 아니면 직접 명령하신건가요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
예전 COW 분들이 PR 올리신거 보니까 Enum 사용하라는 피드백도 있고 또 이번 과제 요구 사항에 Enum을 활용하라고 하셔서 제가 작성했습니다!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이건 너무너무 잘하셨어요! 이대로 쭉 스스로 해보시면 분명 실력이 많이 늘겁니다.!!
return LottoRank.valueOf(matchCount, matchBonus); | ||
} | ||
|
||
public static double calculateYield(List<LottoRank> results, int purchaseAmount) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
stream 사용법을 모르고 GPT를 쓰셨다면 직접 다시 공부해보셔야합니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
네 알겠습니다 다시 공부하고 작성하겠습니다!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
먼저 혼자 코드를 작성하면서 고민하는 시간이 정말 중요합니다..
지피티를 아예 사용을 하지 말라는 말은 하지 않습니다. 근데 공부의 목적인 과제 코드를 짜달라고 하는 건 정말 도움이 되질 않습니다. 오류를 마주했을 때 해결하는 목적으로 사용하는 것은 괜찮습니다. 하지만, 무에서 유를 만드는 역할을 지피티가 해버린다면, 정말.. 의미없는 짓이라고 할 수 있습니다.
그리고 남의 코드를 보는 것도 좋습니다. 제가 추천하는 방법이고요. 하지만, 혼자 고민을 해보는 시간을 가지는 것이 정말 중요합니다. 많은 시간을 고민하고 최선을 다해서 코드를 작성했는데, 남은 어떻게 짰는지 보면, 정말 이렇게도 짤 수 있구나라는 생각이 들면서 와닿기 때문입니다!
조금 부족해도 괜찮으니 혼자 코드를 작성해보길 바랍니다! 그리고 오류가 난다면 조금은 고민해보는 시간을 가진 후에 멘토에게 질문 해주는 것도 좋습니다! 화이팅입니다!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
일단 application에 너무 많은 코드가 있습니다. 그로인해서 static 메서드가 너무 많아요.
이러면 유지보수나 모듈화가 안됩니다. 객체 지향의 장점중 하나인 모듈화가 안되는 게 너무 아쉽고 그로인해서 추상화, 다형성도 안되요.
그리고 데이터를 책임지는 객체가 없는 게 아쉬워요. 지금 코드는 만들고 방출, 판단하고 방출. 클래스 코드내에서만 존재해서 이게 정말 요구사항만 충족하는 느낌이여서 그 부분이 아쉬워요.
객체를 만들고 객체끼리의 메시지를 통한 상호작용으로 프로그램이 흘렀으면 좋을 것 같아요.
진짜 기능별로만 나뉜 느낌이여서 전체적으로 보기는 좋았습니다. 한번 디벨롭 해보시죠~!
커스텀 예외처리는 나중에 하구 객체 분리 먼저 해주세요.
- 입출력과 비즈니스 로직 분리
- 데이터를 책임지는 객체가 있으면 좋을 것 같다.
- application 클래스 책임 확 줄이기. (애초에 얘는 프로그램을 시작하는 부분만을 나타내는 겁니다.)
- 객체 분리
- 매직넘버, 상수 처리 다시 하기
src/main/java/lotto/Application.java
Outdated
String inputPurchaseAmount = "구입금액을 입력해 주세요."; | ||
System.out.println(inputPurchaseAmount); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
매직넘버, 상수에 대한 이해가 아직 안되신 것 같아요.
이러한 문자열은 소스코드내에 있는 것은 유지보수성에 좋지 않습니다. 외부 파일에다가 저장하는 것이 아니더라도 상수 클래스에 두는 방식은 어떨까요.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
따로 상수 패키지 만들어서 빼겠습니다
src/main/java/lotto/Lotto.java
Outdated
public static Lotto generate() { | ||
List<Integer> numbers = Randoms.pickUniqueNumbersInRange(minimumNumber, maximumNumber, countNumber); | ||
Collections.sort(numbers); | ||
return new Lotto(numbers); | ||
} | ||
|
||
public Lotto(List<Integer> numbers) { | ||
validate(numbers); | ||
validateNumber(numbers); | ||
validateRedundancy(numbers); | ||
validateRange(numbers); | ||
this.numbers = numbers; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
혹시 이거 어떤 의미로 이렇게 하신건지 여쭤봐도 될까요?
노리고 하신건지 궁금해서 그렇습니다!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
위에꺼는 오름차순으로 정렬하는 법을 찾아보니까 Collections의 sort 메소드라는게 있다고 해서 썼고
아래꺼는 객체지향이라는걸 아직 잘 이해 못해서 로또 숫자를 검증할 때 번호 개수 -> 번호 중복 유무 -> 지정된 범위 내에 위치 하는지 절차적으로 검증해야 겠다고 생각해서 저렇게 작성 했습니다
src/main/java/lotto/Application.java
Outdated
new Lotto(winningNumbers); | ||
return winningNumbers; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이 부분에서 Lotto 객체는 왜 만드신건가요?? 예외 체크를 하기위한거라면 이 객체를 반환하는 건 어땠을까요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
입력받은 당첨 번호가 유효한 번호인지 검증하기 위해서 Lotto 객체를 만들었습니다!
Lotto 객체를 반환하는 형식으로 한번 다시 작성해보겠습니다..
src/main/java/lotto/LottoRank.java
Outdated
public static LottoRank valueOf(int matchCount, boolean matchBonus) { | ||
if (matchCount == 6) { | ||
return FIRST; | ||
} | ||
if (matchCount == 5 && matchBonus) { | ||
return SECOND; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
보너스 번호 아이디어를 이렇게 하셨군요. 좋은 것 같아요
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
감사합니다
src/main/java/lotto/Application.java
Outdated
int[] rankCount = new int[LottoRank.values().length]; | ||
for (LottoRank rank : results) { | ||
rankCount[rank.ordinal()]++; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
당첨 횟수를 세는 방법이 좋네용.
enum의 ordinal()도 좋지만, 유지보수면에서 순서를 넣는 것을 권장한다 하더라구요.
https://sedangdang.tistory.com/242
또 enum이여서 저도 저렇게 한적이 많은 것 같지만 values().length 보다 메소드를 만들어서 저게 어떤 것을 의미하는 지, 순위의 총 개수를 반환한다 라는 메소드 명이면 더 좋을 것 같아요.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
수정하긴 해봤는데 이게 맞는 방향인지는 잘 모르겠네요...
int totalPrize = results.stream() | ||
.mapToInt(LottoRank::getPrizeMoney) | ||
.sum(); | ||
int percentConversion = 100; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
매직넘버 처리가 빠졌네요. 특정 클래스에서 상수처리를 한다면 필드에서 해주세요.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
수정했습니다
src/main/java/lotto/Application.java
Outdated
} catch (IllegalArgumentException e) { | ||
System.out.println(e.getMessage()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
혹시 커스텀 예외처리도 맛 보는 것도 추천드립니다! 후에 웹 서비스할 때 http 프로토콜로 통신을 하는 데 그때도 상황에 맞는 응답값을 줘야해요. 예행 연습으로 한번 맛보세요.
https://veneas.tistory.com/entry/Java-%EC%BB%A4%EC%8A%A4%ED%85%80-%EC%98%88%EC%99%B8-%EB%A7%8C%EB%93%A4%EA%B8%B0Custom-Exception#google_vignette
src/main/java/lotto/Application.java
Outdated
List<Lotto> purchasedLotto = generateLotto(purchaseAmount); | ||
printPurchasedLotto(purchasedLotto); | ||
List<Integer> winningNumbers = inputWinningNumbers(); | ||
int bonusNumber = inputBonusNumber(); | ||
|
||
List<LottoRank> results = calculateResults(purchasedLotto, winningNumbers, bonusNumber); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이렇게 List 자료형을 너무 좋아하십니다.
혹시 일급 컬렉션이라고 아시나요?
그리고 이렇게하니까 Application class 내부에서 하는 행위가 많아지는 것 같아요.
한번 이 부분을 어떻게 바꿀 수 있을까 고민해보세요.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
객체 분리가 가장 중요한 것 같습니다.
고민을 정말 많이해주시고, 책임을 맡길 어떤 객체가 있을 지, 객체를 많이 생성해주세요
- 객체 분리
- 객체 분리
- 객체 분리
public class Application { | ||
|
||
private static final int pricePerLotto = 1000; | ||
|
||
public static void main(String[] args) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
main 클래스는 어플리케이션을 실행하는 용도로만 사용하는 것이 좋습니다.
비즈니스 로직과 관련된 코드를 전부 분리해주세요
|
||
import java.util.ArrayList; | ||
import java.util.List; | ||
|
||
public class Application { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
�책임을 전부 분리해주세요. 객체 분리 고민이 너무 부족한 것 같습니다.
src/main/java/lotto/Lotto.java
Outdated
private final static int minimumNumber = 1; | ||
private final static int maximumNumber = 45; | ||
private final static int countNumber = 6; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
로또 요구사항에 필드 생성을 하면 안된다는 말이 있습니다! 요구사항을 지켜주세요
private final static int maximumNumber = 45; | ||
private final static int countNumber = 6; | ||
|
||
public static Lotto generate() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
모든 메소드는 생성자 아래에 위치 시켜야 합니다.
private final static int maximumNumber = 45; | ||
private final static int countNumber = 6; | ||
|
||
public static Lotto generate() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lotto 팩토리 메소드를 사용한 것이 정말 좋은 것 같습니다.
왜 팩토리 메소드를 사용하면 좋을까? 한번 찾아보시길 바랍니다.
src/main/java/lotto/LottoRank.java
Outdated
return resultMessage; | ||
} | ||
|
||
public static LottoRank valueOf(int matchCount, boolean matchBonus) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
stream or 반복문으로 바꿔도 좋을 것 같습니다.
LottoResultView에서 static 선언 삭제 수정 해야될 점 남겨둠
과제를 완벽하게 하지 못했습니다.
함수를 최대한 작게 만드려고 노력했는데 클래스 구분을 잘 못한 것 같고 매직넘버도 너무 많은 것 같고 많이 부족한 거 같습니다.
솔직히 GPT 많이 썼고 인터넷에 올라와 있는거나 이전 COW 사람들이 올린 PR 보면서 해본다고 해봤는데 많이 부족한거 같네요 앞으로 공부 더 열심히 하겠습니다.
JDK 버전도 11로 맞춰두고 했는데 제 컴퓨터에선 작동엔 문제가 없는데 계속해서
WARNING: An illegal reflective access operation has occurred
WARNING: Illegal reflective access by camp.nextstep.edu.missionutils.Console (file:/C:/Users/lc011/.gradle/caches/modules-2/files-2.1/com.github.woowacourse-projects/mission-utils/1.0.0/dad5230ec970560465a42a1cade24166e6a424f4/mission-utils-1.0.0.jar) to field java.util.Scanner.sourceClosed
WARNING: Please consider reporting this to the maintainers of camp.nextstep.edu.missionutils.Console
WARNING: Use --illegal-access=warn to enable warnings of further illegal reflective access operations
WARNING: All illegal access operations will be denied in a future release
이런 오류가 뜨네요 찾아보니까 자바 9이상에서 발생하는 스캐너 관련 오류 인거 같은데 실행에 문제 없으면 무시해도 된다고 해서 일단 제출해봅니다.
열심히 하겠습니다.