Refactor [#87] 홈 피트 리팩토링 완료 #89

binisnil · 2025-01-15T11:38:37Z

👻 PULL REQUEST

🛠️ PR POINT

홈 피드 리팩토링 완료했습니다.
TableView -> DiffableDataSource CollectionView로 변경했습니다.
기존에 흩어져있던 네트워크 통신 코드를 HomeAPI에 모아두었습니다.
기존에 View로 나타나던 PopupView를 PopupViewController로 분리하였고, PopupViewModel을 새롭게 추가하였습니다.

💡 참고사항

기존 HomeView, HomeViewController를 지우지 않고, 'MigratedHome' 이라는 네이밍으로 파일을 만들어 작업했습니다. 승인 되면, 기존 파일 지우고 이름 변경하도록 하겠습니다!

📸 스크린샷

기능	스크린샷
페이징
좋아요, 프로필탭, 댓글 탭, 셀 탭
삭제하기
밴하기
신고하기

📟 관련 이슈

Resolved: [Refactor] 홈 코드 재구성 #87

JinUng41

많은 부분 수정을 거듭하셨을텐데, 정말 고생많으셨습니다.

코멘트는 각각 읽어보시면 좋을 것 같구요.
추가적으로 더 남겨보자면,
저 같은 경우 변수/상수 선언에 있어, 접근제어자에 따라 그 후 var/let에 따라 변수를 선언하는 편이거든요.

final class Foo {
    // internal + variable
    var name: String?
    var number: Int?
    
    // internal + let
    let id: Int
    let date: Date
    
    // private + variable
    private var phoneNumber: Int?
    private var homeNumber: Int?
    
    // private + let
    private let address: String
    private let text: String
}

이러한 흐름으로 변수/상수 선언의 순서를 지키고 있습니다.
이처럼 객체 내에서 선언되는 변수/상수의 순서를 잘 정리해 보시면 어떨까요?

Combine은 함수형 프로그래밍을 따르며, 코드가 간결한 장점이 있는데요.
클로저 내에서 self에 접근하기 위해, 약한 참조 [weak self]를 하게 되고 guard let self else { return }을 작성하게 됩니다.
하지만 중간중간 클로저에서 바로 self를 참조하게 되더라고요.
비동기적으로 동작하는 클로저에서 self를 직접 참조하게 되면 순환 참조의 여지가 발생할 수 있는 것을 잘 들어보셨을거라 생각합니다.
제가 이러한 불편함 때문에 Combine+의 일부로써, 지난번에 withUnretained, just, fail을 구현해보았습니다.
이 기회에 한번 사용해 보시면 좋을 것 같아요.

JinUng41 · 2025-01-15T14:01:33Z

Wable-iOS/Global/Extention/UIView+.swift

+
+// MARK: - UIGestureRecognizer Combine Publisher
+
+extension UIView {
+    func gesturePublisher<T: UIGestureRecognizer>(_ gestureRecognizer: T) -> AnyPublisher<T, Never> {
+        GesturePublisher(view: self, gestureRecognizer: gestureRecognizer).eraseToAnyPublisher()
+    }
+}
+
+// MARK: - GesturePublisher 정의
+
+struct GesturePublisher<T: UIGestureRecognizer>: Publisher {
+    typealias Output = T
+    typealias Failure = Never
+
+    private let view: UIView
+    private let gestureRecognizer: T
+
+    init(view: UIView, gestureRecognizer: T) {
+        self.view = view
+        self.gestureRecognizer = gestureRecognizer
+    }
+
+    func receive<S>(subscriber: S) where S : Subscriber, Failure == S.Failure, Output == S.Input {
+        let subscription = GestureSubscription(subscriber: subscriber, view: view, gestureRecognizer: gestureRecognizer)
+        subscriber.receive(subscription: subscription)
+    }
+}
+
+// MARK: - GestureSubscription 정의
+
+final class GestureSubscription<S: Subscriber, T: UIGestureRecognizer>: Subscription where S.Input == T {
+    private var subscriber: S?
+    private let gestureRecognizer: T
+    private weak var view: UIView?
+
+    init(subscriber: S, view: UIView, gestureRecognizer: T) {
+        self.subscriber = subscriber
+        self.gestureRecognizer = gestureRecognizer
+        self.view = view
+
+        self.view?.isUserInteractionEnabled = true
+        self.view?.addGestureRecognizer(gestureRecognizer)
+        self.gestureRecognizer.addTarget(self, action: #selector(handleGesture))
+    }
+
+    func request(_ demand: Subscribers.Demand) {
+    }
+
+    func cancel() {
+        subscriber = nil
+        view?.removeGestureRecognizer(gestureRecognizer)
+    }
+
+    @objc private func handleGesture() {
+        _ = subscriber?.receive(gestureRecognizer)
+    }
+}


오호,, 상당히 흥미로운 내용이네요..
UIView 내에서 사용되는 내용이라면, UIView 내부로 감쌀 것 같아요.
extension UIView { }로 말이죠.

그리고 이걸 왜 구현했는지, 무엇을 위해선지가 가장 중요할 것 같아요.
혹시 이유를 알 수 있을까요?

사진을 탭할 때의 탭 제스처가 필요했는데,, 찾아보니 이렇게 하라는 내용을 봐서요..! 혹시 다른 방법이 있다면 알려주세요!!

CombineCocoa에 Gesture에 따른 퍼블리셔를 사용할 수 있도록 구현되어 있는 것으로 알고 있어요.
그래서 위 구현사항은 필요가 없을수도 있겠다는 생각이 들었는데요.

이미지뷰라면, userInteraction과 관련된 속성을 true로 변경하고, @objc메서드를 선언하여 이미지뷰가 터치되었을 때 어떠한 동작을 할 지 정의할 수 있는 것으로 알고 있습니다.
또한 이미지뷰의 터치를 뷰모델에 전달해야 한다면, @objc 메서드에서 서브젝트의 send를 호출하여 이미지뷰가 터치되었을 때 신호를 전달할 수 있겠네요.

JinUng41 · 2025-01-15T14:03:21Z

Wable-iOS/Presentation/Home/ViewController/HomePopupViewController.swift

+import Combine
+import CombineCocoa
+import UIKit
+
+import SnapKit


CombineCocoa는 외부 라이브러리라 SnapKit처럼 따로 작성하는 것이 좋아 보이네요.

JinUng41 · 2025-01-15T14:03:57Z

Wable-iOS/Presentation/Home/ViewController/HomePopupViewController.swift

+    init(viewModel: PopupViewModel, popupType: PopupViewType) {
+        self.viewModel = viewModel
+        self.rootView = WablePopupView(popupType: popupType)
+        super.init(nibName: nil, bundle: nil)
+    }


popupType에 따른 생성자 처리 좋네요.

JinUng41 · 2025-01-15T14:06:30Z

Wable-iOS/Presentation/Home/ViewController/HomePopupViewController.swift

+
+    override func viewDidLoad() {
+        super.viewDidLoad()
+        view = rootView


loadView에서 하는 것이 좋아보이네요.

viewDidLoad는 기본적으로 뷰가 메모리에 올려지고 난 뒤에 불려지는 메서드이기 때문에, 그 과정에서 뷰를 바꾸게 된다면 메모리 낭비로 이어질 것 같다는 생각이 들었습니다.

JinUng41 · 2025-01-15T14:07:04Z

Wable-iOS/Presentation/Home/ViewController/HomePopupViewController.swift

+    override func viewWillAppear(_ animated: Bool) {
+        super.viewWillAppear(animated)
+    }
+


만약 추가적인 작업이 필요하지 않다면, 지워도 괜찮을 것 같네요~

JinUng41 · 2025-01-15T14:17:48Z

Wable-iOS/Presentation/UIComponents/WablePopupView.swift

@@ -20,6 +27,8 @@ final class WablePopupView: UIView {
    // MARK: - Properties

    weak var delegate: WablePopupDelegate?
+    var cancelBag: CancelBag?


cancelBag이 옵셔널인 이유가 있을까요?
팝업 형태에 따라 필요없는 경우가 존재해서 일까요?

홈 외에 다른 곳에서 사용되는 팝업 뷰에는 cancelBag이 필요하지 않아서 옵셔널로 두었는데, 수정할까요?

아 홈 외에 다른 곳에서 팝업 뷰가 불려지곤 하군요.
그렇다면, private var가 맞는 것 같아요.

JinUng41 · 2025-01-15T14:19:38Z

Wable-iOS/Presentation/Home/Views/MigratedHomeView.swift

+        self.addSubviews(homeTabView,
+                         collectionView,
+                         writeFeedButton,
+                         loadingView)


addSubviews( homeTabView, collectionView, writeFeedButton, loadingView )

가 좀 더 나을 듯한데, 어떻게 생각하세요?

JinUng41 · 2025-01-15T14:20:08Z

Wable-iOS/Presentation/Home/Views/MigratedHomeView.swift

+// MARK: - Extensions
+
+extension MigratedHomeView {


private extension으로 선언하고, 하위 코드에서 private키워드를 지워도 좋을 것 같네요.

JinUng41 · 2025-01-15T14:21:19Z

Wable-iOS/Presentation/Home/Views/Cells/MigratedHomeFeedCell.swift

+        if data.isGhost {
+            print("\(data.memberNickname)\n\(data.isGhost)")
+            grayView.alpha = 0.85
+        } else {
+            grayView.alpha = CGFloat(Double(-memberGhost) / 100)
+        }


print문은 지우고, 삼항 연산자를 이용해 볼 수 있을 것 같아요.

JinUng41 · 2025-01-15T14:22:39Z

Wable-iOS/Presentation/Home/Views/Cells/MigratedHomeFeedCell.swift

+    // MARK: - Components
+
+    let grayView: UIView = {
+        let view = UIView()
+        view.backgroundColor = .wableWhite
+        view.alpha = 0
+        view.isUserInteractionEnabled = false
+        return view
+    }()
+
+    var infoView = FeedInfoView()
+    var feedContentView = FeedContentView()
+    var bottomView = FeedBottomView()
+    var divideLine = UIView().makeDivisionLine()
+
+    var profileImageView: UIImageView = {
+        let imageView = UIImageView()
+        imageView.image = ImageLiterals.Image.imgProfileSmall
+        imageView.isUserInteractionEnabled = true
+        return imageView
+    }()
+
+    private var menuButton: UIButton = {
+        let button = UIButton()
+        button.setImage(ImageLiterals.Icon.icMeatball, for: .normal)
+        return button
+    }()
+
+    var seperateLineView: UIView = {
+       let view = UIView()
+        view.backgroundColor = .gray200
+        view.isHidden = true
+        return view
+    }()


UIComponent가 추후 변경의 여지가 없다면, let으로 선언되는 것이 의도가 명확해 보일 것 같아요.

JinUng41

고생하셨습니다.

코드가 훨씬 간결해진 부분들이 보기 좋네요.
이번 코멘트와 지난 코멘트들 확인 부탁드려요~

JinUng41 · 2025-01-16T13:52:54Z

Wable-iOS/Presentation/Home/Supports/PopupType.swift

+enum PopupViewType {
+    case delete
+    case report
+    case ghost
+    case ban
+
+    var title: String {
+        switch self {
+        case .delete: return StringLiterals.Home.deletePopupTitle
+        case .report: return StringLiterals.Home.reportPopupTitle
+        case .ghost: return StringLiterals.Home.ghostPopupTitle
+        case .ban: return "밴하기 ㅋㅋ"
+        }
+    }
+
+    var content: String {
+        switch self {
+        case .delete: return StringLiterals.Home.deletePopupContent
+        case .report: return StringLiterals.Home.reportPopupContent
+        case .ghost: return ""
+        case .ban: return "너이놈밴머거랏!!!"
+        }
+    }
+
+    var leftButtonTitle: String {
+        switch self {
+        case .delete: return StringLiterals.Home.deletePopupUndo
+        case .report: return StringLiterals.Home.reportPopupUndo
+        case .ghost: return StringLiterals.Home.ghostPopupUndo
+        case .ban: return "함봐줌"
+        }
+    }
+
+    var rightButtonTitle: String {
+        switch self {
+        case .delete: return StringLiterals.Home.deletePopupDo
+        case .report: return StringLiterals.Home.reportPopupDo
+        case .ghost: return StringLiterals.Home.ghostPopupDo
+        case .ban: return "밴ㄱㄱ"
+        }
+    }
+}


JinUng41 · 2025-01-16T13:54:20Z

Wable-iOS/Presentation/Home/ViewController/HomePopupViewController.swift

+    override func loadView() {
+        super.loadView()
+        view = rootView
+    }


커스텀뷰로 뷰컨트롤러의 루트뷰를 설정하고자 할 때에는 super.loadView()를 호출하지 않아도 되는 것으로 이해하고 있습니다.
super.loadView()는 스토리보드로 뷰를 동작시키고자 할 때 호출하는 것으로 알고 있어요.

JinUng41 · 2025-01-16T13:56:52Z

Wable-iOS/Presentation/UIComponents/WablePopupView.swift

-    func setInitialPopup(type: PopupViewType) {
-        switch type {
-        case .delete:
-            popupTitleLabel.setTextWithLineHeight(
-                text: StringLiterals.Home.deletePopupTitle,
-                lineHeight: 28.8.adjusted,
-                alignment: .center
-            )
-            popupContentLabel.text = StringLiterals.Home.deletePopupContent
-            cancleButton.setTitle(StringLiterals.Home.deletePopupUndo, for: .normal)
-            confirmButton.setTitle(StringLiterals.Home.deletePopupDo, for: .normal)
-
-        case .report:
-            popupTitleLabel.setTextWithLineHeight(
-                text: StringLiterals.Home.reportPopupTitle,
-                lineHeight: 28.8.adjusted,
-                alignment: .center
-            )
-            popupContentLabel.text = StringLiterals.Home.reportPopupContent
-            cancleButton.setTitle(StringLiterals.Home.reportPopupUndo, for: .normal)
-            confirmButton.setTitle(StringLiterals.Home.reportPopupDo, for: .normal)
-
-        case .ghost:
-            popupTitleLabel.setTextWithLineHeight(
-                text: StringLiterals.Home.ghostPopupTitle,
-                lineHeight: 28.8.adjusted,
-                alignment: .center
-            )
-            popupContentLabel.text = ""
-            cancleButton.setTitle(StringLiterals.Home.ghostPopupUndo, for: .normal)
-            confirmButton.setTitle(StringLiterals.Home.ghostPopupDo, for: .normal)
-        case .ban:
-            popupTitleLabel.setTextWithLineHeight(
-                text: "밴하기 ㅋㅋ",
-                lineHeight: 28.8.adjusted,
-                alignment: .center
-            )
-            popupContentLabel.text = "너이놈밴머거랏!!!"
-            cancleButton.setTitle("함봐줌", for: .normal)
-            confirmButton.setTitle("밴ㄱㄱ", for: .normal)
-        }
+    private func configurePopup(type: PopupViewType) {
+        popupTitleLabel.setTextWithLineHeight(
+            text: type.title,
+            lineHeight: 28.8.adjusted,
+            alignment: .center
+        )
+        popupContentLabel.text = type.content
+        cancleButton.setTitle(type.leftButtonTitle, for: .normal)
+        confirmButton.setTitle(type.rightButtonTitle, for: .normal)
    }


훨씬 간결해지고 좋네요.
다만 private extension 코드 영역인 것 같아서 메서드의 private 키워드는 없어도 될 것 같아요.

JinUng41 · 2025-01-16T13:59:21Z

Wable-iOS/Presentation/UIComponents/WablePopupView.swift

 protocol WablePopupDelegate: AnyObject {
-    func cancleButtonTapped()
+    func cancelButtonTapped()
    func confirmButtonTapped()
    func singleButtonTapped()
 }


팝업의 가지 수가 다양하고, 모든 경우에서 모든 버튼을 사용하지 않는 것으로 이해하고 있습니다.
물론 현재의 경우, 홈 뷰컨트롤러에서 팝업을 띄우다 보니 위와 같은 형태가 된 것 같아요.

만약 팝업 뷰의 버튼에 대한 이벤트를 처리하고자, 델리게이트를 따를 때 모든 경우를 무조건 따라야 하는 경우가 발생할 것 같은데 어떻게 생각하실까요?
만약 다른 뷰에서 팝업을 취소 용도만 사용하고자 하는데, 델리게이트에서 모든 경우를 모두 따르라고 요구한다면, ISP 또는 SRP에 위배된다고 생각이 들어서요.

binisnil added 14 commits January 15, 2025 18:37

[Chore] #87 - 밴하기 토스트 추가

d92e60d

[Chore] #87 - TabBar 수정

0b48041

[Fix] #87 - Popup을 VC로 수정, PopupType 도입

b6dee4b

[Feat] #87 - PopUpViewModel 구현

79175e6

[Fix] #87 - DiffableDataSource로 변경함에 따라 링크 터치이벤트 처리방식 변경

95d6b81

[Feat] #87 - HomeView, ViewController 재구성

8b460d9

[Feat] #87 - HomeFeedDTO가 Hashable을 준수하도록 변경

d9149a0

[Fix] #87 - HomeAPI 구현

29a77b8

[Feat] #87 - HomeViewModel 새로 구현

112e919

[Feat] #87 - 밴하기 기능을 위한 바인딩 추가

28e83ef

[Chore] #87 - HomeAPI EndPoint추가

8eb9b6b

[Feat] #87 - UIGestureRecognizer Combine Publisher 추가

73f3d21

[Feat] #87 - 글 작성 완료 후 이벤트 설정을 위한 클로저 추가

f33c643

[Fix] #87 - 삭제 후 페이징 시 페이징 안되는 문제 해결

4169f18

binisnil added Refactor 🍪윤빈 labels Jan 15, 2025

binisnil requested a review from JinUng41 January 15, 2025 11:38

binisnil self-assigned this Jan 15, 2025

JinUng41 requested changes Jan 15, 2025

View reviewed changes

[Fix] #87 - 코드리뷰 반영

4fb3fcb

JinUng41 requested changes Jan 16, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor [#87] 홈 피트 리팩토링 완료 #89

Refactor [#87] 홈 피트 리팩토링 완료 #89

binisnil commented Jan 15, 2025

JinUng41 left a comment

JinUng41 Jan 15, 2025

binisnil Jan 15, 2025

JinUng41 Jan 16, 2025

JinUng41 Jan 15, 2025

JinUng41 Jan 15, 2025

JinUng41 Jan 15, 2025

JinUng41 Jan 15, 2025

JinUng41 Jan 15, 2025

binisnil Jan 15, 2025

JinUng41 Jan 16, 2025

JinUng41 Jan 15, 2025

JinUng41 Jan 15, 2025

JinUng41 Jan 15, 2025

JinUng41 Jan 15, 2025

JinUng41 left a comment

JinUng41 Jan 16, 2025

JinUng41 Jan 16, 2025

JinUng41 Jan 16, 2025

JinUng41 Jan 16, 2025

Refactor [#87] 홈 피트 리팩토링 완료 #89

Are you sure you want to change the base?

Refactor [#87] 홈 피트 리팩토링 완료 #89

Conversation

binisnil commented Jan 15, 2025

👻 PULL REQUEST

🛠️ PR POINT

💡 참고사항

📸 스크린샷

📟 관련 이슈

JinUng41 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JinUng41 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