-
Notifications
You must be signed in to change notification settings - Fork 35
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
[보스 몬스터 잡기] 최원준 미션 제출합니다 #3
base: jhon3242
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.
안녕하세요 원준님!
우아한테크코스 백엔드 5기 말랑입니다~
코드 너무 잘 작성해 주셨네요😊
코드를 살펴보며 몇가지 궁금한 점들과 생각해 볼 만한 점들에 대해서 커멘트 남겼으니 확인 부탁드려요!
추가로 질문에 대한 답은 따로 커멘트로 달아드리겠습니다!
(모바일로 진행중이라 확인하기가 어려워서.. 양해 부탁드려요:) )
|
||
|
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.
공백이 두칸이네요~
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.
빈칸은 별 신경을 쓰지 않았는데 불필요한 공백을 없애는 방식으로 작성해야겠네요 감사합니다!
import static bossmonster.domain.GameOption.DELIMITER; | ||
|
||
public class MainController { | ||
public static final GameService service = new GameService(); |
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.
public static으로 설정하신 이유가 있으실까요?
이러한 구조를 가진다면 어떤 문제점이 생길 수 있을까요?
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.
stateless 하게 서비스를 사용하고 싶어서 static
을 사용했는데 지금보니까 public
일 필요는 없다고 느껴지네요 오히려 외부에서 컨트롤러를 통해 서비스에 접근할 수 있어 보안성에도 안좋아 질 것 같습니다!
또한 이러한 구조를 가진다면 의존성에도 문제가 생길 것 같네요 생성자를 통해서 주입 받는 방식이 더 확장 가능한 설계가 되지 않을까 싶네요 ㅎㅎ
|
||
private Boss initBoss() { | ||
try { | ||
int bossHp = Converter.stringToInt(InputView.readBossHp()); |
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.
이곳에서 converter를 사용하신 이유가 따로 있으신가요?
InputView에 readInt()등의 메서드를 만들어 주는 것은 어떻게 생각하시나요?
추가로 InputView의 메서드 네이밍을 readBossMp로 짓는 순간부터 InputView는 논리적으로 비즈니스 로직과 의존성을 갖게 된다고 생각합니다.
(보스의 MP를 입력받는다는 정보를 알고 있는 상황이 되니까요)
이런 상황에서 어떠한 문제점이 있을 수 있고, 또 그에 반해 어떠한 이점을 얻을 수 있는 구조인지 원준님의 생각이 궁금합니다!
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에서 readInt
등을 사용하면 '값을 읽어 들임' 책임에 더해 '값을 변환함' 이라는 책임이 추가되어 단일 책임 원칙에 위배된다고 판단했어서 위와 같이 컨트롤러에서 converter 를 사용해서 변환해주었습니다! 하지만 이 부분을 작성하면 숫자를 입력 받는 작업이 자주 일어나서 매번 변환해주는 것이 가독성를 해친다는 느낌도 많이 받았어요 말랑님은 어떻게 생각하시는지 궁금합니다!
메서드 네이밍을 통해서도 의존성을 가질 수도 있다는 것을 처음 깨달게 되었네요 감사합니다! 이런 식으로 네이밍을 하면 비즈니스 로직이 변경될 때 메서드명도 변경되어야 하는 경우가 발생할 수도 있겠네요! 이를 해결하기 위해서는 어떤 방식이 있을지 궁금합니다! 처음 드는 생각은 readInt(Message.READ_BOSS)HP)
등 처럼 readInt
, readString
과 같은 일관된 메서드로 출력할 메시지를 건내주는 방식인데 이런식으로 사용하면 readBossHp()
보다는 가독성이 떨어지지 않을까요??
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에서 readInt 등을 사용하면 '값을 읽어 들임' 책임에 더해 '값을 변환함' 이라는 책임이 추가되어 단일 책임 원칙에 위배된다고 판단했어서 위와 같이 컨트롤러에서 converter 를 사용해서 변환해주었습니다! 하지만 이 부분을 작성하면 숫자를 입력 받는 작업이 자주 일어나서 매번 변환해주는 것이 가독성를 해친다는 느낌도 많이 받았어요 말랑님은 어떻게 생각하시는지 궁금합니다!
정수값, 문자열 등 여러 값을 읽어드림도 값을 읽어 드림
이라는 책임을 가질 수 있을 것 같아요.
또한 만일 원준님 말씀대로 readInt
를 사용하는 것이 InputView가 단일 책임 원칙을 위배한다고 가정해 볼게요.
그랬을 때 발생하는 문제점이 무엇이 있다고 생각하셨나요?
단일 책임 원칙을 무엇을 위해 지켜야 하는지 고민해보시면 좋을 것 같습니다!
무엇보다 현재와 같은 구조라면 정수가 필요할 때 마다, read를 통해 문자열을 입력받아서 정수로 바꾸어주는 작업을 여러 곳에서 진행하게 되면 중복 코드도 많이 발생할 것 같습니다.
결론적으로 저라면 readInt
를 두고 사용할 것 같아요!
(편하니까요)
Player player = new Player(); | ||
initPlayerName(player); | ||
initPlayerHPAndMP(player); |
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.
Boss는 생성자로 인자를 넣어주는 반면, 플레이어의 경우에는 setter를 통해 세팅해주는 것으로 보이는데, 두 구조가 통일성이 없다고 생각하시지 않으시나요?
이러한 구조룰 어떤 식으로 개선할 수 있을까요?
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.
이 부분이 가장 고민했던 부분 중 하나입니다! 플레이어 또한 생성자 인자를 통해 생성해주고 싶었는데 new Player(name, hp, mp)
와 같이 로직을 작성하면 생성자를 실행할 때 입력 값의 검증이 시작되어서 hp/mp
부분에서 예외가 발생하면 name
부터 다시 입력받아야 되는 문제가 발생했었습니다. 그래서 어쩔 수 없이 각각 setter
를 통해 따로 입력 받고 검증하는 방식으로 로직을 작성하게 되었어요 ㅠㅠ 그런데 지금 생각해보니까 Name
이라는 별도의 객체를 만들어서 검증을 처리해주면 해결할 수 도 있다는 생각이 드네요?
Name name = readName()
Stats stats = readStats()
Player player = new Player(name, stats)
위와 같은 방식으로 로직을 작성하면 각자의 생성자에서 예외 검증을 통해 해결 할 수 있지 않을까 싶네요 ㅎㅎ
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.
맞습니다 ㅎㅎ
그런데 네이밍을 추가로 언급드리면,
- 이름은 플레이어만 입력받으며, 플레이어의 이름에 대해서만 비즈니스 규칙이 존재합니다.
- Stats은 보스와 플레이어의 비즈니스 규칙이 각각 다릅니다.
따라서 Name, Stats 보다는 PlayerName과 PlayerStats이 도메인을 더 잘 표현할 수 있는 네이밍이라 생각합니다 ㅎㅎ
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.
아하 그렇겠네요 놓치고 있었던 부분을 알게되었네요 감사합니다!!
|
||
private void proceedRade(Boss boss, Player player) { | ||
OutputView.printRadeStart(); | ||
while (boss.isAlive() && player.isAlive()) { |
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.
해당 조건은 게임이 지속되는 것을 정의하는 비즈니스 규칙을 포함하는 것 같은데요,
이렇게 비즈니스 규칙을 컨트롤러에서 관리한다면 Service는 어떠한 역할을 갖게 되는 것인지 궁금합니다!
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.
이것도 고민했던 것 중 하나였는데 비즈니스 로직은 최대한 서비스에 두고 싶었으나 비즈니스 규칙을 검증하고 다시 입력 받는 작업을 위해서는 컨트롤러에 작성할 수 밖에 없다고 판단했습니다ㅠㅠ 그래서 뷰를 사용할 필요가 없는 비즈니스 로직은 Service 에 작성하고 그 외의 경우에는 컨트롤러에 남게 되었어요. proceedRade
도 비즈니스 로직이 포함되어 있지만 중간 중간에 출력을 해줘야 하는 부분이 있어서 컨트롤러에 남게 되었습니다. 이런 경우를 해결하기 위해 어떤 방법이 있을까요?? 제 나름대로 고민했지만 위와 같이 작성하는게 최선이라고 판단했습니다ㅠㅠ
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.
여기에는 많은 방법들이 있을 수 있을 수 있겠지만, 다 설명드리긴 어려워서 몇가지만 말씀드려볼게요!
- Service에
게임을 계속 진행할 수 있는가?
를 나타내는 메서드를 만들 수 있을 것 같아요. - Boss와 Player를 묶은 어떠한 객체를 만들어서, 해당 객체에
게임을 계속 진행할 수 있는가?
를 표현하도록 할 수도 있을 것 같아요. - 지금 상태도 괜찮다고 생각해요!
3번에 대해 조금 더 말씀을 드리면, 서비스 자체가 현재 당장 필요한 것인지도 한번 고민해 보시면 좋겠어요!
(서비스의 역할이 무엇인지에 대해 정해진 답은 없지만, 이를 바라보는 여러 시각이 존재하니 같이 찾아보셔도 좋을 것 같아요!
이렇다할 정답은 없지만요!)
이것도 고민했던 것 중 하나였는데 비즈니스 로직은 최대한 서비스에 두고 싶었으나
비즈니스 규칙을 서비스에 두어야 할 필요가 있을까요?
서비스 말고도 이러한 것을들 잘 표현할 수 있는 객체를 만들어 보는 건 어떨까요?
조금 더 예를 들어 볼게요.
만약 플레이어를 만드는 로직을 서비스에 작성한다고 가정하면
public Player createPlayer(String name) {
if (name.length < 0 || name.length > 5) {
// 예외
}
return new Player(name);
}
위와 같이 비즈니스 규칙에 대한 검증을 서비스에서 처리하는 방법이 있을 수 있겠죠?
그런데 다음과 같이도 만들 수 있어요!
public Player createPlayer(String name) {
return new Player(name);
}
class Player {
public Player(String name) {
validate(name);
this.name = name;
}
private void validate(String name) {
if (name.length < 0 || name.length > 5) {
// 예외
}
}
}
이렇듯 반드시 서비스에 비즈니스 규칙을 두어야 할 필요는 없으니, 여러 방법들과 각 방법들의 장단점에 대해 학습해 보시고, 현재 구조에서 어떤 방법이 가장 좋은 방법일지를 고민해 보신 뒤 적용해보시면 좋겠습니다 ㅎㅎ
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.
시야가 조금 확장된 것 같네요 감사합니다!!
isLegalSize(playerStats); | ||
isLegalSumValue(playerStats); |
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.
여기도 is대신 validate이요!
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.
생각보다 많은 곳에서 validate
보다 is
를 사용했네요 ㅎㅎ 반성합니다!
validateStats(stats); | ||
this.hp = new Point(stats.get(GameOption.HP_INDEX)); | ||
this.mp = new Point(stats.get(GameOption.MP_INDEX)); | ||
this.attackCount = GameOption.INITIAL_ATTACK_COUNT; |
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.
Status에서 사용되눈 상수들을 Options에서 일관적으로 관리한다면 어떤 문제가 생길 수 있을까요!?
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.
Options에서 일관적으로 관리하면 Stats
가 아닌 다른 곳에서도 사용할 가능성이 높아지게 되고 이는 의도치 않는 에러로 이어질수도 있겠네요! 확실히 특정 객체에서만 사용되면 해당 객체 내부에서 static final
필드값으로 사용하는게 좋겠네요! 감사합니다!
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.
👍
attackCount++; | ||
} | ||
|
||
public int calculateAttackDamage(AttackType attackType) { |
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.
Stats 클래스의 역할은 무엇인가요?
attack이라는 행위에 대해서 알고 있는 것에 문제가 없을까요?
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.
상태를 관리하는 객체인데 굳이 attack
에 행위에 대해서는 알 필요가 없네요! 오히려 의존성만 생기는 결과인것 같습니다! 단순히 int
를 받는 형식으로 수정하는게 더 좋을 것 같네요!
return attackType.getDamage(); | ||
} | ||
|
||
private boolean isMagicAttackWithLackMp(AttackType attackType) { |
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.
(위와 마찬가지 질문)
public void handleCost(AttackType attackType) { | ||
if (attackType.equals(AttackType.PHYSICAL)) { | ||
handlePhysicalAttack(); | ||
} | ||
if (attackType.equals(AttackType.MAGIC)) { | ||
handlerMagicAttack(); | ||
} | ||
} |
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.
이처럼 if문을 통해 처리한다면 어떤 문제가 발생할 수 있을까요~?
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.
앞으로 공격 타입이 추가될 때 마다 무수한 if
문이 이어지겠네요! 이를 해결하기 위해선 어떤 방법이 있을까요? Switch 문을 사용한다고 해서 근본적인 해결책이 될 것 같지는 않은데 어떤 방법이 있을지 궁금합니다!
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.
attackType에 그러한 역할을 넘겨주는 것도 좋아보여요~
여기에도 여러가지 방법이 있을 수 있게는데요,
attackType.xxx(this);
위처럼 AttackType 내에서 처리하도록 할 수 있겠죠?
또는
mp += attackType.useMp;
등도 사용할 수 있을 것 같구요~
(쓰고보니 위 방법은 쪼금 네이밍이 에매하네요 :))
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.
아 그러네요 attackType
에게 위임하면 깔끔하게 해결할 수 있겠네요 감사합니다!!
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.
setter 의사용
Player 객체에서 생성자 하나로 예외 검증하는 방식으로 처리하고 싶었으나 이름 정보 , HP,MP를 각각 입력 받고 예외 발생 시 해당 입력부터 다시 입력 받는 작업을 위해서는 따로 입력 받고 검증하는 방식으로 처리해야 했습니다. 그러다 보니 결국 setter를 사용하게 되었는데 이를 사용하지 않고 초기화 하려면 어떤 방식으로 하면 좋을지 궁금합니다.
생성자로 주입해 주면 되겠죠?
그런데 문제는 잘못된 값이 입력되눈 경우 다시 입력받는 부분일 것 같아요.
생성자로 이 둘을 처리한다면 첫번째 값이 잘못되었을 때 첫번째 값만 다시 입력받고 싶은데 그렇게 처리할 수 없으니까요!
지금 당장 생각나는 방법은 두 가지가 있는데요,
첫 번째는 첫번째 값만 다시 입력받도록 하지 않고, 문제가 발생한다면 이름과 스탯을 모두 다시 입력받도록 하는 방법이 있을 것 같아요!
(이게 궁금했던 건 아니겠지만 이런 식으로도 간단하게 문제를 해결할 수 있다는 관점에서 말씀드린거에요!)
두번째는 플레이어의 이름과 상태를 각각 객체로 정의한 뒤 이들을 생성자로 받아주는 방법이 있을 것 같아요!
(잘 이해가 되지 않으시면 다시 질문 주셔도 좋습니다:))
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.
메시지 문자열 관리 방식
뷰나 예외 발생 시 출력해야 하는 문자열을 각각 Enum 과 별도의 객체에서 관리하는 방식으로 코드를 작성하였습니다. 원래는 Enum으로 통일하여 메시지를 관리하려고 했으나 Enum 사용 시에 예외 출력 시 System.out.println(ExceptionMessage.NUMBER_FORMAT.getMessage()) 와 같이 뒤에 항상 getMessage() 와 같은 별도의 문자열 변환 메서드를 적어줘야 했기 때문에 ExceptionMessage 는 별도의 static final 필드 값만 가지는 객체로 관리하였습니다. 통일성을 위해서 하나의 방식으로 통일하는 것이 좋아 보이는데 Enum, 별도의 객체, 내부에 static final 변수 선언 중에서 어떠한 방식을 사용하는 게 좋을지 궁금합니다.
취향에 따라 다르겠지만 저라면 두 가지 선택지 중에서는 객체에서 관리했을 것 같습니다!
사용이 편리하니까요!
그런데 한 가지 질문 드리고 싶은게 있는데요, 굳이 이러한 메세지들을 상수화해서 관리해야 할까요?
이에 대해 의견을 듣고 싶습니다
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.
일급 컬렉션의 사용으로 발생하는 전달(?) 메서드
Player 객체를 처음 만들고 내부 필드를 생성하다 보니 책임의 분할이 필요하다고 느껴질 정도로 복잡해졌습니다. 그래서 HP, MP 를 관리하는 Stats 객체를 만들었고 Stats 객체 내부에는 Point 라는 객체를 만들어서 HP, MP 를 관리하였습니다. 이런 식으로 객체 안에 객체가 생기다 보니 플레이어의 HP 정보를 가져오기 위해 Player -> Stats -> Point -> Stats -> Player 와 같은 흐름이 발생했고 중간에 있는 Stats 같은 경우 값을 전달만 하는 메서드가 종종 발생했습니다. 이러한 메서드가 발생하는 건 어쩔 수 없다고 생각하고 넘겨야 하는 건지 아니면 다른 해결책이 있는지 궁금합니다.
물론 상황에 따라서 값을 단순히 전달만 하는 메서드가 발생하는 부분은 어쩔 수 없다고 생각합니다!
그러나 이런 메서드가 많아지는 경우, 해당 클래스가 정말 필요한가 생각해 볼 필요가 있는 것 같아요!
지금 상태도 괜찮다고 생각하지만, 이러한 부분에 대해 고민이 있으시다면 조금 더 생각해보시고 다른 방법도 시도해보시면 좋을 것 같아요:)
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.
좋은 피드백 정말 감사합니다! 해결책을 알려주시긴 보단 질문을 해주셔서 다시 한 번 생각해보는 계기가 되었네요! 많은 것을 얻어갈 수 있었습니다! 감사합니다!
|
||
private Boss initBoss() { | ||
try { | ||
int bossHp = Converter.stringToInt(InputView.readBossHp()); |
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에서 readInt
등을 사용하면 '값을 읽어 들임' 책임에 더해 '값을 변환함' 이라는 책임이 추가되어 단일 책임 원칙에 위배된다고 판단했어서 위와 같이 컨트롤러에서 converter 를 사용해서 변환해주었습니다! 하지만 이 부분을 작성하면 숫자를 입력 받는 작업이 자주 일어나서 매번 변환해주는 것이 가독성를 해친다는 느낌도 많이 받았어요 말랑님은 어떻게 생각하시는지 궁금합니다!
메서드 네이밍을 통해서도 의존성을 가질 수도 있다는 것을 처음 깨달게 되었네요 감사합니다! 이런 식으로 네이밍을 하면 비즈니스 로직이 변경될 때 메서드명도 변경되어야 하는 경우가 발생할 수도 있겠네요! 이를 해결하기 위해서는 어떤 방식이 있을지 궁금합니다! 처음 드는 생각은 readInt(Message.READ_BOSS)HP)
등 처럼 readInt
, readString
과 같은 일관된 메서드로 출력할 메시지를 건내주는 방식인데 이런식으로 사용하면 readBossHp()
보다는 가독성이 떨어지지 않을까요??
Player player = new Player(); | ||
initPlayerName(player); | ||
initPlayerHPAndMP(player); |
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.
이 부분이 가장 고민했던 부분 중 하나입니다! 플레이어 또한 생성자 인자를 통해 생성해주고 싶었는데 new Player(name, hp, mp)
와 같이 로직을 작성하면 생성자를 실행할 때 입력 값의 검증이 시작되어서 hp/mp
부분에서 예외가 발생하면 name
부터 다시 입력받아야 되는 문제가 발생했었습니다. 그래서 어쩔 수 없이 각각 setter
를 통해 따로 입력 받고 검증하는 방식으로 로직을 작성하게 되었어요 ㅠㅠ 그런데 지금 생각해보니까 Name
이라는 별도의 객체를 만들어서 검증을 처리해주면 해결할 수 도 있다는 생각이 드네요?
Name name = readName()
Stats stats = readStats()
Player player = new Player(name, stats)
위와 같은 방식으로 로직을 작성하면 각자의 생성자에서 예외 검증을 통해 해결 할 수 있지 않을까 싶네요 ㅎㅎ
|
||
private void proceedRade(Boss boss, Player player) { | ||
OutputView.printRadeStart(); | ||
while (boss.isAlive() && player.isAlive()) { |
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.
이것도 고민했던 것 중 하나였는데 비즈니스 로직은 최대한 서비스에 두고 싶었으나 비즈니스 규칙을 검증하고 다시 입력 받는 작업을 위해서는 컨트롤러에 작성할 수 밖에 없다고 판단했습니다ㅠㅠ 그래서 뷰를 사용할 필요가 없는 비즈니스 로직은 Service 에 작성하고 그 외의 경우에는 컨트롤러에 남게 되었어요. proceedRade
도 비즈니스 로직이 포함되어 있지만 중간 중간에 출력을 해줘야 하는 부분이 있어서 컨트롤러에 남게 되었습니다. 이런 경우를 해결하기 위해 어떤 방법이 있을까요?? 제 나름대로 고민했지만 위와 같이 작성하는게 최선이라고 판단했습니다ㅠㅠ
OutputView.printRadeStart(); | ||
while (boss.isAlive() && player.isAlive()) { | ||
OutputView.printRadeInfo(boss, player); | ||
initAttackType(player); |
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.
아하 그럴 수 있겠네요 좋은 관점을 제시해주셔서 감사합니다!
while (boss.isAlive() && player.isAlive()) { | ||
OutputView.printRadeInfo(boss, player); | ||
initAttackType(player); | ||
int playerToBossDamage = service.attackBoss(boss, player); |
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.
책임과 역할에 집중에서 설계하기 보단 단순히 "뷰를 사용하지 않는 비즈니스 로직은 서비스에 둬야겠다" 라는 생각으로 작성하다 보니 서비스가 어떤 책임을 가지고 어떤 역할을 하는지 명확하게 설계하지 못했네요 다음 부터는 책임과 역할에 집중해 봐야겠습니다!
isBlank(name); | ||
isLegalLength(name); |
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.
공감합니다! 감사합니다!
validateStats(stats); | ||
this.hp = new Point(stats.get(GameOption.HP_INDEX)); | ||
this.mp = new Point(stats.get(GameOption.MP_INDEX)); | ||
this.attackCount = GameOption.INITIAL_ATTACK_COUNT; |
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.
Options에서 일관적으로 관리하면 Stats
가 아닌 다른 곳에서도 사용할 가능성이 높아지게 되고 이는 의도치 않는 에러로 이어질수도 있겠네요! 확실히 특정 객체에서만 사용되면 해당 객체 내부에서 static final
필드값으로 사용하는게 좋겠네요! 감사합니다!
isLegalSize(playerStats); | ||
isLegalSumValue(playerStats); |
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.
생각보다 많은 곳에서 validate
보다 is
를 사용했네요 ㅎㅎ 반성합니다!
attackCount++; | ||
} | ||
|
||
public int calculateAttackDamage(AttackType attackType) { |
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.
상태를 관리하는 객체인데 굳이 attack
에 행위에 대해서는 알 필요가 없네요! 오히려 의존성만 생기는 결과인것 같습니다! 단순히 int
를 받는 형식으로 수정하는게 더 좋을 것 같네요!
public void handleCost(AttackType attackType) { | ||
if (attackType.equals(AttackType.PHYSICAL)) { | ||
handlePhysicalAttack(); | ||
} | ||
if (attackType.equals(AttackType.MAGIC)) { | ||
handlerMagicAttack(); | ||
} | ||
} |
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.
앞으로 공격 타입이 추가될 때 마다 무수한 if
문이 이어지겠네요! 이를 해결하기 위해선 어떤 방법이 있을까요? Switch 문을 사용한다고 해서 근본적인 해결책이 될 것 같지는 않은데 어떤 방법이 있을지 궁금합니다!
매직 리터럴을 사용하여 어떤 메시지를 출력하는지 의미를 전달하기 위해 상수화해서 관리하는게 아닐까요? 또한 상수화 해서 관리하면 출력 메시지 변경시에도 해당 상수를 사용하는 곳을 찾아서 수정할 필요가 없어서 훨씬 수월할 것 같고요 혹시 또 다른 이유가 있을까요?? |
public static String readBossHp() {
System.out.println(ViewMessage.READ_BOSS_HP);
return getValidatedString();
}
public static String readPlayerName() {
System.out.println(ViewMessage.READ_PLAYER_NAME);
return getValidatedString();
}
public static String readPlayerInfo() {
System.out.println(ViewMessage.READ_PLAYER_HP_MP);
return getValidatedString();
}
public static String readAttackType() {
System.out.println(ViewMessage.READ_ATTACK_TYPE);
return getValidatedString();
} public static String readBossHp() {
System.out.println("보스 몬스터의 체력을 입력해주세요.");
return getValidatedString();
}
public static String readPlayerName() {
System.out.println("플레이어의 이름을 입력해주세요.");
return getValidatedString();
}
public static String readPlayerInfo() {
System.out.println("플레이어의 HP와 MP를 입력해주세요.");
return getValidatedString();
}
public static String readAttackType() {
System.out.println("공격 타입의 번호를 입력해주세요.");
return getValidatedString();
} 위 두 방법 중 어떤 것이 더 편하게 읽히나요? 물론 상황마다, 사람마다 느끼는 데 차이가 있을 수는 있겠지만, 저는 아래 방법이 좀 더 끌려서 질문드려봤습니다 :) |
@jhon3242 |
@shin-mallang |
저는 이렇다 할 기준은 없고 그때 그때 가독성이 더 좋아 보이는 방식으로 사용합니다! |
[고민 사항]
setter 의사용
Player
객체에서 생성자 하나로 예외 검증하는 방식으로 처리하고 싶었으나이름 정보
,HP,MP
를 각각 입력 받고 예외 발생 시 해당 입력부터 다시 입력 받는 작업을 위해서는 따로 입력 받고 검증하는 방식으로 처리해야 했습니다. 그러다 보니 결국setter
를 사용하게 되었는데 이를 사용하지 않고 초기화 하려면 어떤 방식으로 하면 좋을지 궁금합니다.메시지 문자열 관리 방식
뷰나 예외 발생 시 출력해야 하는 문자열을 각각 Enum 과 별도의 객체에서 관리하는 방식으로 코드를 작성하였습니다. 원래는 Enum으로 통일하여 메시지를 관리하려고 했으나 Enum 사용 시에 예외 출력 시
System.out.println(ExceptionMessage.NUMBER_FORMAT.getMessage())
와 같이 뒤에 항상getMessage()
와 같은 별도의 문자열 변환 메서드를 적어줘야 했기 때문에ExceptionMessage
는 별도의static final
필드 값만 가지는 객체로 관리하였습니다. 통일성을 위해서 하나의 방식으로 통일하는 것이 좋아 보이는데 Enum, 별도의 객체, 내부에static final
변수 선언 중에서 어떠한 방식을 사용하는 게 좋을지 궁금합니다.일급 컬렉션의 사용으로 발생하는 전달(?) 메서드
Player
객체를 처음 만들고 내부 필드를 생성하다 보니 책임의 분할이 필요하다고 느껴질 정도로 복잡해졌습니다. 그래서 HP, MP 를 관리하는Stats
객체를 만들었고Stats
객체 내부에는Point
라는 객체를 만들어서 HP, MP 를 관리하였습니다. 이런 식으로 객체 안에 객체가 생기다 보니 플레이어의 HP 정보를 가져오기 위해Player
->Stats
->Point
->Stats
->Player
와 같은 흐름이 발생했고 중간에 있는Stats
같은 경우 값을 전달만 하는 메서드가 종종 발생했습니다. 이러한 메서드가 발생하는 건 어쩔 수 없다고 생각하고 넘겨야 하는 건지 아니면 다른 해결책이 있는지 궁금합니다.[후기]
전체적으로 프리코스 4주차 미션같은 느낌이 많이 들었습니다. 당장 프리코스 문제로 나와도 이상하지 않을 정도로 퀄리티가 높았습니다. 하지만 몇 가지만 조금 추가되면 좋을 것 같아서 아래에 정리했습니다. 프리코스를 진행하기 전에 이런 기회를 주셔서 감사하고 앞으로도 과제가 더 나와서 더 많은 것을 얻어갈 수 있으면 좋겠습니다 감사합니다!
MP 부족 시 어떻게 해야 할지 명시
플레이어가 마법 공격 시에 MP 가 부족할 때 어떻게 해줄지 명시해 주면 좋을 것 같습니다. 물론 각자 판단해서 작성하도록 의도하신 거라면 상관없지만 질문이 많이 올라올 것 같습니다. 😅
Junit 이 추가 되면 좋겠습니다.
난이도 하향? 을 위해서 Junit 을 추가하지 않으신 것 같기도 하지만 Junit 이 없어서 리펙터링 하는데 시간이 많이 걸렸습니다 ㅠㅠ 추후 다른 과제에서는 추가되면 좋겠습니다!