프로젝트 매니저 [STEP 2-2] Moon

[hojun-jo]

@havilog
안녕하세요 하비!
프로젝트 매니저 STEP 2-2 PR 입니다!
코어 데이터를 활용하여 CRUD를 구현하고 두 번째 화면과 뷰 모델의 바인딩은 Combine을 활용할 수 있도록 시도해 보았습니다.
이번 리뷰도 잘 부탁드립니다~😀

고민했던 점

🔍 Combine을 활용한 바인딩

assign을 이용해 바인딩을 시도했으나 텍스트필드에 값이 반영되지 않아 sink를 이용해 확인하고자 했습니다.
뷰 컨트롤러가 화면에 보이기까지 sink에 8번 진입하며 title에 “Asdf”와 “”(빈 문자열)이 번갈아 반복되어 나오고 있었습니다.
반복적으로 sink에 진입한 이유는 bindViewModelToView와 bindViewToViewModel이 같은 프로퍼티를 사용하기 때문에 계속 순환되었던 것으로 추정하고 있습니다.

private func setUpBindings() {
    bindViewModelToView()
    bindViewToViewModel()
}

private func bindViewToViewModel() {
    titleTextField.publisher(for: \.text)
        .assign(to: \.title, on: viewModel)
        .store(in: &cancellables)
...
}

private func bindViewModelToView() {
    viewModel.$title
        .receive(on: RunLoop.main)
//            .assign(to: \.text, on: titleTextField)
        .compactMap { $0 }
        .sink(receiveValue: { [weak self] title in
            self?.titleTextField.text = title
        })
        .store(in: &cancellables)
...
}

따라서 뷰 모델에서 뷰에 데이터를 보내는 프로퍼티와 뷰에서 데이터를 받는 프로퍼티를 나누어 순환이 생기지 않도록 했습니다.

final class ToDoDetailViewModel {
    // 모델
    private let todo: ToDo

    // 뷰에 데이터 보내는 용
    let todoSubject: CurrentValueSubject<ToDo, Never>
    
    @Published var category: String?
    @Published var isEnableEdit: Bool = true
    @Published var background: UIColor?
    
    // 뷰에서 데이터 받는 용
    var title: String?
    var deadline: Date = .init()
    var body: String?
...
}

extension ToDoDetailViewController {
    private func setUpBindings() {
        bindViewModelToView()
        bindViewToViewModel()
    }
    
    private func bindViewToViewModel() {
        titleTextField.publisher(for: \.text)
            .assign(to: \.title, on: viewModel)
            .store(in: &cancellables)
    ...
    }
    
    private func bindViewModelToView() {
        viewModel.todoSubject
            .sink(receiveValue: {
                self.titleTextField.text = $0.title
                self.bodyTextView.text = $0.body
            })
            .store(in: &cancellables)

        [titleTextField, datePicker, bodyTextView].forEach {
            viewModel.$isEnableEdit
                .assign(to: \.isUserInteractionEnabled, on: $0)
                .store(in: &cancellables)
            viewModel.$background
                .assign(to: \.backgroundColor, on: $0)
                .store(in: &cancellables)
        }
    }
}

🔍 바인딩이 잘 이루어지지 않는 문제

    private func bindViewToViewModel() {
        titleTextField.publisher(for: \.text)
            .assign(to: \.title, on: viewModel)
            .store(in: &cancellables)
        datePicker.publisher(for: \.date)
            .assign(to: \.deadline, on: viewModel)
            .store(in: &cancellables)
        bodyTextView.publisher(for: \.text)
            .assign(to: \.body, on: viewModel)
            .store(in: &cancellables)
    }

위의 함수를 처음 실행했을 때는 뷰 모델에 값이 들어오는 것을 확인 했습니다. 하지만 유저가 TextField에 입력했을 때는 값이 들어오고 DatePicker를 돌리거나 TextView에 입력 했을 때는 값이 들어오지 않는 문제가 있었습니다.

