5
\$\begingroup\$

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"
  }
}

```
\$\endgroup\$
7
  • \$\begingroup\$ Here in code review, you should be posting code that compiles and works. This code doesn't compile. \$\endgroup\$
    – Daniel T.
    Jul 28, 2020 at 18:12
  • \$\begingroup\$ This code compiles but will require third party libraries. \$\endgroup\$ Jul 28, 2020 at 18:15
  • \$\begingroup\$ I wanted to know like how can I make ViewModel more better? \$\endgroup\$ Jul 28, 2020 at 18:16
  • \$\begingroup\$ What third party libraries? What library provides the ViewController type? What library provides the ViewModel type? Put imports on the code to show what libraries you are depending on. \$\endgroup\$
    – Daniel T.
    Jul 28, 2020 at 18:17
  • 1
    \$\begingroup\$ Yeah now I added import statements \$\endgroup\$ Jul 28, 2020 at 18:19

1 Answer 1

2
\$\begingroup\$

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)
    }
}
```
\$\endgroup\$
8
  • \$\begingroup\$ How do I call fetchCaregivers from ViewController at multiple places? as once you create Observable you can't emit an event to it. You can see in my ViewController I am calling fetchCaregivers from 2 places. \$\endgroup\$ Jul 30, 2020 at 18:33
  • \$\begingroup\$ What are the two causes that lead to the fetch effect? What do you mean by "you can't emit an event to it"? You don't exist in the code so you can't do anything. \$\endgroup\$
    – Daniel T.
    Jul 30, 2020 at 18:36
  • \$\begingroup\$ Sorry, I mean cannot emit a value to Observable here from a different place other than configure method. As value can only be emitted to Observable while creating it. Am I right? \$\endgroup\$ Jul 30, 2020 at 18:39
  • \$\begingroup\$ No, you are not right. For example myButton.rx.tap can emit values long after the Observable was created. \$\endgroup\$
    – Daniel T.
    Jul 30, 2020 at 18:40
  • \$\begingroup\$ Yeah, that's correct. \$\endgroup\$ Jul 30, 2020 at 18:42

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Not the answer you're looking for? Browse other questions tagged or ask your own question.