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

Step4 Feedback #26

Open
jinios opened this issue May 14, 2018 · 4 comments
Open

Step4 Feedback #26

jinios opened this issue May 14, 2018 · 4 comments
Assignees
Labels
feedback Feedback from JK refactor
Milestone

Comments

@jinios
Copy link
Owner

jinios commented May 14, 2018

카드게임 앱 미션 Step4 주요 피드백

@jinios jinios added the feedback Feedback from JK label May 14, 2018
@jinios jinios added this to the Step4 milestone May 14, 2018
@jinios jinios self-assigned this May 14, 2018
@jinios
Copy link
Owner Author

jinios commented May 14, 2018

객체 위임 관계

이름을 결정할 때 mainViewController 내부에서 다른 곳에 위임하는 객체를 cardGameDelegate로 정하고
카드게임을 전체 관리하는 싱글톤 객체를 CardGameManager로 하도록 하세요.
카드게임매니저가 ViewController 입장에서 델리게이트(위임한) 객체가 되는겁니다.

@jinios
Copy link
Owner Author

jinios commented May 14, 2018

네이밍 이슈

  • 객체 타입 이름에 Delegate를 붙이기보다는 프로토콜 이름에 붙이는 편입니다. 참고하세요.
  • UIView에서 draw- 시작하는 것은 Rendering 규칙에 맞춰서 Core Graphics 수준에서 그릴 때 사용하는 용어입니다. 하위 뷰 객체를 설정하는 경우에 사용하면 헷갈릴 수 있습니다. configure- 혹은 setup- 같은 다른 표현을 사용하는게 좋겠습니다.
  • 위에 있는 drawRefresh()와 redraw()도 명확하게 구분이 안되는 것 같습니다. draw- 보다 다른 표현을 사용해서 명확하게 구분해주면 좋겠습니다. 다시 그리기 위한 표현은 invalidate- 도 많이 쓰는 표현입니다.
  • 위에 deckView와 foundationView는 drawDefault()로 동일한데 stackView는 newDraw() 네요? 같은 동작인데 함수 이름이 특별히 다른 이유가 있나요?

@jinios
Copy link
Owner Author

jinios commented May 14, 2018

간단한 코드 이슈

class CardStacksView: UIView {
    var gameManager: CardGameManageable = CardGameDelegate.shared()
    var wholeStackManager: (CardStackManageable & Stackable)!
    var oneStackViews = [OneStack]()
 // ...
    convenience init() {
        self.init(frame: CGRect(x: 0, y: PositionY.bottom.value,
                                width: 414, height: 736 - PositionY.bottom.value))
        self.wholeStackManager = gameManager.getWholeStackDelegate()
    }
  • gameManager가 단지 wholeStackManager 만 가져오기 위한거라면 인스턴스 변수로 선언할 필요까지는 없고 여기서 CardGameDelegate.shared().getWholeStackDelegate() 로 접근해도 될 것 같네요.
    • CardStacksView에서 CardGameDelegate가 하는 일이 별로 없이 스택객체만 전달해주는 역할만 하기때문에 인스턴스변수로 선언해야하나 말아야하는지 고민했던 부분이다. 결국 가독성을 높일 겸 인스턴스변수로 선언하게되었는데 이정도는 오히려 바로 접근하도록 만드는 것이(특히 싱글톤객체이기때문에) 더 좋은 방법인 것 같다.

  • 습관적으로도 모든 var 변수는 private으로 선언하도록 하세요.
  • required init?(coder aDecoder: NSCoder)에 fatalError를 남겨놓기 보다는 동일하게 동작하도록 지정생성자를 연결해주는 게 좋습니다.

@jinios
Copy link
Owner Author

jinios commented May 14, 2018

ViewController 중복코드 분리

  • deckViewDidDoubleTap()과 stackViewDidDoubleTap() 두 개 함수가 비슷한 부분이 많은 것 같은데... 합쳐서 공통화할 수 있을까요?

jinios added a commit that referenced this issue May 14, 2018
jinios added a commit that referenced this issue May 14, 2018
CardGameDelegate -> CardGameManager
jinios added a commit that referenced this issue May 14, 2018
jinios added a commit that referenced this issue May 14, 2018
CardGameDelegate -> CardGameManager
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feedback Feedback from JK refactor
Projects
None yet
Development

No branches or pull requests

1 participant