[Alex] Step4 다형성을 적용한 리팩토링 #105

SongTaehwan · 2022-03-18T06:54:02Z

작업 내용

ColoredRectangle, ImageRectangle 의 상위 타입인 Shape 추상 클래스 구현
프로토콜을 활용해 딕셔너리 키 타입 추상화
ShapeViewFactory 구현
ViewController 구체 타입 참조를 프로토콜로 대체

학습 키워드

다형성

구체 타입을 프로토콜로 다형성 및 추상화하라는 이전 피드백을 바탕으로 리펙토링을 했습니다. 변경사항이 많아 피드백을 부탁드리고 싶습니다 :)

…o improve polymorphism

…e polymorphism Also apply changes from model to ViewController

…arily

…er to keep encapsulation

…d code

SongTaehwan · 2022-03-18T07:04:50Z

DrawingApp/DrawingApp/Model/Shape.swift

+class Shape: Shapable, Hashable {
+    let id: String
+    let size: Size
+    let origin: Point
+
+    init(id: String, origin: Point, size: Size) {
+        self.id = id
+        self.origin = origin
+        self.size = size
+    }
+}


프로토콜을 딕셔너리의 키 타입으로 사용하기 위해 여러 시도를하였습니다.
결국 Protocol 은 Hashable 을 채택할 수 없어 부득이하게 모델의 공통점만 모아 Hashable 을 채택하는 추상 클래스를 만들었습니다.
추상 클래스로써의 목적을 가지고 있지만 생성자가 사용 가능하다는 점 때문에 오히려 구체타입에 가깝다는 생각이 듭니다.
Hashable 을 채택하기 위해 클래스를 직접 구현했지만 Hashable 과 같이 제너릭을 포함하는 프로토콜을 타입으로 사용하려면 직접 구현해야하는 건 피할 수 없는 걸까요?

Shape는 구체 타입이죠. 프로토콜들을 채택하면 그 타입으로 대체할 수 있을 뿐이죠
이렇게 Hashable 채택해서 구현하면 Shape로 생성은 해야 하지만, 프로토콜 타입으로 보는 인스턴스들은 사전에 넣을 수 있을 겁니다.

SongTaehwan · 2022-03-18T07:10:56Z

DrawingApp/DrawingApp/Controller/ViewController.swift

+        if let backgroundAdaptableShape = shape as? BackgroundAdaptable {
+            let hexString = Color.toHexString(backgroundAdaptableShape.backgroundColor)
+            self.controlPanelView.setColorButtonTitle(title: hexString)
+        }
+
+        if let AlphaAdaptableShape = shape as? AlphaAdaptable {
+            self.controlPanelView.setAlphaSliderValue(value: AlphaAdaptableShape.alpha)
+        }

-        self.controlPanelView.setColorButtonControllable(enable: rectangle.isType(of: Rectangle.self))
+        let isConformed = (shape as? ImageAdaptable) == nil


프로토콜로 대체하면서 이전에 없던 if 문과 타입케스팅 생겼습니다. 차라리 별도의 함수로 분리하는 것이 더 좋을까요?

맞아요. 고민스러운 부분이네요. 추상화를 했기 때문에 적절한 것을 찾지 못해서 어쩔 수 없죠.
그만큼 역할을 잘 나눈거죠. 이벤트 하나에서 역할에 따라 다르게 출력해야 하니까 생기는 현상이니까 괜찮습니다.
그래서 유즈케이스 같은 것을 두고 planeDidSelectItem가 발생하면, 거기서 분기해서 각기 다른 흐름을 만들기도 합니다

SongTaehwan · 2022-03-18T07:15:37Z

DrawingApp/DrawingApp/Model/ColoredRectangle.swift

-    let id: String
-    let size: Size
-    let origin: Point
-
-    var diagonalPoint: Point {
-        let maxX = self.origin.x + self.size.width
-        let maxY = self.origin.y + self.size.height
-        return Point(x: maxX, y: maxY)
-    }
-


Shape 클래스가 공통 데이터를 가지고 있고 ColoredRectangle 은 backgroundColor 와 alpha, ImageRectangle 은 image 와 alpha 데이터를 소유합니다.
alpha 를 다루지않는 도형이 추가될 것을 염두해 Shape 에 포함시키지 않았습니다.

SongTaehwan · 2022-03-18T07:22:12Z

DrawingApp/DrawingApp/Model/ImageRectangle.swift

+    private var imageURL: URL?
+
+    var imagePath: String? {
+        return self.imageURL?.path
+    }


