Skip to content
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

[윤혁] 1주차 2단계 문자열계산기 #24

Merged
merged 4 commits into from
Jul 26, 2020

Conversation

malibinYun
Copy link
Member

@malibinYun malibinYun commented Jul 24, 2020

미션 제출합니다.

궁금한건 Java에서는 한 메소드만을 지닌 interface를 람다로 바로 쓸 수 있고, 코틀린에서는 SAM이라는걸 통해서 람다로 쓸 수 있게 변환해준다고 들었습니다. 코틀린 1.4이상부터는 interface도 람다로 쓸 수 있다고 들었는데, 그 이전에는 불가능한건지도 궁금합니다.
코틀린에서 interface로 선언한 것을 람다로 쓸 수 없어 Operator의 프로퍼티로 람다를 갖게 했습니다. 가독성을 위해 typealias 키워드를 사용했습니다.
override코드를 쓰고 싶지 않고 한 줄로 해결하고싶어 위 방법대로 했으나, 어떤 방법이 더 나은방법인지 잘 모르겠습니다.

Copy link

@kyucumber kyucumber left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

안녕하세요 1주차 미션 리뷰어를 맡은 김규남이라고 합니다.

간단한 몇가지 코멘트를 남겨두었으니 확인 부탁드립니다.

@@ -0,0 +1,21 @@
package step2

typealias CalculateStrategy = (leftValue: Operand, rightValue: Operand) -> Operand

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이 부분은 취향의 차이겠지만 당장은 CalculateStrategy가 사용되는 곳이 Operator.kt 내부밖에 없기 때문에 typealias를 사용하지 않고 조금 길더라도 (leftValue: Operand, rightValue: Operand) -> Operand로 정의하는건 어떨까요?

typealias CalculateStrategy = (leftValue: Operand, rightValue: Operand) -> Operand

enum class Operator(private val symbol: String, private val strategy: CalculateStrategy) {
ADD("+", Operand::add),

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

아래와 같은 질문을 남겨주셨더라고요

코틀린 1.4이상부터는 interface도 람다로 쓸 수 있다고 들었는데, 그 이전에는 불가능한건지도 궁금합니다.

이전 자바에서 (a,b) -> a + b와 같이 표현하는 방식이 불가능하냐고 질문 주신게 맞을까요?
제가 잘못 이해했을수도 있는데 만약 이 질문이 맞다면 아래와 같이 Java와 유사하게는 사용이 가능합니다. (현재는 Operand의 value가 private이라 변경이 필요합니다.)

ADD("+", { a, b -> Operand(a.value + b.value)}),

만약 Double 타입이었다면 아래와 같이 정의되겠네요

ADD("+", { a, b -> a + b}),

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

혹시 질문의 의도가 이와 다르다면 DM을 남겨주시거나, 리뷰 재 요청 시 코멘트 남겨주시면 확인하겠습니다.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

기존에는 람다식을 활용해서 코드를 짜지 않고,

    interface CalculateStrategy {
        fun calculate(leftValue: Operand, rightValue: Operand): Operand
    }

이 인터페이스를 내부적으로 만들어서 매개변수로 갖게 했습니다. 이렇게 하면 람다로 표현할 수 없더라구요.. 또, Operand::add 이것또한 넣을 수 없게되었구요.. 제가 잘못 알고 못쓰는 것일 수 있습니다 ㅠㅠ

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

아, 질문을 다시 읽어보았습니다.

질문의 일부만 보고 넘어가버려서 질문 내용을 제대로 이해하지 못했네요.

말씀하신 부분은 저도 코틀린을 처음 사용할 때 고생을 많이 했던 것 같네요

higher-order function의 경우 제가 예시로 남겨드린 것 처럼 깔끔하게 lambda 처리가 가능한데 인터페이스의 경우 제 기억에는 lambda 사용이 깔끔하게 잘 되지 않았던 것 같습니다.

Java interface

자바 코드에 정의된 인터페이스의 경우 말씀하신것 처럼 SAM 변환을 이용해 아래처럼 사용이 가능하고..

ADD("+", CalculateStrategy { a, b -> a + b });

Kotlin interface

