3
\$\begingroup\$

I have tried to develop a simple product listing app in RxSwift with MVVM design pattern. I have managed to achieve it but I need someone to review my code, such as:

  1. Have I followed the proper RxSwift features?
  2. Is binding of TableViewCells item with ViewController's ViewModel correct?
  3. Anything I am missing here?

Note: Also, it is available on GitHub.

ViewController.swift

// MARK:- View's MVVM Binding Method
func configure(with viewModel: ViewModelType) {
    // DataSource implementation
    let dataSource = RxTableViewSectionedReloadDataSource<SectionOfProducts>(
        configureCell: { dataSource, tableView, indexPath, item in
            let cell = tableView.dequeueReusableCell(withIdentifier: "ProductCell", for: indexPath) as! ProductCell
            cell.prepareCell(with: item)

            // Binding Cell item with viewModel's input
            cell.buttonLike.rx.tap
                .map{_ in item}
                .bind(to: self.viewModel.input.likedProduct)
                .disposed(by: cell.disposeBag)

            return cell
    })
    self.dataSource = dataSource

    // Binding reachedBottom trigger with viewModel's input for pagination of products
    tableViewProducts.rx.reachedBottom.asObservable()
        .bind(to: viewModel.input.nextPageTrigger)
        .disposed(by: disposeBag)

    // Bind refresh control to viewModel
    refreshControl.rx.controlEvent(.valueChanged)
        .bind(to: self.viewModel.input.refreshTrigger)
        .disposed(by: disposeBag)

    // Binding viewModel's output's products with tableview items
    viewModel.output.products.asObservable()
        .bind(to: tableViewProducts.rx.items(dataSource: dataSource))
        .disposed(by: disposeBag)
}

ViewModel.swift

class ProductsViewModel: ViewModelProtocol {
    // Input consists of user inputs such as like any product, pull to refresh and get data for next page
    struct Input {
        let likedProduct: AnyObserver<ProductModel>
        let refreshTrigger: AnyObserver<Void>
        let nextPageTrigger: AnyObserver<Void>
    }

    // products are provided as output show on ViewController
    struct Output {
        let products: Driver<[SectionOfProducts]>
    }

    let input: Input
    let output: Output

    private let disposeBag = DisposeBag()

    private let likedProductSubject = PublishSubject<ProductModel>()
    private let refreshTriggerSubject = PublishSubject<Void>()
    private let loadAfterTriggerSubject = PublishSubject<Void>()

    init(_ interactor: ProductsInteractorProtocol) {
        // Init Output
        input = Input(likedProduct: likedProductSubject.asObserver(),
                      refreshTrigger: refreshTriggerSubject.asObserver(),
                      nextPageTrigger: loadAfterTriggerSubject.asObserver())
        //>>>>>>>>>> Please review my code from here
        // When ViewModel initializes products are requested from the Interactor
        var products = interactor.getProducts()

        // Detect when like button is tapped in the ViewController's tableView's cell
        likedProductSubject.subscribe ({ (event) in
            print("\(String(describing: event.element?.productName))")
        }).disposed(by: disposeBag)

        // To get next gage data I have used danielt1263/PaginationNetworkLogic.swift: Link: https://gist.github.com/danielt1263/10bc5eb821c752ad45f281c6f4e3034b
        let source = PaginationUISource(refresh: refreshTriggerSubject.asObservable(), loadNextPage: loadAfterTriggerSubject.asObservable(), bag: disposeBag)
        let sink = PaginationSink(ui: source, loadData: interactor.getNextProducts(page:))

        // Concat new products with the previous products
        let newProducts = sink.elements.asObservable()
        products = Observable.concat([products, newProducts]).scan([], accumulator: +)
        //<<<<<<<<<<<< Till here

        // Init Output
        output = Output(products: products.asDriver(onErrorJustReturn: []))
    }
}

ProductsInteractor.swift

final class ProductsInteractor: ProductsInteractorProtocol {

    var nextURLString: String?

    /// Sending a post request to get products
    ///
    /// - Returns: Array structured according to RxDataSources requirement
    func getProducts() -> Observable<[SectionOfProducts]> {
        var products: [ProductModel]?
        let requestData = ProductsRQM(categoryId: 0, subCategoryId: 0, typeId: 0, customerId: "11")
        return Observable.create { [weak self] (observer) -> Disposable in
            APIRequests.shared.post(requestModel: requestData, requestPath: .requestProducts) { (error, data) in
                let jsonDecoder = JSONDecoder()
                let responseModel = try? jsonDecoder.decode(ProductsBase.self, from: data!)

                // Products from the api are stored in products variable
                products = responseModel?.data?.item
                self?.nextURLString = responseModel?.data?.links?[0].href
                let section = [SectionOfProducts(header: "", items: products ?? [])]
                observer.onNext(section)
                observer.onCompleted()
            }
            return Disposables.create {}
        }
    }

    /// Sending a get request to get products
    ///
    /// - Returns: Array structured according to RxDataSources requirement
    func getNextProducts(page: Int) -> Observable<[SectionOfProducts]> {
        var products: [ProductModel]?
        return Observable.create { [weak self] (observer) -> Disposable in
            if self?.nextURLString != nil {
                APIRequests.shared.get(requestURL: self?.nextURLString ?? "", callBack: { (error, data) in
                    let jsonDecoder = JSONDecoder()
                    let responseModel = try? jsonDecoder.decode(ProductsBase.self, from: data!)

                    // Products from the api are stored in products variable
                    products = responseModel?.data?.item

                    // If true then nextPage is available else not
                    if responseModel?.data?.links?[0].rel != "previousPage" {
                        self?.nextURLString = responseModel?.data?.links?[0].href
                    } else { // Else assigning nextURLString to nil to prevent unnecessary api call
                        self?.nextURLString = nil
                    }
                    let section = [SectionOfProducts(header: "", items: products ?? [])]
                    observer.onNext(section)
                    observer.onCompleted()
                })
            }
            return Disposables.create {}
        }
    }
}
\$\endgroup\$
1
\$\begingroup\$

This is a good first attempt and a lot of good can be said about it, but I'm going to focus on what I see are problems that should be fixed. Don't let this dissuade you though!

  • ControllerType and ViewModelProtocol are completely useless. If you aren't going to use them to constrain some generic function then get rid of them.

  • Your APIRequests.get(requestURL:callBack:) doesn't call the callBack if the requestURL can't be used to create a URL. This will cause silent errors. In general, make sure that all possible paths call the callBack when you are making async functions. Also, RxCocoa has a couple of wrappers around dataTask. I suggest you use them in your APIRequests class.

  • APIRequests is a class with no state. This is pointless. Move the functions out of the class and get rid of it.

  • ProductsInteractor.nextURLString is Optional. There is no point in making Strings optional. An empty string is the same as no string.

  • APIRequest doesn't need to be a protocol; it's just a data bucket. Make it a struct and adjust other code as necessary.

  • Regarding ProductsViewModel: I'm not a fan of this style of view model; It seems like a lot of boilerplate to me. In any case, you should have unit tests for it.

  • Your ProductsInteractor.getProducts() and ProductsInteractor.getNextProducts(page:) functions only emit if the network request was successful, they never emit errors and if self?.nextURLString == nil then getNextProducts(page:) doesn't emit anything at all. In general, make sure that all possible paths through the block of an Observable.create all call the observer.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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