final class ToDoDetailViewController: UIViewController {
...
    // datePicker의 값이 변경될 때마다 뷰 모델에 데이터를 보낼 수 있도록 타겟 추가
    private func setUpDatePickerBinding() {
        datePicker.addTarget(
            self,
            action: #selector(bindDatePicker),
            for: .valueChanged
        )
    }
...
    // 뷰에서 입력 받은 데이터를 뷰 모델로 보냄
    private func bindViewToViewModel() {
        titleTextField.publisher(for: \.text)
            .assign(to: \.title, on: viewModel)
            .store(in: &cancellables)
        bindDatePicker()
        bodyTextView.textPublisher
            .assign(to: \.body, on: viewModel)
            .store(in: &cancellables)
    }
    
    // 최초 실행 시 한 번만 값을 보내기 때문에 값이 변경 될 때마다 값을 보낼 수 있도록
    // datePicker.addTarget의 selector로 사용
    @objc
    private func bindDatePicker() {
        datePicker.publisher(for: \.date)
            .assign(to: \.deadline, on: viewModel)
            .store(in: &cancellables)
    }
...
}

extension UITextView {
    var textPublisher: AnyPublisher<String, Never> {
        NotificationCenter.default
            .publisher(for: UITextView.textDidChangeNotification, object: self)
            .compactMap { $0.object as? UITextView } // 위 publisher의 리턴이 Notification이기 때문에 전달한 object(Any 타입)를 UITextView로 캐스팅
            .compactMap(\.text) // 위에서 캐스팅한 텍스트 뷰에서 text 추출
            .eraseToAnyPublisher() // 위에서 받은 text를 Publisher로 감쌈
    }
}

따라서 DatePicker에는 addTarget으로 값이 변경되었을 때 바인딩을 다시 수행할 수 있도록 했습니다. TextView는 extension을 통해 텍스트가 바뀌었을 때 값을 보낼 수 있도록 textPublisher를 만들었습니다.

조언을 얻고 싶은 점

🔍 DataManager에서 저장을 수행할 때 Notification

코어데이터에 저장을 수행한 후 그 내용을 뷰에 업데이트 해야 하기 때문에 DataManager가 알림을 보내고 뷰 모델이 알림을 받아 데이터를 업데이트하여 뷰에 표시하고 있습니다. 현재 구조가 모델이 변경 되었을 때 뷰 모델에 알림을 보내는 부분이 맞을까요? 그렇다면 Notification 외에 이를 수행할 수 있는 방법이 있을까요?

final class DataManager {
...
    func saveContext() {
        guard viewContext.hasChanges else {
            return
        }
        
        do {
            try viewContext.save()
            postCalledSaveContext()
        } catch let error as NSError {
            print("Unresolved error: \(error), \(error.userInfo)")
        }
    }
...
    private func postCalledSaveContext() {
        NotificationCenter.default
            .post(
                name: NSNotification.Name("CalledSaveContext"),
                object: nil
            )
    }
}

final class ListViewModel {
...
    private func setUpNotifications() {
...
        // dataManager에서 저장이 이루어졌을 때 모델을 다시 로드
        NotificationCenter.default
            .addObserver(
                self,
                selector: #selector(loadTodoList),
                name: NSNotification.Name("CalledSaveContext"),
                object: nil
            )
...
    }
    
    @objc
    private func loadTodoList() {
        todoList = dataManager.fetchToDoList()
    }
}

🔍 고민했던 점의 바인딩이 잘 이루어지지 않는 문제

처음에는 아래 코드로 바인딩을 했습니다. 그런데 텍스트 필드만 바인딩이 유지 되고 데이트 픽커와 텍스트 뷰는 처음 실행할 때 한 번만 값을 받고 이후에는 값을 받지 못 했습니다. 이 원인을 알 수 있는 방법이 있을까요?

