Skip to content
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

[202101947 김민지] 1주차 미션을 제출합니다. #3

Open
wants to merge 46 commits into
base: main
Choose a base branch
from

Conversation

alsswl
Copy link

@alsswl alsswl commented Feb 17, 2024

멋쟁이 사자처럼 백엔드 12기 김민지 미션 제출합니다.

Copy link
Contributor

@shin-mallang shin-mallang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

안녕하세요 민지님~
멋사에서 뵙네요 ㅎㅎ:)

1단계 미션 너무 잘 구현해 주셨어요 ~ 💯
커밋 단위도 정말 잘 나누어 주셨네요 💯 💯
이번 미션에서는 이정도만 작성해도 충분히 잘 작성한 코드지만, 잘 작성해 주셨으니 조금만 더 귀찮게 해보겠습니다..ㅎㅎ

지금은 하나의 Application 코드에 모든 로직을 작성해 주셨어요!
이번 미션은 간단하기 떄문에 이렇게 작성해도 문제는 없지만,
구현해야하는 기능이 복잡해 질 수록 이렇게 하나의 클래스에 모든 기능을 구현하게 되면 유지보수에 어려움을 겪을 수 있습니다~
클래스를 조금 나누어 책임을 분리하여, 가독성과 유지보수성을 올릴 수 있게 코드를 변경한 뒤 저에게 말씀해주시면 추가 피드백 드릴게요!

또한 코드에 주석을 조금 많이 작성해주신 것 같아요!
주석을 달아주셔서 코드를 이해하는데 많은 도움이 되었지만, 주석 역시도 유지보수의 대상이므로 너무 주석을 사용하게 되면 나중에 힘들어질 수 있어요~

대부분의 주석을 지우고, 메서드를 분리하여 행위를 좀 더 명확히, 주석 없이도 이해할 수 있도록 코드를 변경해 주시면 감사하겠습니다!

리뷰 반영 후 제게 따로 말씀해주시면 추가 피드백 드리겠습니다!

추가로 컨벤션 문서를 확인하고 잘 지켜주세요 :)

@@ -1,7 +1,142 @@
package baseball;

import java.io.*;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

필요없는 Import 같아요~
save on actions는 적용하셨나요?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

원래 import는 나중에 귀찮지 않게 기본적으로 저렇게 하던 습관이 있어서 해뒀던 것 같아요ㅠㅠ save on action 적용하도록 하겠습니다!
혹시 필요없는 import를 하면 가독성이 안좋아진다는 점 이외에 다른 안좋은점이 있나요??

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

사실 가독성보단 컴파일 시 살짝 느려졌다..? 뭐 그런 문제 때문에 불호하는 사람들도 많더라구요~
근데 인텔리제이 간단히만 설정하면 딱히 * 안쓰시더라도 문제 없으실거예요~

