Conversation
mintjordy
left a comment
mintjordy
left a comment
There was a problem hiding this comment.
요구사항대로 구현하느라 고생많았어요 😏
메서드, 클래스명, 매직넘버 사용 등을 위주로 간략한 피드백 남겼으니 확인후 수정해주세용~
궁금한 점은 따로 질문 주세요~
README.md
Outdated
| @@ -0,0 +1,3 @@ | |||
| # 기능 목록 | |||
|
|
|||
| <ol></ol> No newline at end of file | |||
| public static int computer = (int) (Math.random() * (100 - 1) + 111); | ||
| public static String msg = ""; | ||
| public static Scanner scanner = new Scanner(System.in); | ||
| public static Strike strike = new Strike(); | ||
| public static int zone = 0; | ||
| public static int coin = 0; | ||
|
|
There was a problem hiding this comment.
static 변수는 스레드 사이에서도 공유할 수 있는 클래스 변수이기 때문에 외부의 값 변경에 취약합니다.
어떤 경우에 static 변수를 사용하는게 좋을까요?
저같은 경우엔 클래스 변수를 클래스에서 사용하는 상수 (불변의 값)를 선언할때 사용합니다.
아래 포스트를 참조해보면 좋을것 같아요.
| } | ||
|
|
||
| public Boolean verification(int zones) { | ||
|
|
| public String zone(int zones, int computer) { | ||
| System.out.println(computer); | ||
| System.out.println(zones); | ||
| if (!verification(zones)) { | ||
| System.out.println("3자리의 숫자만 입력 가능 합니다."); | ||
| return ""; | ||
| } | ||
| int ball = 0, str = 0; | ||
| for (int i = 0; i < String.valueOf(zones).length(); i++) { | ||
| if (String.valueOf(zones).charAt(i) == String.valueOf(computer).charAt(i)) { | ||
| str++; | ||
| continue; | ||
| } | ||
| for (int j = 0; j < 3; j++) { | ||
| if (String.valueOf(zones).charAt(i) == String.valueOf(computer).charAt(j)) ball++; | ||
| } | ||
| } | ||
| return result(str, ball); | ||
| } |
There was a problem hiding this comment.
zone이라는 메서드명으로는 무슨 일을 하는 메서드인지 예측하기 힘들어요.
무슨일을 하고 무엇을 반환하는지 메서드명을 통해 명시 할수 있는 이름이 있으면 좋을거 같아요.
| System.out.println("3자리의 숫자만 입력 가능 합니다."); | ||
| return ""; | ||
| } | ||
| int ball = 0, str = 0; |
There was a problem hiding this comment.
str처럼 축약된 변수명은 의미를 파악하기 힘들어져요. 가급적이면 메소드, 변수등 이름은 의미 파악하기 쉽도록 축약은 삼가야해요
| System.out.println(zones); | ||
| if (!verification(zones)) { | ||
| System.out.println("3자리의 숫자만 입력 가능 합니다."); | ||
| return ""; |
There was a problem hiding this comment.
해당 메서드를 통해 반환하고자 하는 값은 무엇인가요?
과연 이 메서드에서 공백을 반환하는 것이 필요할까요?
제가 보기에는 잘못된 입력에 대한 예외처리가 필요해 보이는 부분이네요
| import java.util.Scanner; | ||
|
|
||
| class Strike { | ||
| public String zone(int zones, int computer) { |
There was a problem hiding this comment.
zones라는 매개변수명이 의미하는 내용이 무엇인지 파악하기 힘드네요.
마찬가지로 computer라는 매개변수명 또한 보다 명시적인 이름이 필요해보여요
| if (str == 3) { | ||
| System.out.println("3개의 숫자를 모두 맞히셨습니다! 게임 종료"); | ||
| System.out.println("게임을 새로 시작하려면 1, 종료하려면 2를 입력하세요"); | ||
| return "clear"; | ||
| } else if (str == 0 && ball == 0) { | ||
| rst = "nothing"; | ||
| } | ||
| if (str > 0) rst = str + "스트라이크 "; | ||
| if (ball > 0) rst += ball + "볼 "; |
There was a problem hiding this comment.
3, 0 처럼 코드에 직접적으로 값을 선언한 데이터를 매직넘버라고 해요.
매직넘버 대신 상수를 선언해서 사용하는건 어떨까요?
힌트로 상수들은 static final 필드를 통해 선언해요.
| return rst; | ||
| } | ||
|
|
||
| public Boolean verification(int zones) { |
There was a problem hiding this comment.
애매한 매개변수명!
메소드는 동사형으로 사용!
boolean을 반환하는 메소드의 적절한 이름은 어떻게 나와야 할까요?
There was a problem hiding this comment.
그리고 Strike 클래스 내부 메서드들중 접근 제어자 수준을 public 보다 강하게 설정해야할 것들이 보이네요~
|
|
||
| import java.util.Scanner; | ||
|
|
||
| class Strike { |
There was a problem hiding this comment.
이 클래스는 무슨 역할을 하는 클래스일까요?
사용자와 컴퓨터의 숫자를 계산후 결과를 반환 기능을 수행하는 클래스인데 Strike라는 클래스명이 적합할까 의문이 드네요.
클래스명은 해당 클래스의 인스턴스 역할이 명시적으로 드러나도록 지어줘야해요~
2 participants
No description provided.