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

Step1 : 문자열 덧셈 계산기 #773

Merged
merged 15 commits into from
Jan 27, 2025
Merged

Step1 : 문자열 덧셈 계산기 #773

merged 15 commits into from
Jan 27, 2025

Conversation

cjcjon
Copy link

@cjcjon cjcjon commented Jan 26, 2025

안녕하세요, 리뷰어님. 1단계 리뷰 요청드립니다.
현재 업무를 코틀린으로 진행중이어서 자바가 아닌 코틀린으로 구현하였는데 혹시 확인 필요한 사항이 있으면 알려주시면 감사하겠습니다.
그리고 이후에 kotlin으로 테스트하기 편하게 kotest를 사용하는게 어떨까 생각중인데 혹시 괜찮을지 알려주시면 좋겠습니다.

작업내용

  • 문자열 덧셈 계산기 기능 구현
  • Step0 요구사항 반영
    • getter, setter 위치
    • exception 원인 명시

Copy link

@Rok93 Rok93 left a comment

Choose a reason for hiding this comment

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

준수님 1단계 미션 잘 구현해주셨습니다. 👍👍
먼저 kotlin, kotest 사용하셔도 괜찮습니다. 😉
추가적으로 몇몇 코멘트 남겨두었는데, 확인 부탁드릴게요! 😃
잘 이해가 안되거나 어려운 코멘트있으면 편하게 질문주세요. 🙏

src/main/kotlin/stringcalculator/StringCalculator.kt Outdated Show resolved Hide resolved
src/main/kotlin/stringcalculator/StringCalculator.kt Outdated Show resolved Hide resolved
src/main/kotlin/stringcalculator/StringCalculator.kt Outdated Show resolved Hide resolved
Comment on lines 18 to 32
private fun divideDelimiterAndInput(input: String): DelimiterInput {
val matcher = Pattern.compile("//(.)\n(.*)").matcher(input)

return if (matcher.find()) {
DelimiterInput(
input = matcher.group(2),
delimiters = DEFAULT_DELIMITER + matcher.group(1),
)
} else {
DelimiterInput(
input = input,
delimiters = DEFAULT_DELIMITER,
)
}
}
Copy link

Choose a reason for hiding this comment

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

연산자와 피연산자를 분리하는 역할을 다른 객체에게 위임해보면 어떨까요? 😃

Copy link
Author

@cjcjon cjcjon Jan 27, 2025

Choose a reason for hiding this comment

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

StringCalculator의 기능이 많지 않고 역할에 따라 코드를 분할하면 분할된 코드를 따라가서 확인해야해서 현 상태에서는 하나의 객체로 구현하는게 낫다고 생각해 하나로 구현했는데 역할에 따라 나누는게 나을 수 있겠네요.
감사합니다!


