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 #981

Merged
merged 5 commits into from
Nov 22, 2023
Merged

Step1 #981

merged 5 commits into from
Nov 22, 2023

Conversation

sendkite
Copy link

안녕하세요.

다른 개인적인 일이 많아서, 늦게 미션을 시작했습니다.
잘부탁드립니다!

bottom-up 방식으로 작성해보려고 노력했습니다.

Copy link
Member

@malibinYun malibinYun left a comment

Choose a reason for hiding this comment

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

안녕하세요 :)
1단계 미션 고생 많으셨어요.
늦게 시작하셨더라도 진행하는 것에 큰 의미가 있다고 생각해요!
완주까지 응원할게요 💪

다음 미션 진행을 위해 머지하겠습니다
피드백은 다음 미션에 반영해주세요!

Comment on lines +2 to +3

class StringAddCalculator {
Copy link
Member

Choose a reason for hiding this comment

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

객체 안에 가변으로 관리되는 멤버변수가 없으니, object 키워드를 활용해보셔도 좋겠어요 :)

object StringAddCalculator {
...

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 +9 to +10

val customDelimiterRegex = Regex("//(.)\n(.*)").find(text)
Copy link
Member

Choose a reason for hiding this comment

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

Regex 객체는 생성 비용이 많이 드니, 한 번만 만들어두고 재활용해보는 건 어떨까요?

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 +16 to +17

private fun parseTokens(text: String, customDelimiter: String): List<Int> {
Copy link
Member

Choose a reason for hiding this comment

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

토큰들을 파싱하는 것은 다른 객체로 분리해봐도 좋겠네요!

Comment on lines +18 to +19
require(text.contains("-").not()) { "음수를 입력할 수 없습니다." }
require(text.contains(Regex("[,:${customDelimiter}]"))) { "숫자와 지정된 구분자만 입력할 수 있습니다." }
Copy link
Member

Choose a reason for hiding this comment

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

어떤 값이 들어와서 exception이 발생했는지 에러 메시지에 담아 표시해주는 건 어떨까요?

Comment on lines +14 to +19
private lateinit var calculator: StringAddCalculator

@BeforeEach
fun setUp() {
calculator = StringAddCalculator();
}
Copy link
Member

Choose a reason for hiding this comment

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

StringAddCalculator는 불변객체여서, 매 테스트마다 생성하지 않고, 필드에 한 개만 생성해도 괜찮아 보이네요! :)

@malibinYun malibinYun merged commit 7faa674 into next-step:sendkite Nov 22, 2023
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