[보스 몬스터 잡기] 강세현 미션 제출합니다. #8
Conversation
Battle 클래스 추가, attackBossMonster 메소드 추가, Player/BossMonster 클래스에 관련 메소드 추가
attackPlayer 메소드 추가, Player 클래스에 관련 메소드 추가
Player 클래스의 getter 메소드, Mp 회복 메소드명 수정
전투 종료 확인 메소드 추가
maxMp 멤버 변수 추가
Battle 클래스 테스트 추가, Battle 클래스의 attackPlayer 메소드 내 랜덤 함수 관련 수정
…Battle turnCount 멤버 변수 삭제, attackPlayer 매개변수 수정
GameView 클래스 추가, initialSetting에 필요한 입출력 메소드 추가
화면 출력에 필요한 maxHp 변수, getter 추가
화면 출력을 위해 리턴 타입 int로 변경
화면 출력을 위한 statusDto 클래스 추가
playerName 멤버 변수 추가
|
리뷰 반영해서 수정 후 커밋 완료했습니다!
|
There was a problem hiding this comment.
안녕하세요 세현님~
명절 잘 보내시고 계신가요? :)
리뷰 반영 너무 잘 해주셨어요!!
기존 코드도 좋았지만, 수정 후에는 코드가 정말 많이 깔끔해진 것 같은 느낌이 들어요 ㅎㅎ
이번 리뷰에서는 다움 피드백을 드릴테니 충분히 고민해 보시고 적용해 보셨으면 좋겠습니다!
-
생성자 대신 setter를 많이 이용하는 모습을 보여주셨는데, 한번 생성자를 적응 활용해 보시는건 어떨까요?
Setter를 사용하여 객체를 초기화한다면, 객체가 생성되눈 시점에는 불안전한 형태로 생성될 수밖에 없는데요, 이러한 상황에서 어떠한 문제점이 발생할 수 있을지 고민해 보시면 좋을 것 같습니다!
또한 생성자를 통해 필요한 값을 입력받으면 어떤 점이 좋을지도 함깨 고려해 보시면 좋겠어요!
(또한 setter 자체를 싫어하는 사람도 있기는 한데, 이건 참고만 해주세요! 저는 도메인 로직을 담고만 있다면 큰 문제는 안된다고 생각하긴 하는데, setter 자체가 이미 안 좋게 굳어진 인식이 있어서 사용을 꺼려하는 사람들도 많습니다) -
System.in 대신 inputview 자체를 모킹하여 controller를 테스트해보셨으면 좋겠어요!
이를 통해 현재 구조의 문제점과, 테스트하기 쉬운 구조에 대해 학습하실 수 있으실 거라 생각해요:)
이제 질문에 대한 답변 드릴게요!
위에서 언급한 방식으로 테스트를 진행하면 1번 질문에 대한 답은 얻으실 수 있으실 거 같아 건너뛸게요!
- 컨트롤러에서 Player 클래스를 전역 변수로 선언하면, 여러 플레이어로 기능이 확장될 때 문제가 발생할 수 있다고 하셨었는데, 현재 수정한 코드에서는 확장하게 된다면 List 클래스를 전역 변수로 선언하고 play() 메서드에서 add하면 될 것같다고 생각하는데 괜찮은지 궁금합니다
물론 그래도 문제는 되지 않겠지만, 사용자가 많아지면 list가 너무 많은 객체를 가지고 있게 될 수도 있겠죠?
사실 그것보다는, 컨트롤러의 play() 메서드 내에서 player를 생성하고 사용한다면, 이는 사용자 별로 관리되어 질 것이므로 굳이 list를 쓸 이유는 없을 것 같아요:)
- 컨트롤러에서 다른 뷰나 도메인등에서 내부 로직 메서드를 호출하지않게 하려고 play() 메서드 이외에 다른 모든 메서드에는 private로 설정했습니다. 이 경우, 컨트롤러의 테스트는 play() 메서드밖에 하지못하는데 괜찮은지 궁금합니다. 저는 play() 메서드만 테스트하더라도 로직상 다른 클래스에서 선언하지않을 메서드는 private로 선언하는 것이 더 좋을 것 같다고 생각합니다.
굳이 private 메서드는 테스트할 필요가 없습니다!
관련해서는 다음 글을 추천드릴게요!
도메인과 서비스에 대해서 생각해봤는데, 도메인만이 가지는 역할은 도메인에게 역할을 위임하도록 해서 불필요한 서비스 생성은 줄이도록하고, 공통적으로 가지게 되거나 상호간의 로직이 복잡한 비즈니스 로직에 관련해서는 서비스 클래스를 만들어 로직을 다루는 것이 좋을 것 같다고 생각했습니다.
이 부분에 대해서는 저도 무어라 딱 대답을 드리긴 어렵네요 ㅎㅎ..
서비스라는게 문맥에 따러 여러 의미를 갖기도 하고, 사람들마다 상황마다 사용하는 방법이 다르기에 정답을 말씀드리기는 어려울 것 같아요!
(어렵다기보단 저도 정답은 모릅니다 ㅎㅎ..😭)
말씀하신 것을 토대로 보면 마틴 파울러님이 언급하신 서비스 레이어의 역할과 유사한 역할을 한다는 생각이 드네요 :)
DTO는 컨트롤러나 뷰에 데이터를 전달할 때 메서드의 매개변수가 3개를 넘어가게 되면 사용하는 것이 좋을 것 같다고 생각했습니다. 많은 종류의 데이터가 전달되지않는데 DTO를 만들고 전달하는 것은 다소 비효율적일 것이라 느꼈습니다. 하지만 많은 종류의 데이터를 주고받는다면 데이터를 전달하는 메서드를 추가하는 것과 함께 DTO를 추가하는 것도 고려해봐야할 것 같습니다.
이렇게 기준을 잡으신 것도 좋지만, 통일성에 대해서도 한번 고민해 보시면 좋겠습니다
누가 세현님의 코드를 보았을 때 어느곳에서는 dto, 어느곳에서는 도메인 객체를 뷰에 전달한다면 혼란스럽지 않을까요?
또한 말씀드렸다시피 도메인 객체를 뷰에 그대로 전달하는 것이 편할 수는 있지만, 그만큼 문제를 일으킬 가능성도 높습니다!
저는 이런저런 방법들 다 좋다고 생각합니다;)
상황에 맞게 적절한 방법을 선택하기 위해서는 각각의 방법의 장단점에 대해서 알고 있어야겠죠?
너무 고생하셨고 남은 명절도 잘 보내세요:)
쉬엄쉬엄 하시죠 😊
|
|
||
| public class GameController { | ||
|
|
||
| private final int END = 0; |
There was a problem hiding this comment.
END에 static을 붙이려는 생각을 하지 않았었습니다..
static에 대한 개념은 배웠지만, 언제 사용해야할지 생각을 안 해봤었어요.
찾아보면서 생각해보니 컨트롤러 내의 메서드들에서 END는 메서드를 종료한다는 의미로 공통적으로 0으로 유지되어 사용되니 static을 붙여도 될 것 같아요.
|
|
||
| private final int END = 0; | ||
|
|
||
| private int turnCount; |
There was a problem hiding this comment.
turnCount 역시나 이렇게 필드로 설정해두면 이후 사용자가 늘어났을 때, 사용자별로 turnCount를 관리할수가 없지 않을까요?
There was a problem hiding this comment.
그렇네요. Player 클래스와 마찬가지로 play() 메서드 내에서 선언하고 매개변수로 전달하면서 다뤄야겠어요.
| private InputView inputView; | ||
| private OutputView outputView; |
There was a problem hiding this comment.
뷰는 변하지않으니 final 선언을 한 뒤에 객체를 생성하는 것도 좋다고 생각합니다.
| inputView = new InputView(); | ||
| outputView = new OutputView(); |
There was a problem hiding this comment.
Play 내부에서 설정되어야 하는 것인지에 대해 고민해 보셨으면 좋겠습니다!
System.in 말고 inputview와 outputview를 모킹해서 테스트를 한번 진행해 보려 하신다면, 현재 상황에서 무엇이 문제점인지 금방 알 수 있으실거예요!
| progressBossHpSetting(bossMonster); | ||
| progressPlayerNameSetting(player); | ||
| progressPlayerStatusSetting(player); | ||
|
|
| if (turnCount == 1) { | ||
| printBossMonsterView(BOSS_NORMAL); | ||
| } | ||
|
|
||
| if (turnCount > 1 && player.getHp() > 0) { | ||
| printBossMonsterView(BOSS_DAMAGED); | ||
| } | ||
|
|
||
| if (turnCount > 1 && player.getHp() == 0) { | ||
| printBossMonsterView(BOSS_WIN); | ||
| } |
There was a problem hiding this comment.
보스의 현재 상태는 BossMonster에서 관리해도 괜찮을 것 같아요!
| public void printBossHpException() { | ||
| System.out.println(PREFIX + "보스 체력은 100이상, 300이하여야합니다."); | ||
| } | ||
|
|
||
| public void printPlayerNameException() { | ||
| System.out.println(PREFIX + "플레이어의 이름은 5자 이하만 가능합니다."); | ||
| } | ||
|
|
||
| public void printPlayerStatusException() { | ||
| System.out.println(PREFIX + "HP는 1이상, HP와 MP의 합이 200이 되도록 입력해주세요."); | ||
| } | ||
|
|
||
| public void printAttackTypeException() { | ||
| System.out.println(PREFIX + "1 또는 2를 입력해주세요."); | ||
| } | ||
|
|
||
| public void printLackOfMPException() { | ||
| System.out.println(PREFIX + "마법 공격에 필요한 MP가 부족합니다."); | ||
| } |
There was a problem hiding this comment.
이 부분 예외 메세지는 비즈니스 로직을 반영하고 있는 것 같아요~
비즈니스 예외는 도메인 객체 내에서 발생시키니, 그곳에서 예외 메세지를 관리하면 관리 포인트가 한곳으로 줄어들지 않을까요?
There was a problem hiding this comment.
넵 관리하는 측면에서 비즈니스 로직과 관련된 예외 메시지는 도메인에서 다루는 것이 더 좋다고 생각합니다.
경우에 따라 다른 예외 메시지는 뷰에서 출력되기때문에 컨트롤러에서 각각 뷰의 다른 예외 출력 메서드를 출력하는 것이 더 역할 분배가 잘 될 것이라 생각했는데, 어떤 비즈니스 로직에서 예외가 생겼을 때 해당 메시지가 발생하는지 찾기가 힘들 수 있겠네요.
| private ByteArrayOutputStream outputStream; | ||
|
|
||
| public void systemIn(String input) { | ||
| System.setIn(new ByteArrayInputStream(input.getBytes())); |
| void playWithCorrectInput() { | ||
| // given | ||
| GameController gameController = new GameController(); | ||
| systemIn("100\ntest\n100,100\n2\n2\n2\n1\n1\n2"); |
There was a problem hiding this comment.
이렇게 테스트 하는 방법 말고, InputView를 모킹하는 방법으로 테스트하는 것은 어떨까요?
GameController는 InputView를 사용하고 있는 것만 알아야 하는데, inputview의 내부에서 System.in을 통해 입력을 받아오는 것을 알고있으면 문제가 될 수 있지 않을까요?
(언제 문제가 될까요? ㅎㅎ)
There was a problem hiding this comment.
언제 문제가 될지 잘 모르겠네요.. 수정하면서 생각해볼게요.
| @DisplayName("보스 몬스터 HP 세팅 - 입력값 정상") | ||
| @Test | ||
| void setHp_with_correct_input() { |
There was a problem hiding this comment.
ParameterizedTest라는 것을 학습해보고 적용해보면 도움되실 것 같아요:)
|
명절 연휴에도 리뷰해주셔서 감사합니다..!! |
|
안녕하세요 말랑님! 명절 연휴 잘 보내셨나요?
|
|
안녕하세요 세현님 😃
너무 잘 경험하셨네요 👍👍
좋습니다:)
우선 한가지 질문을 드릴게요!
그러나 말씀해주신대로, 결국 If문을 사용하게 되는 것은 동일한데요, 만약 이것이 싫다면 다음과 같은 구조도 생각해 볼 수 있을 것 같아요! interface AttackType {
boolean supportTypeNum(int num);
int getDamage();
int getMpChange();
}
class MagicType implement AttackType {
@Override
public boolean supportTypeNum(int num) {
return num == 1;
}
}
...이런 식으로 둔 후, class AttackTypes {
private final List<AttackType> types;
// 생성자
public AttackType get(int num) {
return types.stream()
.filter(it -> it.supportTypeNum(num))
.findAny()
.orElseThrow();
}
}이런 식으로도 풀어볼 수 있을 것 같아요 :) |
|
앗 저도 알림을 못 봐서 지금 확인했네요.. 리뷰 감사드립니다! 뷰에 따라 예외 메세지가 다르게 처리되어야되는 경우에는 우선 도메인에서 처리하던 예외 메세지를 없애야할 것 같아요. 그리고 AttackRepository라는 네이밍을 한 이유는 AttackType을 저장소에서 꺼내온다는 의미로 생각해서였어요. |
안녕하세요. 크루분들 모두 바쁘실텐데 코드 리뷰를 받을 수 있는 기회를 만들어주셔서 감사합니다!
우선 구현 할 때, 이전에 개인 프로젝트로 간단한 게시판 웹사이트를 만들어 본 경험이 있어서 이 경험을 기반으로 mvc 패턴을 활용해봤습니다. 그리고 클래스들은 크게 도메인, 서비스, dto, 컨트롤러, 서비스, 예외 처리로 구분했습니다.
README에 있는 자바 컨벤션과 커밋 컨벤션을 최대한 준수하고, 변수와 메소드명을 신경쓰면서 개발하려다보니 혼자 개발할 때보다 생각할 부분이 꽤 많았습니다. 또한 미션 조건이 있다보니 조건 중 놓친 부분은 없는지, 예외 처리에 부족함은 없었는지를 고려하는 것이 좀 어려웠던 것 같습니다.
아쉬운 점은 코드 리뷰를 받아본 적이 없어서 제 코드가 가독성이 좋은지, 클래스 구분같은 것이 잘 되어 있는지 잘 모르는 상태라는 점과, 테스트쪽은 아직 공부가 부족해서 다소 부족하게 구현했다는 점입니다.
궁금한 점은 앞서 적은 것처럼 제 코드에 대한 전반적인 부분이 어떤지와 고쳐야 할 점이 어떤 것들이 있는지 궁금합니다.