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

[문자열 덧셈 계산기] 양민석 미션 제출합니다. #127

Open
wants to merge 20 commits into
base: main
Choose a base branch
from

Conversation

minseok0416
Copy link

No description provided.

Copy link

@m6z1 m6z1 left a comment

Choose a reason for hiding this comment

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

1주차 정말 고생 많으셨습니다! 👏

expression을 nullable로 처리하신 부분은, 문자열이 비어 있을 가능성을 고려하신 것으로 보입니다. 그런데 처음에만 비어 있는지 확인하고, 이후로는 nullable하지 않게 선언하면 여러 군데에서 추가적인 null 체크를 피할 수 있을 것 같아요 😊

이번 주도 화이팅입니다! 함께 열심히 해봐요 💪

Comment on lines +8 to +9
var text = expression
var spliter = Seperator(expression)
Copy link

Choose a reason for hiding this comment

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

클래스 내부에서만 사용된다면 접근제어자 중 private 을 붙이는 게 어떨까요!?

Copy link
Author

Choose a reason for hiding this comment

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

접근제어자 사용이 미숙하다고 생각했는데, 좋은 지적 감사합니다!

}

class Seperator(val expression: String?) {
var seperator = ",|:"
Copy link

Choose a reason for hiding this comment

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

변하지 않는 구분자라면 val 로 선언해도 될 것 같아요!

Copy link
Author

Choose a reason for hiding this comment

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

지금보니 그렇네요..! 비슷한 기능을 하는 객체에서 변화가 생기는걸 착각했나 봅니다. 좋은 지적 감사합니다!

Copy link

@hongmyeoun hongmyeoun left a comment

Choose a reason for hiding this comment

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

미션 고생하셨습니다. 2주차도 화이팅

}
return result
}

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.

이런 온라인 테스트가 처음이라 파일 분리에 대해 고민하다가 제공된 파일내에서 작성하는 것을 선택했는데 저도 이 부분이 조금 아쉽네요. 좋은 지적 감사합니다!!

Comment on lines +7 to +8
class InputExpression(val expression: String?) {
var text = expression

Choose a reason for hiding this comment

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

expression 을 nullable type 으로 받았으니, null 체크 연산을 실행하고, 로직을 하는 건 어떨까요 ?
유저의 입력이 non-null 이라면 이후 로직에서 ? 연산자나 !! 연산자를 사용하지 않고 사용할 수 있을 것 같습니다 !

Copy link
Author

Choose a reason for hiding this comment

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

스스로도 null-safety를 활용하는 능력이 미숙하다고 생각하고 있습니다. 좋은 지적 감사합니다!!


fun calculate(text: List<String>): Int {
var result = 0;
for (txt in text) {

Choose a reason for hiding this comment

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

txt 와 text 는 혼동의 여지가 있는 변수명인 것 같습니다.
조금 더 구분이 가능한 네이밍이면 변수명만 보고도 어떤 역할을 하는 녀석인지 알기 쉬울 것 같아요 !

Copy link
Author

Choose a reason for hiding this comment

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

급하게 작성했던 기억이 나는 아쉬운 네이밍이라고 생각합니다. 좋은 지적 감사합니다!!

Copy link

@ikseong00 ikseong00 left a comment

Choose a reason for hiding this comment

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

관심사, 역할 별로 파일의 분리가 있다면 훨씬 읽이 좋은 코드가 될 것 같습니다 !

}

@Test
fun `구분자로 입력식 쪼개기`() {

Choose a reason for hiding this comment

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

하나의 테스트 함수에서 2개의 테스트 케이스를 진행하셨는데 커스텀 구분자를 지정한 경우와 지정하지 않은 경우를 나누어서 테스트를 하는게 어떨까요?

Copy link
Author

Choose a reason for hiding this comment

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

테스트 케이스의 그룹화와 세분화의 기로에서 그룹화를 택한 테스트 케이스였네요. 좋은 지적 감사합니다!!


class Seperator(val expression: String?) {
var seperator = ",|:"
fun usedCustomSeperator(): Boolean {

Choose a reason for hiding this comment

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

Seperator가 커스텀 구분자인지 반환하는 함수이므로 isCustomSeperator가 좀 더 자연스러울 것 같습니다.

Copy link
Author

Choose a reason for hiding this comment

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

작성하면서도 부자연스럽다고 생각했던 함수명입니다. 좋은 지적 감사합니다!!

return 0
}
val num = Integer.parseInt(txt)
if (num < 0) {

Choose a reason for hiding this comment

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

요구 사항에서 양수와 구분자로 이루어진 문자열이라 하였는데 0일 경우에도 예외를 발생시켜야 할 것 같습니다.

Copy link
Author

Choose a reason for hiding this comment

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

당연한걸 왜 놓쳤는지...너무 아쉽네요. 좋은 지적 감사합니다!!

}
}

fun parseNumber(txt: String?): Int {

Choose a reason for hiding this comment

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

전반적으로 nullable한 타입이 많이 보이는 것 같은데 non-null 타입을 통해 Kotlin의 null safety를 활용하는 것이 좋을 것 같습니다.

Copy link
Author

Choose a reason for hiding this comment

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

스스로도 null-safety를 활용하는 능력이 미숙하다고 생각하고 있습니다. 좋은 지적 감사합니다!!

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.

5 participants