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

[사다리 미션] 황현식 미션 제출합니다. #6

Open
wants to merge 6 commits into
base: choon0414
Choose a base branch
from

Conversation

Choon0414
Copy link

코드는 잘 돌아가는데 리팩터링을 크게 못한 것 같아 아쉽네요.
최대한 간결하게 짠다고 짠 것 같은데 여전히 복잡한 느낌..
수정할 부분 피드백 부탁드립니다!

@Choon0414 Choon0414 changed the base branch from main to choon0414 July 20, 2024 19:43
Copy link

@20HyeonsuLee 20HyeonsuLee 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 +31 to +35
private List<User> generateUsers(List<String> users) {
return users.stream()
.map(user -> new User(user, users.indexOf(user)))
.toList();
}

Choose a reason for hiding this comment

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

별도의 generater로 분리한 이유가 있을까요?

Copy link
Author

Choose a reason for hiding this comment

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

최대한 기능 단위로 쪼개보려고 분리했습니다!
이 정도는 메서드 분리 안하고 InputView에서 바로 처리하는 것도 좋아보이네요


public class LadderGame {

private final LadderMaker ladderMaker = new LadderMaker();

Choose a reason for hiding this comment

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

Ladder Maker로 분리한게 인상깊네요!

return user;
}

public int move(int position, Line line) {

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.

처음엔 당연히 게임에서 움직이는 거라 생각했었는데 사다리의 역할로 볼 수도 있겠네요!

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