Binding ViewModel to ViewController using RxSwift

Question

I have a ViewController and ViewModel. Here I am using the .subscribe() method too many times to bind title property and etc.

As you can see in the bindActions method I am subscribing to action.fetchCaregivers inside and calling fetchCaregivers().

How can I make this code a bit better?

import UIKit
import RxDataSources

class PatientCaregiversViewController: ViewController {
  
  var tableView: UITableView!
  var viewModel: PatientCaregiversViewModel!
  
  static func initalise() -> PatientCaregiversViewController {
    let viewController = PatientCaregiversViewController()
    let dataProvider = AlternateCaregiverRemoteRepo()
    viewController.viewModel = PatientCaregiversViewModel(dataProvider: dataProvider)
    return viewController
  }
  
  override func setupView() {
    super.setupView()
    
    setupTableView()
    configureEmptyDataSetView()
  }
  
  override func bindViews() {
    super.bindViews()
    
    // Bind State
    viewModel.state.title
      .subscribe(onNext: { [weak self] text in
        self?.title = text
      }).disposed(by: viewModel.disposeBag)
    
    // Bind View
    viewModel.state.displayData
      .bind(to: tableView.rx.items(dataSource: getDataSource()))
      .disposed(by: disposeBag)
    
    bindEmptyDataSet(observable: viewModel.state.emptyDataSetState)
      .subscribe(onNext: { [weak self] in
        self?.viewModel.action.fetchCaregivers.onNext(())
      }).disposed(by: viewModel.disposeBag)
  }
  
  override func finishedLoading() {
    super.finishedLoading()
    
    viewModel.action.fetchCaregivers.onNext(())
  }
}

extension PatientCaregiversViewController {
  private func setupTableView() {
    tableView = UITableView()
    tableView.translatesAutoresizingMaskIntoConstraints = false
    
    tableView.tableHeaderView = UIView(frame: .zero)
    tableView.tableFooterView = UIView(frame: .zero)
    view.addSubview(tableView)
    [tableView.topAnchor.constraint(equalTo: view.topAnchor),
     tableView.bottomAnchor.constraint(equalTo: view.bottomAnchor),
     tableView.leadingAnchor.constraint(equalTo: view.leadingAnchor),
     tableView.trailingAnchor.constraint(equalTo: view.trailingAnchor)]
      .forEach({ $0?.isActive = true })
    
    view.layoutIfNeeded()
  }
  
  func getDataSource() -> RxTableViewSectionedReloadDataSource<RxAnimatableTableSectionModel>! {
    let dataSource = RxTableViewSectionedReloadDataSource<RxAnimatableTableSectionModel>(
      configureCell: { [weak self] dataSource, tableView, indexPath, item in
        switch item {
        case let model as CaregiverInviteCellModel:
          let cell = tableView.dequeueReusableCell(withIdentifier: "CaregiverInviteTableViewCell", for: indexPath) as! CaregiverInviteTableViewCell
          cell.data = model
          return cell
          
        default:
          return UITableViewCell()
        }
    })
    return dataSource
  }
}

import Foundation
import RxSwift
import RxCocoa
import RxDataSources

class PatientCaregiversViewModel: ViewModel {
  
  struct Action {
    let fetchCaregivers = PublishSubject<()>()
    let inviteCaregiver = PublishSubject<()>()
    let refreshList = PublishSubject<()>()
  }
  
  struct State {
    let title = BehaviorRelay<String>.init(value: "CAREGIVERS")
    let displayData = BehaviorRelay<[RxAnimatableTableSectionModel]>(value: [])
    let emptyDataSetState = BehaviorRelay<DataState>.init(value: .loading(title: "", message: ""))
    let showInviteLink = BehaviorRelay<Bool>.init(value: false)
    let invitTapped = PublishSubject<()>()
  }
  
  var action: Action
  var state: State
  var dataProvider: AlternateCaregiverRemoteRepo
  
  init(dataProvider: AlternateCaregiverRemoteRepo) {
    self.dataProvider = dataProvider
    action = Action()
    state = State()
    super.init()
    
    bindActions()
  }
}

// MARK:- PRIVATE METHODS.
extension PatientCaregiversViewModel {
  private func bindActions() {
    action.fetchCaregivers
      .subscribe(onNext: { [weak self] _ in
        self?.fetchCaregivers()
      }).disposed(by: disposeBag)
    
    action.inviteCaregiver
    .subscribe(onNext: { [weak self] _ in
      self?.state.invitTapped.onNext(())
    }).disposed(by: disposeBag)
  }
  
  private func fetchCaregivers() {
    state.emptyDataSetState.accept(loadingState)
    dataProvider.getAllCaregivers()
    .subscribe { [weak self] (event) in
        guard let this = self else { return }
        this.state.emptyDataSetState.accept(.failed(title: this.failedTitle, message: this.failedTitle))
    }.disposed(by: disposeBag)
  }
}

// MARK:- MESSAGES.
extension PatientCaregiversViewModel {
  var loadingState: DataState {
    let title = "Loading Caregivers"
    let message = "Please wait while we load caregivers for you."
    return DataState.loading(title: title, message: message)
  }
  
  var noDataState: DataState {
    let title = "No Caregiver linked"
    let message = "Please invite an alternate caregiver by clicking the invite button."
    return DataState.noData(title: title, message: message)
  }
  
  var failedTitle: String {
    return "Failed to get Caregivers"
  }
}

```

Answer 1

I'm looking at just the view model for this review... You can make this code better by getting rid of all the relays and subjects. These are supposed to be rarely used but you are using them for every property it seems.

The key here though is to isolate each piece of output and provide it with its own subscription.

Also, I noticed that most of the code in the view model isn't getting used, so I removed the unused bits.

class PatientCaregiversViewModelʹ {
    struct Action {
        let fetchCaregivers: Observable<Void>
        let inviteCaregiver: Observable<Void>
    }

    struct State {
        let emptyDataSetState: Observable<DataState>
        let invitTapped: Observable<Void>
    }

    let dataProvider: AlternateCaregiverRemoteRepo

    init(dataProvider: AlternateCaregiverRemoteRepo) {
        self.dataProvider = dataProvider
    }

    func configure(action: Action) -> State {
        let caregivers = Observable.merge(action.fetchCaregivers, Observable.just(()))
            .flatMapLatest { [dataProvider] in dataProvider.getAllCaregivers() }
            .share(replay: 1)

        let emptyDataSetState = Observable.merge(
            action.fetchCaregivers.map { DataState.loading(title: "Loading Caregivers", message: "Please wait while we load caregivers for you.") },
            caregivers.map { _ in DataState.failed(title: "Failed to get Caregivers", message: "Failed to get Caregivers") } // this seems quite wrong. Why aren't you inspecting the emission of getAllCaregivers() to see if the data is correct? Why aren't you using the information for anything?
        )

        let inviteTapped = action.inviteCaregiver
        return State(emptyDataSetState: emptyDataSetState, invitTapped: inviteTapped)
    }
}
```