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

2단계 - 사다리(생성) #1601

Open
wants to merge 12 commits into
base: jimbaemon
Choose a base branch
from
Open

Conversation

jimbaemon
Copy link

조금 늦었습니다 :)
Integer Stream 제외하곤 함수형 stream 사용할 경우가 많이 안보이고
Integer Stream 의 경우 forEach 처리가 더 좋아보여서 ForEach 로 해봤는데 ㅜㅜ
수업 의도를 잘 못따라 가겠네요

Copy link

@lxxjn0 lxxjn0 left a comment

Choose a reason for hiding this comment

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

재현님, 미션 구현하시느라 수고 많으셨습니다!!

간단하게 먼저 피드백 남겨두었는데 출력 로직과 도메인 로직의 구분에 포커스를 둬서 한번 개선해보면 좋을 것 같아요!! 👍🏼

Comment on lines +4 to +5
private final static int MAX_NAME_LENGTH = 5;
private final String name;
Copy link

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 +21
@Override
public String toString() {
return String.format("%6s", name);
}
Copy link

Choose a reason for hiding this comment

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

toString 메서드의 의미와 사용 용도에 대해서 한번 알아보면 좋을 것 같아요!! 현재는 출력을 위한 용도로 사용되고 있는데 toString의 올바른 용도에 맞게 사용되지 못한 것 같아요!! 아래 글을 참고해보시면 좋을 것 같습니다 👍🏼

https://hudi.blog/java-correct-purpose-of-tostring/

import java.util.List;
import java.util.stream.Collectors;

public class Players {
Copy link

Choose a reason for hiding this comment

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

toString을 올바르게 사용한다면 현재 Players 일급컬렉션은 큰 의미를 가지기가 어려울 것 같아요!!

Comment on lines +8 to +9
SecureRandom secureRandom = new SecureRandom();
return secureRandom.nextBoolean();
Copy link

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;
Copy link

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) {
Copy link

Choose a reason for hiding this comment

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

간단한 조건문이지만 private 메서드로 뽑아서 이름을 부여한다면 코드를 읽기 더 좋을 것 같아요!!

Comment on lines +15 to +16
SecureRandom secureRandom = new SecureRandom();
return new Line(secureRandom.nextBoolean());
Copy link

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);
Copy link

Choose a reason for hiding this comment

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

PREV_INDEX가 0이면 해당 로직이 조금 이상할 것 같아요!!

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