Skip to content

[그리디] 염지환 자동차 경주 3, 4단계 제출합니다! #114

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

Merged
merged 34 commits into from
Mar 24, 2025

Conversation

JihwanYeom
Copy link

안녕하세요! 이번에 3, 4단계 미션을 제출하게되었습니다
죄송합니다 제가 시간에 쫒겨 개발하다 4단계 미션을 마저 다 구현하지 못하고 Car객체에 대한 기능 테스트만
구현하게 되었습니다. README도 아직 미완성이지만 구현한 3단계 기능이라도 제출하고자합니다
이번 미션에서는 기능 구현에 많은 시간을 쏟았던 것 같습니다. 일급 컬렉션, 정적 팩토리 메서드, MVC패턴 등
새로 배우는 개념을 코드에 적용하는데도 시간이 걸렸습니다.

무엇보다 저 개념들의 목적을 잊지 않고 코드를 유연하게 짜는 것이 제일 어려웠습니다. 각 개념을 잘 구현해도
코딩하다가 막히는 일이 많았던 것 같습니다. 리뷰어님께서 사용하시는 프로그램 설계 방법도 조금 공유해주시면
감사하겠습니다!

JihwanYeom and others added 11 commits March 12, 2025 22:01
추상화를 통한 변경 영역 축소
공통
Reformat을 통한 서식 통일

Car 클래스
name 필드 final 지정
가독성 증가를 위한 상수 추가
isSamePosition()메서드 추가

Game 클래스
리스트 생성 시 <>연산자 사용
미사용 라인 삭제
난수 생성 방식 변경
마지막 줄 개행 추가
enhanced for loop 사용
가독성 향상을 위한 상수 필드 추가
assertj를 이용한 테스트로 변경
MVC모델 개념을 활용하여 domain, view, controller를 나누어 구현
Car의 기능에 대한 테스트만 구현

전략 패턴을 적용한 NumberGenerator로 구현
Copy link

@TaeyeonRoyce TaeyeonRoyce left a comment

Choose a reason for hiding this comment

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

미션 수행 하시느라 고생 많으셨습니다, 지환님!👍
짧은 시간에 여러 개념들을 학습 하시고, 반영 하시기 어려우셨을텐데 고생 하셨어요.
현재 미션은 제출 기한이 있고, 새로 배운 개념은 많은데 적용하기는 어렵고... 힘드셨을 것 같아요.
그래도 끝까지 의견 많이 주고 받으면서 잘 마무리 하면 좋겠네요!

몇가지 코멘트 남겨두었어요. 확인하시고 미완성된 요구사항들도 부담 되지 않는 선에서 도전해보시죠!
막히는 부분이 있거나 질문이 생기신다면 편하게 코멘트나 DM으로 남겨주세요. 같이 해보죠!

3, 4단계 PR에 관한 커밋만 남기려면 어떻게 해야 할 지 학습해 보시면 좋을 것 같습니다. 우선은 제가 커밋으로 구분해서 볼게요!


남겨주신 질문.
프로그램 설계 방법도 조금 공유해주시면...
프로젝트를 하거나, 이런 미션을 하게 되면 분명하게 잘 사용 되는 설계 방식이 존재 할 겁니다! 그걸 중점적으로 찾아보고, 왜 그렇게 설계하는가?를 집중적으로 찾고 고민하고 적용하는 편인 것 같아요.

현재 자동차 경주라는 미션은 레퍼런스가 굉장히 많습니다. 다른 사람들의 PR, 스터디 시간에서 학습하는 키워드들이 존재하죠. 지환님이 "처음부터 안보고 설계 해 볼거야!"를 실현 할 수 있는 역량이 충분하지 않다고 생각이 드신다면, 같은 미션을 수행하는 다른 사람들의 다양한 설계 방식을 참고하시는 게 좋아요.
MVC패턴이나 다양한 아키텍쳐 혹은 패키지 구조 등 설계와 관련하여 지환님이 지금 단계에서 습득하시는 정보들은 수많은 시행착오 끝에 경험이 축적된 정보들입니다. 이런 정보들을 습득하고, 그 위로 지환님만의 주관과 의도가 담긴 설계를 시도 하는 것이 좋다고 생각해요.

