-
Notifications
You must be signed in to change notification settings - Fork 14
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
[숫자 야구 게임] 김신희 미션 제출합니다. #13
base: shinheekim
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
느낀점
- MVC 패턴으로 분리한 것에 칭찬! 하지만 특정 클래스의 역할이 애매모호한 경우가 있네요 이 부분에 대해 다시 고민해보고 재구성해보세요.
- 전체적인 클래스명, 메서드명 그리고 변수명을 봤을 때는 명확한 역할을 하는 것 같아서 가독성이 좋았어요.
- 테스트 코드를 통해서 로직에 대해 여러케 이스를 검증하시면 좋겠어요.
궁금한 점에 대한 답변
- 코드 컨벤션이 어느 부분에서 잘 지켜지지 않은건지
- 구글 기준 컨벤션은 저도 잘 모르기 때문에 명확한 피드백을 드리기 어렵네요 ㅎ.. 코드를 봤을때 static final의 순서가 뒤 바낀 경우도 있어서 이런 부분에 대해 기준을 정하면 좋을 것 같아요.
- 로직에 대한 피드백
- 어떤 부분을 개선해야할지 댓글로 따로 적어놨어요!
- static 메서드가 많이 보이는데 어떤 상황일 때 static 메서드를 사용할지 고민을 하시면 좋을 것 같아요.
- 코드 가독성 등
- 제가 느끼기에는 좋은 가독성을 가지고 있는 것 같습니다. 주석만 달면 베스트
return strikeCount; | ||
} | ||
|
||
public void setBallAndStrikeCountList(int ballCount, int strikeCount) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
만약 GameNumberValidateService를 사용하는 클래스 or 메서드가 set를 쓰면 데이터의 일관성이 떨어지지 않을까요?
import java.io.InputStreamReader; | ||
import java.util.Scanner; | ||
|
||
public class InputView { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
해당 클래스는 입력에 관련한 메서드들을 가지고있지만 사용자의 입력을 View보단 Controller에 역할을 위임하는 것이 좋을 것 같아요.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
제가 잘못 이해해서 말씀 드린 것같습니다 그대로 InputView 쓰셔도 될거같아요!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
헉..!감사합니다 사실 이거 어제하다가 머리빠지는줄 알았습니다..
return playerStatus; | ||
} | ||
|
||
// 예외처리 ex. 세 자리 수 입력 x |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
플레이어에 해당하는 데이터 모음 클래스, 플레이어 데이터 검증 클래스
이렇게 두가지 역할을 가지고 있는 것 같아요. SOLID 법칙을 생각해보면 S에 해당하는 단일 책임 법칙에 위배되지 않을까요?
실행 과정
어려웠던 점
궁금한 점
(피드백이 많을 것으로 예상.......하기 때문에 전체적으로 묶어서 말씀해주셔도 될 거같습니다.)