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

[1단계 - 자동차 경주 구현] 성하(김성훈) 미션 제출합니다. #491

Merged
merged 60 commits into from
Feb 12, 2023

Conversation

sh111-coder
Copy link

안녕하세요!
리뷰 요청 드립니다!!

구현 진행하면서 궁금했던 부분 질문드립니다!


✅ 클래스 생성 시 커밋을 어떻게 할지에 관한 부분

  1. 미리 구조를 생각해놓고 모든 메소드 시그니처만 있는 상태로 커밋
    커밋 메시지 - feat(A) : A 선언
    ex)
class A {
    public void function1() {

    }
    public int function2() {
            return 0; // 임시 값
    }
}
  1. 클래스 생성 + 처음 하나의 기능 구현 커밋
    커밋 메시지 - feat(A) : function1 구현
    ex)
class A {
    public void function1() {
          ... (구현 로직)
    }
}

이렇게 2가지 방식이 있는데, 저는 2번을 사용하는데 팀원은 1번 방식을 사용하여
이번 미션에서는 1번 방식으로 구현 및 커밋하게 되었습니다!

구현을 진행하다 보니, 클래스 생성 전에 미리 해당 객체의 메소드 시그니처를 모두 정해놓고
클래스를 생성하여서 일부 기능이 구현이 안 되었더라도 메소드 시그니처가 생성이 되어 있어서
프로그램 구현 흐름을 보기에는 좋았던 것 같습니다.

하지만 클래스 생성 시에 function2 처럼 반환 타입이 있으면
해당 반환타입에 임시 값을 줘서 생성해야 하는 것이 불편하다고 느껴졌습니다.
리뷰어님은 어떻게 생각하시는지 궁금합니다!


✅ RandomNumberGenerator에서 static 사용 고민

static을 사용하면 클래스가 메모리에 올라가는 시점에 메모리 영역에
생성돼서 메소드 하나가 여러 곳에서 호출이 될 때 사용 시 따로 인스턴스를
만들지 않고도 메모리 영역에서 꺼내서 사용할 수 있다는 장점이 있어서 사용했습니다.
저희 코드에서는 메소드를 사용하는 곳이 현재 컨트롤러 한 곳이어서
해당 장점이 효과가 없을 수 있지만,
추후에 메소드를 사용하는 곳이 늘어날 수 있다는 확장 가능성을 고려해서 사용했습니다.

그렇지만 현재 코드에서는 사용하는 곳이 하나라서
GC가 처리해주지 않는 메모리 영역에 생성되는 것이 오히려 비효율적이지 않을까 해서
static을 사용하지 않는 것을 고려했는데,

static을 사용하지 않는 것이 좋을지 사용하는 것이 좋을지 고민이 되어서 질문드립니다!

@sh111-coder
Copy link
Author

추가 질문이 생겨서 질문드립니다!
리뷰를 받고 접근 제어자를 추가했는데 Intellij에서 노란줄로 해당 필드가 maybe final 이라고 된 걸 확인했습니다!

private OutputView outputView = OutputView.getInstance();

저는 final을 선언하는 이유가 해당 필드가 불변으로 값이 조작되는 것을 막기 위해 사용한다고 생각했습니다.
여기서는 OutputView가 이미 Out OutputView.getInstance()로 싱글톤으로 생성되어
항상 같은 인스턴스를 받아 저장하기 때문에 굳이 final로 막아야 하는 걸까? 라는 생각이 들었는데
명시적으로 해당 필드가 불변임을 알리기 위해 final을 붙여야 하는 걸까요?

@sh111-coder
Copy link
Author

sh111-coder commented Feb 10, 2023

첫 번째 PR 시 질문했던 점들에 대해 너무 답변을 잘 해주셔서 감사합니다!
답변을 보고 아래와 같이 생각을 해보았습니다!! 😃

1) 클래스 생성 시 커밋을 어떻게 할지에 관한 부분

이 부분 답변에 대해서 페어와 얘기를 나눠보았는데,
페어도 1번 방식으로 했을 때 클래스 생성 시 메소드 시그니처를 먼저 만들어 놓을 뿐,
그 다음 기능을 구현할 때는 기능 목록 단위대로 커밋하는 것이기 때문에 문제가 없지 않을까? 라는 생각이더라고요!
이 부분에 대해서는 어떻게 생각하시는지 궁금합니다!