다만, 단순히 보고 사용하는 것이 아니라 여러 구조들을 보면서, "이 사람은 왜 이렇게 했지?", "이 구조는 어떤 장점이 있지?", "아까 다른 방식이랑은 어떤 차이가 있지?" 등 고민을 하면서 보셔야 합니다. 그리고 지환님만의 분석과 고민이 섞인 레퍼런스들을 종합하여 적절하게 적용하면 됩니다.
그럼 아마 레퍼런스에서 많이 벗어나지 않은 구조의 결과물이 나올거에요. 지금은 그렇지만 이런 스터디나 미션으로 계속 훈련 하시다 보면 구조에 대해 이해도가 높아지고, 좋은 설계에 대해 고민하던 근육들이 작용해서 설계하는 것에 대한 감을 잡으실 수 있을 겁니다!

다시 질문으로 돌아와서,,
추후 프로젝트를 하거나, 새로운 언어 혹은 프레임워크를 배우거나, 시스템를 직접 구성해야 하는 등 다양한 상황에서 "설계"를 고민 해야 한다면 현재 상황과 비슷한 레퍼런스를 최대한 찾고, 그동안 훈련 해왔던 설계에 대한 저만의 고민을 덧붙여 나아갈 것 같습니다.

Comment on lines 12 to 15
Racing racing;
Cars carList;
Cars winnerList;
int RoundNumber;

Choose a reason for hiding this comment

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

Controller에 변경 가능한 형태의 상태(변수)가 존재하네요!
서로 다른 메서드에서 공용으로 사용 되기 때문에 이런 구조가 나온 것 같아요.

run()에서 메서드 내부에서만 사용 되는 변수도 제거 해볼 수 있고, playRace()에서도 사용 되는 변수는 호출할 때 필요한 값을 argument 전달하여 불필요한 상태 공유를 제거 할 수 있을 것 같아요.

메서드 내부에서만 사용 되는 변수도 제거

Cars winners = racing.findWinners();
OutputView.printWinners(winners.getCars());

인자로 넘겨 의미 전달하기

public void playRace(Cars cars, int roundNumber)

이렇게 줄여보면, RacingController는 상태가 없이도 정상적으로 run()을 실행 할 수 있어요.
선언한 변수들은 RacingController의 상태변수가 아니라 run()에서만 필요한 값이었던 것 같아요.


지환님이 작성해주셨던 객체의 변수를 인스턴스 변수라고도 불러요.
객체지향 관점에선 하나의 클래스에서 너무 많은 인스턴스 변수가 있는 것을 지양합니다.
특히 공유 되는 자원이면 더더욱 기피해야 합니다.

왜 인스턴스 변수를 줄이는 방향으로 설계 하는지, 인스턴스 변수는 어떤 의미를 가지는지 한번 고민해보세요!

Copy link
Author

Choose a reason for hiding this comment

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

좋은 리뷰 감사합니다! 늘 공용으로 사용하는 변수가 많아지면 메서드 매개변수 관리가 어려워져서
습관적으로 인스턴스 변수로 선언했는데 객체지향에선 지양해야 하는군요...

왜 공유 되는 자원일 수록 인스턴스 변수를 기피해야 하는지 제 나름대로 고민한 결론은
인스턴스 변수로 선언하고 여러 메서드에서 해당 변수를 사용하게 되면 해당 인스턴스
변수가 어디서 어떻게 사용되고 있는지 파악하기 힘들어져 추후 코드를 수정하거나 확장할 때
해당 변수를 고려하기 매우 힘들어지기 때문인 것 같았어요. 변수의 안정성도 떨어질 것 같고요

매개변수로 전달하니 변수가 어떤 메서드에 어떻게 쓰이는지 비교적 명확하게 보이기 때문에
훨씬 코드를 파악하기 좋은 코드가 된 것 같습니다!

}
}