캡슐화를 유지할 목적으로 getter 역시 메소드처럼 메모리가 동작한다고 해서 외부 객체가 직접 속성에 접근할 수 있게 하는 것보다 getter 를 두었습니다.
결과적으로 속성에 접근하는 것과 크게 다르지 않아 캡슐화가 제대로 이루어진건지 잘 모르겠습니다.

Plane과 다르게 내부에서 저장을 위해서 생성되는 이런 타입들은 어쩔 수 없이 값을 가져가야 하는 경우가 생깁니다.
이런 타입들에 대해서는 캡슐화를 걱정할 필요가 없습니다. 거의 대부분 만들어진 상태로 값이 바뀔 일이 거의 없죠
그럴 때는 그냥 private(set)을 해도 됩니다.

SongTaehwan · 2022-03-18T07:26:26Z

DrawingApp/DrawingApp/Utility/ShapeViewFactory.swift

+enum ShapeViewFactory {
+    static func makeView(ofClass type: ShapeViewable.Type, with data: Shapable) -> ShapeViewable? {
+        switch type {
+        case is ColoredRectangleView.Type:
+            return RectangleViewFactory.makeView(ofClass: ColoredRectangleView.self, with: data)
+        case is ImageRectangleView.Type:
+            return RectangleViewFactory.makeView(ofClass: ImageRectangleView.self, with: data)
+        default:
+            return nil
+        }
+    }
+}


RectangleViewFactory 는 구체 타입을 리턴하고 ShapeViewFactory 은 다형성을 위해 프로토콜로 추상화하였습니다.
다른 PR 에 달아주신 피드백을 보면 구체타입을 리턴하는 것을 지양하는 것하라는 말씀이신 것 같아 이렇게 용도를 구분하고 나누어도 될지 궁금합니다 :)

위에 비슷한 타입이 있던데 팩토리가 어떤게 진짜인가요

모델 팩토리와 동일한데 아쉬운 것은 새로운 타입이 늘어날 때마다 기존 타입을 생성하는 코드와 관련된 부분을 수정해야 한다는 점입니다.
특히 모델 타입과 매칭해야 하니 구체 타입을 알 수 밖에 없으니 억지로 합칠 필요는 없습니다. ShapeViewable로 리턴한다면 메소드는 구분해도 별 차이 없을 겁니다.

godrm

프로토콜로 분리하는 시도는 잘 처리하셨습니다.
다음 내용들을 확인해보고, 소소한 부분만 개선하면 될 것 같습니다

godrm · 2022-03-18T08:11:57Z

DrawingApp/DrawingApp/Controller/ViewController.swift

+        if let backgroundAdaptableShape = shape as? BackgroundAdaptable {
+            let hexString = Color.toHexString(backgroundAdaptableShape.backgroundColor)
+            self.controlPanelView.setColorButtonTitle(title: hexString)
+        }
+
+        if let AlphaAdaptableShape = shape as? AlphaAdaptable {
+            self.controlPanelView.setAlphaSliderValue(value: AlphaAdaptableShape.alpha)
+        }

-        self.controlPanelView.setColorButtonControllable(enable: rectangle.isType(of: Rectangle.self))
+        let isConformed = (shape as? ImageAdaptable) == nil


맞아요. 고민스러운 부분이네요. 추상화를 했기 때문에 적절한 것을 찾지 못해서 어쩔 수 없죠.
그만큼 역할을 잘 나눈거죠. 이벤트 하나에서 역할에 따라 다르게 출력해야 하니까 생기는 현상이니까 괜찮습니다.
그래서 유즈케이스 같은 것을 두고 planeDidSelectItem가 발생하면, 거기서 분기해서 각기 다른 흐름을 만들기도 합니다

godrm · 2022-03-18T08:18:17Z

DrawingApp/DrawingApp/Model/ImageRectangle.swift

+    private var imageURL: URL?
+
+    var imagePath: String? {
+        return self.imageURL?.path
+    }


Plane과 다르게 내부에서 저장을 위해서 생성되는 이런 타입들은 어쩔 수 없이 값을 가져가야 하는 경우가 생깁니다.
이런 타입들에 대해서는 캡슐화를 걱정할 필요가 없습니다. 거의 대부분 만들어진 상태로 값이 바뀔 일이 거의 없죠
그럴 때는 그냥 private(set)을 해도 됩니다.

godrm · 2022-03-18T08:19:04Z

DrawingApp/DrawingApp/Model/ImageRectangle.swift

