-
Notifications
You must be signed in to change notification settings - Fork 708
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
2단계 - 사다리(생성) #1601
base: jimbaemon
Are you sure you want to change the base?
2단계 - 사다리(생성) #1601
Conversation
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.
재현님, 미션 구현하시느라 수고 많으셨습니다!!
간단하게 먼저 피드백 남겨두었는데 출력 로직과 도메인 로직의 구분에 포커스를 둬서 한번 개선해보면 좋을 것 같아요!! 👍🏼
private final static int MAX_NAME_LENGTH = 5; | ||
private final String name; |
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.
상수와 변수 사이에는 공백을 두면 가독성을 높일 수 있어요!!
@Override | ||
public String toString() { | ||
return String.format("%6s", name); | ||
} |
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.
toString
메서드의 의미와 사용 용도에 대해서 한번 알아보면 좋을 것 같아요!! 현재는 출력을 위한 용도로 사용되고 있는데 toString의 올바른 용도에 맞게 사용되지 못한 것 같아요!! 아래 글을 참고해보시면 좋을 것 같습니다 👍🏼
import java.util.List; | ||
import java.util.stream.Collectors; | ||
|
||
public class Players { |
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.
toString을 올바르게 사용한다면 현재 Players 일급컬렉션은 큰 의미를 가지기가 어려울 것 같아요!!
SecureRandom secureRandom = new SecureRandom(); | ||
return secureRandom.nextBoolean(); |
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.
SecureRandom 클래스는 처음 보는데 덕분에 좋은 정보를 알게 된 것 같아요!! 👍🏼
import java.util.Objects; | ||
|
||
public class Line { | ||
private final Boolean hasLine; |
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.
해당 부분은 Wrapper 타입을 꼭 사용할 필요는 없을 것 같아요!!
} | ||
|
||
public static Line create(final Line prevLine) { | ||
if (prevLine == null || !prevLine.hasLine) { |
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.
간단한 조건문이지만 private 메서드로 뽑아서 이름을 부여한다면 코드를 읽기 더 좋을 것 같아요!!
SecureRandom secureRandom = new SecureRandom(); | ||
return new Line(secureRandom.nextBoolean()); |
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.
라인 생성 전략을 사용하지 않고 코드가 직접 대입되어 있는 것 같아요!!
if (index == 0) { | ||
return null; | ||
} | ||
return result.get(index - PREV_INDEX); |
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.
PREV_INDEX가 0이면 해당 로직이 조금 이상할 것 같아요!!
조금 늦었습니다 :)
Integer Stream 제외하곤 함수형 stream 사용할 경우가 많이 안보이고
Integer Stream 의 경우 forEach 처리가 더 좋아보여서 ForEach 로 해봤는데 ㅜㅜ
수업 의도를 잘 못따라 가겠네요