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

Java baseball game 김태근 #5

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

Conversation

geunskoo
Copy link

@geunskoo geunskoo commented Oct 3, 2022

소요시간 : 12시간

풀이 4시간 + 깃 공부 8시간 정도 걸렸던 것 같습니다.
기능 별로 커밋하지 못한 것이 아쉽습니다.
다음 시도에 개선 해보겠습니다!!

@geunskoo geunskoo self-assigned this Oct 3, 2022
@shkisme shkisme requested review from shkisme and fienestar October 3, 2022 08:55
Copy link
Contributor

@shkisme shkisme left a comment

Choose a reason for hiding this comment

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

클래스 분리도 잘 하셨고, 메서드가 하나의 역할만 하도록, 인덴트가 3을 넘지 않도록 코드를 잘 작성하신 것 같아요!! 👍
테스트 코드도 강의에서 본 것 처럼 작성해보시면 도움 될 것 같습니다😄

}



Copy link
Contributor

Choose a reason for hiding this comment

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

소개해드렸던 formatter를 사용해보시는게 좋을 것 같습니다! 포메터 적용을 하지 않으면, 수정사항이 띄어쓰기까지 반영되면서 나중에 변경사항을 확인할 때 불편하다는 단점이 있습니다.
https://withhamit.tistory.com/411

Copy link
Author

Choose a reason for hiding this comment

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

넵!! 수정해서 반영하겠습니다.!!

import java.util.Scanner;

public class Player {
List<Integer> myNum = new ArrayList<>();
Copy link
Contributor

Choose a reason for hiding this comment

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

긴 이름에 대한 거부감이 있으시겠지만, 필드나 메서드 네이밍을 할 때는 축약 없이 하면 좋을 것 같습니다! myNum 대신 myNumberList 이렇게 네이밍을 하시면 좋을 것 같습니다!

public int checkStrike() {
int total=0;
for (int i=0; i<playerNum.size(); i++){
if (playerNum.get(i) == comNum.get(i)){
Copy link
Contributor

@shkisme shkisme Oct 3, 2022

Choose a reason for hiding this comment

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

== 대신에 값 자체를 비교하는 equals() 메서드를 사용해보는 것은 어떨까요?
아래 블로그 주소의 관련 글을 참고해보세요!
https://velog.io/@sorzzzzy/TILJava-equals-vs

Copy link
Author

Choose a reason for hiding this comment

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

오우.. 링크까지 감사합니다 !!
주어진 메서드를 더 잘 활용하는 연습을 해야겠습니다😅

System.out.println(gameManager.getComNum());
gameManager.gameStart();

if (!gameManager.resumeGame()){
Copy link
Contributor

Choose a reason for hiding this comment

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

resumeGame() 메서드는 boolean 타입을 반환하고 있습니다.
이런 경우에는 is~ ( ex. isResumeGame)과 같이 네이밍을 하는 것은 어떨까요?
아래는 자바 메서드 네이밍에 관해 잘 작성되있는 글입니다!
https://tecoble.techcourse.co.kr/post/2020-04-26-Method-Naming/

네이밍이 진짜 제일 힘든거 같습니다...😂

Copy link
Author

Choose a reason for hiding this comment

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

메서드 네이밍에 조금 더 신경써야함을 크게 느꼈습니다...! 😨
링크 감사합니다!

return total;
}
public boolean continueGame(){
if (checkStrike()!=3){
Copy link
Contributor

Choose a reason for hiding this comment

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

contineGame 메서드는 아래와 같이 코드를 작성할 수 있을 것 같습니다!

public boolean continueGame(){
        return checkStrike() != 3;
    }

Random random = new Random();

while (true) {
if (comNum.size() == 3) break;
Copy link
Contributor

Choose a reason for hiding this comment

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

이처럼 종료 조건이 명확할 때는, true 대신 종료 조건을 명시하는 것이 가능하지 않을까요?

@shkisme
Copy link
Contributor

shkisme commented Oct 3, 2022

아 그리고 소요시간 보고 정말 수고하셨다고 생각했습니다...!!!👍👍👍 확실히 자바 챌린지 진행하면서 git 도 많이 써보게 되는 것 같아요.
저같은 경우는 버전 관리를 소스트리 로 하고 있습니다! 추천드려용
https://www.sourcetreeapp.com/

@geunskoo
Copy link
Author

geunskoo commented Oct 4, 2022

시간 내어 리뷰 작성 해주셔서 감사합니다 🤩👍

@geunskoo
Copy link
Author

geunskoo commented Nov 7, 2022

수정 완료했습니다 !

Copy link
Contributor

@shkisme shkisme left a comment

Choose a reason for hiding this comment

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

수고하셨습니다! 리뷰 드린 부분 몇 가지만 수정하시면 다음 챌린지로 넘어가셔도 좋을 것 같습니다! 👍

private List<Integer> comNum = new ArrayList<>();
public void setComNum(List<Integer> comNum) {
this.comNum = comNum;
public void setComNum(List<Integer> n) {
Copy link
Contributor

Choose a reason for hiding this comment

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

아직 축약어인 메서드들의 이름 수정을 안하신 것 같아요!
IntelliJ 에서 macOS는 ⇧F6 , Windows/Linux에서는 Shift+F6을 사용하여 이름 변경 리팩토링이 안전하게 일괄적으로 가능합니다!
IntelliJ에는 정말 편리한 단축키 & 기능이 많으니 사용해보시고 익히시는 것도 좋은 것 같아요! 😊 (저도 아직 무지성 Alt + Enter만 하지만...😂)

Copy link
Author

Choose a reason for hiding this comment

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

앗,,, 다 살펴보았다고 생각했는데,, 꼼꼼한 리뷰 감사합니다. 매의 눈이십니다😯
편의기능 추천도 감사합니다!

Scanner sc = new Scanner(System.in);
int choice = sc.nextInt();

switch (choice) {
Copy link
Contributor

Choose a reason for hiding this comment

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

혹시 switch 문을 사용하신 이유가 있을까요?

Copy link
Author

Choose a reason for hiding this comment

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

자료구조 수업에서 switch구문이 확장성면에서 좋다고해서 수정해보았습니다. 하하😋

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.

조건이 여러가지인 경우에는, (아니면 조건에 따라서 다른 종류의 객체를 반환해야 할때...) switch 문이 여러개의 if-else문 보다 깔끔하고 확장성 있지만, 이처럼 조건식이 몇 가지 없을 때는 단순하게 표현하는 것도 좋다고 생각합니다!
물론 이건 개인적인 성향이니 참고만 하시면 좋을 것 같아요!😊

int choice = sc.nextInt();

switch (choice) {
case 1:
Copy link
Contributor

Choose a reason for hiding this comment

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

이런 경우에는 Enum을 활용해서 코드를 작성하시면 좋습니다! 딱 읽었을 때, 1 이 어떤 의미인지 한눈에 파악하기 어렵기 때문입니다!
자주 사용하는 중요한 상수들은 Enum으로 표현해보세요! ㅎㅎ

public enum GameSignal {
  RESTART(1), END(2);

  int signal;

  GameSignal(int signal) {
    this.signal = signal;
  }
  // add getter...
}

Copy link
Author

Choose a reason for hiding this comment

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

아하...! enum을 통해서 와... 신기합니다
감사합니다 수정하겠습니다.

Copy link
Contributor

@shkisme shkisme left a comment

Choose a reason for hiding this comment

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

수고하셨습니다! 다음 챌린지 진행하셔도 좋을 것 같아요!! 👍

Copy link
Contributor

@shkisme shkisme left a comment

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.

2 participants