+class ImageRectangle: Shape, ImageControllable, Notifiable {
+    private(set) var alpha: Alpha {
+        didSet {
+            self.notifyDidUpdated(key: .updated, data: self.alpha)


didSet을 사용하는 경우는 이 값이 struct냐 class냐에 따라 동작이 다릅니다.
alpha 자체가 바뀌는 경우와 alpha 속성이 바뀌는 경우를 구분해서 비교해보세요

godrm · 2022-03-18T08:20:58Z

DrawingApp/DrawingApp/Model/ImageRectangle.swift

+    func notifyDidCreated() {
        NotificationCenter.default.post(name: .ImageRectangleModelDidCreated, object: self)
    }
+
+    func notifyDidUpdated(key: NotificationKey, data: Any) {
+        NotificationCenter.default.post(name: .RectangleModelDidUpdated, object: self, userInfo: [key: data])


음.. 이런 하위 타입들은 class여도 immutable 형태나 struct - let 형태로 값 타입으로 사용하는 경우도 많습니다
모든 값을 이렇게 Notification post를 해야 하는 건 아닙니다. 캡슐화가 이루어지는 타입들에서는 필요할 수 있는데, 그렇지 않으면 그냥 값 자체를 매개변수로 전달하거나 리턴값으로 전달하는 경우에 사용하는 경우도 많으니까요

godrm · 2022-03-18T08:22:36Z

DrawingApp/DrawingApp/Model/Plane.swift

-protocol Notifiable: AnyObject {
-    func notifyDidCreated()
-}
-
 class Plane {
    private var items = [String: Shapable]()


String 인걸 보니 Id 값을 이용해서 키를 사용하는 것 같습니다
Shape를 사용한다면 (프로토콜 타입인지 모르겠지만) 타입 자체를 비교하도록 생각해보세요
그래야 Id를 가져오지 않고 타입을 비교하게 됩니다.
카드 게임에서 카드 속성을 가져오지 않고 카드 자체를 비교했던 것처럼요
아래 언급이 있는 것 같은데, Shape가 구체타입이군요. 이 자리를 뭘로 선언해야 할까요?

타입 자체의 비교를 고려한다면 딕셔너리보다는 배열로 바꾸는 것이 적합할 것 같습니다.
원하는 객체를 찾을 땐 동일성 비교로 찾을 수 있고 배열의 타입도 구체타입 대신 프로토콜([Shapable])을 사용할 수 있을 것 같습니다 :)

godrm · 2022-03-18T08:29:10Z

DrawingApp/DrawingApp/Model/Shape.swift

+class Shape: Shapable, Hashable {
+    let id: String
+    let size: Size
+    let origin: Point
+
+    init(id: String, origin: Point, size: Size) {
+        self.id = id
+        self.origin = origin
+        self.size = size
+    }
+}


Shape는 구체 타입이죠. 프로토콜들을 채택하면 그 타입으로 대체할 수 있을 뿐이죠
이렇게 Hashable 채택해서 구현하면 Shape로 생성은 해야 하지만, 프로토콜 타입으로 보는 인스턴스들은 사전에 넣을 수 있을 겁니다.

godrm · 2022-03-18T08:31:27Z

DrawingApp/DrawingApp/Utility/RectangleFactory.swift

+    static func makeShape(ofClass type: Shapable.Type) -> Shapable? {
+        switch type {
+        case is ColoredRectangle.Type:
+            return self.makeColoredRectangle()
+        case is ImageRectangle.Type:
+            return self.makeImageRectangle()
+        default:
+            return nil
+        }


이 부분이 아쉬운 것은 새로운 타입이 늘어날 때마다 기존 타입을 생성하는 코드와 관련된 부분을 수정해야 한다는 점입니다.
type.init()으로 다형성으로 동작하면 가장 좋습니다
그렇지 않으면 팩토리에서는 구체 타입을 알 수 밖에 없으니 억지로 합칠 필요는 없습니다. Shapable로 리턴한다면 메소드는 구분해도 된다고 생각하세요.

godrm · 2022-03-18T08:33:18Z

DrawingApp/DrawingApp/Utility/RectangleViewFactory.swift

-            guard let path = data.image?.path, let image = UIImage(contentsOfFile: path) else { return nil }
+            return self.makeImageRectangleView(with: data)
+        default:
+            return nil


여기도 모델 팩토리와 동일한데 아쉬운 것은 새로운 타입이 늘어날 때마다 기존 타입을 생성하는 코드와 관련된 부분을 수정해야 한다는 점입니다.
특히 모델 타입과 매칭해야 하니 구체 타입을 알 수 밖에 없으니 억지로 합칠 필요는 없습니다. ShapeViewable로 리턴한다면 메소드는 구분해도 별 차이 없을 겁니다.

godrm · 2022-03-18T08:34:37Z

DrawingApp/DrawingApp/Utility/ShapeFactory.swift

+enum ShapeFactory: ShapeBuildable {
+    static func makeShape(ofClass type: Shapable.Type) -> Shapable? {
+        switch type {
+        case is ColoredRectangle.Type:
+            return RectangleFactory.makeShape(ofClass: ColoredRectangle.self)
+        case is ImageRectangle.Type:
+            return RectangleFactory.makeShape(ofClass: ImageRectangle.self)
+        default:
+            return nil
+        }


어라. 생성자 ShapeFactory가 또 있네요? RectangleFactory랑 다른건 뭐죠?

godrm · 2022-03-18T08:35:14Z

DrawingApp/DrawingApp/Utility/ShapeViewFactory.swift

+enum ShapeViewFactory {
+    static func makeView(ofClass type: ShapeViewable.Type, with data: Shapable) -> ShapeViewable? {
+        switch type {
+        case is ColoredRectangleView.Type:
+            return RectangleViewFactory.makeView(ofClass: ColoredRectangleView.self, with: data)
+        case is ImageRectangleView.Type:
+            return RectangleViewFactory.makeView(ofClass: ImageRectangleView.self, with: data)
+        default:
+            return nil
+        }
+    }
+}


위에 비슷한 타입이 있던데 팩토리가 어떤게 진짜인가요

모델 팩토리와 동일한데 아쉬운 것은 새로운 타입이 늘어날 때마다 기존 타입을 생성하는 코드와 관련된 부분을 수정해야 한다는 점입니다.
특히 모델 타입과 매칭해야 하니 구체 타입을 알 수 밖에 없으니 억지로 합칠 필요는 없습니다. ShapeViewable로 리턴한다면 메소드는 구분해도 별 차이 없을 겁니다.

SongTaehwan added 13 commits March 16, 2022 20:05

fix: Fix test cases

52a4fb8

refactor: Refactor ColorFactory

a0acaa7

refactor: Define protocols for RectangleView and ImageRectangleView t…

13f4b28

…o improve polymorphism

build: Set test coverage visible on Report Instpector

7ef57a4

refactor: Define protocols for Rectangle and ImageRectangle to improv…

432834e

…e polymorphism Also apply changes from model to ViewController

refactor: set id property of Rectangle model as dictionary key tempor…

522582e

…arily

test: Add test cases for Utility and Model

191c437

refactor: Hide image data of ImageRectangle and expose imagePath gett…

f37334d

…er to keep encapsulation

refactor: Implement Shape class as abstract class and refactor relate…

69229fe

…d code

refactor: Abstarct variables of model in ViewController

9d61c70

refactor: Improve level of polymorphism with protocols

c7597e4

refactor: Refactor factories and model protocols

bc31471

feat: Implement ShapeViewFactory and RectangleViewFactory

535e81c

SongTaehwan commented Mar 18, 2022

View reviewed changes

godrm requested changes Mar 18, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Alex] Step4 다형성을 적용한 리팩토링 #105

[Alex] Step4 다형성을 적용한 리팩토링 #105

SongTaehwan commented Mar 18, 2022

SongTaehwan Mar 18, 2022 •

edited

Loading

godrm Mar 18, 2022

SongTaehwan Mar 18, 2022

godrm Mar 18, 2022

SongTaehwan Mar 18, 2022

SongTaehwan Mar 18, 2022

godrm Mar 18, 2022

SongTaehwan Mar 18, 2022

godrm Mar 18, 2022

godrm left a comment

godrm Mar 18, 2022

godrm Mar 18, 2022

godrm Mar 18, 2022

godrm Mar 18, 2022

godrm Mar 18, 2022

SongTaehwan Mar 18, 2022

godrm Mar 18, 2022

godrm Mar 18, 2022

godrm Mar 18, 2022

godrm Mar 18, 2022

godrm Mar 18, 2022

[Alex] Step4 다형성을 적용한 리팩토링 #105

Are you sure you want to change the base?

[Alex] Step4 다형성을 적용한 리팩토링 #105

Conversation

SongTaehwan commented Mar 18, 2022

작업 내용

학습 키워드

SongTaehwan Mar 18, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

godrm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SongTaehwan Mar 18, 2022 •

edited

Loading