Step1 - 문자열 계산기 #1122
Step1 - 문자열 계산기 #1122
Conversation
커스텀 구분자가 들어갔을때 //와 \n은 검증하지 않는 로직 추가 필요.
| @@ -0,0 +1,46 @@ | |||
| package calculator | |||
|
|
|||
| data class StringCalculator(val totalNumber: Int) { | |||
There was a problem hiding this comment.
StringCalcualtor를 data class로 두셨군요?
data class를 사용하신 이유가 있으실까요? 🤔
There was a problem hiding this comment.
StringCalculator가 totalNumber 라는 상태 값을 가지는 것으로 보이는데, 이 상태를 사용하는 위치는 없어보이네요. 🤔
상태 값이 필요하지 않다면 굳이 둘 필요가 없지 않을까요? 😃
There was a problem hiding this comment.
감사합니다. 생각해보니 테스트에서 상태값을 따로 쓰지도 않고 객체끼리 비교등도 하지 않는데 굳이 싶네요
data class의 장점은 불필요한 보일러플레이트 제거라고 생각하는데요, 현시점에서 사용되지는 않으니 제거 처리하였습니다
| data class StringCalculator(val totalNumber: Int) { | ||
|
|
||
| companion object { | ||
| fun calculate(input: String?): Int { |
There was a problem hiding this comment.
kotlin은 nullable type과 non-nullable type을 구분하기 때문에, 애초에 입력으로 null을 재한하는 것이 좋다고 생각해요.
이 부분은 미션 요구사항에서 null인 경우 0을 반환한다고하는 조건이 있기 때문에 이를 위한 null 입력을 받는 케이스를 고려하신 것으로 이해했습니다. 😃
There was a problem hiding this comment.
넵 맞습니다. 미션상에 null인경우 0반환 조건이 있어서 받는 케이스를 고려했습니다 :)
| private fun parseDelimiterAndText(input: String?): Pair<String, String?> { | ||
| var delimiter = "[,:]" | ||
| var text = input | ||
| val result = text?.let { Regex("//(.)\n(.*)").find(it) } |
There was a problem hiding this comment.
Regex는 꽤 비싼 객체인데요. 아래의 글을 참고해서 생성 방식을 개선해보면 좋을 것 같습니다. 😃
There was a problem hiding this comment.
좋은 조언 감사합니다 (_ _) 캐싱전에 일단 컴패니언 객체밖에 없어서 object로 변경했습니다.
인스턴스화를 방지해보기 위해서입니다.
그 다음에 val로 정규표현식을 선언해서 캐싱해보았습니다.
| delimiter = it.groupValues[1] | ||
| text = it.groupValues[2] |
There was a problem hiding this comment.
이 코드를 처음보는 살마의 입장에서는 1, 2와 같은 숫자를 한눈에 파악하기 어려울 수 있을 것 같아요.
There was a problem hiding this comment.
감사합니다
둘다 인덱스를 나타내는 것이니 CUSTOM_DELIMITER_INDEX와 같이 표현해보았습니다.
| if (input.length == 1 && input.all { it.isDigit() }) { | ||
| return input.toInt() | ||
| } |
There was a problem hiding this comment.
굳이 여기에 이런 1개의 숫자만있는 케이스에 대해 분기처리하지 않아도, 동일한 결과를 얻을 수 있을 것 같습니다. 😃
| var text = input | ||
| val result = text?.let { Regex("//(.)\n(.*)").find(it) } | ||
| result?.let { | ||
| delimiter = it.groupValues[1] |
There was a problem hiding this comment.
기능 요구사항에 따르면
앞의 기본 구분자(쉼표, 콜론) 외에 커스텀 구분자를 지정할 수 있다. 커스텀 구분자는 문자열 앞부분의 “//”와 “\n” 사이에 위치하는 문자를 커스텀 구분자로 사용한다.
같이 말하고있어요. 즉, 커스텀 구분자가있다고해서 커스텀 구분자로만 구분이되는 것이 아닌 기본 구분자 + 커스텀 구분자로 구분이 되는 것이 요구사항인 것으로 보여요. 🤔
지금의 로직은 delimiter를 완전히 커스텀 구분자로 변경하는 것 같아서 의도한대로 동작하지 않을 것 같습니다.
테스트 코드에 “//;\n1,2:3;4” 와 같이 기본 구분자 + 커스텀 구분자가 섞인 요소도 추가하고 테스트를 통과하도록 로직을 수정해보시면 좋을 것 같습니다. 😉
There was a problem hiding this comment.
좋은 말씀 감사합니다
커스텀 구분자가 추가되더라도 기본 구분자도 같이 검증하도록 수정했습니다
| return splitString(text, delimiter) | ||
| } | ||
|
|
||
| private fun parseDelimiterAndText(input: String?): Pair<String, String?> { |
There was a problem hiding this comment.
StringCalcualtor가 여러 역할을 수행하고있는 것 같은데요. 🤔
구분자와 수식을 구분하는 역할은 다른 객체에게 위임해보면 어떨까요? 😃
| delimiter = it.groupValues[1] | ||
| text = it.groupValues[2] | ||
| } | ||
| return Pair(delimiter, text) |
There was a problem hiding this comment.
Pair객체를 만들고 싶다면 to infix 함수를 활용할 수도 있�어요! 😃
public infix fun <A, B> A.to(that: B): Pair<A, B> = Pair(this, that)| return Pair(delimiter, text) | |
| return delimiter to text |
안녕하세요 리뷰가 많이 늦었네요 ㅠㅜ
현업에 계속 치이니 자꾸 미뤄지는 것 같습니다
최대한 간결하고, 생성시에 계산을 한번에 하도록 만들고 싶었는데요, 너무 클래스 1개에 다 구현한게 아닌가 하는 생각도 듭니다.( 특히 delimiter와 text를 클래스로 따로 빼고, 거기에 로직을 밀어 넣고 싶은데 좀 잘 안되네요)
리뷰해주시는대로 추가로 수정해보겠습니다