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

숫자 야구게임 구현 #2774

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

Conversation

tjdxo1193
Copy link

No description provided.

Copy link

@Dev-Yesung Dev-Yesung left a comment

Choose a reason for hiding this comment

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

안녕하세요 성태님 코드작성하느라 고생하셨습니다. 개인적으로 아쉬움이 많이 남는 코드인데요. 그래도 이런식으로 구현하면서 학습하시면 크게 도움이 될거 같네요. 인터넷에 잘 작성된 코드들이 많으니 그 코드들 참고해서 학습하는 것도 좋습니다! 고생하셨어요

@@ -12,6 +12,7 @@ repositories {

dependencies {
implementation 'com.github.woowacourse-projects:mission-utils:1.1.0'
implementation 'org.projectlombok:lombok:1.18.22'

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.

앗 네!

public static void main(String[] args) {
// TODO: 프로그램 구현
GameLauncher launcher = new NumberBaseBallGameLauncher();
launcher.startGame();

Choose a reason for hiding this comment

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

사소한 거긴한데 GameLauncher가 start란 메소드를 사용하는 경우는 게임이 시작되는 경우니까 메소드 명에서 Game이란 단어를 빼도 좋을거 같네요(꼭 따를 필요는 없지만 클린코드 지침에도 중복된 단어 사용에 관한 내용이 있습니다. 학습하면 좋을거 같네요)

Copy link
Author

Choose a reason for hiding this comment

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

네! 이런 도메인의 코드를 짜보지 않아서 생각하지못하고 짰네요! 감사합니다!

@Override
public void startGame() {
PrintMessage.printlnMessage(GameProgressMessage.START);
initGame();

Choose a reason for hiding this comment

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

인터페이스의 역할에 관해 고민해보는게 좋을거 같아요! 인터페이스를 사용할 때는 객체 사이의 메시지를 주고 받을 때일텐데, 여기서 init메서드가 play 메서드 안으로 들어가면 인터페이스로 선언하는게 의미가 있을까 싶네요. 인터페이스를 왜 사용하는지 그렇다면 추상클래스랑은 어떤 차이가 있고 어떤 상황에서 둘을 사용할지 공부해보세요~

Copy link
Author

Choose a reason for hiding this comment

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

네 감사합니다!

import baseball.enums.OrNot;

import static camp.nextstep.edu.missionutils.Console.readLine;
public class NumberBaseBallGameLauncher implements GameLauncher{

Choose a reason for hiding this comment

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

게임런처를 인터페이스로 선언했다는 건 확장성이 있다는 것으로 인식했습니다. 예를 들어 게임런처의 내부 동작이 다른 동작이나 구현을 사용하게 된다던지? 그렇게 되면 객체제향의 OCP를 위반하지 않고 기존 구현체의 수정 없이 새로운 구현체를 만들게 되겠죠? NumberBaseBall이라고 만들었는데,NumberBaseBall이란게 위의 설명에 부합한 이름은 아닌거 같네요. 숫자야구게임에 숫자가 안쓰이고 다른게 쓰일 수 있을까요? 구현체에 걸맞는 이름이 필요해보여요

Copy link
Author

Choose a reason for hiding this comment

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

생각하지 못했던거 같습니다 앞으로 이름을 신중하게 지어야겠네요!


@Override
public void startGame() {
PrintMessage.printlnMessage(GameProgressMessage.START);

Choose a reason for hiding this comment

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

MVC 패턴에 관한 학습이 필요할거 같아요. 게임 실행 클래스가 메시지에 의존하고 있는데 이렇게되면 뷰의 구현체가 달라지거나 메서드가 삭제될 경우 게임런처 구현체에도 그 영향이 전달됩니다. 즉, 단일책임원칙 위배에요...!

Copy link
Author

Choose a reason for hiding this comment

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

네! 생각해서 다시 리팩토링하겠습니다!

public enum GameFlag {
END,
START,
ING,

Choose a reason for hiding this comment

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

ING 상태가 필요한 경우가 있나요? 게임에 관한 판단은 게임 시작과 게임 끝일 때만 있으면 될거 같아요!

Copy link
Author

Choose a reason for hiding this comment

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

네! 절차지향적 코딩을 하다보니 이런 부수적인 좋지 않은 코드가 나온거 같네여


import baseball.Result;

public enum GameProgressMessage {

Choose a reason for hiding this comment

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

더 좋은 방식으로 처리할 수 있을거 같아요. 그리고 메시지를 관리하는 클래스에서 상태에 따라 메시지를 만드는 로직이 들어가 있는게 어색합니다

@@ -0,0 +1,15 @@
package baseball.enums;

public enum OrNot {

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.

게임을 할건지 말건지를 사용하기 위해서 했는데
한번밖에 안쓰이는 곳인데 이넘은 과한거 같아서 다시 수정하겠습니다


public class Application {

public static void main(String[] args) {

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.

전에는 절차지향적인 코드를 작성했던걸 깨닫고 OOP측면에서 설계하고 모델링하는 방식을 배워 다시 적용하려고 합니다.

public class Application {

public static void main(String[] args) {
Game game = new BaseBallGame(
Copy link

@Dev-Yesung Dev-Yesung Nov 7, 2023

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.

참고자료로 가져온 코드는 아니고... 다시 새롭게 짜는 코드입니다!

Choose a reason for hiding this comment

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

그렇다면 굿... 처음 코드보다 훨씬 접근 방식이 좋네요


import java.util.HashSet;

public class GameInputValue {

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.

맞습니다..

checkValidUserInput();
}

public boolean isEqaulsUserAndComputerValue() {

Choose a reason for hiding this comment

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

뭔가 행동을 보면 Judgement와 비슷한거 같은데 역할이 중복되는거 같아요

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.

None yet

2 participants