public boolean isInPosition(int position) {

Choose a reason for hiding this comment

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

값이 같은지를 비교하는 메서드군요.
이름이 기능을 설명하지 못하는 것 같아요.
대중적으로 isInPosition이라는 네이밍을 보았을 때, 혹은 Car객체를 사용하는 다른 개발자가 "isInPosition 이런 메서드도 제공하네?" 하고 사용 하였을 때 혼동이 오진 않을지 생각보시면 적절한 이름을 찾는데 도움이 된답니다!


public class Cars {

private List<Car> cars;

Choose a reason for hiding this comment

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

가능한 곳은 최대한 불변을 유지해주세요.

Suggested change
private List<Car> cars;
private final List<Car> cars;

불변을 유지하려는 이유는 무엇일까요?

Copy link
Author

Choose a reason for hiding this comment

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

불변을 유지해야 하는 이유는 해당 변수가 잘못된 값이나 객체로 재할당되지 않게 하기 위함이라고 생각합니다!
의식적으로 늘 체크하고 불변을 유지해야 하는 변수는 잘 설정해야겠어요

}

public Cars findCarsInPosition(int position) {
List<Car> carsInPosition = new ArrayList<>();

Choose a reason for hiding this comment

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

사용하지 않는 변수는 과감하게 지워두세요!

Suggested change
List<Car> carsInPosition = new ArrayList<>();

}

public static Cars create(String[] carNames, NumberGenerator numberGenerator) {
List<Car> newCars = new ArrayList<Car>();

Choose a reason for hiding this comment

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

Suggested change
List<Car> newCars = new ArrayList<Car>();
List<Car> newCars = new ArrayList<>();

Comment on lines 39 to 43
return new Cars(
cars.stream()
.filter(car -> car.isInPosition(position))
.collect(Collectors.toList())
);

Choose a reason for hiding this comment

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

Stream을 잘 활용 하셨네요!

하나의 라인에서 여러 동작이 이루어져서 depth가 깊어졌네요.
괄호 안에서 여러 연산이 이루어지면 가독성이 많이 떨어져요. 글은 왼쪽에서부터 읽지만, 코드 흐름(연산)은 가장 안쪽의 괄호에서 이루어지잖아요!

Suggested change
return new Cars(
cars.stream()
.filter(car -> car.isInPosition(position))
.collect(Collectors.toList())
);
List<Car> carsInPosition = cars.stream()
.filter(car -> car.isInPosition(position))
.collect(Collectors.toList());
return new Cars(carsInPosition);

이렇게 분리해서 적용하면 좋아보여요! 이름을 주어서 stream 연산이 어떤 역할을 수행 하는지도 표현 할 수 있겠죠?

@@ -1,4 +1,5 @@
import org.junit.jupiter.api.Test;

Choose a reason for hiding this comment

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

import 다시 돌려주세요...ㅠ

@@ -0,0 +1,12 @@
import domain.NumberGenerator;

public class MovableNumberGenerator implements NumberGenerator {

Choose a reason for hiding this comment

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

전략패턴을 활용하셨군요!👍

Car의 기능에 대한 테스트 수정
Cars의 기능에 대한 테스트 코드 작성
Game클래스가 Racing클래스로 변경됨에
따른 수정
Controller 클래스의 run 메서드를 제외한 기능에 대해 테스트 코드 작성
View 패키지의 OutputView클래스 중 Formatting 관련 메서드의 테스트 코드 작성
Controller에 불필요한 인스턴스 변수를
줄이기 위해 지역 변수로 변환
Car 객체의 isInPosition을 hasSamePosition으로 변경하고 이에 따라 hasSamePosition을 사용하는 메서드들도 수정
누락되었던 사용자 입력 안내 메시지 출력 기능 추가
프로젝트 구조 변경에 따른 README
수정
# Conflicts:
#	src/test/java/CarTest.java
#	src/test/java/GameTest.java
@JihwanYeom
Copy link
Author

안녕하세요! 뒤늦게 리뷰 반영하게 되어 죄송합니다. 리뷰어님께서 리뷰해주신 사항이었던 인스턴스 변수 최소화, 변수명 변경
stream 가독성 개선 등을 완료하였습니다.

또한 미완성이었던 테스트 코드도 전부 작성하였습니다

이번 미션을 하면서 궁금한 점이 있었습니다

  1. 현재 Cars객체가 일급 컬렉션으로 사용되면서 Cars의 각 요소를 체크해야 하는 메서드의 경우 Cars의 getCars 메서드를
    통해 리스트를 반환받아 사용하고 있습니다. 일급 컬렉션을 사용하는 목적을 고려할 때 이러한 사용은 올바르지 않을 것 같은데
    해결 방안을 더 찾아 보아야 할까요?

  2. 현재 Cars 와 Car 두 객체를 생성할 때 전부 NumberGenerator를 매개변수로 받도록 하였는데 Cars를 생성할 때
    RandomNumberGenerator 외에 다른 NumberGenerator를 넣을 일이 없었습니다. 이런 경우에는 Cars에 NumberGenerator
    객체를 매개변수로 받을 필요가 없을까요? 혹은 추후 기능 확장을 대비해 남겨두는게 좋을까요?

  3. 컨트롤러 객체에 입력 길이 제한을 초과하였을 때 예외를 발생시켜 프로그램을 종료하도록 하는 메서드를 만들어 두었는데
    이 메서드가 어느 패키지에 있는 것이 적합한지 궁금해서 찾아보니 컨트롤러와 뷰 중 더 잘 예외를 처리할 수 있는 패키지에 넣으라는 답변만 얻을 수 있었습니다. 이런 경우에 이런 코드에서는 입출력 오류에 대한 예외를 어디에서 처리하는 것이 좋은지 리뷰어님의 의견이 궁금합니다!

  4. 도메인의 Cars 객체에 테스트를 편하게 하기 위해 Car의 리스트를 매개변수로 받아 이를 토대로 Cars객체를 생성하는 of 메소드를 만들어 두었는데 테스트만을 위한 메서드를 만드는 것은 지양해야 할까요?

  5. 현재 테스트코드에서는 매 메서드마다 Cars객체를 따로 만들어 테스트를 진행하는데, 이에 따라 중복되는 코드가 많이 발생하는 것을 볼 수 있었습니다. 하지만 각 테스트는 독립적이어야 한다는 생각에 인스턴스 변수를 만들거나 중복을 줄일 수 있는 메서드를 만들지는 않았는데, 혹시 이런 것들은 어느 정도는 허용해도 되는 부분이 있을까요?

  6. 마지막으로 코드 외에 질문입니다. 제가 아직 깃을 이용해서 프로젝트를 커밋하고 버전관리를 하는 데에 익숙하지 않아 매번 이 기능 고치는 와중에 저 기능 고치다가 어느새 한 커밋 단위가 너무 커진다는 느낌을 받았습니다. 아마 코드에 불필요한 의존성이 많아서 그럴 수도 있다는 생각도 들었어요... 한 커밋 단위를 만들 때 미리 목표를 정해두는 게 도움이 될까요?

뒤늦게 많은 질문 드리게 되어 죄송합니다!

Comment on lines +72 to +75
게임 실행 메서드가 실행되면 사용자에게 자동차 이름과 라운드 수를 입력받아
레이싱을 수행합니다. 레이싱이 끝나면 우승자 리스트를 출력하고 메서드가
종료됩니다.

Choose a reason for hiding this comment

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

추가된 요구사항에 맞게 기능 명세서도 업데이트 하셨네요!
문서화도 관리 대상입니다. 아주 좋은 습관이에요👍👍

Comment on lines 6 to 10
public static void main(String[] args) {

new RacingController().run(new RandomNumberGenerator());

}
Copy link

@TaeyeonRoyce TaeyeonRoyce Mar 22, 2025

Choose a reason for hiding this comment

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

줄바꿈도 의도가 담길 수 있어요. 필요 없는 줄바꿈이라면, 오해하지 않게 깔끔하게 관리해주세요

Suggested change
public static void main(String[] args) {
new RacingController().run(new RandomNumberGenerator());
}
public static void main(String[] args) {
new RacingController().run(new RandomNumberGenerator());
}

Copy link

@TaeyeonRoyce TaeyeonRoyce 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 40 to 44
public void checkCarNameLengths(List<String> names) {
if (names.stream().anyMatch(name -> name.length() > NAME_LENGTH_LIMIT)) {
throw new IllegalArgumentException("이름의 길이는 " + NAME_LENGTH_LIMIT + "자 이하여야 합니다.");
}
}

Choose a reason for hiding this comment

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

검증 위치에 대해 질문 남겨주신 것도 포함하여 여기서 답변 드릴게요!
하나의 유즈케이스에서 여러 검증이 필요하다 보니 각 검증 로직을 어디에 담아야 할지에 대한 의문은 당연하게 생길 것 같아요.

"유즈케이스" ex) 참여할 자동차의 이름을 입력한다.

위 예시로 고민해보면 다음과 같은 검증들이 이루어져야 해요. (적절한 예시를 위해 몇개의 요구사항을 추가 해봤어요)

  1. 자동차 이름은 빈 값일 수 없다.
  2. 10개 이상의 자동차는 참여 할 수 없다. (추가됨)
  3. 자동차 이름은 5글자 이상일 수 없다.
  4. 중복된 이름은 입력 될 수 없다. (추가됨)

"각 검증들이 어디에서 이루어져야 할까?"에 대한 답변으로는 해당 규칙이 어느 객체(계층)의 규칙일까? 를 고민 해보시면 좋아요!
각각 하나씩 뜯어볼게요.


자동차 이름은 빈 값일 수 없다.

자동차 이름에 대한 규칙은 자동차 객체의 요구사항이에요.
해당 이름으로 자동차를 생성할 때 검증을 한다면 자동차 객체는 어디서 인스턴스화(new Car()) 를 해도 해당 규칙을 보장합니다.

  • hint) 자동차 이름을 담당하는 객체를 추가로 만들수도 있어요! 원시값 포장

빈 값에 대한 검증은 입력단에 추가할 수도 있어요. fail-fast라는 용어가 있는데요. 빈 값에 대한 검증을 입력단에서 검증하고 처리한다면, 불필요하게 new Car() 로직을 실행 하지 않아도 됩니다!

10개 이상의 자동차는 참여 할 수 없다.

설명을 위해 추가한 요구사항이에요. 이런 규칙이 있다면 어디서 검증해야 할까요?
우선, 위에서 설명드렸던 것처럼 입력 단계에서 충분히 확인 할 수 있는 규칙이라면 fail-fast관점으로 입력단에서 검증할 수 있을 것 같아요.
그리고, 자동차 경주 게임에 참여하는 자동차들이 지켜야 하는 규칙이라면 Cars를 생성하는 단계에서 검증하여 해당 규칙을 보장할 수도 있겠네요!

자동차 이름은 5글자 이상일 수 없다

이 규칙 역시 자동차 이름의 규칙이에요. 위 설명과 유사합니다!

중복된 이름은 입력 될 수 없다. (추가됨)

해당 규칙도 Cars에서 규칙을 보장할 수 있고, 입력단에서도 검증할 수 있을 것 같아요.


설명이 길었지만, 정리하자면 해당 규칙이 어느 객체(계층)의 규칙일까?를 고민하고 그 객체에서 해당 규칙을 보장할 수 있도록 구성해야 하고, fail-fast 할 수 있다면 하는게 좋습니다.

"입력단에서 검증을 하는데, 객체에서도 검증해야 할까?" 라는 의문이 들 수도 있을 것 같아요.
입력단에서의 검증은 있으면 좋은거지, 객체의 규칙을 보장할 수 없기 때문에 신뢰할 순 없습니다.
자동차 이름의 길이에 대한 검증을 InputView.class에서만 이루어진다면, test 코드나 다른 view에서 new Car()를 실행 했을 때 자동차 이름의 길이를 보장하지 못하기 때문이에요.

천천히 읽어보시고, controller에서 이루어지고 있는 검증에 대해 다시 한번 생각해보시죠!

Comment on lines 15 to 21
public static Cars create(List<String> carNames, NumberGenerator numberGenerator) {
List<Car> newCars = new ArrayList<>();
for (String carName : carNames) {
newCars.add(new Car(carName, numberGenerator));
}
return new Cars(newCars);
}

Choose a reason for hiding this comment

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

남겨주신 질문

현재 Cars 와 Car 두 객체를 생성할 때 전부 NumberGenerator를 매개변수로 받도록 하였는데 Cars를 생성할 때 RandomNumberGenerator 외에 다른 NumberGenerator를 넣을 일이 없었습니다. 이런 경우에는 Cars에 NumberGenerator 객체를 매개변수로 받을 필요가 없을까요? 혹은 추후 기능 확장을 대비해 남겨두는게 좋을까요?

전략 패턴을 도입한건 온전히 Car 때문이에요. 다른 전략을 선택하는 것 역시 Car 객체를 생성할 때만 존재합니다.
그렇기 때문에 Cars를 생성할 때는 굳이 필요한 매개변수는 아닙니다.

Cars에 대해 다시 고민해볼까요?
현재 create라는 팩토리 메서드에서 Car가 아닌 자동차 이름과 생성 전략을 매개변수로 받고 있습니다.
이게 과연 Cars라는 자동차 집단의 역할일까요?

각각의 이름과 생성전략을 가지고 있는, 이미 만들어진 List<Car>를 통해 생성하는 건 어떨까요?
Car의 생성은 책임지지 않고, Car의 집합인 Cars만의 책임을 유지해보는 겁니다!
단독으로 존재하는 Car에서는 담아낼 수 없는 책임(요구사항)들, 예를 들면 자동차 이름은 중복 될 수 없다, 자동차들을 전진 시킨다, 현재 자동차들의 최대 위치를 찾는다 등등이요!

Comment on lines 23 to 25
public static Cars of(List<Car> cars) {
return new Cars(new ArrayList<>(cars));
}

Choose a reason for hiding this comment

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

새롭게 선언해서 외부와의 참조를 끊어냈네요👍
생성할 때 뿐만아니라, 현재 필드를 제공할 때(getter)도 방어적 복사를 사용할 수 있어요.


public class Racing {

private final Cars carList;

Choose a reason for hiding this comment

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

사소하지만, 변수이름에 Collection 구현체 담지 마세요!
나중에 Set으로 수정하면 이름도 같이 바꿔야 해서요...!

Suggested change
private final Cars carList;
private final Cars cars;


@Override
public int generateNumber() {
return new Random().nextInt(10);

Choose a reason for hiding this comment

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

new Random()을 매번 선언하면 불필요한 자원 낭비가 될 수 있어요.
상수로 선언하여 한번만 생성해서 성능을 좀 더 높일 수 있답니다!

Scanner를 선언 하신 것 처럼요!

Comment on lines 29 to 34
StringBuilder formatted;
formatted = new StringBuilder(car.getName() + " : ");
for (int i = 0; i < car.getDistance(); i++) {
formatted.append("-");
}
return formatted.toString();

Choose a reason for hiding this comment

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

String의 함수를 잘 활용하면 이렇게도 줄여볼 수 있어요

Suggested change
StringBuilder formatted;
formatted = new StringBuilder(car.getName() + " : ");
for (int i = 0; i < car.getDistance(); i++) {
formatted.append("-");
}
return formatted.toString();
return car.getName() + " : " + "-".repeat(car.getDistance());

Comment on lines 37 to 47
public static void printWinners(List<Car> winners) {
List<String> winnerNames = new ArrayList<>();
for (Car winner : winners) {
winnerNames.add(winner.getName());
}
System.out.println(formatWinnerNames(winnerNames) + "가 최종 우승했습니다.");
}

public static String formatWinnerNames(List<String> cars) {
return cars.toString().replace("[", "").replace("]", "");
}

Choose a reason for hiding this comment

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

이렇게도 줄여 볼 수 있어요!
stream을 잘 활용하면 굉장히 강력해 보이지 않나요?

Suggested change
public static void printWinners(List<Car> winners) {
List<String> winnerNames = new ArrayList<>();
for (Car winner : winners) {
winnerNames.add(winner.getName());
}
System.out.println(formatWinnerNames(winnerNames) + "가 최종 우승했습니다.");
}
public static String formatWinnerNames(List<String> cars) {
return cars.toString().replace("[", "").replace("]", "");
}
public static void printWinners(List<Car> winners) {
String winnerNames = winners.stream()
.map(Car::getName)
.collect(Collectors.joining(", "));
System.out.println(winnerNames + "가 최종 우승했습니다.");
}

Comment on lines 49 to 51
public List<Car> getCars() {
return cars;
}

Choose a reason for hiding this comment

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

@TaeyeonRoyce
Copy link

TaeyeonRoyce commented Mar 22, 2025

남겨주신 질문에 대한 답변


현재 Cars객체가 일급 컬렉션으로 사용되면서 Cars의 각 요소를 체크해야 하는 메서드의 경우 Cars의 getCars 메서드를

통해 리스트를 반환받아 사용하고 있습니다. 일급 컬렉션을 사용하는 목적을 고려할 때 이러한 사용은 올바르지 않을 것 같은데
해결 방안을 더 찾아 보아야 할까요?

질문에 답이 있네요. getCars()로 해당 객체의 캡슐화를 저해하는 방식은 안좋아요.
하지만, getter가 항상 잘 못되었다고 생각하진 않아요. 필요하다면 사용해야죠!

// 외부클래스
public runCars(Cars cars) {
    List<Car> runningCars = cars.getCars();
    for (Car car : runningCars) {
        car.move();
    }
}

이런 구조가 캡슐화를 저해하는 것 같아요.

public void move() {
    for (Car car : cars) {
        car.move();
    }
}

해당 객체에서 수행 할 수 있는, 책임을 부여할 수 있는 것들은 응집력 있게 내부에 존재하도록 하는게 중요합니다.
절대적으로 getter를 지양하기 보단, 무분별한 getter의 사용을 지양하면 좋습니다!


현재 Cars 와 Car 두 객체를 생성할 때 전부 NumberGenerator를 매개변수로 받도록 하였는데 Cars를 생성할 때

RandomNumberGenerator 외에 다른 NumberGenerator를 넣을 일이 없었습니다. 이런 경우에는 Cars에 NumberGenerator
객체를 매개변수로 받을 필요가 없을까요? 혹은 추후 기능 확장을 대비해 남겨두는게 좋을까요?

#114 (comment)


컨트롤러 객체에 입력 길이 제한을 초과하였을 때 예외를 발생시켜 프로그램을 종료하도록 하는 메서드를 만들어 두었는데

이 메서드가 어느 패키지에 있는 것이 적합한지 궁금해서 찾아보니 컨트롤러와 뷰 중 더 잘 예외를 처리할 수 있는 패키지에 넣으라는 답변만 얻을 수 있었습니다. 이런 경우에 이런 코드에서는 입출력 오류에 대한 예외를 어디에서 처리하는 것이 좋은지 리뷰어님의 의견이 궁금합니다!

#114 (comment)


도메인의 Cars 객체에 테스트를 편하게 하기 위해 Car의 리스트를 매개변수로 받아 이를 토대로 Cars객체를 생성하는 of 메소드를 만들어 두었는데 테스트만을 위한 메서드를 만드는 것은 지양해야 할까요?

네, 테스트만을 위한 메서드는 지양하는게 좋아요. 테스트에서만 사용하려고 구성한 메서드가 프로덕션에서도 오용할 수 있는 가능성이 있기 때문이에요. 테스트를 위한 메서드가 필요하다면, 해당 객체를 응집력 있게 혹은 확장성 높게 정의 하지 못했다고 볼 수도 있거든요.
#114 (comment)
이 코멘트처럼, of()가 테스트에서만 사용 되어야 하는가? 도 고민 해볼 수 있을 것 같아요.


현재 테스트코드에서는 매 메서드마다 Cars객체를 따로 만들어 테스트를 진행하는데, 이에 따라 중복되는 코드가 많이 발생하는 것을 볼 수 있었습니다. 하지만 각 테스트는 독립적이어야 한다는 생각에 인스턴스 변수를 만들거나 중복을 줄일 수 있는 메서드를 만들지는 않았는데, 혹시 이런 것들은 어느 정도는 허용해도 되는 부분이 있을까요?

테스트는 독립적일수록 좋습니다. 테스트끼리의 종속이 많아지면, 프로덕션에서 코드가 추가될 때 마다 테스트를 구성하는데 비용이 굉장히 늘어나기 때문이에요.
테스트가 무거워지는것에 대한 고민도 중요합니다. 생산성과 연관이 깊은 수치에요. 테스트를 수행하는 속도가 3분이라면, 생산성이 많이 떨어지겠죠?
테스트 속도를 높이는 방법은 여러가지지만, 현재 고민하시는 단계(변수의 중복)에서는 Test Fixture에 대해 학습하고 적용해보시면 좋을 것 같습니다!


마지막으로 코드 외에 질문입니다. 제가 아직 깃을 이용해서 프로젝트를 커밋하고 버전관리를 하는 데에 익숙하지 않아 매번 이 기능 고치는 와중에 저 기능 고치다가 어느새 한 커밋 단위가 너무 커진다는 느낌을 받았습니다. 아마 코드에 불필요한 의존성이 많아서 그럴 수도 있다는 생각도 들었어요... 한 커밋 단위를 만들 때 미리 목표를 정해두는 게 도움이 될까요?

처음에는 커밋단위에 대해 감 잡기가 어려울 것 같아요.
커밋을 하는 본질적인 이유에 대해 찾아보시면 감을 잡을 수 있지 않을까 싶습니다.
git이라는 형상관리, 그리고 github이라는 git으로 구성된 원격 repository가 어떤 목적으로 활용 되고 있는지...!

저는 commit을 할 때, 기능 단위로 하려고 해요. 기능1, 기능2, 기능3 으로 쌓아올리면, 롤백을 할 때도 편합니다.
또, 하나의 PR에 달린 커밋을 보면서 작업자가 어떤 방식으로, 어떤 과정으로 해당 기능을 개발 했는지 파악하기도 용이합니다.

next-step/java-lotto-clean-playground#1 해당 PR을 참고하시면 조금 감이 잡히지 않을까 싶어요!

Car 객체의 이름을 String 에서
원시값 포장을 통해 Name 클래스로 변경

Name 클래스에 이름 길이, 공백 여부
예외 처리 추가

Cars 클래스에 차 갯수 중복 여부 예외처리 추가

Controller 에서 차 이름 길이 검증 메서드 삭제

Cars 클래스의 of 메서드 이름을 from 으로 변경
Inputview 클래스에 예외처리 함수
작성
이름과 난수 생성기를 매개변수로 받아 Cars 객체를 생성하는 create 메서드를 삭제

대신 from 메서드를 이용해 Controller 에서 만들어진 List<Car>객체를 매개변수로 받아 Cars 객체 생성

Cars 의 getCars 메소드에서 방어적 복사 사용

그 외 변수명 수정(carList -> cars)
Random 객체를 상수로 선언
StringBuilder 와 Stream 을 활용한 코드로 수정
@JihwanYeom
Copy link
Author

안녕하세요 리뷰어님! 리뷰어님이 작성해주신 리뷰와 질문에 대한 답변까지 코드에 한번 반영해 보았습니다 특히 입력 검증 기능을 객체와 입력단에 전부 추가해 보면서 두 방식의 장단점을 생각해 볼 수 있었던 것 같아요!

그리고 일급 컬렉션과 정적 팩토리 메서드를 사용하는 이유 역시 조금 더 명확하게 알 것 같습니다 단순히 일급 컬렉션으로 모든 컬렉션을 대체하면 되는 줄 알았는데 두 객체의 역할이 각각 있다는 것도 알게 된 것 같아요.

stream, StringBuilder 와 같이 편리한 메서드들은 조금 더 공부해봐야겠어요! 완전히 제 것으로 만들어서 편하게 사용하면 생산성에 크게 도움이 될 것 같아요!

질문에 대한 답변이 정말 큰 도움이 된 것 같아요. 스스로 진도가 느린 편이라고 생각하는데, 리뷰어님의 답변을 보고 오래 고민하면서 프로그래밍을 하면서 생각하는 습관이 정말 많이 늘어난 것 같습니다. 많은 신경 써주셔서 감사합니다!

@boorownie boorownie merged commit 00fca49 into next-step:jihwanyeom Mar 24, 2025
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.

3 participants