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

[step2] 문자열 계산기 #958

Open
wants to merge 9 commits into
base: bperhaps
Choose a base branch
from
Open

Conversation

bperhaps
Copy link

@bperhaps bperhaps commented Dec 26, 2022

안녕하세요~ 드디어 시작합니다!!
첫 pr이다보니, 일단은 코드에 대한 평가를 먼저 받고 싶어요.
코틀린으로 처음 코딩해보니 어렵네요.. 아직까지는 자바가 너무 편하기도 하고요...
step1, step2를 함께 진행했습니다.

몇가지 궁금한 부분은...

  • companion을 이런식으로 사용하는게 좋은지 모르겠어요. 정적 팩터리 메소드를 구현하기 위해서 쓰기는 했는데. 이렇게 막 사용해도 되는 키워드인가요??

  • SAM을 사용하면 좋은점이 무엇인가요?? 가독성..? 은 크게 체감이 안되고.. 코틀린은 왜 SAM을 채택했을까요?

리뷰 부탁 드립니다! 감사합니다~

Copy link

@hyunniiii hyunniiii left a comment

Choose a reason for hiding this comment

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

안녕하세요 민성님~
몇가지 코멘트 남겼으니 확인 부탁드립니다 :)

Comment on lines +23 to +35
companion object
}

fun Calculator.Companion.from(input: String): Calculator {
return Calculator(Inputs.from(extractInputs(input)))
}

private fun extractInputs(input: String): List<CalculatorInput> {
return input.split(" ").asSequence()
.filter { it.isNotBlank() }
.map { CalculatorInput.from(it) }
.toList();
}

Choose a reason for hiding this comment

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

Suggested change
companion object
}
fun Calculator.Companion.from(input: String): Calculator {
return Calculator(Inputs.from(extractInputs(input)))
}
private fun extractInputs(input: String): List<CalculatorInput> {
return input.split(" ").asSequence()
.filter { it.isNotBlank() }
.map { CalculatorInput.from(it) }
.toList();
}
companion object {
fun from(input: String): Calculator {
return Calculator(Inputs.from(extractInputs(input)))
}
private fun extractInputs(input: String): List<CalculatorInput> {
return input.split(" ").asSequence()
.filter { it.isNotBlank() }
.map { CalculatorInput.from(it) }
.toList();
}
}

https://pearlluck.tistory.com/722

companion object는 이렇게 사용하시면 될 것 같습니다 :)

Comment on lines +14 to +17
val hasNextNumber: Boolean
get() {
return numbers.size > numbersPosition
}

Choose a reason for hiding this comment

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

Suggested change
val hasNextNumber: Boolean
get() {
return numbers.size > numbersPosition
}
val hasNextNumber: Boolean
get() = numbers.size > numbersPosition

이렇게 하면 조금 더 깔끔해질 것 같아요!

Comment on lines +44 to +53
fun Inputs.Companion.from(values: List<CalculatorInput>): Inputs {
values.validateFormat()

val groupBy = values.groupBy { it.getType() }

return Inputs(
groupBy[Number::class.java] as List<Number>,
groupBy[Operation::class.java] as List<Operation>
)
}
Copy link

@hyunniiii hyunniiii Dec 28, 2022

Choose a reason for hiding this comment

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

이 부분도 이 코멘트 처럼 companion object 부분을 수정해주세요 :)

Comment on lines +64 to +66
if (this[i] !is Number) {
throw IllegalArgumentException("인풋 포멧 오류")
}

Choose a reason for hiding this comment

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

kotlin 의 require을 사용해보는건 어떨까요?

https://seosh817.tistory.com/155

또한 어떤 에러인지 알 수 있도록 에러 메시지를 조금 더 상세하게 작성해주세요!

Comment on lines +94 to +104
private fun CalculatorInput.getType(): Class<out CalculatorInput> {
if (this is Number) {
return Number::class.java
}

if (this is Operation) {
return Operation::class.java
}

throw IllegalArgumentException("잘못된 타입")
}

Choose a reason for hiding this comment

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

when을 사용하면 조금 더 간결하게 표현할 수 있을 것 같아요!

Comment on lines +7 to +13
fun CalculatorInput.Companion.from(input: String): CalculatorInput {
return when {
input.isTypeOf(Number::class.java) -> Number(input.toInt())
input.isTypeOf(Operation::class.java) -> Operation.from(input)
else -> throw IllegalArgumentException("알 수 없는 입력값.")
}
}

Choose a reason for hiding this comment

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

이 부분도 변경할 수 있을 것 같습니다 :)

@hyunniiii
Copy link

코틀린의 장점 중 하나가 가독성이라고 생각합니다.
코틀린은 자바보다 간결하게 작성 가능합니다.
SAM 가독성이 아직 체감이 안된다고 하셨지만 조금 더 코틀린을 사용하시다보면 체감하실 수 있을 것 같아요!
지금은 자바에 더 익숙해서인지 코드가 다서 복잡하게 구현되어있는 것 같아요!
앞으로 조금 더 코틀린스럽게 작성하시는 연습을 해보시면 좋을 것 같습니다 😄

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.

2 participants