    ADD("+", object: CalculateStrategy {
        override fun calculate(left: Double, right: Double): Double {
            return left + right
        }
    });

코틀린은 위와 같이 조금 지저분하게 처리해야 했었는데..

말씀하신 내용을 보고 1.4 RELEASE 문서를 찾아본 다음에 테스트 해보니 이제 아래와 같이 정의가 가능하네요.

fun interface CalculateStrategy {
    fun calculate(left: Double, right: Double): Double
}

enum class Operator(
    private val symbol: String,
    private val calculateStrategy: CalculateStrategy
) {
    ADD("+", CalculateStrategy { a, b -> a + b });

    fun calculate(left: Double, right: Double): Double {
        return calculateStrategy.calculate(left, right)
    }
}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

아아 그럼 1.4 이전에서는 못쓰는게 맞군요 ㅠㅠ 아쉽습니다...
그나저나 이렇게 상세하게 코드를 쓰셔서 알려주시다니 너무너무 감사합니다!!! ㅠㅠ 리뷰어님의 정성이 느껴집니다.. 정말 감사드립니다 😁😁😁

Comment on lines +16 to +20
private fun validateInputIsNotZero(input: Operand) {
if (input.value == 0.0) {
throw IllegalArgumentException("0으로는 나눌 수 없습니다.")
}
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

예외처리 잘 구현해주셨습니다.

알고 계실 것 같지만 추가로 공유 드리면
0으로 나눌 때, 타입 별 아래와 같은 결과가 반환됩니다.

  • Int 타입의 경우 ArithmeticException
  • Double을 0으로 나눌 경우 Infinity가 반환

따라서 때에 따라 IllegalArgumentException를 던지지 않아도 ArithmeticException을 직접 잡아주실수 있습니다.

ADD("+", Operand::add),
MINUS("-", Operand::minus),
MULTIPLY("*", Operand::multiply),
DIVIDE("/", Operand::divide);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

혹시 validate의 수행을 위해 Operand 클래스를 만들고 Double 타입을 래핑하신거라면

조금 지저분해지겠지만, 아래와 같이 사용할수도 있겠네요!

    DIVIDE("/", { a, b ->
        if (b == 0.0) { throw IllegalArgumentException("0으로는 나눌 수 없습니다.") }
        a / b
    });

companion object {
fun findBySymbol(symbol: String): Operator {
return values().find { operator -> operator.symbol == symbol }
?: throw IllegalArgumentException("$symbol 기호에 해당하는 연산은 존재하지 않습니다.")

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Elvis Operator를 잘 사용해주셨네요!

private const val INDEX_OF_OPERAND = 1

fun calculate(splitValues: List<String>): Operand {
var firstValue = Operand(splitValues[0].toDouble())

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

변수 명이 firstValue인데 최종 연산 이후 리턴값으로 반환되고 있어서 조금 혼란을 줄 수 있겠네요

변수명을 변경하는건 어떨까요?

Comment on lines 12 to 14
val operator = Operator.findBySymbol(splitValues[INDEX_OF_OPERATOR + i])
val operand = Operand(splitValues[INDEX_OF_OPERAND + i].toDouble())
firstValue = operator.calculate(firstValue, operand)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

아래 남긴 코드가 좋다고는 말씀 못 드리겠지만 with 같은 함수도 활용할 수 있겠네요

with(Operator.findBySymbol(splitValues[INDEX_OF_OPERATOR + i])) {
    firstValue = this.calculate(firstValue, Operand(splitValues[INDEX_OF_OPERAND + i].toDouble()))
}

Comment on lines +11 to +16
@CsvSource("+,ADD", "-,MINUS", "*,MULTIPLY", "/,DIVIDE")
@ParameterizedTest
fun `유효한 연산 기호 찾는지 확인`(symbol: String, operator: Operator) {
// then
assertThat(Operator.findBySymbol(symbol)).isEqualTo(operator)
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

전체적으로 @ParameterizedTest를 적절하게 잘 사용해주셨습니다.

Copy link

@kyucumber kyucumber left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

안녕하세요, 리뷰 후 바로 다시 요청 주셨었군요

남겨주신 내용에 대해 답변 추가로 달아두었고, 요구사항 누락된 게 있어 코멘트 남겨두었습니다.

@@ -0,0 +1,3 @@
package step2.view

fun getSplitStringForCalculate() = readLine()!!.split(" ")

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

입력값이 null이거나 빈 공백 문자일 경우 IllegalArgumentException throw

입력값이 null인 경우 NPE가 발생하겠네요! 위 요구사항에 대한 기능 추가가 필요해보입니다.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

아.. 이걸 빼먹었군요.. 죄송합니다 ㅠㅠ 요구사항을 제대로 파악하지 못했나봅니다. 바로 피드백 반영하겠습니다.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

아래 코드도 깔끔한 느낌은 아니지만, !! Operator를 사용하는것 보다는 나을 수 있어서 참고용으로 남겨드렸습니다.

fun getSplitStringForCalculate() = readLine()?.split(" ") ?: run {
    throw IllegalArgumentException("null이 올 수 없다.")
}

typealias CalculateStrategy = (leftValue: Operand, rightValue: Operand) -> Operand

enum class Operator(private val symbol: String, private val strategy: CalculateStrategy) {
ADD("+", Operand::add),

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

아, 질문을 다시 읽어보았습니다.

질문의 일부만 보고 넘어가버려서 질문 내용을 제대로 이해하지 못했네요.

말씀하신 부분은 저도 코틀린을 처음 사용할 때 고생을 많이 했던 것 같네요

higher-order function의 경우 제가 예시로 남겨드린 것 처럼 깔끔하게 lambda 처리가 가능한데 인터페이스의 경우 제 기억에는 lambda 사용이 깔끔하게 잘 되지 않았던 것 같습니다.

Java interface

자바 코드에 정의된 인터페이스의 경우 말씀하신것 처럼 SAM 변환을 이용해 아래처럼 사용이 가능하고..

ADD("+", CalculateStrategy { a, b -> a + b });

Kotlin interface

    ADD("+", object: CalculateStrategy {
        override fun calculate(left: Double, right: Double): Double {
            return left + right
        }
    });

코틀린은 위와 같이 조금 지저분하게 처리해야 했었는데..

말씀하신 내용을 보고 1.4 RELEASE 문서를 찾아본 다음에 테스트 해보니 이제 아래와 같이 정의가 가능하네요.

fun interface CalculateStrategy {
    fun calculate(left: Double, right: Double): Double
}

enum class Operator(
    private val symbol: String,
    private val calculateStrategy: CalculateStrategy
) {
    ADD("+", CalculateStrategy { a, b -> a + b });

    fun calculate(left: Double, right: Double): Double {
        return calculateStrategy.calculate(left, right)
    }
}

@malibinYun
Copy link
Member Author

readLine에서 null을 받아오는 경우는 어떻게 처리해야면 좋을지 몰라 null인경우 "" 공백문자를 받게 처리했습니다.
그 이후 공백인경우에는 split해도 List에 공백값이 들어있기에 그러한 경우 validation check를 수행했습니다.

Copy link

@kyucumber kyucumber left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

고생 많으셨습니다.

readLine의 nullable 값 예외 처리에 대해 코멘트 하나를 남겨두었으니 확인부탁드릴게요

다음 단계 진행을 위해 머지하겠습니다.

@@ -0,0 +1,3 @@
package step2.view

fun getSplitStringForCalculate() = readLine()!!.split(" ")

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

아래 코드도 깔끔한 느낌은 아니지만, !! Operator를 사용하는것 보다는 나을 수 있어서 참고용으로 남겨드렸습니다.

fun getSplitStringForCalculate() = readLine()?.split(" ") ?: run {
    throw IllegalArgumentException("null이 올 수 없다.")
}

@kyucumber kyucumber merged commit 0038a75 into next-step:nightmare73 Jul 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants