-
Notifications
You must be signed in to change notification settings - Fork 452
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
[2단계 - 자동차 경주 리팩토링] 에버(손채영) 미션 제출합니다. #791
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.
안녕하세요 에버!
2단계 빠르게 구현해주셨네요!👍
1단계에서 이미 많은 부분을 잘 해주셔서
코멘트를 남길게 많이없네요!
간단한 코멘트 남겼으니 확인부탁드려요
|
||
import java.util.Random; | ||
|
||
public class PowerManager { |
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.
PowerManager 👍🏻
객체 책임을 분리한 덕분에 랜덤 값에 대한 테스트가 가능해졌네요!
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.
맞습니다 랜덤 값 테스트를 위해 많이 고민해보았어요.
저의 설계에 100%의 확신이 없었는데 확신이 생겼네요 감사합니다 😊
public boolean checkPowerRange() { | ||
Integer randomNumber = powerManager.generatePower(); | ||
return powerManager.isSufficientPower(randomNumber); | ||
} |
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.
checkPowerRange 메서드명을 살펴볼까요?
이 작명은 해당 메서드를 쓰는 주체가 PowerRange 에 따라 무언가가 달라진다는 비지니스 로직을 알게 돼요.
쓰는 입장에서는 이 자동차가 앞으로 가? 아님 멈춰? 이것만 알면 될 것 같아요.
추가로 boolean 변수명은 보통 is, has, can 과 과같은 조동사나 동사원형으로 이뤄져요!
그러면 메서드 명만 보고도 반환타입이 boolean 이구나 예측할 수 있어서 좋을 것 같아요 ㅎㅎㅎ
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.
그렇겠네요.. 파워의 범위를 체크하는 로직을 굳이 드러낼 필요가 없네요!
리팩토링 과정에서는 네이밍도 더욱 고민해보는 시간이 필요하다고 생각하게 되었습니다
피드백 감사합니다!
@DisplayName("자동차 이름이 5자가 넘을 경우 에러가 발생한다.") | ||
@ParameterizedTest |
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를 기준으로 4, 5, 6 을 테스트 하는 것을 말해요.
1~ 5까지만 자동차 이름이 허용되면 아래 영역만 테스트해도 신뢰도가 80 이상 된다고 믿는 테스팅 방법이에요
- -1, 0, 1 ,
- 4,5,6
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을 제외한 값들을 테스트 케이스에 추가해보았어요!
감사합니다 🥰
수달이 말씀주신 부분 반영하여 추가 커밋 하였습니다. |
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단계도 고생 많으셨어요!
안녕하세요 수달!
1단계 구현에 이어 2단계 리팩토링 리뷰 요청 드립니다.
1단계 미션 피드백에서 정말 많은 도움을 받았는데,☺️
아직 코드 리뷰 받는 것이 어색해 수달의 피드백을 받고 혼자 생각하는 시간과 코드 수정 시간만 갖고 넘어간 것 같아요.
앞으로는 조금 더 적극적으로 생각을 코멘트 해달라는 수달의 피드백 반영하려고 합니다
이번 미션에서는
PowerStrategy
와PowerGenerator(현 PowerManager)
클래스의 책임 분배에 신경을 썼어요.1단계에서
PowerStrategy
에 대한 단위 테스트를 작성할 때'랜덤값 생성 후 4 이상인지 비교'하는
PowerStrategy
의checkRandomNumberRange()
를 테스트하기 어려워다른 메서드들의 테스트를 통해 간접적으로 해당 테스트의 동작을 확인했는데요.
1단계에서 수달이 리뷰해 주었던 내용을 참고해 다시 한 번 2단계 리팩토링을 거치면서
랜덤 숫자 생성 역할과 숫자 범위 체크 역할을
PowerGenerator
로 분리시켜PowerStrategy
에서 사용하도록 하였습니다.하지만 그럴 경우 '파워를 생성'하는
PowerGenerator
가 '파워의 범위를 체크'하는 책임까지 부여하게 됩니다.그리하여 클래스 이름을
PowerManager
로 변경하여 책임의 범위를 조금 넓혀보았어요.1단계에서는 페어와 함께였지만, 2단계에서는 저 혼자의 미션이라 아직 클래스의 책임을 분배하는 과정이 많이 어색한데
저의 이러한 생각의 흐름과 설계 방법이 어떤지 수달의 생각이 궁금합니다!
이 외에도 반복된 코드를 줄이고, 구현한 코드를 업그레이드하는 과정을 거쳤습니다.
감사합니다!