Skip to content

Comments

[포키] 사다리 구현 5단계 - 실행결과 출력#190

Open
forkyy-dev wants to merge 14 commits intocodesquad-members-2022:seokho-hamfrom
forkyy-dev:mission5
Open

[포키] 사다리 구현 5단계 - 실행결과 출력#190
forkyy-dev wants to merge 14 commits intocodesquad-members-2022:seokho-hamfrom
forkyy-dev:mission5

Conversation

@forkyy-dev
Copy link

안녕하세요 브라이언!
로또 미션을 진행하느라 사다리미션 5단계를 이제야 제출합니다😅

<기능 추가>

  • 당첨 목록을 입력받고, 출력할 수 있는 기능을 추가했습니다.
  • 사다리 타기의 결과를 구하는 로직을 추가했습니다.
  • 모든 유저의 결과, 각 유저의 결과를 입력에 따라 출력하는 기능을 추가했습니다.

<리뷰 적용>

  • String 배열 타입의 매개변수를 List 타입으로 받도록 변경했습니다.
  • toString으로 출력하던 부분들을 제거하고, 유저에게 동일하게 보여야하는 부분은 OutputView 클래스로 옮겼습니다.

<고민 사항>
현재 GameService 클래스의 생성자 내에서 각 User, Item, Ladder Creator가 사용되고 있습니다.
코드를 보다보니 서비스에서 직접 이 객체들에 접근해서 사용하는 부분이 별로 좋지 않은 방식이라는 생각이 들어서 어떻게 수정할지 고민하다가 결국 브라이언의 피드백을 받고 싶어서 그냥 제출했습니다.

  1. 생성자를 통해 각 Creator를 주입받는다.
  • 각각의 Creator는 단순히 객체를 생성하는데만 사용되는데 굳이 주입을 받아야하는걸까 의문이 들었습니다.
  1. Controller에서 각각의 Creator를 사용해서 객체를 생성하고, 생성한 객체를 생성자에 넘겨준다.
  • 서비스에서 가져야하는 로직을 컨트롤러에서 가지고 있으면 안된다는 생각과 여기서 객체를 생성하면 객체 내부의 값에 직접 접근할 수 있겠다는 생각이 들어서 이렇게 짜지는 않았습니다.

어떤 방식으로 사용하는게 좋을지 브라이언의 의견이 듣고 싶습니다. 아니면 학습해볼 키워드도 좋습니다!

주말에 pr을 보내서 죄송합니다:) 남은 주말 잘보내세요!

함석호 added 14 commits February 20, 2022 22:25
- 동일하게 보여야하는 데이터는 View에서 가지고 있도록 변경
추가

- InputUtil에 예외처리하는 메서드 제거
- Controller에서 서비스에게 result를 매개변수로 전달하도록 변경
- User, Result를 담는 자료구조를 List에서 Map으로 변경
- ladder, result, user 패키지로 분리 및 각각 Creator클래스 생성
Item으로 변경

- Ladder 클래스에 포인트를 이동시키는 메서드, 좌우 이동 가능 여부 판별
메서드 추가
- Line에 이전, 다음 요소 반환하는 메서드 추가
- OutputUtil 클래스 출력 메서드, addPadding 메서드 추가
- playGame 메서드를 실행해서 결과를 저장.
- GameService 결과를 담는 Map타입의 results 변수선언
- 게터메서드와 헷갈릴 수 있는 메서드명이라고 생각되어 변경했습니다.
@forkyy-dev forkyy-dev added the review-BE Improvements or additions to documentation label Feb 27, 2022
@forkyy-dev forkyy-dev requested a review from wheejuni February 27, 2022 07:43
@forkyy-dev forkyy-dev changed the title [포키] - 사다리 미션 5단계 제출 [포키] 사다리 구현 5단계 - 실행결과 출력 Feb 27, 2022
@forkyy-dev forkyy-dev removed the request for review from wheejuni February 27, 2022 10:26
@wheejuni wheejuni self-assigned this Feb 27, 2022
@wheejuni wheejuni self-requested a review February 27, 2022 21:56
Copy link