while (randomNum.size() != 3) {
int ranNum = Randoms.pickNumberInRange(1, 9);
if (!randomNum.contains(ranNum)) {
// System.out.println(ranNum);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이러한 주석은 코드의 유지보수성을 떨어뜨리게 됩니다~
불필요한 주석은 제거해주세요!

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

네 다음에는 테스트 용도로 쓴 코드들 주석으로 빼지 않고 다 삭제하도록 하겠습니다!

Comment on lines 13 to 14
static void pickNum() {
// 수 중복되지 않게 3가지 뽑기
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

주석은 왜 달아주셨을까요?
pickNum만으로 의미가 불분명하여 달아주신 것일까요?

만약 그런 것이라면 주석을 제거하고 pickNum을 좀 더 구체적인 이름의 메서드로 변경해보는 것은 어떨까요?


마찬가지로 접근 제어자가 default네요~


제공드린 컨벤션 문서를 보시면, 메서드는 사용 순서대로 선언한다는 규칙을 적어드렸어요!
민지님이 작성하신 코드는 민지님만 보는 것이 아니기 때문에, 다른 사람이 코드를 쉽게 파악할 수 있도록 메서드를 배치해 주셔야 해요!
제공드린 문서를 참고해서 메서드의 순서를 재배치해주시면 감사하겠습니다!

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

주석을 달면 무조건 좋다는 생각을 했었는데 이제 고쳐야 할 것 같습니다!

메서드의 순서배치를 할 때 두 번 넘게 호출되는 메서드는 처음 호출될 때를 기준으로 배치하면 될까요?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

주석을 달면 무조건 좋다는 생각을 했었는데 이제 고쳐야 할 것 같습니다!

주석은 대체로 나쁜 경우가 많습니다 ㅎㅎ.
가급적 사용하지 않으려고 노력해보시는 것도 좋을 것 같아요 :)

메서드의 순서배치를 할 때 두 번 넘게 호출되는 메서드는 처음 호출될 때를 기준으로 배치하면 될까요?

네 맞습니다

public class Application {

static LinkedList<Integer> randomNum;
static LinkedList<Integer> userNum;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

접근 제어자가 private이 아닌 default 접근제어자를 사용해 주셨어요?
무슨 이유가 있을까요?

별 이유가 없다면 접근 제어자는 최소로 유지하는게 유지보수에 많은 도움을 준답니다.!

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

평소 알고리즘 문제를 풀 때 return하는 과정이 귀찮아서 저렇게 전역변수로 거의 모든 변수를 설정하는 습관이 있었는데 앞으로는 조금 귀찮더라도 private로 하는 습관을 가지겠습니다!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

좋습니다~

Comment on lines 52 to 57
if (randomNum.contains(userNum.get(index))) {
if (index != randomNum.indexOf(userNum.get(index))) {
return 1;
}
}
return 0;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

프로그래밍 요구 사항을 보면 다음 요구사항이 있어요!

indent(인덴트, 들여쓰기) depth를 3이 넘지 않도록 구현한다. 2까지만 허용한다.
예를 들어 while문 안에 if문이 있으면 들여쓰기는 2이다.
힌트: indent(인덴트, 들여쓰기) depth를 줄이는 좋은 방법은 함수(또는 메소드)를 분리하면 된다.

이에 맞게 코드를 변경해볼까요?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

혹시 메서드 안에 if문 안에 if문이라서 인텐트가 2를 초과하는건가요??

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

아이고 이건 문제 없었네요 죄송합니다..ㅎㅎ
제가 착각했군요

else {
System.out.printf("낫싱\n");
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

불필요한 공백이네요!

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

넵 수정하겠습니다!

// TODO: 숫자 야구 게임 구현

while (true) {
/* 서로 다른 3개의 수를 랜덤으로 뽑는다. -> LinkedList로 구현 */
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ArrayList가 아닌 LinkedList로 구현하신 이유가 궁금해요!
설명해주실 수 있으실까요?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

평소에 linkedList로 많이 했기때문에 익숙해서 사용했던 것 같습니다!
혹시 LinkedList에 비해서 ArrayList가 성능면에서 더 좋은건지 궁금합니다!

Copy link
Contributor

@shin-mallang shin-mallang Feb 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

혹시 LinkedList에 비해서 ArrayList가 성능면에서 더 좋은건지 궁금합니다!

상황에 따라 다르긴 한데, 일반적으로 어레이리스트만 써도 대부분의 경우 문제 없습니다~
제가 답을 알려 드려도 좋은데 이 둘간의 성능은 많은 블로그에 정리되어 있어서 직접 찾아보시는게 더 좋을 것 같아요 :)

Comment on lines 98 to 123
// 사용자에게 입력받은 수를 넣는 LinkedList
userNum = new LinkedList<>();

// user에게 수 입력받기
System.out.printf("숫자를 입력해주세요");
String[] userArr = Console.readLine().split("");

// 사용자가 수를 잘못 입력했는지 확인
checkUserNum(userArr);

// 입력받은 수를 LinkedList에 넣기
userNum.add(Integer.parseInt(userArr[0]));
userNum.add(Integer.parseInt(userArr[1]));
userNum.add(Integer.parseInt(userArr[2]));

// 같은 수가 같은 자리에 있으면 스트라이크, 같은 수가 다른 자리에 있으면 볼
strike = countStrike();
ball = countBall();

// 3개의 숫자를 모두 맞히면 종료
if (strike == 3) {
System.out.println("3스트라이크");
break;
} else {
printAnswer(strike, ball);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이렇게 주석으로 구분된 기능들을 모두 메서드로 추출해보는 것은 어떨까요~?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

네 한번 해보겠습니다!

@alsswl
Copy link
Author

alsswl commented Feb 17, 2024 via email

@shin-mallang
Copy link
Contributor

아고 뭔가 답변이 이상하게 달렸네요 ㅎㅎ
커멘트에 대해 답변 다는 법 모르시면 알려그릴게요~

Copy link
Contributor

@shin-mallang shin-mallang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

안녕하세요 민지님 ~
클래스 분리를 잘 해주셧네요~~ 💯
클래스와 메서드의 접근 제어자 신경을 좀 더 써주시면 좋을 것 같아요~

지금도 너무 잘 하셨지만, 조금 더 피드백을 드려볼게요~

지금 분리한 클래스를 보면 대부분 HandleXXX 이라는 이름을 가지고 있는데요,
이는 조금 객체지향스럽지 못한 클래스들 같아요!
(객체지향이 뭔지 모르시겠다면 조금 찾아보시면 좋을 것 같구요~)

그래서 코드를 좀 더 객체지향스럽게 작성하시는 연습을 위해,
몇 가지 제한을 드리도록 하겠습니다~

이에 맞춰서 코드를 다시 작성해 주시면 감사하겠습니다~~
(아 물론 지금 코드도 좋습니다만, 객체지향 연습을 위한 미션이라 생각해주세요 :))

  1. 클래스는 다음 클래스만을 사용한다.
  • BaseballNumber: 숫자야구 게임에서 사용될 숫자 정보를 가지고 있으며, 숫자 비교 등의 역할을 수행.
  • Hint: 요구사항에는 다음과 같은 문장이 있다.
같은 수가 같은 자리에 있으면 스트라이크, 다른 자리에 있으면 볼, 같은 수가 전혀 없으면 포볼 또는 낫싱이란 힌트를 얻고, 그 힌트를 이용해서 먼저 상대방(컴퓨터)의 수를 맞추면 승리한다.

이곳에서 언급된 힌트의 역할을 수행한다.

  • Application: BaseBallNumber와 Hint를 가지고 숫자 야구 애플리케이션의 총 진행을 책임진다.

어려우실 수도 있겠지만, 한번 도전해 보셨으면 좋겠어요~~
이렇게 구현하고 난 뒤, 지금의 코드와 어떤 차이가 있는지 곰곰히 생각해보는것도 좋을 것 같아요~


import java.util.List;

class Count {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

클래스는 별다른 이유가 없다면 public으로 설정해주세용

Comment on lines 16 to 17
int doGameSet = 1;
int doUserSet = 1;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

정확히 뭘 의미하는지 아직 파악하기 어려운 것 같아요..!!

Comment on lines 27 to 32
userNum = new ArrayList<>();

String[] userArr = handleUserNum.getUserNum();

handleUserNum.checkUserNum(userArr);
handleUserNum.addNumToList(userNum, userArr);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이 행위들이 결국은 userNum에 유저가 입력한 값을 채우기 위한 일련의 과정으로 보이네요!
이를 handleUserNum.getUserNum() 내부에서 수행하게 되면 더 깔끔해질 것 같아요~


class Count {

int countStrike(List<Integer> randomNum, List<Integer> userNum) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

마찬가지로 접근저어자 신경써주세요~

Comment on lines 8 to 20
if (strike == 3) {
System.out.println("3스트라이크");
return 0;
} else if (strike != 0 && ball != 0) {
System.out.printf("%d볼 %d스트라이크\n", ball, strike);
} else if (strike != 0 && ball == 0) {
System.out.printf("%d스트라이크\n", strike);
} else if (strike == 0 && ball != 0) {
System.out.printf("%d볼\n", ball);
} else {
System.out.printf("낫싱\n");
}
return 1;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

조금 더 간단하게 바꿔볼까요?

추가로 return 0, return 1이 무엇을 의미하는지 코드를 보는 사람이 이해할 수 있을까요?

Comment on lines 29 to 38
int endOrAgain(int again) {
if (again == 1) {
return 1;
} else if (again == 2) {
System.out.println("게임종료");
return 0;
} else {
throw new IllegalArgumentException();
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

마찬가지로 매직 넘버를 사용해서 의미를 파악하기 아직 힘든 것 같아요~

Copy link
Contributor

@shin-mallang shin-mallang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

코드 가독성이 많이 좋아진 것 같아요 💯
추가적인 피드백 남겼으니 반영해주시면 감사하겠습니다~

Comment on lines 46 to 48
userNum = new ArrayList<>();

userNum = baseballNumber.getUserNum();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. 이 코드 사이에 공백이 들어갈 이유가 없는 것 같아요~
  2. userNum을 외부에서 미리 세팅할 필요가 있을까요? while문 내부에서 다음과 같이 바로 사용하면 더 코드가 간결해질 것 같아요!
List<Integer> userNum = baseballNumber.getUserNum();

}

private static int endOrAgain(int again) {
int ContinueGameSet = YES;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

변수명 시작은 소문자입니다~

Comment on lines +8 to +12
public static final int YES = 1;
public static final int NO = 0;

public static final int USER_WANT_CONTINUE = 1;
public static final int USER_WANT_FINISH = 2;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

코드 가독성이 많이 좋아진 것 같아요~

}

public List<Integer> getUserNum() {
System.out.printf("숫자를 입력해주세요");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

System.out
Console.readLine()은 모두 Application에서만 쓰도록 변경해볼까요?
BaseballNumber는 다른 작업은 신경쓰지 않고, BaseballNumber의 역할에만 집중할 수 있도록요!

BaseballNumber의 역할은 뭐라고 생각하시나요?

Comment on lines 14 to 22
public int countStrike(List<Integer> randomNum, List<Integer> userNum) {
int cnt = 0;
for (int i = 0; i < NUMBER_OF_CASE; i++) {
if (randomNum.get(i) == userNum.get(i)) {
cnt++;
}
}
return cnt;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

힌드를 BaseBallNumber의 비교 결과로 리턴되도록 바꿔보는 것은 어떨까요?
비교의 역할이 Hint에 있어야 할지, BaseBallNumber에 있어야 할지 고민해 보시면 좋겠어요~

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants