Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Step2 #2324
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
base: bb-worm
Are you sure you want to change the base?
Step2 #2324
Changes from all commits
0421b95
b1014ad
1702140
3e3dfdc
dd877b8
400b0a9
76cac9c
348199e
7c642d4
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
일부러 람다 사용해보려고 ConnectStrategy를 람다로 표현했습니다. 그런데 오히려 코드 읽기가 어려워지는 거 같아서.. 이런 경우엔 따로 클래스로 빼는 게 나을까요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
클래스도 좋고 변수로 선언해도 좋을 것 같아요
null을 활용한 설계가 아니라면 억지로 사용하지 않으셔도 됩니다 😃
기능 목록을 정의하실 때
마지막 포인트는 null 이다.
라는 내용이 있었는데요.실제로 null을 활용했다면 Optional을 활용할 수 있는 지점이 아닐까 싶어요.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
생성자는 유효성 검증과 변수 초기화에 집중하고,
생성 로직은 정적 팩토리 메서드나 별도의 클래스로 분리하면 어떨까요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
매번 생성자가 복잡해질 때마다 고민이 있었는데 말씀하신 것처럼 분리해보겠습니다
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Line 객체는 특별한 기능이 없는 것은 같아요
Points와 어떤 차이가 있나요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
엇 그러게요.. 작업하면서도 뭔가 이상하다 싶었는데 의미없이 사용하고 있었네요.
합쳐놓겠습니다!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
주 생성자를 두어 호출하면 어떨까요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
좋습니다!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
파싱과 관련된 로직은 view 또는 controller로 이동하면 어떨까요?
People 이라는 도메인은 구분기호에 따라 역할이 달라지지 않을 것 같아요.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
말씀하신 것처럼 People이 구분자를 가지고 있을 필요가 없네요. 변경해두겠습니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
좌우연결을 가진 상태로 끝까지 구현해 보셨어도 좋았을 것 같아요.
마지막 질문에 대한 답변을 드리자면 생성 단계에서 막아주는게 좋지 않을까 생각이 드는데요 😃
질문 내용으로 보아 여러 시도와 많은 고민 하신 것 같아 약간의 참고용 힌트를 드리면 어떨까 싶어요.
(힌트일뿐 정답이 아닙니다)
Point가 처음, 중간, 마지막에 대한 값을 스스로 결정할 수 있다면 어떨까요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
설명 감사합니다!
좌우연결을 가진 상태로 한 번 더 구현해보겠습니다..!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Point가 좌우연결을 가진 구조라면 유효성 검증도 단순화할 수 있을 것 같아요
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
예외 테스트 👍
예외 메시지까지 함께 검증하면 좋을 것 같아요.
Point, Points, Ladder, Person 에 대한 단위 테스트도 추가하면 어떨까요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
보완해보겠습니다!