-
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
[보스 몬스터 잡기] 이도현 미션 제출합니다. #6
base: ldhapple
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기 말랑이라고 합니다:)
미션 고생하셨고 코드 잘 봤습니다!
깔끔하게 작성해 주셔서 리뷰하는데 큰 어려움이 없었어요!
코드를 전체적으로 보았을 때, 계층화나 싱글톤같은 디자인 패턴을 이미 어느정도 학습하신 것 같다고 느꼈습니다👍
전체적인 피드백을 드릴게요!
잘 알려진 코드 스타일 가이드를 따라 코드를 작성해 주셨으면 좋겠습니다!
(메서드 순서나 공백 등)
커밋 단위를 세분화해주시면 코드를 이해하는데 더 도움이 될 것 같습니다 :)
계층화나 디자인 패턴과 같은 것들을 어떠한 기준을 가지고 적용하시는지가 조금 불명확한 것 같아서 의견을 여쭙고 싶습니다.
예를 들어 현재 싱글톤 패턴을 왜 사용하셨는지, 또는 왜 사용하지 않으셨는지 궁금합니다!
또한 이를 사용했을 때 어떠한 문제점이 있는지 등을 고민해 보셨다면 그 내용을 알려주시면 피드백 하는데 좋을 것 같습니다:)
객체를 객체답게 사용하기보단 데이터를 담는 그릇 정도의 역할로만 사용하시는 것 같다는 느낌을 받았어요!
이러한 구조가 장점을 갖는 경우도 있겠지만 단점을 갖는 경우도 많을 것 같은데, 이들의 장단점에 대해 고민해 보시면 좋겠습니다!
(간단하게는 코드가 객체지향적이라기보단 절차지향적이라는 느낌을 많이 받았어요!)
private static final PlayerController playerController = new PlayerController(); | ||
public void setHp(int hp){ |
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 class BossController { | ||
|
||
private static final Boss boss = Boss.getInstance(); |
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를 싱글톤으로 만들어 주셨네요!
이렇게 만드신 이유가 있으실까요?
또 이러한 구조는 어떤 상황에 문제가 될 수 있을까요?
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.
억지로 BossController, PlayerController를 만들다보니 이곳저곳에서 Player와 Boss 객체를 가져와 사용했을 때 의도치 않은 객체의 변경이 있을까해서 싱글톤으로 구현했습니다. 이런 구조는 테스팅하기 매우 힘들다는 단점이 있습니다.
여기서는 싱글톤 패턴이 아닌 클래스 변수에 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.
질문의 의도와는 조금 다르지만 테스팅하기 어렵다는 단점도 있을 수 있겠네요!
그러나 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.
같은 객체를 공유해야해서 원하지 않는 결과를 도출할 것 같습니다.
여러 사용자가 동시에 사용할 수 있도록 하려면 제 생각에는 구조를 바꾸어 Player 객체나 Boss 객체를 한 곳에서만 생성하도록 하고 다른 곳에서 사용해야 한다면 그 객체를 주입하는 객체를 따로 두어 해결해야 할 것 같습니다.
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와 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.
지금 구조에서 컨트롤러를 3개로 나누어 미처 생각하지 못한 부분인데 말씀대로 꼭 정의하지 않고 입력에 따라 생성하는 과정으로 진행해도 좋을 것 같습니다. 좋은 의견 감사합니다. :)
public class BossController { | ||
|
||
private static final Boss boss = Boss.getInstance(); | ||
private static final PlayerController playerController = new PlayerController(); |
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.
객체의 생성의 관점에서 싱글톤 패턴을 사용한 것이 아닌 객체의 변경 위험에 대한 대처로 싱글톤 패턴을 사용했던 것이라 신경쓰지 못하고 그냥 만든 것 같습니다.
어떤 패턴을 사용할 때 왜 사용하는지, 꼭 사용해야 하는지에 대해 잘 생각하고 적용해야될 것 같습니다.
System.out.printf("보스가 공격 했습니다. (입힌 데미지: %d) \n", damage); | ||
} | ||
|
||
public void getAttack(int damage){ |
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.
피격받음을 표현하고 싶은데 네이밍이 getXXX이라 일반적인 getter를 생각해서 혼동을 줄 수 있을 것 같아요!
if(boss.getCurrent_hp() < 0) { | ||
boss.setCurrent_hp(0); | ||
}else{ | ||
boss.setState(2); |
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.
상태를 2로 만든다가 어떤 의미인지 한눈에 파악할 수 있을까요?
private final Scanner sc = new Scanner(System.in); | ||
private final PrintMessage printer = new PrintMessage(); | ||
private static final BossController bossController = new BossController(); | ||
private static final PlayerController playerController = new PlayerController(); | ||
public void run(){ |
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.
static이 가장 위로, static과 non-static 사이에 공백 한칸,
필드와 메서드 사이 공백 한 칸 부탁드려요!
if(hp < 100 || hp > 300){ | ||
throw new IllegalArgumentException("[ERROR] 보스몬스터의 체력은 100 이상 300 이하여야 합니다."); | ||
} |
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에 대한 비즈니스 규칙을 main 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.
지금 당장은 문제가 없을 수 있지만 규모가 커지고 해당 부분을 여러 곳에서 사용해야 한다면 수정할 때 문제가 생길 것 같습니다.
개선해보면 지금의 구조에서는 bossController에서 해당 부분을 메서드로 만들어 사용할 수 있을 것 같습니다.
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.
지금 당장은 문제가 없을 수 있지만 규모가 커지고 해당 부분을 여러 곳에서 사용해야 한다면 수정할 때 문제가 생길 것 같습니다.
👍👍
개선해보면 지금의 구조에서는 bossController에서 해당 부분을 메서드로 만들어 사용할 수 있을 것 같습니다.
반드시 bossController가 필요한지에 대해서도 고민해 보시면 좋을 것 같아요 :)
try{ | ||
System.out.println("플레이어의 이름을 입력해주세요."); | ||
String name = sc.next(); | ||
if(name.isEmpty() || 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.
마찬가지로 플레이어에 대한 비즈니스 규칙을 main controller에서 검증하고 있는데, 이런 경우 어떠한 문제점이 생길 수 있을까요?
|
||
import domain.Player; | ||
|
||
public class PlayerController { |
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.
PlayerController, BossController가 필요한 구조인지 한번 고민해 보시면 좋을 것 같습니다!
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 객체와 Boss 객체에서 처리하는 것이 이 프로그램에서는 더 깔끔했을 것 같습니다. 구현하면서도 Player와 Boss에서 처리하면 쉽게 되는 것을 처리하지 않으려 Controller에서 억지로 처리했던 부분이 몇몇 있습니다.
따로 구현한 이유는 Player, Boss를 Model로 각 컨트롤러를 Controller로, PrintMessage 부분을 View로 구성해보고 싶었기 때문입니다.
앞으로는 저런 구조에 억지로 맞출 필요 없이 각 프로그램에 알맞은 구조로 구현해야 될 것 같습니다.
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.
특정 구조에 얽매이는 것은 좋지 않다고 생각합니다 :)
그리고 하나의 Controller에서 Player와 Boss라는 Model을 처리하는 구조도 괜찮아 보입니다!
@@ -0,0 +1,43 @@ | |||
package domain; | |||
|
|||
public class Boss { |
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에도 해당되는 내용인데요, 이 두 클래스는 비즈니스 로직을 처리하기 보단 값을 보관하는 용도로밖에 사용되지 않는 것 같습니다!
이러한 이유가 이들의 비즈니스 로직을 controller에서 처리하고 있기 때문인데요, 이들을 어떻게 개선하면 좋을 지 한번 고민해 보시면 좋을 것 같습니다!
보스 몬스터가 플레이어를 공격하는 것으로 한 가지 예시를 드릴게요
class Controller {
public void attackPlayer() {
int 랜덤데미지 = ...;
player.setHp(랜덤데미지);
if (player.getHp() < 0) {
player.setHp(0);
}
}
}
위가 지금까지 작성한 코드와 비슷한 느낌으로 작성한 코드인데요, 이를 조금 더 객체지향적으로 바꿔보면
class Controller {
public void attackPlayer() {
boss.attack(player);
}
}
class Boss {
public void attack(Player player) {
int 랜덤데미지 = attackDamage();
player.attacked(랜덤데미지);
}
}
class Player {
public void attacked(int damage) {
hp -= damage;
if (hp < 0) {
hp = 0;
}
}
}
위와 같이 작성할 수 있어요!
위 구조가 기존 코드에 비해 어떤 장단점을 가지는지에 대해서는 고민해보시고 의견 말씀해주시면 감사하겠습니다 :)
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.
플레이어의 속성값과 보스의 속성값을 각각의 객체에서 담당하여 각 객체가 담당하는 역할이 명확해지고 추후 수정이 쉽다는 장점이 있을 것 같습니다.
👍
단점으로는 제 생각엔 딱히 없는 것 같지만 굳이 꼽자면 코드만 보았을 때 프로그램에 대한 이해가 지금의 코드보다는 조금 어려워질 수 있다고 생각합니다.
👍
코드를 객체지향적으로 작성할수록 캡슐화되는 부분이 많아지기 때문에 코드를 이해하는 데 더 많은 depth를 타고 들어가야 하므로 이해가 어려워질 수도 있습니다.
그래서 항상 객체지향을 고집하기보다는 상황에 따라 장단점을 잘 비교하여 코드를 작성하는 것이 중요하다 생각합니다 :)
이어서 질문에 대한 답변 드릴게요!
컨트롤러를 제어하는 컨트롤러의 관점으로 보았을 때는 사용해도 괜찮다고 생각합니다!
단점은 이 둘 모두 똑같다고 생각합니다!
Domain 객체가 무엇이라고 생각하시는지 답변을 듣고 싶어요:D |
고민 사항
Spring을 학습하다가 순수 Java코드의 구조를 설계 해보려하니 간단한 기능임에도 어려움을 겪었습니다.
구현하면서 이건 아닌 것 같은데 했던 부분에 대해 피드백을 받아보고 싶습니다.