-
Notifications
You must be signed in to change notification settings - Fork 74
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
[LBP] 박세은 자동차 미션 1-3단계 제출합니다. #91
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.
안녕하세요, 세은! 👋
미션하느라 고생 많으셨습니다!
- 1단계와 2단계의 테스트 코드를 어떤 방식으로 작성해야 할지 감이 잡히지 않아,
우선 doesNotThrowAnyException()을 사용해 예외 발생 여부를 확인하는 방식으로 구현하였습니다. 이 방식이 의도된 방향과 맞을까요?
또한, 랜덤값처럼 결과값을 지정할 수 없는 경우에는 어떻게 테스트 코드를 작성해야 할지 궁금합니다!
다음 단계 미션과 관련된 내용이라 지금은 넘어가는게 좋을 것 같습니다!
- 현재 List cars = new ArrayList<>();를 활용하여 자동차 객체를 저장하고 있는데, 다른 자료구조를 사용하는 것이 더 효율적일까요?
ex. Map<Integer, Car>
일급 컬렉션을 학습할 때가 온 것 같네요. Car
와 위치
는 key-value로 성립할 수 없기에 Map으로 만드는 것은 어렵지 않을까 합니다. 😄
- 자동차의 상태를 boolean으로 반환하는 것이 적절할까요?
true 또는 false 값을 반환하는 것보다 0 또는 1로 이동 거리를 반환하는 것이 더 직관적인 코드일지 궁금합니다.- Car 객체를 활용한 전체적인 코드 구조가 적절한지 조언 부탁드립니다.
- 각 메서드가 하나의 책임만 갖도록 최대한 분리하였습니다. 작성된 메서드들이 너무 가볍거나 무겁지는 않은지 조언 부탁드립니다.
- 코드 컨벤션, 메서드 네이밍 등 코드 스타일 전반에 대한 조언도 주시면 감사하겠습니다!
코멘트로 남겨두었습니다! 확인해주시고, 코드 리팩터링하신 후 RC 남겨주세요!
파이팅입니다! 👍
src/main/java/Car.java
Outdated
public int getRandom() { | ||
double randomValue = Math.random(); | ||
return (int) (randomValue * 9); | ||
} | ||
|
||
public boolean getStatus() { | ||
int randomValue = getRandom(); | ||
boolean result = true; | ||
|
||
if (randomValue <= 3) { | ||
result = false; | ||
} | ||
|
||
return result; | ||
} | ||
|
||
public void tryMove() { | ||
if(getStatus()) { | ||
this.position++; | ||
} | ||
} |
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 int getRandom() { | |
double randomValue = Math.random(); | |
return (int) (randomValue * 9); | |
} | |
public boolean getStatus() { | |
int randomValue = getRandom(); | |
boolean result = true; | |
if (randomValue <= 3) { | |
result = false; | |
} | |
return result; | |
} | |
public void tryMove() { | |
if(getStatus()) { | |
this.position++; | |
} | |
} | |
public void tryMove() { | |
Random rand = new Random(); | |
if(rand.nextInt(10) > 3) { | |
this.position++; | |
} | |
} |
이 코드는 어떤가요? 자동차의 이동이 난수에 의해 좌우된다는 걸 생각하면 자연스럽다고 생각합니다. 훨씬 이해하기 쉽고, 유지보수하기도 쉽습니다. 매직 넘버(10
, 3
)들도 상수화하면 더 좋겠죠.
물론, 제가 제안 드린 코드도 생각해볼 점은 있습니다. 세은이 궁금해했던 각 메소드의 책임의 관점에서, 사실 단일 책임을 가지진 않았습니다. tryMove()
메소드는 난수도 생성하고, 그에 따라 자동차의 이동도 담당합니다.
다르게 생각해봅시다. 이동을 위한 난수 생성이 과연 자동차의 책임일까요?🤔 세은은 어떻게 생각하시나요?
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개의 메서드를 하나로 통합할 수 있는 방법이 있었군요!
난수 생성 > 조건 확인 > position 증가라는 로직을 순서대로 작성하면서, 각 책임별로 메서드를 분리해야 한다고 생각했는데 Random 클래스를 활용하면 따로 메서드를 만들지 않아도 되어 코드가 더 간결해지는 것 같습니다.
다만, 질문해 주신 것 처럼 "메서드의 책임"을 기능별이 아닌 Car 클래스의 역할을 기준으로 판단한다면, 굳이 여러 메서드로 기능을 나눌 필요가 없을 것 같다는 생각이 드는데, 제가 이해한 것이 맞을까요?
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.
남겨주신 질문이 조금 잘 이해가 가지 않습니다..! 😢
Car
클래스가 가질 여러 책임(기능)들을 굳이 여러 메소드로 나눠 만들 필요가 없다는 말씀이실까요?
그렇다면 제 답은 아니오입니다. 메소드 분리를 기계적으로 할 필요도 없지만 그렇다고 해서 하나의 메소드가 하나의 일만 하도록 분리하지 않는 것도 좋은 방법은 아닙니다. 일단 한 메소드가 여러 기능을 도맡게 되면 코드의 재사용성도 많이 낮아질 뿐더러 유지보수하기도 힘들어집니다.
제가 저 질문을 드린 원래 목적은 자동차 역할과 자동차 경주 역할의 기능들을 더 확실하게 분리해보시길 바란 것이었습니다. 난수 생성이 자동차가 해야할 일인지, 정말 자동차가 해도 괜찮을지를 여쭤보고 싶었습니다. 😁
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.
앗 제가 잘못 설명한 듯 합니다!
제가 말하고 싶었던 것은 책임을 기준으로 판단하는 대상이 메서드뿐만 아니라 클래스도 포함된다는 점입니다.
즉, 기능별로 클래스를 나눈 뒤, 각 클래스 내부에서 다시 책임별로 메서드를 나누면
하나의 클래스 안에 모든 메서드를 작성하는 것이 아니라, 여러 클래스에서 필요한 기능을 가져와 활용하는 형태로 구성할 수 있다는 점을 말하고 싶었습니다,,
제가 저 질문을 드린 원래 목적은 자동차 역할과 자동차 경주 역할의 기능들을 더 확실하게 분리해보시길 바란 것이었습니다. 난수 생성이 자동차가 해야할 일인지, 정말 자동차가 해도 괜찮을지를 여쭤보고 싶었습니다.
한길님이 질문하신 의도에 맞춰 제 답변을 정리하자면
이전 커밋에서 난수 생성은 Car 객체의 position과 관련된 메서드라고 생각하여 Car 클래스에 포함했었습니다.
그러나 난수 생성 기능은 자동차가 전진하기 위한 조건에 해당하는 부분이며, 자동차 경주가 이루어질 때 필요한 로직입니다.
따라서 이 기능을 명확히 나누어 보자면, 자동차 객체의 position보다는 "자동차 경주의 전진 조건"에 더 가깝다는 것을 알게 되었습니다.
결론적으로 생각해보았을 때, 이 부분은 자동차의 책임이 아닌, 자동차 경주의 책임으로 보는 것이 더 적절할 것 같습니다!
조언해주신 부분들을 반영하여 리팩토링을 진행하였습니다! 수정되거나 추가된 부분
질문getRaceAttemptCount 메서드에서 try-catch 문을 활용하여 시도할 횟수가 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.
고생하셨어요, 세은! 👋
전반적으로 개선된 코드를 보니 열심히 하신 것 같군요! 😁
남겨주신 질문에 대한 답은 제가 코멘트로 남겨드렸으나, 명확한 답이 아닌 것 같습니다..
추가로 궁금하시거나 원하던 내용의 답이 아니라면 다시 말씀해주세요!
다음 RC에는 머지하도록 하겠습니다! 😁
개인적인 일정으로 인해 내일 4단계를 시작하게 된다면 기한 내 마무리하기 어려울 것 같아, 현재 열려있는 #91 PR을 머지해주시면, 이후 4단계 PR을 생성하여 함께 올리겠습니다! 혹시 불편을 끼쳐드렸다면 죄송합니다. 앞으로는 일정을 잘 관리하여 차질없이 진행하도록 하겠습니다! 🙇♂️ |
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.
안녕하세요, 세은! 👋
일정 때문에 미리 미션을 진행하신 건 크게 문제가 되지 않습니다. 😄
오히려 일정 관리를 잘하는 개발자라는 생각이 드네요!
이번 단계 미션은 이만 머지하도록 하겠습니다! 👍
고생하셨어요!
구현방식
질문
우선 doesNotThrowAnyException()을 사용해 예외 발생 여부를 확인하는 방식으로 구현하였습니다. 이 방식이 의도된 방향과 맞을까요?
또한, 랜덤값처럼 결과값을 지정할 수 없는 경우에는 어떻게 테스트 코드를 작성해야 할지 궁금합니다!
List<Car> cars = new ArrayList<>();
를 활용하여 자동차 객체를 저장하고 있는데, 다른 자료구조를 사용하는 것이 더 효율적일까요?Map<Integer, Car>
true 또는 false 값을 반환하는 것보다 0 또는 1로 이동 거리를 반환하는 것이 더 직관적인 코드일지 궁금합니다.