-
Notifications
You must be signed in to change notification settings - Fork 2
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
[Feature] 화이트보드에 그림 추가 기능 구현(1차) #77
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이번 PR도 수고하셨습니다 ~~ 어푸루부 ~
public private(set) var origin: CGPoint? | ||
private(set) var minPoint: CGPoint? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이 minPoint
는 현재 reset 함수에서만 nil로 바꿔주고 있는데 어떤 용도로 사용하는 건가요 ??
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이런,, 중간에 로직이 바뀌어서 이제 필요 없는 프로퍼티입니다. 삭제하도록 하겠습니다
|
||
final class DrawObjectUseCaseTests: XCTestCase { | ||
var useCase: DrawObjectUseCaseInterface! | ||
var mockRepository: WhiteboardObjectRepositoryInterface! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
요기선 Lint 안걸리나요 ?? 강제 옵셔널 ???
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
아마 let이 아니라서 Lint에는 안걸릴 것 같습니다..!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
저도 딴과 동일하게 이해했습니다!
func action(input: Input) { | ||
enum Input { | ||
//TODO: - tool을 바꾸는(선택하는) case 추가 | ||
case startDrawing(startAt: CGPoint) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이 함수는 왜 func startDrawing(at point: CGPoint)
이런식으로 Argument Label 안붙이셨는지 순수 ! 궁금합니다 !!
enum에서는 사용하지 못하나요 ??
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
넵 enum에는 붙이지 못하더라구요 ㅜㅜ
다만 해당 네이밍의 경우 통일성 있게 startAt -> at으로 수정하면 좋을 것 같습니다.
|
||
let renderer = UIGraphicsImageRenderer(size: drawingObject.size) | ||
let image = renderer.image { context in | ||
context.cgContext.setLineWidth(5) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lineWidth 5하면 어느 정도의 굵기인지 궁금합니다 ~~
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이전 프로젝트에서 동일한 굵기로 진행했는데요, 아래 영상 정도의 굵기입니다.
video-converter.com.2.mp4
현재 MVP에는 선의 색상, 굵기에 관한 내용이 없어서 임의 처리했습니다.
하지만 좋은 설계는 아닌거 같아서 보다 유연하게 동작할 수 있게 색상과 굵기를 파라미터로 받아 처리하도록 수정하겠습니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ViewModel구현 까지 깔끔하네요!!
잘 배워 갑니다ㅎㅎ
Approve!합니다!만 class로 구현한 usecase부분 코멘트 답은 듣고 싶습니다 :)
@@ -4,13 +4,13 @@ | |||
// | |||
// Created by 이동현 on 11/13/24. | |||
// | |||
import Foundation |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Foundation을 import 해주신 이유가 있을까요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
파일 생성 시 자동 생성되는 Foundation을 지우지 않았습니다. 수정하도록 하겠습니다!
/// 그리기를 시작하는 메서드 | ||
/// - Parameter point: 시작 지점 CGPoint | ||
func startDrawing(at point: CGPoint) | ||
|
||
/// 그림 그리는 중에 새로운 점을 추가하는 메서드 | ||
/// - Parameter point: 추가할 점의 CGPoint | ||
func addPoint(point: CGPoint) | ||
|
||
/// 그림 그리기를 종료하는 메서드 | ||
/// - Returns: 완성된 그림 객체 (옵셔널) | ||
func finishDrawing() -> DrawingObject? | ||
|
||
/// 그림을 전송하는 메서드 | ||
/// - Parameter drawingObject: 전송할 그림 오브젝트 | ||
func send(drawingObject: DrawingObject) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
뷰모델에서 이런 로직을 담당 할 것이라고 착각하고 있었는데, PR 보면서 유즈케이스를 다시 이해하게 됐습니다..!!
좀 더 공부 해봐야할 것 같네요..!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
딴의 코멘트를 보고 저도 다시 한 번 생각을 해보았는데요, 전송 같은 경우는 다른 유즈케이스로 빼도 좋을 것 같습니다.
그림 뿐 아니라 여러개의 화이트 오브젝트 "화이트 보드 오브젝트를 전송한다" 라는 유즈케이스를 사용할 수 있을 것 같습니다!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
엇 ! 여기서 잠시 더 궁금한게 생겼는데요
왜 UseCaseInterface의 접근제어자가 public인가요 ? 다른 모듈에서 사용하나요 ?
let (xPoints, yPoints) = points.reduce(into: ([CGFloat](), [CGFloat]())) { | ||
$0.0.append($1.x) | ||
$0.1.append($1.y) | ||
} | ||
|
||
guard | ||
let origin, | ||
let minX = xPoints.min(), | ||
let maxX = xPoints.max(), | ||
let minY = yPoints.min(), | ||
let maxY = yPoints.max() | ||
else { return nil } | ||
|
||
let size = CGSize(width: maxX - minX, height: maxY - minY) | ||
let adjustedPoints = points.map { | ||
CGPoint(x: $0.x - minX, y: $0.y - minY) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
포인트를 0,0 기준으로 변경 시켜주는 코드 이군요..
알고리즘 구욷
|
||
import Foundation | ||
|
||
public final class DrawObjectUseCase: DrawObjectUseCaseInterface { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
struct로 해도 될 것 같은데, class로 해주신 이유가 따로 있을까요..??
class로 해주었을 때, 접근제어자로 잘 관리는 해주고 있지만 레퍼런스로 다른 곳에서 접근이 가능 할 위험이 있을 것 같아요..!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
어제 딴과 class와 struct의 선택 기준에 대해 이야기를 나누고, apple의 아티클을 찾아봤습니다.
https://developer.apple.com/documentation/swift/choosing-between-structures-and-classes
위 아티클에서는 data의 identity를 control할 일이 있으면 class로 구현하라고 나와있습니다.
프로퍼티로 repository를 가지고 있어 class로 설정하였습니다.
추후 구현에서 repository가 근거리 통신을 지원해주는 객체를 class로 가지고 있을것이라 예상됩니다. (세션을 하나 생성해서 이를 공유해야하기 때문, 즉 identity를 control 해야하는 상황)
이렇게 되면 여러 레포지토리가 하나의 통신 객체를 공유할 것이므로 repository도 class로 정의할 것입니다.
마지막으로, 현재 설계에서는 각 usecase가 object를 전송하는 같은 repository를 공유할 것입니다. 따라서 repository를 class로 정의한 이유와 같은 이유로 UseCase도 class로 정의했습니다.
다만 위에 코멘트에서 말씀드렸듯, 전송 기능이 빠지게 되면서 repository를 가지고 있지 않게 된다면 struct를 사용하도록 수정할 것 같습니다!
} | ||
} | ||
|
||
final class MockWhiteboardObjectRepository: WhiteboardObjectRepositoryInterface { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
확실히 추상화로 해놓으니, 목 구현체를 만들어 테스트하기가 용이해 보이네요..!!
// | ||
// DrawingRenderer.swift | ||
// Presentation |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Util과 Common 그룹에 관해서 내일 얘기해보면 좋을 것 같습니다 :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
좋습니다! 컨벤션을 정하면 좋을 것 같네요!!!
struct DrawingRenderer: DrawingRendererInterface { | ||
func render(drawingObject: DrawingObject) -> UIImage? { | ||
guard let startPoint = drawingObject.points.first else { return nil } | ||
|
||
let renderer = UIGraphicsImageRenderer(size: drawingObject.size) | ||
let image = renderer.image { context in | ||
context.cgContext.setLineWidth(5) | ||
context.cgContext.setStrokeColor(UIColor.black.cgColor) | ||
context.cgContext.move(to: startPoint) | ||
|
||
for point in drawingObject.points.dropFirst() { | ||
context.cgContext.addLine(to: point) | ||
} | ||
|
||
context.cgContext.strokePath() | ||
} | ||
return image | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
그림을 그릴 때 Point를 얼마나 저장할 진 모르겠지만,
UIGraphicsImageRenderer에 point를 이용해서 그림을 그려 줄 수 있군요..?
Point 저장을 어떻게 처리 해 주실지 기대도 됩니다!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
엔티티가 이미지 자체를 바이너리로 가지고 있게 할까도 생각했는데, point 배열로 그때 그때 이미지를 만들어주면 좋을 것 같다 생각했습니다!
d877acf
to
7819558
Compare
🌁 Background
👩💻 Contents
✅ Testing
📝 Review Note
📣 Related Issue