2) RandomNumberGenerator에서 static 사용 고민

답변을 보고 생각을 해보니, 객체의 '캡슐화'를 말씀하신 것이 맞을까요?!
제가 처음 생각했을 때는 너무 메모리 영역, 즉 성능 관점에서만 static을 바라봤었는데
리뷰어님의 답변을 보고 생각을 해봤을 때 확실히 객체지향적 관점에서 static이 좋지 않다고 생각되었습니다!

객체의 정보는 캡슐화되어 보호, 은닉되어야 하는데 static으로 선언하게 되면
외부에서 객체를 생성하지 않고도 접근하여 어떤 정보가 있는지 파악할 수 있기 때문에 단점이 되는게 아닐까요?

제 생각이 맞는지 궁금합니다!!

@sh111-coder
Copy link
Author

추가 질문이 있어 질문 드립니다!!

1차 PR 코드에서는 생각을 못하고 일급 컬렉션 클래스인 RacingCars에서
List<Car>를 반환하는 getter를 사용해서
List<Car>를 꺼낸 뒤, 컨트롤러에서 해당 List<Car>를 Map으로 가공하여
View에게 넘겨줘서 View에서 Map을 사용해서 출력하도록 했었습니다.

생각해보니, 단순히 getter를 사용해서 컬렉션을 반환하는 것은
일급 컬렉션을 사용하는 이유 중에 상태와 행위를 한 곳에서 관리한다는 이유에
위배? 되는 것 같다고 생각되었습니다.
그래서 생각하기로는,
뭔가 모든 작업들을 일급 컬렉션 클래스에서 가공을 진행하여 반환해준 것을 나머지 계층에서 사용해야한다고 느껴졌습니다!

그런데 getter를 없애려고 하니, 컨트롤러 단에서 기능을 수행할 때 List<Car>를 받아오기가 힘들어졌습니다.

이러한 부분에서 충돌이 발생하는데
핵심 비즈니스 로직간의, 예를 들면 Car에서 RacingCars.getCars()를 호출하는 경우가 아니라
View나 Controller에서 RacingCars.getCars()를 호출하는 경우는 괜찮은 건지 궁금합니다!

src/main/java/racingcar/model/Car.java Show resolved Hide resolved
src/main/java/racingcar/model/Car.java Show resolved Hide resolved
src/main/java/racingcar/model/RacingCars.java Show resolved Hide resolved
src/main/java/racingcar/model/Car.java Outdated Show resolved Hide resolved
Comment on lines 36 to 46
private void race(int tryNum) {
outputView.printRacingResultMessage();
for (int repeatIndex = 0; repeatIndex < tryNum; repeatIndex++) {
List<Car> currentCars = racingCars.getCars();
for (Car currentCar : currentCars) {
int randomValue = RandomNumberGenerator.generate();
currentCar.move(currentCar.canMoving(randomValue));
}
outputView.printCurrentRacingCarsPosition(convertRacingCarsResultForPrint(currentCars));
}
}
Copy link
Member

Choose a reason for hiding this comment

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

RacingCarController에서 랜덤값을 생성해주는 RandomNumberGenerator.generate()를 의존하고있기때문에 테스트하기가 힘들거에요. 이런 경우 RandomNumberGenerator를 static하게 사용하는 방식보다는 인터페이스를 사용해서 해당 값을 Application에서 주입시켜주는건 어떨까요?

그렇게되면 Controller를 단위테스트할때 랜덤 숫자가 아니라 고정된 숫자를 생성해주는 클래스를 주입해서 테스트할 수 있을 것 같아요.

https://tecoble.techcourse.co.kr/post/2021-10-04-strategy-command-pattern/ 링크의 전략패턴 부분을 참고해보시고 너무 어렵다면 DM 주셔도 좋습니다 😊

Copy link
Member

@begaonnuri begaonnuri left a comment

Choose a reason for hiding this comment

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

안녕하세요 성하!

리뷰 반영 잘 해주셨네요. 코드가 점점 깔끔해지는것같아요 👍🏻

몇가지 리뷰랑 답변 남겼으니 확인 부탁드릴게요~

아 그리고 이전 리뷰에 접혀있는 부분을 확인 못하신것같은데 이것도 한번 더 확인 부탁드릴게요 😊

