[dale] - Step5 리턴타입 class -> protocol 변경 #92
Conversation
| let shape = notification.userInfo?[Plane.NotificationKeyValue.shape] as? Shape | ||
| let selectedView = viewFinder[shape] as? ShapeViewAble | ||
| self.shapeViewBoard.setSelectedView(of: selectedView) | ||
| self.shapePropertyChangeBoard.setPropertyBoard(with: shape) |
There was a problem hiding this comment.
빈영역 터치시 테두리 선택을 취소하는 과정에서 의도적으로 nil값(선택된사각형이 Plane에 없을때 nil return) 을 사용하게 되었는데, 이런경우에 nil 값을 사용해도 되는지 궁금합니다!
There was a problem hiding this comment.
그럼요 nil을 의도적으로 사용해도 됩니다. 다만 그게 너무 억지로 쓴 느낌이나 흐름상 어색하지 않으면 됩니다.
여기서는 shape가 nil이 되는 걸까요?
| import Foundation | ||
|
|
||
| class PhotoRectangle: Shape{ | ||
| var backgroundImage: Data |
There was a problem hiding this comment.
항상 속성은 private var를 우선적으로 고려하세요
|
|
||
| extension PhotoRectangle : CustomStringConvertible{ | ||
| var description: String { | ||
| return "(\(self.id)), \(position), \(self.size), \(self.backgroundImage) , \(self.alpha)" |
There was a problem hiding this comment.
Data 타입은 이렇게 description을 하는게 큰 의미는 없습니다. 존재하는지 혹은 크기가 바뀌는지 정도만 봐도 다행이죠
| func addShape(shape: Shape) { | ||
| self.shapes.append(shape) | ||
| NotificationCenter.default.post(name: Plane.NotificationName.addShape, object: self, userInfo: [Plane.NotificationKeyValue.shape:shape]) |
There was a problem hiding this comment.
여기 Plane에서는 addShape()할 때 생성의 책임이 여기 없네요
Plane이 모델을 생성하는 역할이 없는데, 외부에서 생성하는 게 적절할까요
아니면 여기서 생성하고 추가까지 하는게 적절할까요? 어떻게 생각하세요?
There was a problem hiding this comment.
Plane에서 생성하는게 맞는것 같습니다. 처음 설계시 사각형을 만드는 팩토리메소드가 인스턴스메소드라 팩토리에 대한 인스턴스는 VC가 가지고 있어야 한다고 생각을 했는데, 현재 팩토리메소드가 타입메소드 형태로 바뀌어 Plane에서 생성할 수 있습니다.
| if (shape.position.x...(shape.position.x + shape.size.width)).contains(position.x) && (shape.position.y...(shape.position.y + shape.size.height)).contains(position.y) { | ||
| self.selectedShape = shape |
There was a problem hiding this comment.
이런 부분도 shape의 속성을 가져와서 비교하는 게 아니라 shape.contains(x,y) 처럼 일을 시키면 좋을 것 같습니다.
카드 속성을 가져와서 비교하지 말고 카드 자체를 비교했던 것처럼요
| class Shape{ | ||
| private(set) var id : Id = Id() | ||
| private(set) var size : Size | ||
| private(set) var position : Position | ||
| private(set) var alpha : Alpha | ||
|
|
||
| init(size: Size, position : Position, alpha : Alpha) { | ||
| self.size = size | ||
| self.position = position | ||
| self.alpha = alpha | ||
| } | ||
|
|
||
| func setAlpha(to alpha: Alpha) { | ||
| self.alpha = alpha | ||
| } | ||
| } | ||
|
|
||
| extension Shape: Equatable, Hashable { |
There was a problem hiding this comment.
어.. Shape가 프로토콜인 줄 알았는데 상위 클래스였네요. 상위 클래스가 있어도 되는데 프로토콜로 추상화할 부분도 시도해보세요.
|
|
||
| extension Shape: Equatable, Hashable { | ||
| static func == (lhs: Shape, rhs: Shape) -> Bool { | ||
| return ObjectIdentifier(lhs) == ObjectIdentifier(rhs) |
There was a problem hiding this comment.
Shape 객체가 같다는 어떤 의미가 있을까요?
a. id만 같으면 같은 객체인가? 완전히 동일한 객체를 의미하는 것인가?
b. id는 다르지만 모든 속성이 동일하고 같은 위치에 있으면 같다고 할 수 있는가?
a와 b 경우에 대한 것을 구분할 수 있으면 좋습니다.
https://lucas.codesquad.kr/masters-2022/course/모바일-iOS-클래스/포커게임-앱/카드덱-구현하고-테스트하기
에 참고자료로 올라온 내용 중에 Identity와 Equality를 다시 확인해보세요.
ObjectIdentifier가 같다는 것은 어떤 것을 의미할까요?
그래서 질문은 == 연산으로 무엇을 비교하려고 하는 것인가, 그 의도가 중요한 것 같습니다. class 경우는 === 연산도 있는데 어떤 역할을 하려는 것인가 궁금합니다.
There was a problem hiding this comment.
처음 설계 당시에는 b와 같이 모든 속성이 동일하면 같다 라고 판단을 하였는데, 아주 적은 확률이라도 겹칠경우가 있어 두 도형의 참조값을 비교하는게 확실한 방법같아서 === 와 Objectidentifier사이에서 고민하게 되었습니다.
=== 연산의 경우 lhs rhs 로 옵셔널 값을 받아, nil 인경우도 비교를 하지만 Shape의 경우 nil을 가질수 없다고 판단하여 Objectidentifier를 사용하게 되었습니다.
| static func makeRandomShape(in screenSize : (Double,Double)) -> Shape? { | ||
| let randomPosition = RandomGenerator.generatePosition(screenSize: screenSize) | ||
| let randomColor = RandomGenerator.generateColor() | ||
| let randomAlpha = RandomGenerator.generateAlpha() | ||
|
|
||
| return Rectangle(size: Size(width: 150, height: 120), position: randomPosition, color: randomColor, alpha: randomAlpha) | ||
| } | ||
|
|
||
| static func makePhotoRectangle(in screenSize : (Double, Double), imageData: Data) -> Rectangle? { | ||
| static func makePhotoShape(in screenSize : (Double, Double), imageData: Data) -> Shape? { |
There was a problem hiding this comment.
팩토리에서 생성하는 타입을 Shape 말고 프로토콜로도 추상화해보면 좋겠습니다.
아마도 서로 다른 역할이 있을 겁니다
| func borderVisible(_ enable: Bool) | ||
| } | ||
|
|
||
| protocol ShapeViewAble: AlphaMutable, BorderMutable { |
There was a problem hiding this comment.
이런 경우는 이렇게 합치기 보다는 ShapeViewAble 선언하는 부분에 AlphaMutable & BorderMutable 이런 식으로 조합하는 것도 좋습니다. 그래야 필요한 프로토콜만 선택해서 합칠 수 있습니다
There was a problem hiding this comment.
새로운걸 알게 되었습니다.. 적용해보도록 하겠습니다!
| static func makeView(of shape : Shape) -> ShapeViewAble? { | ||
| let size = shape.size | ||
| let position = shape.position | ||
| let alpha = shape.alpha | ||
| let shapeFrame = CGRect(x: position.x , y: position.y, width: size.width, height: size.height) | ||
| switch shape { | ||
| case let rectangle as Rectangle: | ||
| let color = rectangle.backgroundColor | ||
| return RectangleView(frame: shapeFrame, color: color, alpha: alpha) | ||
| case let photoRectangle as PhotoRectangle: | ||
| let imageData = photoRectangle.backgroundImage | ||
| return PhotoRectangleView(from: shapeFrame, imageData: imageData, alpha: alpha) | ||
| default: |
There was a problem hiding this comment.
생성자에서 꼭 하나의 함수로 다 만들려고 하지 않아도 됩니다.
그러다보면 타입을 비교하게 되고, 비교문이 들어가게 되고 복잡해집니다
팩토리는 생성하는 역할이라 구체 타입을 어느 정도 사용할 수 밖에 없으니 나눠서 처리하고 나머지들만 프로토콜로 접근하도록 두는 것도 좋습니다. 참고하세요
| guard let shapeView = shapeView as? UIView else {return} | ||
| self.shapeViewBoard.addSubview(shapeView) |
There was a problem hiding this comment.
ShapeViewFactory.makeView() 리턴 타입에서는 UIView 와 프로토콜 조합으로 사용하는 것도 괜찮을 것 같네요
작업목록
추가설명
5단계 진행하기전 추상화 과정에서 전체적으로 파일이 많이 변경되어 PR을 나누어 보내게 되었습니다.