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

[숫자야구게임] 정은채 미션 제출합니다. #2081

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Goldchae
Copy link

💗💗💗💗💗🚀

Copy link

@songjayhyun songjayhyun left a comment

Choose a reason for hiding this comment

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

수고하셨습니다 ㅎㅎㅎ!!! UML도 작성하시고, 리드미 작성에 배워가네요 !!

전반적으로 코드에 대한 이해를 돕기 위해서 주석을 다시느라 고생하셨을텐데... ㅜㅜ 오히려 많은 주석은 가독성을 떨어뜨리는 효과를 낳기도 한답니다. 그리고 줄바꿈을 많이 쓰는 스타일이신 것 같은데 이 또한 가독성을 해 할 수 있습니다.

1주차 고생하셨습니다 !!! 화이팅

* @Method : getCampInput()
* @return : String
*/
private String getCampInput() {

Choose a reason for hiding this comment

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

campInput이라는 이름이 이해하기 살짝 어려웠네요... ! userInput으로만 해도 메소드의 기능을 유추하기 쉬울 것 같습니다 ㅎㅎ!

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 +47 to +49



Choose a reason for hiding this comment

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

Suggested change

공백은 최대한 줄여주는 게 좋겠습니다 ㅎㅎ

Copy link
Author

Choose a reason for hiding this comment

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

앗 공백에 대해선 생각을 못해봤네요! 그렇군요..!

* @Method : putConsoleOutput()
* @param : string
*/
private void putConsoleOutput(String print) {

Choose a reason for hiding this comment

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

굳이 메소드로 빼주지 않고 output 메소드에 넣어줘도 될 것 같아요 ㅎㅎ 변수 명도 print보단 더 명확하면 이해하기 쉬울 것 같습니다 !

Copy link
Author

Choose a reason for hiding this comment

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

감사합니다!

//<사용자>플레이어 값 입력받기
userGamer.play();
// 입력받은 값이 적절한지 <관리자>플레이어에게 값 넘기고 검사받기
manageGamer.errorGame(userGamer.userNumbers); // 적절하지 않을 시 IllegalArgumentException 발생

Choose a reason for hiding this comment

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

errorGame이라는 이름보단 검증하다라는 뜻의 validate어떨까요 ㅎㅎ?

Copy link
Author

Choose a reason for hiding this comment

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

오 좋습니다! 영어공부 열심히 해야겠네요ㅜㅜ!

manageGamer.errorGame(userGamer.userNumbers); // 적절하지 않을 시 IllegalArgumentException 발생
// 검사통과 시 <사용자>플레이어 값 확정
userGamer.setUserNumberArr();
this.userNumberArr = userGamer.getUserNumberArr();

Choose a reason for hiding this comment

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

왠지 메소드 내 지역 변수로 뺄 수 있을 것 같네요 ~!


if (ballResult == 0 && strikeResult == 0) {
sb.append(Const.NOTHIING).append("\n");
}else if(ballResult == 0) {

Choose a reason for hiding this comment

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

Suggested change
}else if(ballResult == 0) {
} else if(ballResult == 0) {

사소하지만...

Copy link
Author

Choose a reason for hiding this comment

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

앗 이런 건 잘 몰랐는데 감사합니다! 자동으로 맞춰주는 모듈을 깔아야 할까 봐요ㅜ

Choose a reason for hiding this comment

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

우테코 커뮤니티 가시면 자동으로 포맷팅 설정하는 글 있습니다~ 읽어보시면 좋을 듯 해요

public static final String BALL = "볼";

// ManageGamer용 상수
public static final String STARTGAME = "숫자 야구 게임을 시작합니다.\n";

Choose a reason for hiding this comment

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

Suggested change
public static final String STARTGAME = "숫자 야구 게임을 시작합니다.\n";
public static final String START_GAME = "숫자 야구 게임을 시작합니다.\n";

더 보기 편할 것 같습니다 ㅎㅎ

Copy link
Author

Choose a reason for hiding this comment

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

앗 상수에는 _도 쓴다는 걸 잊고있었네요!! 감사합니다!!

"게임을 새로 시작하려면 1, 종료하려면 2를 입력하세요.\n";

// UserGamer용 상수
public static final String USERNUMINPUT = "숫자를 입력해주세요 : ";

Choose a reason for hiding this comment

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

Suggested change
public static final String USERNUMINPUT = "숫자를 입력해주세요 : ";
public static final String USER_NUM_INPUT = "숫자를 입력해주세요 : ";

여기두 ㅎㅎ!!

Copy link
Author

Choose a reason for hiding this comment

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

💗💗

//<심판자>플레이어 정답 판정
if (judgeGamer.correct) {
//<관리자>플레이어 게임 종료
gaming = manageGamer.endGame();

Choose a reason for hiding this comment

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

endGame()이라고 하면 게임을 종료시키는 느낌이 나는 것 같습니다 .. ! bool값을 반환하는 함수인지를 나타낼 수 있도록 앞에 is, has, can, should 등으로 시작하면 좋겠네요 ㅎㅎ

Copy link
Author

Choose a reason for hiding this comment

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

좋아요! 조언 감사합니다!

* @Class_name : FacadeProcess
*
* Create Date : 2023-10-21
* Create User : 정은채

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.

자바독은 잘 몰라서ㅜㅜ넘어렵네요!ㅜ

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

@jongwooo jongwooo left a comment

Choose a reason for hiding this comment

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

@Goldchae 사소한 사항들 확인해봤습니다. 아래 리뷰 의견 참고하여 다음 미션에서 적용해보세요. 1주차 고생 많으셨습니다:)

Comment on lines +20 to +21
private ArrayList<Integer> comNumberArr;
private ArrayList<Integer> userNumberArr;

Choose a reason for hiding this comment

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

Suggested change
private ArrayList<Integer> comNumberArr;
private ArrayList<Integer> userNumberArr;
private List<Integer> comNumberArr;
private List<Integer> userNumberArr;

필드 변수의 타입을 인터페이스로 작성하시면 좋을 것 같습니다! 이렇게 하면 나중에 코드를 확장하거나 변경할 때 훨씬 유연하게 대처할 수 있어요. 예를 들어 나중에 ArrayList를 사용하다가 다른 리스트 구현체로 변경해야 할 때, 코드의 변경을 최소화할 수 있겠죠?

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 +21 to +25
/**
* description : ComGamer의 게임 플레이
*
* @Method : play
*/

Choose a reason for hiding this comment

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

Suggested change
/**
* description : ComGamer의 게임 플레이
*
* @Method : play
*/
/**
* description : ComGamer의 게임 플레이
*
* @Method : play
*/

@songjayhyun 님께서 말씀주신 대로 인덴트 설정 참고하시면 좋을 듯 합니다:)

Copy link
Author

Choose a reason for hiding this comment

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

앗ㅜㅜ 이렇게 된 게 있었군요ㅠ



// 컴퓨터의 랜덤 숫자리스트
private ArrayList<Integer> comNumberArr;

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.

헉 그렇군요! 순서 고민이 많았는데 감사합니다!!

*
* @Method : setNumberArr
*/
public void setNumberArr(ArrayList<Integer> _comNumArr, ArrayList<Integer> _userNumArr) {

Choose a reason for hiding this comment

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

Suggested change
public void setNumberArr(ArrayList<Integer> _comNumArr, ArrayList<Integer> _userNumArr) {
public void setNumberArr(ArrayList<Integer> comNumArr, ArrayList<Integer> userNumArr) {

파라미터도 일관성 있는 이름을 사용하시는 것이 좋습니다. 다른 메서드와 동일하게 언더바(_)를 제거해보세요.

Copy link
Author

Choose a reason for hiding this comment

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

앗 이름이 같으면 _ 추가하는 거라고 알고 있었는데 아닌가 보군요ㅠㅠ 감사합니다!

Choose a reason for hiding this comment

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

public void setNumberArr(ArrayList<Integer> comNumArr, ArrayList<Integer> userNumArr) {
  this.comNumArr = comNumArr;
  this.userNumArr = userNumArr;
}

파라미터와 클래스 내 멤버 변수의 이름이 같은 경우, 이를 구분하기 위해 this를 붙이면 됩니다! 2주차부터 적용해보세요:)

}else if(strikeResult == 0) {
sb.append(ballResult).append(Const.BALL).append("\n");
}else {
sb.append(ballResult).append(Const.BALL).append(" ").append(strikeResult).append(Const.STRIKE).append("\n");
Copy link

@jongwooo jongwooo Oct 28, 2023

Choose a reason for hiding this comment

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

문자열의 형태를 지정하는 메서드를 통하여 반복되는 append()를 줄여보세요.

Comment on lines +59 to +63
public void errorGame(String userString) throws IllegalArgumentException {
if (userString.length() != 3 || !isInteger(userString)) {
throw new IllegalArgumentException();
}
}
Copy link

@jongwooo jongwooo Oct 28, 2023

Choose a reason for hiding this comment

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

입력값의 길이가 3이 아닌 경우숫자가 아닌 경우를 나누어 예외 처리할 수 있을 것 같아요. isInteger()에서 parseInt()가 실패하면 예외를 반환하는 것처럼 말이죠!

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 +85 to +86
for (int i = 0 ; i <3 ; i++) {
for (int j = 0 ; j <3 ; j++) {

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.

앗 그렇군요! 너무 사소해서 상관없다고 생각했는데 그게 더 나을 것 같네요!

* @Method : campReadline()
* @return : String
*/
public static String campReadline() {

Choose a reason for hiding this comment

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

camp.nextstep.edu.missionutils.Console에서 다른 패키지로의 변경과 같은 코드의 확장성을 위해, campReadline() 대신 readLine()과 같이 작성하시는 것은 어떤가요?

Copy link
Author

Choose a reason for hiding this comment

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

좋습니다!

*
* @Method : play()
*/
public void play();

Choose a reason for hiding this comment

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

인터페이스에 선언된 모든 메서드는 기본적으로 public이기 때문에, 작성하지 않으셔도 됩니다:)

Suggested change
public void play();
void play();

Copy link
Author

Choose a reason for hiding this comment

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

넵넵!

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.

3 participants