[Step1] 문자열 덧셈 계산기 #477
[Step1] 문자열 덧셈 계산기 #477
Conversation
There was a problem hiding this comment.
미수님 안녕하세요 😃
로또 미션을 함께하게된 김경록입니다. 🙇♂️
1단계 미션 잘 진행해주셨네요. 👍
특히 BehaviorSpec을 활용해서 테스트 코드를 given, when, then 구조로 깔끔하게 잘 작성해주신 점이 인상적이었어요. 🤩
말씀하신 것처럼 companion factory method에 많은 로직이 들어간 것 같아서 관련해서 어떻게 개선하면 좋을지 코멘트 남겨두었습니다. 🙏
이외에도 몇몇 코멘트 남겨두었으니 확인해서 반영해주세요. 🙏
혹시나 미션 진행하면서 어려운 점이 있으시면 언제든지 DM주세요! 🙂
| companion object { | ||
| fun from(expression: String?): Calculator { | ||
| return Calculator(Expression.from(expression)) | ||
| } | ||
| } |
There was a problem hiding this comment.
정적 팩토리 메서드를 사용하신 이유가 있을까요? 🤔
단순 편의를 위함이라고 생각되는데 이런 용도라면 부 생성자를 생성하는 방식도 충분히 고려해볼만한 것 같아요. 🤔
constructor(expression: String?) : this(Expression.from(expression))혹은 따로 부 생성자를 만들 필요 없이 일단은 Calculator 객체를 생성할 때, 외부에서 Expression 객체를 생성해서 Calculator의 생성자 파라미터로 전달하는 방식을 사용하고 추후 Calculator 객체 생성이 너무 불편하다고 느껴지면 부생성자를 만드는게 좋지 않을까 싶어요.
정적 팩토리 메서드(Static Factory Method)는 왜 사용할까? 글을 읽어보시면 좋을 것 같아요. 😃
| @@ -0,0 +1,21 @@ | |||
| package stringcalculator | |||
|
|
|||
| data class Expression(val tokens: List<Int>) { | |||
There was a problem hiding this comment.
상태 값의 네이밍을 tokens 이라는 네이밍보다는 numbers(= 숫자들)나 operands(= 피연산자들)와 같은 네이밍이 더 적절하다고 생각되는데, 미수님은 어떻게 생각하시나요? 😃
There was a problem hiding this comment.
추가적으로 Expression 객체가 객체로써 가지는 책임과 역할이 없는 것 같아요. 🙂
expression 객체가 상태 값으로 List<Int>를 가지는게 좋을지에 대해 먼저 고민해보면 좋을 것 같아요. 👀
Expression은 수식, 식이라는 개념을 가지고 있는데, Expression 객체는 “”, "1,2", "1,2,3", “1,2:3”과 같은 실제로 입력받는 식을 상태로 가지는게 더 적절하지 않을까 싶어요. 🤔
그리고 expression 객체는 수식에서 피연산자들을 분리하는 역할을 하게하면 어떨까 싶어요. 😃
그렇다면 Calculator 객체는 Expression 객체와의 협력을 통해서 자신의 역할을 충실하게 해낼 수 있을 것 같아요. 😉
예를들면 expression.extractOperands()의 결과 값이 operands(List 타입)를 반환할 수 있을 것 같아요.
| val delimiter = it.groupValues[1] | ||
| it.groupValues[2].split(delimiter, *DEFAULT_DELIMITER).map(String::toInt) |
There was a problem hiding this comment.
다른 개발자가 이 코드를 봤을 때, 1, 2 같은 숫자가 어떤 의미를 표현하는지 알 수 있을까요? 🤔
이런 숫자들을 매직 넘버라고해요. 😃
아래의 글을 참고해보시고 매직 넘버의 의미를 명확하게 변경해보면 어떨까요? 🙌
의미가 불분명한 매직 넘버를 상수로 선언하라.
(아래의 0의 케이스도 같이 반영해주세요 🙏)
| return DEFAULT_EXPRESSION | ||
| } | ||
| val result = DEFAULT_REGEX.find(expression) | ||
| val tokens = result?.let { |
There was a problem hiding this comment.
tokens보다는 numbers, operands 같은 네이밍이 의미를 더 잘 드러낼 수 있을 것 같아요. 🤔
| } ?: expression.split(*DEFAULT_DELIMITER).map(String::toInt) | ||
| require(tokens.all { token -> token >= 0 }) { "음수는 인자로 받을 수 없습니다." } |
There was a problem hiding this comment.
문자열 덧셈에 fit한 (ex. Operand or PositiveNumber) VO를 만들어보면 어떨까요? 😃
혹시 VO 개념이 낯설다면 아래의 글을 참고해보시면 좋을 것 같아요. 😉
VO(Value Object)란 무엇일까?
| import io.kotest.core.spec.style.BehaviorSpec | ||
| import io.kotest.matchers.shouldBe | ||
|
|
||
| class ExpressionTest : BehaviorSpec({ |
There was a problem hiding this comment.
BehaviorSpec을 활용해서 given, when, then 구조에 맞춰 테스트 코드 잘 작성해주셨네요. 👍🤩
Expression 객체도 행위에 대한 검증을 해보도록 변경해보면 어떨까요? 🤔
안녕하세요 리뷰어님
문자열 덧셈 계산기 리뷰 요청드립니다!
companion factory method에 많은 로직이 들어간 것 같아, 많이 고민되었는데요.
이부분 의견 주시면 감사하겠습니다!
앞으로 잘 부탁드립니다 😀