private func bindViewToViewModel() {
        titleTextField.publisher(for: \.text)
            .assign(to: \.title, on: viewModel)
            .store(in: &cancellables)
        datePicker.publisher(for: \.date)
            .assign(to: \.deadline, on: viewModel)
            .store(in: &cancellables)
        bodyTextView.publisher(for: \.text)
            .assign(to: \.body, on: viewModel)
            .store(in: &cancellables)
}

🔍 @Published인 변수의 타입과 관련된 에러

뷰 모델에서 뷰의 배경색을 업데이트하기 위해 background 변수를 가지고 있습니다. 이 변수가 옵셔널이 아닐 때에 assign 부분에서 아래와 같은 에러가 나타납니다.

Key path value type 'UIColor?' cannot be converted to contextual type 'Published.Publisher.Output' (aka 'UIColor')

여기서 assign은 UIColor(뷰 모델이 가지고 있는 background 변수)를 UIColor?(titleTextField, datePicker, bodyTextView의 backgroundColor)에 대입하는 것이라 생각했는데 에러 메시지를 보면 UIColor?를 UIColor로 변환할 수 없다는 내용인 것 같습니다. 이런 에러의 원인은 어떻게 찾아봐야 할까요?

final class ToDoDetailViewModel {
...
    @Published var background: UIColor?
...
}

extension ToDoDetailViewController {
...
    // 뷰 모델의 데이터를 받아 뷰에 적용
    private func bindViewModelToView() {
...
        [titleTextField, datePicker, bodyTextView].forEach {
...
            viewModel.$background
                .assign(to: \.backgroundColor, on: $0)
                .store(in: &cancellables)
        }
    }
}

[havilog]

수고하셨어요 문~

[havilog]

뷰모델은 웬만하면 외부로 부터 주입받을 수 있도록 설계하는게 좋을 것 같아요.
이유는 init시점에 다른 뷰모델을 넣어줄 수 있게 만들어 의존성을 분리해 테스트를 할 수 있게 만들 수 있기 때문이에요

마찬가지로 뷰모델이 의존하고 있는 dataManager도 외부에서 주입해주면 좋을 것 같아요.

또한 이와 관련하여 dependency injection이 무엇이고, 왜 필요한지도 공부해보시면 좋을 듯합니다.

[havilog]

버튼의 액션은
어떤 일을 할지
가 아닌,
어떤 액션이 일어났는지
로 네이밍 해주시면 좋을 것 같아요

[havilog]

뷰모델에 액션을 노티피케이션으로 주도록 설계하신 이유가 있을까요?
delegate, closure, notification 등 객체간의 데이터 전달 방법에 대해 각각의 활용 목적과 장단점에 대해 명확하게 정의해주시면 좋을 것 같습니다.

[havilog]

ListHeader는 단순히 로직 없이 화면만 표시해주는 뷰 같은데,
같은 뷰모델을 공유하면 의존성이 강결합되어 유지보수성이 떨어질 수도 있을것 같아요.
데이터를 넣어줄 때에는 최대한 raw한 데이터를 넣어주는건 어떨까요?

[havilog]

요거는 view를 initialize하는 시점에 해도 괜찮을 것 같아요

[havilog]

init시점에 너무 많은 일들을 해주고 있는 것 같아요.
isEnableEdit 같은 경우 파라미터로 주입받지 않는다면 프로퍼티 선언부에 초기값을 넣어주어도 괜찮을거 같구요

[havilog]

위 코멘트가 해결된다면 이 initializer로 사라져도 될 거 같아요

[havilog]

UIColor같은 경우에는 뷰모델이 아닌 뷰의 영역이라고 생각되어요.
viewmodel은 UIKit을 import하지 않고 돌아가도록 설계하면 좋을 것 같습니다.
즉 색깔의 값이 아닌 뷰의 상태를 정의하고,
뷰에서는 뷰모델에 정의된 상태를 옵저빙해서 색깔을 바꿀 수 있도록 설계해주세요

[havilog]

prepareForReuse가 빠져있는거 같네요

[havilog]

요 아이는 class여야할 필요가 있을까요?