보스 잡기 미션 완료했습니다. 리뷰 부탁드립니다. #19
Conversation
…스를 받아서 저장하도록 변경 Player와 BossMonster 각각 인스턴스 생성 시, 생성자로 전달되는 인자를 검증하고 입력을 다시 받기 위함
| ## 주요 메시지 구성 (공용 인터페이스) | ||
|
|
There was a problem hiding this comment.
요청 사항을 굉장히 깔끔하게 적어주신 것 같아요!
하지만 우려되는 점도 있는데요. 정성스레 작성해주신 문서지만 동료 입장에서는 함수 이름이나 인자 등을 글로 표현하는 것보다 함수 형태를 보여주는 것이 가독성이 더 좋을 수도 있겠다는 생각을 했어요!
책들도 보면 글보다는 코드로 먼저 보여주잖아요:)
public void start() {
// 게임 시작 로직
}이런 느낌으로요!
어떻게 생각하시나요?
There was a problem hiding this comment.
아직은 기능 목록 문서에 대해서 누군가 본다는 생각보다는 제가 구현에 앞서서 공부한 내용으로 이해하기 쉽게 정리하는 것을 목적으로 작성했었는데요. 개발은 혼자 하는 것이 아니기에 말씀 주신 부분에 대해서도 신경을 쓰면서 작성을 해야 할 것 같습니다. 감사합니다.
제가 밑에서부터 리뷰에 대한 답글을 남겼습니다. 리뷰를 요청하고 받아 보는게 처음이라 이렇게 도움이 많이 되는지 몰랐네요. 아직 공부를 시작한지 얼마 되지 않아 코드 읽기가 많이 힘드셨을텐데 늦은 시간에도 리뷰 해주셔서 감사합니다. 주신 의견들 모두 참고해서 제가 개선해 나가야 할 방향인 것 같습니다. 리뷰 감사드립니다. 좋은 하루 되세요!
| public static <T> T retryInput(Supplier<T> supplier) { | ||
| while (true) { | ||
| try { | ||
| return supplier.get(); | ||
| } catch (Exception exception) { | ||
| OutputView.printError(exception); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
이렇게 만들 생각을 하진 못했는데, 제네릭을 정말 잘 활용하신 것 같아요!
하지만 함수명에서 오해의 소지가 있다고 생각해요. 첫 시도를 할 때도 해당 함수를 사용하기 때문인데요. 오류가 나지 않을때까지 계속 재입력 받는 것이기 때문에 다른 네이밍이 어울릴 것 같아요! 음... 마땅히 떠오르는 네이밍은 없네요ㅠㅠ
There was a problem hiding this comment.
추가적으로 클래스명이 어울리지 않는듯한 느낌을 받았어요. 스프링을 생각해서 네이밍을 정하신 것 같아서 스프링의 ExceptionHandler와 비교해서 말씀드리겠습니다!
ExceptionHandler의 경우 말 그대로 예외를 핸들링해주는 역할입니다. A라는 예외가 발생하면 ExceptionHandler가 그에 맞게 처리해주고, B라는 예외가 발생하면 그것도 그것의 ExceptionHandler에 맞게 처리해주는 역할이죠. 스프링의 ExceptionHandler처럼 예외를 핸들링하는 그 자체에만 목적이 있어야한다고 생각해요.
하지만 작성하신 코드에서는 예외가 발생했을 때 이벤트가 예외를 감지해 예외를 핸들링하는 것이 아니라, 마치 예외가 발생할법한 함수들을 대신 실행해주는 역할같아요. 넵, 그저 함수형 인터페이스죠. 그렇다면 이것에 맞는 네이밍으로 하는 것이 좋지 않을까요?
만약 ExceptionHandler를 사용하고 싶으면 아래와 같이 되어야할 것 같습니다.
public static T handle(Exception e) {
// 핸들링
}There was a problem hiding this comment.
저도 에러 발생 시, 재입력을 받아야 하는 부분에 대해서 고민을 많이 하다가.. 다른 분들의 코드 리뷰를 보고 여러가지 찾아보다 이런 방법도 있다는 것을 알게 되었습니다. 하나 만들어 두면 활용성이 좋은 것 같아요. 네이밍에 대해서는 말씀 주신 부분처럼 가독성이 안 좋아 질 수 있기 때문에 더 고민이 필요할 것 같습니다. 막상 네이밍에 대해서는 많이 신경을 쓰지 못했던 것 같네요.
클래스명에 관해 의견 주신 부분에 대해서도 감사합니다. 스프링에 대한 개념이 아직 없어서 스프링의 ExceptionHandler가 어떻게 사용되는지 처음 알았네요.. 저는 단순히 예외를 관리한다는 부분에서 저렇게 사용했는데 다른 분들이 읽기에는 헷갈릴 수 있을 것 같습니다. 이 부분도 참고하겠습니다. 감사합니다!
|
|
||
| import bossmonster.view.constants.Message; | ||
|
|
||
| public class Validator { |
There was a problem hiding this comment.
해당 클래스를 exception 패키지에 넣으신 이유가 궁금합니다!
hp + mp는 200이어야한다 등... 비즈니스 로직이 담긴 부분이 있는데 domain 패키지와 분리되어 있어서요!
There was a problem hiding this comment.
처음에는 입력 단계에서 검증하는 부분만 따로 넣고, 도메인 부분에서 검증할 로직은 각 도메인 클래스에서 진행하는 것으로 했었는데요. 하다보니 겹치는 부분이 있는 것 같아서 잘 활용하지 못했습니다. 위에서 의견을 주신 것 처럼 DTO를 활용하면 더 깔끔하게 검증 로직을 분배할 수 있을 것 같습니다.
| private PlayerStatus status; | ||
|
|
||
| public Player(String name, int maxHP, int maxMP) { | ||
| this.name = Validator.validatePlayerName(name); |
There was a problem hiding this comment.
controller에서 Validator를 사용할 때도 있고, domain에서 사용할 때도 있군요.
어떤 기준으로 분리하셨는지 궁금합니다!
There was a problem hiding this comment.
밑에 Validator의 사용에 대해 주신 의견과 동일한 문제네요.. 입출력에 관련된 검증과 도메인 부분의 검증은 분리해서 사용하는게 맞을 것 같습니다. 감사합니다!
| import java.util.ArrayList; | ||
| import java.util.List; | ||
|
|
||
| public class GameHistory { |
There was a problem hiding this comment.
컨트롤러에서 게임을 제어하는 것 보다 RaidGame 클래스에서 전체적인 게임 로직이 돌아가는 형태로 생각하고 구현하다 보니 게임이 실행되면서 히스토리를 남기고 컨트롤러에서 출력을 위해서는 기록에 남겨진 부분을 가져다가 보여줘야 의존성이 떨어질 것 같다고 생각했습니다. 실제로 턴 제 게임을 하면 매 턴마다 턴에 대한 기록이 게임에서도 남게 되는데 이걸 생각해서 만들었던 것 같습니다!
| public boolean isGameStatus() { | ||
| return gameStatus; | ||
| } |
There was a problem hiding this comment.
isFinish()나 isPlaying()와 같은 것은 어떨까요?
There was a problem hiding this comment.
단순히 현재 게임 상태를 묻기 위해 썼던 것 같은데 의견 주신 메서드명이 반환되는 값을 이해하기에 더 직관적이네요!
| } | ||
|
|
||
| private Player createPlayer(String playerName) { | ||
| String[] playerStatus = Validator.validateInputOfNumbers(InputView.readLine()); |
There was a problem hiding this comment.
DTO로 받았다면 인덱스를 사용하지 않고 플레이어 정보를 받았을 수 있을 것 같아요!
There was a problem hiding this comment.
제가 작성한 코드의 InputView는 단순히 입력을 받는 메서드만 존재 하는데요. 이렇게 구성하다 보니 컨트롤러에서 입력받은 부분에 대해서 검증을 하고 변환해서 도메인을 생성하거나 데이터를 넘겨줘야 해서 할 일이 많아지는 단점이 있네요. 말씀주신 것처럼 InputView 쪽에서 입력을 받고 바로 입력에 대한 부분을 검증해서 DTO를 생성하여 건네주는 방식을 연습해봐야 할 것 같습니다. 좋은 의견 감사합니다!
| public void executeTurn(AttackType attackType) { | ||
| if (isGameProgress()) { | ||
| increaseTurn(); | ||
| int playerAttackDamage = DEFAULT_VALUE; | ||
| int monsterAttackDamage = DEFAULT_VALUE; | ||
| playerAttackDamage = executePlayerTurn(attackType); | ||
| if (bossMonster.isAlive()) { | ||
| monsterAttackDamage = executeBossMonsterTurn(); | ||
| } | ||
| setStatus(); | ||
| createTurnHistory(attackType, playerAttackDamage, monsterAttackDamage); | ||
| } | ||
| } |
There was a problem hiding this comment.
컨트롤러와 도메인을 정말 잘 분리하신 것 같습니다!
There was a problem hiding this comment.
감사합니다. 객체지향을 공부하면서 MVC 패턴에 구애받지 않고 도메인 로직은 도메인 로직에서만으로도 돌아갈 수 있도록 구현하고자 연습하고 있습니다. 컨트롤러가 없어도 도메인 내에서는 도메인 로직이 잘 돌아가야 한다고 생각하면서 고민했던 것 같아요!
| public int attack(AttackType attackType) { | ||
| if (attackType.getName().equals(AttackType.ATTACK_TYPE_NORMAL.getName())) { | ||
| recoverMP(attackType.getMpRecover()); | ||
| } | ||
| if (attackType.getName().equals(AttackType.ATTACK_TYPE_MAGIC.getName())) { | ||
| consumeMP(attackType.getMpUsage()); | ||
| } | ||
| return attackType.getDamage(); | ||
| } |
There was a problem hiding this comment.
이 부분 어려우셨을텐데 역할분배 잘하신 것 같아요!
| @@ -0,0 +1,63 @@ | |||
| package bossmonster.view.constants; | |||
|
|
|||
| public enum Message { | |||
There was a problem hiding this comment.
저도 상수를 한 파일로 관리했었는데요.
상수를 한 파일로 관리하게 되면 팀 프로젝트를 진행할 때 상수 파일에서 충돌이 많이 일어나더라구요.
이런 문제를 어떻게 해결하면 좋을까요?
There was a problem hiding this comment.
입출력에 사용되는 메시지들을 InputView와 OutputView에서 각각 사용하다 보니 메시지가 많은 경우에는 자리를 많이 차지하는 것 같아서 한 파일로 관리하고 가져다 쓰는 형태로 해야겠다고 생각했습니다. 제가 공부한지 얼마 되지 않아 팀 프로젝트를 경험해본 적이 아직 없어 어떤 부분에서 충돌이 일어나는지는 잘 모르겠습니다. 분명 상수를 한 파일로 관리하다보면 문제가 생길 수도 있을 것 같은데요. 이 부분은 여러 프로젝트를 진행하면서 조금 더 고민해 봐야 할 부분인 것 같습니다. 의견 감사합니다!
No description provided.