Skip to content

Comments

[2단계 - 자동차 경주 리팩터링] 레넌(조형래) 미션 제출합니다.#350

Merged
24 commits merged intowoowacourse:broraefrom
brorae:step2
Feb 21, 2022
Merged

[2단계 - 자동차 경주 리팩터링] 레넌(조형래) 미션 제출합니다.#350
24 commits merged intowoowacourse:broraefrom
brorae:step2

Conversation

@brorae
Copy link

@brorae brorae commented Feb 15, 2022

안녕하세요. 던!
1단계에서 좋은 인사이트들 많이 주셔서 감사합니다.

1단계 부터 MVC 패턴을 적용하여서 1단계에서 해주신 피드백 반영해서 리뷰 요청드립니다.

#질문 사항

  1. 지난번 질문 사항 1번에서 게임을 진행하는 domain에서 좋을 것 같다는 의견을 주셨는데, 게임을 진행하는 domain을 따로 만들라는 말씀이신가요??
    [1단계 - 자동차 경주 구현] 레넌(조형래) 미션 제출합니다. #294 (review)

  2. Cars의 insertCar() 메소드는 Cars 내부에서만 호출되고 있기 때문에, private으로 바꾸는 것이 좋다고 생각하는데요. insertCar()를 호출한 insertCarFromCarNames() 메소드도 private으로 되어있어 테스트하지 못하는 경우에는 어떻게 하는 것이 좋을까요??

  3. 일급컬렉션 cars에 car객체를 넣을 때, 메소드를 통해서 넣는다면, 나중에 추가적인 string배열을 메소드를 통해 간편하게 추가할 수 있다는 점에서 이점이 있다고 생각하는데, 생성자를 통해 넣을 때와의 추가적인 차이점이 있을까요??

  4. 변하지 않을 것 같은 값들은 모두 final 키워드로 처리해도 괜찮을까요??

  5. getter의 사용은 UI로직에서 사용하는 것 외엔 지양하라는 것을 본적이 있는데, test code에서의 사용은 괜찮을까요?

  6. 비즈니스 로직이라는 단어가 한번에 와닿지가 않는데, 추가적인 설명을 부탁드려도 될까요?

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

안녕하세요 레넌!
1단계에서 이미 MVC 패턴을 적용하셔서 빠르게 요청주셨군요!
간단한 코멘트 남겼으니 확인 부탁드리겠습니다.


따로 질문해주신것들 답변드리겠습니다!

  1. 게임을 진행하고 있는 domain 영역에서 처리하는게 좋겠다는 의미였습니다! 그치만 게임을 진행하는 domain이 따로 있으면 더 좋겠네요! (지금은 controller가 게임을 진행하는 역할도 하고 있군요!)

  2. private메서드를 호출하는 public 메서드의 테스트 코드를 작성하면서 private 메서드의 로직을 검증할 수 있도록 하면 되겠네요!

  3. 게임규칙이 변하는게 아니라면 자동차게임 특성상 경주를 시작한 후에 car객체가 추가로 생성되면 안될 것 같은데요!(게임이 정상적으로 진행되기 어려울 것 같아요) 그렇다면 생성자를 통해서 car객체를 넣는다면 추가로 car객체가 생성되는것을 방지할 수 있겠네요!

  4. final 키워드의 선언은 변경을 허용하지 않는다는 의미입니다. 변하지 않을 것 같아서 final로 선언한다는 것과는 약간 의미가 다르게 느껴지네요. final로 선언하려는 값이 반드시 변경되지 않아야하는지 고민해보세요!

  5. UI로직 외에서 getter의 사용을 지양하라는 말이 어떤 이유로 나왔는지 먼저 고민해볼 수 있을까요? 고민해본 결과가 test code에서는 적용이 되는지까지 생각해보세요!

  6. 간단하게 domain 영역의 로직이라고 보시면 될 것 같습니다. https://medium.com/@su_bak/term-business-logic%EC%9D%B4%EB%9E%80-6d53c4782d73 이 사이트를 참고해봐도 좋겠네요.

@brorae
Copy link
Author