@wheejuni wheejuni left a comment

Choose a reason for hiding this comment

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

수고 많으셨습니다. 👍
코멘트 참고 부탁드리고 아래 질문도 한번 고민해봐주세요.

왼쪽, 오른쪽 방향을 enum 으로 표현할 순 없을까?

public class UserCreator {
private static final int USERNAME_MAXLENGTH = 5;
private static final int USERNAME_MINLENGTH = 1;
private Map<String, User> users;

Choose a reason for hiding this comment

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

User 가 이름도 보관하면 안 될까요?

Copy link
Author

Choose a reason for hiding this comment

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

피드백을 제대로 이해하지 못했는데 설명을 더 부탁드려도 될까요?
현재 User 클래스가 이름을 인스턴스 변수로 가지고 있습니다.

Choose a reason for hiding this comment

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

아 네 그렇군요.. 그런데 왜 Map 을 사용하셨는지 궁금하네요.

Copy link
Author

@forkyy-dev forkyy-dev Feb 28, 2022

Choose a reason for hiding this comment

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

@wheejuni 사용자로부터 유저이름을 입력받으면 해당 이름을 가진 User 객체에 바로 접근하려고 사용했습니다! List로 목록을 가지고 있으면 매번 리스트를 순회하며 찾는것보다는 Map이 더 효율적일것 같아서요.
근데 UserCreator내에 인스턴스 변수로 가질 필요는 없었겠다는 생각은 드네요.


import java.util.*;

public class GameService {

Choose a reason for hiding this comment

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

이 클래스가 수행하는 기능으로 봤을때 GameService 보다는 Game 그 자체에 더 가까운 것 같고요...
그렇다면 지금처럼 UserCreator 를 주입받는지 등의 고민을 할 필요 없이 게임 그 자체에 필요한 데이터만 보관하는 형태, 즉 지금 형태의 구현을 유지해도 될 것 같습니다.

Copy link
Author

@forkyy-dev forkyy-dev Feb 28, 2022

Choose a reason for hiding this comment

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

이 부분에 대해서 질문이 생겼습니다..ㅎㅎ
리뷰를 보고 서비스 객체였으면 Creator를 다른 방식으로 사용해야하는건지, GameService와 Game은 어떤 차이점이 있는건지 궁금합니다.
네이밍의 문제인건가요?

this.name = name;
}

public static Item createResult(String name) {

Choose a reason for hiding this comment

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

Item 인데 createResult 라는 이름을 쓰는게 조금 어색해 보이긴 합니다

Copy link
Author

Choose a reason for hiding this comment

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

처음에 클래스명을 Result로 작성했다가 Item으로 변경했는데 메서드명을 수정을 안했네요🥲

import java.util.Map;

public class ItemCreator {
public static Map<Integer, Item> createResultMap(List<String> results) {

Choose a reason for hiding this comment

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

모든 메소드가 static 이라면 생성자도 private 으로 가려야할 것 같군요

Comment on lines +39 to +47
private int canMoveLeft(Line line, int userPoint) {
if (line.getPrev(userPoint) == 1) return -2;
return 0;
}

private int canMoveRight(Line line, int userPoint) {
if (line.getNext(userPoint) == 1) return 2;
return 0;
}

Choose a reason for hiding this comment

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

여기서 왜 리턴 타입이 int 인지 궁금합니다.

Copy link
Author

Choose a reason for hiding this comment

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

move 메서드에서 인덴트를 1로 유지해야겠다는 생각에 유저가 이동할 값을 바로 리턴하도록 작성했고 그래서 리턴타입을 int로 정했습습니다.

@forkyy-dev
Copy link
Author

아직 제가 서비스라는 객체의 역할을 제대로 이해하지 못하고 서비스라는 이름을 붙여서 사용하고 있었나봅니다..ㅎㅎ
그리고 말씀해주신 enum을 한번 적용해보겠습니다.

이른 아침부터 피드백 해주셔서 감사합니다! 월요일 화이팅하세요😊💪🏻

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

review-BE Improvements or additions to documentation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants