Conversation
swandevson
left a comment
swandevson
left a comment
There was a problem hiding this comment.
- 메소드명은 항상 동사로 시작해야하며, 카멜 케이스(소문자로 시작, 띄어쓰기 부분에 대문자 작성)여야 합니다
- 객체지향적이지 않은 것 같습니다.
Lotto라는 객체에게 행동을 시키기 보다는 단순히 값을 갖고 있는 것처럼 사용하시는 모습이 보입니다. (로또 번호 생성해서 application에서 처리하는 모습 등) - 전체적으로 변수, 메소드 네이밍이 아쉽습니다. 축약어나 의미가 맞지 않은 부분들은 조금 더 고민하고 작성해주시면 좋을 것 같습니다
- 별다른 이유가 없다면 배열보다는 Collection을 사용하시는게 좋습니다.
https://jamie95.tistory.com/99
해당 글 읽어보시면 많이 도움되실 겁니다!!
|
|
||
| int count = Output.userBuyOut(); | ||
|
|
||
| ArrayList<List<Integer>> numberU = new ArrayList<>(); |
There was a problem hiding this comment.
ArrayList<List<Integer>> 보단 List<List<Integer>>가 더 좋아보입니다
다형성을 위해 서브타입으로 지정하기보다 부모타입으로 지정하는 것이 좋습니다.
예를 들어, 객체를 ArrayList가 아닌 LinkedList로 변경해야한다면 변수 타입과 생성 객체를 모두 변경해야하는 상황이 발생하는 경우가 있습니다.
There was a problem hiding this comment.
추가로, numberU라는 변수명이 무엇을 의미하는지 알기 힘듭니다.
List 객체의 경우 s를 붙여 복수형으로 끝내는 것이 일반적입니다.
| List<Integer> luckyNumber = new ArrayList<>(); | ||
| luckyNumber = Output.luckyNumberOut(); | ||
|
|
||
| int plusNumber = Output.plusNumberOut(luckyNumber); | ||
|
|
||
| int[] win = lotto.Lotto.LottoWinning(numberU, luckyNumber, plusNumber, count); |
| public class Input { | ||
|
|
||
| public static int userBuyIn() { | ||
| Scanner sc1 = new Scanner(System.in); |
There was a problem hiding this comment.
해당 클래스는 static 메소드만을 사용하는데 Scanner또한 static 멤버로 사용하는게 좋지 않을까요?
| if(money % 1000 != 0) { | ||
| UserHelp.errorUserBuy(); | ||
| } | ||
|
|
||
| return money / 1000; |
There was a problem hiding this comment.
로또 단위인 1000원을 하드코딩(할당하지 않고 직접 기재)하지 말고, 상수로 사용하는 것이 좋아보입니다
만약 단위가격이 2천원으로 변경된다면 1000원인 부분을 모두 찾아 변경해야할 뿐더러, 오타의 위험성이 있습니다.
| String str = sc.next(); | ||
|
|
||
| String[] strArr = str.split(","); | ||
| List<String> intList = new ArrayList<>(Arrays.asList(strArr)); |
There was a problem hiding this comment.
List와 같이 자료형을 나타내는 것은 변수 네이밍으로 좋지 않은 방법입니다. 즉, 변수명이 자료형에 종속적이면 안됩니다.
만약 추후에 Set과 같은 자료형으로 변경되었을 때 해당 변수도 모두 변경해야하는 문제점이 생기기때문입니다.
| } | ||
|
|
||
| public static int[] LottoWinning(ArrayList<List<Integer>> numberU, List<Integer> luckyNumber, int plusNumber, int count) { | ||
| int[] win = {0, 0, 0, 0, 0}; |
|
|
||
| public Lotto(List<Integer> numbers) { | ||
| validate(numbers); | ||
| validate(numbers); //String 검증용 |
There was a problem hiding this comment.
string 검증용이라고 작성하셨는데, 실제 파라미터는 숫자로 이루어진 리스트입니다.
주석도 명백한 코드의 일부분이니 고려 후 작성해주시면 좋을 것 같습니다
| avg = sum / ((double)(count * 1000)) * 100; | ||
|
|
||
| avg = Math.round(avg * 10); | ||
|
|
||
| return avg / 10; |
There was a problem hiding this comment.
평균값 계산을 나눠서 하지 않고, 한 줄로 끝내거나 메소드를 분리해서 사용하는 것이 더 좋아보입니다
|
|
||
| for (i = 0;i < count; ++i) { | ||
| List<Integer> numbers = Randoms.pickUniqueNumbersInRange(1, 45, 6); | ||
| numbers.sort(Comparator.naturalOrder()); |
| } | ||
|
|
||
| // TODO: 추가 기능 구현 | ||
| public static ArrayList<List<Integer>> LottoNumber(int count) { |
There was a problem hiding this comment.
번호를 생성하신다면 Lotto가 아닌 LottoGenerator 혹은 LottoFactory처럼 Lotto에 관한 객체 생성에 도움을 주는 클래스를 따로 작성하시는 것이 좋을 것 같습니다
2 participants
완료했습니다. 자바가 처음이라 잘 풀었는지는 모르겠습니다