brorae commented Feb 18, 2022

  1. 게임을 진행하고 있는 domain 영역에서 처리하는게 좋겠다는 의미였습니다! 그치만 게임을 진행하는 domain이 따로 있으면 더 좋겠네요! (지금은 controller가 게임을 진행하는 역할도 하고 있군요!)
    네! RacingGame model 추가하였고, 게임은 Application에서 진행할 수 있도록 했습니다. RaceController에 있는 race메소드는 view메소드를 사용해야해서 controller 에서 처리 해주었습니다.
  1. private메서드를 호출하는 public 메서드의 테스트 코드를 작성하면서 private 메서드의 로직을 검증할 수 있도록 하면 되겠네요!
    insertCarFromCarNames 메소드가 view에서 값을 받아서 model cars에 값을 전달해준다고 생각하여 cars에서 raceController로 위치를 변경했는데요, 이렇게 되면 insertCar 메소드가 자연스럽게 public이 되는데 상관 없을까요??
  1. 게임규칙이 변하는게 아니라면 자동차게임 특성상 경주를 시작한 후에 car객체가 추가로 생성되면 안될 것 같은데요!(게임이 정상적으로 진행되기 어려울 것 같아요) 그렇다면 생성자를 통해서 car객체를 넣는다면 추가로 car객체가 생성되는것을 방지할 수 있겠네요!
    생성자로 생성해도 insertCar() 메소드를 통해서 생성될 수 있을 것 같은데, 이러면 insertCar를 private으로 설정해주어야할까요? 위의 문제랑 충돌이 되는 것 같아서요..
  1. UI로직 외에서 getter의 사용을 지양하라는 말이 어떤 이유로 나왔는지 먼저 고민해볼 수 있을까요? 고민해본 결과가 test code에서는 적용이 되는지까지 생각해보세요!
    getter로 모델의 상태를 변화시키는 경우가 많아지면 안되기 때문이라고 생각합니다. 값을 읽는 용도로 가져오기만 한다면 상관없다고 생각합니다.

RaceController의 insertCarFromCarNames()의 단위테스트는 어떻게 진행하면 좋을까요?? insertCar의 단위테스트를 진행했으니 진행하지 않아도 괜찮을까요??

private/public을 클래스별 메소드 사용을 기준으로 둬야하는지 아니면 보안적?인 측면에 둬야하는지 궁금합니다.

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

안녕하세요 레넌! 리뷰어 던입니다.
피드백 잘 반영해주시고 계시네요! 👍
몇가지 코멘트 남겼으니 확인 부탁드릴게요!


따로 질문하신것들 답변드리겠습니다.

2 -> 메서드의 위치가 변경되면서 메서드를 호출하는 곳과의 위치관계가 변경될 수 있으므로 메서드의 접근제어자가 변경되는건 자연스러워 보이네요!
3 -> car객체를 cars로 전달해주는 방법으로 2가지(생성자, 메서드)가 모두 필요하다면 어쩔 수 없겠지만, 지금 경우에선 둘 다 존재할 필요는 없어보여요. 생성자를 통해 cars를 생성한다면 메서드로 car객체를 전달하는 메서드는 없애거나 private메서드로 변경하는것이 맞아보여요! (사용하는곳이 없을테니깐요)

  1. insertCarFromCarNames메서드를 호출한 뒤 생성된 cars의 값을 검증해보면 될 것 같아요.
    메서드 파라미터로 받은만큼 car객체를 생성하는것 외에는 로직이 없어서 테스트코드가 없어도 문제가 될 건 없어보이네요. 그치만 테스트코드 작성 연습할겸 작성해보면 좋을 것 같아요

  2. 외부 객체에서 호출하는 메서드는 public으로 설정하고 내부에서만 호출하는 메서드라면 private으로 설정하면 됩니다! 보안적인 측면은 어떤것을 의미하는걸까요?

@brorae
Copy link
Author

brorae commented Feb 20, 2022

  1. 외부 객체에서 호출하는 메서드는 public으로 설정하고 내부에서만 호출하는 메서드라면 private으로 설정하면 됩니다! 보안적인 측면은 어떤것을 의미하는걸까요?
    죄송합니다. 설명이 부족했네요. 보안적인 측면은 public인 메소드를 외부에서 호출할 수 있는 가능성을 말씀드리려고 했었습니다. 이런 것들은 신경 쓰지 않고, 호출되는 위치에 따라서 접근제한자를 설정해주면 될까요??

RaceController에 있던 race() 메소드는 외부에서 요청을 받아서 RacingGame 도메인이 처리해주는 것이 맞다고 생각하여 위치를 수정했는데요, 이 메소드를 처리하면서 출력되는 부분이 들어가게 되는데, UI 로직과 비즈니스 로직은 분리하라고 들은적이 있어서, 이렇게 처리를 해야하는지, race()메소드를 view에서 처리해줘야하는지 궁금합니다.

감사합니다.

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

안녕하세요 레넌! 리뷰어 던입니다.
질문해주신 부분과 관련해 코멘트 드렸습니다!
확인 부탁드리고 궁금한 점 있으시다면 코멘트나 DM주세요!
조금만 더 힘내봐요! 💪

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

안녕하세요 레넌!
피드백 잘 반영해주셨네요! 👍
첫번째 미션 구현하시느라 고생 많으셨습니다! 💯

@ghost ghost merged commit 633a87d into woowacourse:brorae Feb 21, 2022
This pull request was closed.
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.

1 participant