@begaonnuri
Copy link
Member

begaonnuri commented Feb 11, 2023

#491 (comment) 에 대한 답변입니다.

명시적으로 해당 필드가 불변임을 알리기 위해 final을 붙여야 하는 걸까요?

객체를 정의하는 쪽에서 싱글톤 객체로 정의하는것과 사용하는 쪽에서 재할당이 불가능하도록 설정하는건 충분히 다른 의미를 가질것같아요. 잘 체감이 안되는 이유가 사용처가 하나라서 그럴 수 있을것같아요.

싱글톤 패턴은 사용처가 많아질 경우 의미를 가져요. 매번 생성하지 말고 생성된 하나의 인스턴스를 재사용하자는 의미이니까요. 하지만 지금같이 사용처가 하나라면 어차피 한번 생성되는데 재생성을 막는건 예방 정도로 의미를 가질 것 같아요. 어쩌면 지금은 싱글톤 적용이 오버 엔지니어링일수도 있을것 같네요.

@begaonnuri
Copy link
Member

begaonnuri commented Feb 11, 2023

#491 (comment) 에 대한 답변입니다.

1)

사실 커밋 단위는 실행만 가능(컴파일에러가 발생하지 않는 상태)하다면 정답이 없는 부분이긴 해요. (오답은 있습니다) 다만 저는 구현하면서 도메인지식을 추가로 얻으면서 더 나은 구조가 떠오르기도 하는데 시그니처와 클래스를 미리 정해두면 그렇게 한정되기도 하더라구요. 사전에 충분한 도메인 지식이 있는 경우라면 나쁘지 않은 방법이라고 생각해요.

그리고 반대로 어차피 기능 단위로 커밋이 될텐데 그걸 또 쪼개면 커밋 하는 입장에서 번거롭기도 하더라구요 ㅎㅎ 정답이 없는 부분이니 오답이 아닌 가정 하에 본인의 방식을 정해나가는것도 좋을것같네요 👍🏻

2)

캡슐화를 말한게 맞습니다. 💯 말씀하신것처럼 객체지향적인 관점에서 static이 좋지 않고, 어떻게 보면 도메인에 대한 고민이 덜 된 시그널일 수도 있어요. 하지만 의도적으로 static을 사용하는 경우도 있답니다. 대표적으로 util성 클래스인 Math 클래스와 Objects 클래스가 있어요. Collections도 static 클래스가 대부분이긴 한데 이건 좀 더 다양한 이유가 있어서 추후에 학습해보시는것도 좋을것같네요 😊

@begaonnuri
Copy link
Member

#491 (comment) 에 대한 답변입니다.

getter를 지양해야 하는 이유는 무엇일까요? 여러 이유가 있겠지만 객체 안에서 관리하는 값이 여러 곳에 퍼질 여지가 생기게 되고 책임이 분산될 수 있기 때문이에요. 이것을 객체의 응집력이 떨어진다고 표현하기도 합니다.

하지만 View 영역에서 사용자에게 값을 보여주는 용도로 getter를 사용하고 애플리케이션이 복잡하지 않아서 제어가 가능하다면 getter를 안쓸 이유가 없고 오히려 사용해야하는 영역이라고 생각해요. 변경될 여지가 생기는 것이지 setter처럼 값을 변경해버리는게 아니니까요 😊

Copy link
Member

@begaonnuri begaonnuri 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 +18 to +24
private final NumberGenerator numberGenerator;

public RacingCarController(final NumberGenerator numberGenerator) {
this.outputView = OutputView.getInstance();
this.inputView = InputView.getInstance();
this.numberGenerator = numberGenerator;
}
Copy link
Member

Choose a reason for hiding this comment

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

인터페이스 주입 👍🏻

Comment on lines +3 to +6
public interface NumberGenerator {

int generate();
}
Copy link
Member

Choose a reason for hiding this comment

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

이렇게되면 테스트할땐 고정 숫자를 반환하는 구현체를 통해 테스트를 할 수도 있겠네요 👍🏻 다음 step에서 한번 시도해보셔도 좋을것같아요 😊

@begaonnuri begaonnuri merged commit cb8a7c7 into woowacourse:sh111-coder Feb 12, 2023
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