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단계 - 자동차 경주 구현] 조이(김성연) 미션 제출합니다. #488

Merged
merged 36 commits into from
Feb 9, 2023

Conversation

yeonkkk
Copy link
Member

@yeonkkk yeonkkk commented Feb 9, 2023

안녕하세요! 조이입니다 (함께 코드를 작성한 페어는 하마드입니다!).
많이 부족하지만 코드 리뷰 잘 부탁드리겠습니다!

코드를 작성하면서 주로 고민하고 어려웠던 점은 아래와 같습니다!

질문 사항

  • 이후 변경에 용이하도록 사용자에게 안내를 위해 출력하는 안내 메세지와 에러 메세지를 enum으로 작성하였습니다. 혹시 이렇게 작성하는 것이 불필요한 것은 아닌지 고민이 되어 의견을 여쭙고 싶습니다.
  • 사용되는 상수들을 Constant 에 작성하여 사용하였는데 같은 상수이지만 여러 곳에 사용되어 변수명을 달리하여 사용하는 경우가 있었습니다. 이렇게 사용하는 것이 불필요한지 궁금하며, 더 효율적인 방안이 있을지 궁금합니다.
  • 차의 위치 정보를 차 객체인 Car 와 전체 차를 관리하는 CarRepository 중 어디에 구현해야하는지에 대해 고민이 되었습니다. 처음 구현할 때는 Car 에서 차의 위치 정보를 관리하도록 하였으나, 중간에 별도로 관리하는 것이 의존을 줄일 수 있다고 판단하여 CarRepository에서 관리하도록 수정하였습니다.
  • CarRepository 구현체인 CarRepositoryImplInputController의 생성자를 통해 주입시키도록 구현하였습니다. 초기에는 구현체 로직을 담은 CarRepository 클래스를 구현하였으나, 이후 리팩터링 과정에서 수정하였습니다. 그런데 생성자 주입으로 코드를 수정하는 과정에서 setCarBoard() 의 역할을 하는 insertCarBoard() 를 작성하여 사용하게 되었습니다. setter 사용을 지양해야한다고 알고 있어서 페어와 오래 고민하였으나 다른 방법은 떠오르지 않아 위와 같이 작성하게 되었습니다. 혹시 더 좋은 방법이 있을지 궁금합니다.

혹시 제 코드를 보시면서.. 추가로 보면 좋을 것 같은 내용이 있다면 키워드만 알려주시면 감사하겠습니다!
읽어주셔서 감사합니다 :)

Copy link

@wannte wannte left a comment

Choose a reason for hiding this comment

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

이후 변경에 용이하도록 사용자에게 안내를 위해 출력하는 안내 메세지와 에러 메세지를 enum으로 작성하였습니다. 혹시 이렇게 작성하는 것이 불필요한 것은 아닌지 고민이 되어 의견을 여쭙고 싶습니다.사용되는 상수들을 Constant 에 작성하여 사용하였는데 같은 상수이지만 여러 곳에 사용되어 변수명을 달리하여 사용하는 경우가 있었습니다. 이렇게 사용하는 것이 불필요한지 궁금하며, 더 효율적인 방안이 있을지 궁금합니다.

=> 1. 리팩토링 관점, 2. 협업의 관점 에서 각각 고민해보면 좋을 것 같습니다.

차의 위치 정보를 차 객체인 Car 와 전체 차를 관리하는 CarRepository 중 어디에 구현해야하는지에 대해 고민이 되었습니다. 처음 구현할 때는 Car 에서 차의 위치 정보를 관리하도록 하였으나, 중간에 별도로 관리하는 것이 의존을 줄일 수 있다고 판단하여 CarRepository에서 관리하도록 수정하였습니다.
CarRepository 구현체인 CarRepositoryImpl 을 InputController의 생성자를 통해 주입시키도록 구현하였습니다. 초기에는 구현체 로직을 담은 CarRepository 클래스를 구현하였으나, 이후 리팩터링 과정에서 수정하였습니다. 그런데 생성자 주입으로 코드를 수정하는 과정에서 setCarBoard() 의 역할을 하는 insertCarBoard() 를 작성하여 사용하게 되었습니다. setter 사용을 지양해야한다고 알고 있어서 페어와 오래 고민하였으나 다른 방법은 떠오르지 않아 위와 같이 작성하게 되었습니다. 혹시 더 좋은 방법이 있을지 궁금합니다.

=> Repository 의 구현체를 사용하였을 때 어떤 장점이 있을지에 대해서 고민해보면 더 좋을 것 같습니다! 가능하다면, Car가 위치정보를 관리하는 방법도 진행해보면서 지금의 구조와 어떤 장단이 있을지 고민해보면 좋을 것 같습니다.

사용자가 자동차 목록과 시도 횟수를 입력하면 우승자를 가리는 프로그램

## 구현 기능 목록
- [ ] 자동차 목록을 입력 받는다.
Copy link

Choose a reason for hiding this comment

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

check를 하면서 구현하면, 놓치는 부분 없이 진행할 수 있을 것 같아요~

Comment on lines +9 to +13
void cycleCars(MovingStrategy movingStrategy);
void move(Car car,int number);
boolean isAllowedToMove(int number);
List<Car> findWinners();
Map<Car, Integer> getCarBoard();
Copy link

Choose a reason for hiding this comment

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

공백을 주면 더 읽기 쉬운 코드가 될 것 같습니다.

@Override
public void move(Car car, int number) {
if (isAllowedToMove(number)) {
carBoard.put(car, carBoard.get(car) + ADD_POINT);
Copy link

Choose a reason for hiding this comment

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

개인적으로 상수는 해당 역할(책임)의 클래스에 위치시킵니다.


public class CarRepositoryImpl implements CarRepository{

private Map<Car, Integer> carBoard;
Copy link

Choose a reason for hiding this comment

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

Car와 위치를 별도로 관리하는 방법을 사용하셨네요!

위치의 개념을 Car의 개념과 분리했을 때, 얻는 장단점을 고민해보면 좋을 것 같습니다!

import java.util.Map;
import strategy.MovingStrategy;

public interface CarRepository {
Copy link

Choose a reason for hiding this comment

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

가지고 있는 역할들이, Repository(저장소)에 어울리지 않는 것 같습니다.

다른 네이밍을 사용해봐도 좋을 것 같습니다.

@@ -0,0 +1,9 @@
package strategy;

public class FixedMovingStrategy implements MovingStrategy {
Copy link

Choose a reason for hiding this comment

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

interaface 로 분리해서 테스트 진행하기가 편리해져서 좋네요!

import view.InputView;
import view.OutputView;

public class GamePlay {
Copy link

Choose a reason for hiding this comment

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

InputController.startProgram()

에서도 InputView, OutputView 가 사용되는데요!

InputView, OutputView 의 호출을 한 곳에서 진행하면 어떨까요?

input과 output의 흐름을 한 눈에 확인하면, 흐름을 이해하는 데 쉬울 것 같다는 생각입니다.

Copy link

@wannte wannte left a comment

Choose a reason for hiding this comment

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

안녕하세요 조이. 이번 리뷰어를 맡은 완태입니다.

몇가지 리뷰와 고민해 보면 좋을 점 남겼습니다!

고민해보시고 다음 단계를 진행하며 반영해주시면 되겠습니다. 🙏

@wannte wannte merged commit f7e1e19 into woowacourse:yeonkkk Feb 9, 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