class StringCalculator() {

fun add(input: String?): Int {
Copy link

Choose a reason for hiding this comment

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

메서드 파라미터 변수명도 조금 더 의도를 드러낼 수 있는 네이밍을 지어보면 어떨까요? 😃

문자열 덧셈 계산기에서 메서드 파라미터로 들어오는 값은 어떤 값인지 고민했을 때, 조금 더 의도를 드러낼 수 있는 네이밍이있을 것 같아요.

Copy link
Author

Choose a reason for hiding this comment

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

StringCalculator가 특정 String을 계산한다는 의미 자체를 가지고 있고, input만으로 사용자 입력을 충분히 표현한다고 생각해서 지었는데 조금 더 명확히 이름을 지을 수도 있겠네요.

src/main/kotlin/stringcalculator/StringCalculator.kt Outdated Show resolved Hide resolved
src/main/kotlin/stringcalculator/StringCalculator.kt Outdated Show resolved Hide resolved
src/main/kotlin/stringcalculator/StringCalculator.kt Outdated Show resolved Hide resolved
- MagicNumber 상수화
- 파라미터 이름 명시
- 역할에 따른 객체 분리
- Pattern 객체 캐싱
@cjcjon
Copy link
Author

cjcjon commented Jan 27, 2025

리뷰 반영했습니다.

역할을 세 가지로 나누었습니다.
StringCalculator: 요구사항에 맞게 입력된 문자열에 덧셈 연산을 적용해 결과를 계산합니다.
ExpressionMatcher: 요구사항에 따라 입력된 문자열을 구분자와 문자로 나누어줍니다.
NumberTokenizer: 구분자를 사용해서 입력된 문자열을 숫자들로 변환해줍니다.

고민되는 부분

기능을 작성하다보면 항상 고민되는 부분이 있는데, 여러명이서 협업해서 코드를 작성할 때 역할에 따라 객체를 나누고 작성하는 부분도 중요하지만 다른 동료들이 시간을 많이 사용하지 않고 빠르게 이해하고 유지보수하기 쉬운 코드를 짤 수 있을지도 같이 고민하게 됩니다.
강의와 미션을 구현하는 입장에서는 SRP를 따라서 역할에 맞게 객체를 설계하는게 맞다고 생각하는데, 문자열 덧셈 계산기와 같이 복잡하지 않은 기능을 구현할 때 여러 객체로 분할해서 코드를 만들다보면 결국 동료가 코드를 볼 때 한 눈에 보기 어렵다는 이야기도 나오고, 처음부터 역할을 나누기보다 이후에 새로운 요구사항이 들어올 때 리팩터링하면서 확장하는게 더 좋겠다는 의견도 나오고 다양한 의견이 나옵니다. 물론 역할에 맞게 나누는게 좋다고 하는 동료들도 있고요.
리뷰어님께서도 다양한 의견을 들어오셨을 것 같은데 혹시 제가 고민되는 부분에 대해서 어떤 의견이실지 궁금합니다.
감사합니다~

@cjcjon cjcjon requested a review from Rok93 January 27, 2025 09:43
Copy link

@Rok93 Rok93 left a comment

Choose a reason for hiding this comment

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

준수님 피드백 반영 잘해주셨습니다. 👍👍
이제 본격적으로 2단계로 넘어가볼까요? 🚀
추가적으로 질문주신부분에 대한 답변과 추가 코멘트 남겨두었으니 같이 확인 부탁드릴게요. 😉

@@ -0,0 +1,32 @@
package stringcalculator
Copy link

Choose a reason for hiding this comment

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

고민되는 부분
기능을 작성하다보면 항상 고민되는 부분이 있는데, 여러명이서 협업해서 코드를 작성할 때 역할에 따라 객체를 나누고 작성하는 부분도 중요하지만 다른 동료들이 시간을 많이 사용하지 않고 빠르게 이해하고 유지보수하기 쉬운 코드를 짤 수 있을지도 같이 고민하게 됩니다.
강의와 미션을 구현하는 입장에서는 SRP를 따라서 역할에 맞게 객체를 설계하는게 맞다고 생각하는데, 문자열 덧셈 계산기와 같이 복잡하지 않은 기능을 구현할 때 여러 객체로 분할해서 코드를 만들다보면 결국 동료가 코드를 볼 때 한 눈에 보기 어렵다는 이야기도 나오고, 처음부터 역할을 나누기보다 이후에 새로운 요구사항이 들어올 때 리팩터링하면서 확장하는게 더 좋겠다는 의견도 나오고 다양한 의견이 나옵니다. 물론 역할에 맞게 나누는게 좋다고 하는 동료들도 있고요.
리뷰어님께서도 다양한 의견을 들어오셨을 것 같은데 혹시 제가 고민되는 부분에 대해서 어떤 의견이실지 궁금합니다.

사실 정답이 없는 이야기라 단순히 저의 주관적인 생각입니다.
객체지향적으로 설계하고 SOLID 원칙을 지키며 코딩하는 방식은 유지보수 관점에서 중요하다고 생각하는데, 말씀하신 것 처럼
이런 부분이 과하다고 생각할 수 있다고 생각해요. 저 또한 개인적으로 YANGI 원칙를 선호하기 때문에 저 또한 정말 필요성을 느끼면 진행하는 것을 좋아합니다만 하지만 그렇다고해서 이런 설계를 전혀 고려하지 않는 것은 지양해야한다고 생각해요.

지금은 단순하기 때문에 이런 객체지향적인 설계와 SRP 원칙을 준수하는 등의 행위가 덜 중요하게 느껴질 수 있다고 생각하지만 조금만 더 로직이 복잡해지기 시작하면 이런 설계의 필요성을 금방 느끼게 될거라 생각해요. 🤔

�특히 지금과 같이 문자열 덧셈 계산기의 경우 저희는 이 미션에서만 잠시 다루고 이후로는 유지보수하지 않을 코드이기 때문에 이런 객체지향적인 설계와 SRP를 지키며 코딩하는 것이 조금 과하다고 생각될 수 있지만 말씀하신 것처럼 지금은 학습의 관점에서 하는 것이 조금 더 강한 것은 맞습니다. 😃

Comment on lines +27 to +30
data class Expression(
val delimiters: List<String>,
val input: String,
)
Copy link

Choose a reason for hiding this comment

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

Expression 클래스를 Comapnion Object에서 정의하신 이유가 있으신가요? 🤔

Copy link
Author

@cjcjon cjcjon Jan 27, 2025

Choose a reason for hiding this comment

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

ExpressionMatcher.kt 파일에서 바깥에 같이 정의하거나 stringcalculator 패키지에 정의해도 되는데, 특정 객체를 통해서 주도적으로 생성되는 data class 같은 경우에는 해당 class의 companion 내부에서 정의하는게 명시적이라고 생각해서 companion object 내부에 정의했습니다.

@Rok93 Rok93 merged commit 325ceb3 into next-step:cjcjon Jan 27, 2025
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