2
\$\begingroup\$

I've been coding for some time now and since I am working alone I'm not strict in my coding structure approach(bad idea) and was only focused on getting things done which resulted in MVC(MASSIVE-View-Controllers). Now recently I've started adapting to MVVM and things are looking clean and simple. But I'm not sure if I'm following or breaking the MVVM pattern.

Please review the code below.

MyActivityModel(Model)

// MARK: - Request
struct MyActivityRequest: Convertable{
    var UserID = UD.string(forKey: USER_ID) ?? ""
    var TransactionType = ActivityType.ALL.rawValue
}

// MARK: - Response
struct MyActivityResponse: Codable {
    let Value: [MyActivityObject]?
    let Response: String?
    let ResponseCode: Int?
}

struct MyActivityObject: Codable {
    let Date, Points: String?
    let TransactionType, Amount: String?
    let BranchTitle, TransactionCode: String?
}

ActivityVC(View)

import UIKit

class MyActivityVC:UIViewController{
    
    var activityType = ActivityType.ALL
    let vm = MyActivityVM()
    let common = Common()
    var activityView: UIView!

    @IBOutlet weak var noInternetLabel: UILabel!
    @IBOutlet weak var noInternetView: UIView!

    @IBOutlet weak var noDataLabel: UILabel!
    @IBOutlet weak var noDataAvailableView: UIView!

    @IBOutlet weak var tabelView: UITableView!
    
    override func viewDidLoad() {
        super.viewDidLoad()
            
        self.title = "Transactions".localized()
        SetupViews()
        BindViews()
        LoadData()

    }
    
    func SetupViews(){
        activityView = common.GetActivityView(holderView: self.view)
        noInternetLabel.text = "No active internet connection".localized()
        noDataLabel.text = "You don't have any transactions yet".localized()
    }
    
    override func viewWillAppear(_ animated: Bool) {
        self.navigationController?.setNavigationBarHidden(false, animated: true)
    }

    func BindViews(){
        vm.activities.bind { [weak self] in
            if $0.count == 0 {
                self?.tabelView.isHidden = true
                self?.noDataAvailableView.isHidden = false
            }else{
                self?.tabelView.isHidden = false
                self?.noDataAvailableView.isHidden = true
                self?.tabelView.reloadData()
            }
        }
        vm.loading.bind { [weak self] in
            self?.activityView.isHidden = !$0
        }
        vm.errorMessage.bind { [weak self] in
            guard let self else {return}
            if $0.errorMessage.count > 0 {
                if $0 == .noInternet{
                    self.noInternetView.isHidden = false
                    self.noDataAvailableView.isHidden = true
                    self.tabelView.isHidden = true
                }else{
                    self.common.ShowDialog(_title: nil, _msg: $0.errorMessage, _view: self)
                }
            }
        }
    }

    func LoadData(){
        vm.LoadActivitiesData(activityType: activityType)
    }

}


extension MyActivityVC: UITableViewDataSource{
    func tableView(_ tableView: UITableView, numberOfRowsInSection section: Int) -> Int {
        return vm.activities.value.count
    }
    
    func tableView(_ tableView: UITableView, cellForRowAt indexPath: IndexPath) -> UITableViewCell {
        guard let cell = tableView.dequeueReusableCell(withIdentifier: MyActivityCell.ID) as? MyActivityCell
        else {
            return UITableViewCell()
        }
        cell.activity = vm.activities.value[indexPath.row]
        return cell
    }
    
    
}

MyActivityCell(View)

import UIKit

class MyActivityCell: UITableViewCell {

    static let ID = "activity_cell"
    @IBOutlet weak var branchTitle: UILabel!
    @IBOutlet weak var pointsLabel: UILabel!
    @IBOutlet weak var USDLabel: UILabel!
    @IBOutlet weak var label: UILabel!
    @IBOutlet weak var label2: UILabel!
    @IBOutlet weak var dateLabel: UILabel!
    @IBOutlet weak var activityTypeView: UIView!
    
    var activity : MyActivityObject? {
        didSet{

            branchTitle.text = activity?.BranchTitle
            pointsLabel.text = activity?.Points
            USDLabel.text = activity?.Amount
            dateLabel.text = activity?.Date
            
            guard let type = Int(activity?.TransactionCode ?? "00") else {return}
            
                var color = UIColor.gray
                switch type {
                case 1:
                    color = .systemGreen
                case 2:
                    color = .systemRed
                case 3:
                    color = .systemYellow
                default:
                    color = .gray
                }
                activityTypeView.backgroundColor = color
            
        }
    }
    
    
    override func awakeFromNib() {
        super.awakeFromNib()
        label.text = "PTS".localized()
        label2.text = "USD".localized()
    }


}

MyActivityVM(Viewmodel)

import Foundation

class MyActivityVM{
    var request = MyActivityRequest()
            
    var activities: Observable<[MyActivityObject]> = Observable([MyActivityObject]())
    var loading: Observable<Bool> = Observable(false)
    var errorMessage: Observable<RequestErrorHandling> = Observable(RequestErrorHandling.none)

    func LoadActivitiesData(activityType: ActivityType){

        request.TransactionType = activityType.rawValue
        guard let body =  request.convertToDict() else {return}

        loading.value = true
        URLSession.shared.Request(params: "activities",
                                  body: body,
                                  expecting: MyActivityResponse.self){result in
            DispatchQueue.main.async{ [weak self] in
                guard let self else {return}
                self.loading.value = false
                switch result {
                case .success(let data):
                    if data.Response == "success"{
                        
                        guard let transactions = data.Value else {
                            return
                        }
                        self.activities.value.append(contentsOf: transactions)
                        
                    }
                    
                case .failure(let failure):
                    self.errorMessage.value = failure as! RequestErrorHandling
                }
            }
        }
    }
}

I've omitted the URLSession extension to create and fire the API request. I'm not using callback/closures because the views are bind using Boxing here.

Any comments are appreciated.

PS Not sure if this is the correct place to ask this question so please don't down vote.

\$\endgroup\$

1 Answer 1

3
\$\begingroup\$

The overall MVVM implementation within MyActivityVC seems fine.

FWIW, before you give up on MVC entirely, I might advise referring to Dave DeLong’s A Better MVC posts or the video. It is an interesting take on the “massive view controller” joke.

Also, rather than this Observable implementation, you might consider refactoring to use Combine. Or even Combine in conjunction with SwiftUI. Given the evolution of observable patterns native to Swift, the use of third-party implementations feels like a bit of an anachronism.


A few tactical observations on your code snippets:

  1. It is a bit curious that MyActivityCell is not following the same MVVM pattern, but rather is falling back to more of a MVC/MVP style pattern. I'd either go all in on MVVM, or not do it at all.

  2. As a matter of convention, methods and properties names should start with lowercase letters.

  3. The viewWillAppear should call super.

  4. I would advise renaming tabelView to tableView.

\$\endgroup\$
10
  • \$\begingroup\$ Thank you and appreciate your time reviewing my code. I'm just switching from MVC to MVVM and was wondering if I'm following the pattern or not. But as you mentioned here in 1st point. Can you please comment a bit further here? I honestly cant tell where I'm falling back to MVC. PS. thanks for catching the typo there. I didn't saw it before I read your comment. :D \$\endgroup\$
    – Joe
    Commented Nov 8, 2022 at 15:15
  • \$\begingroup\$ MVVM is the process of binding a view object to a model object. The idea is that if the model object subsequently changes, the UI will be automatically updated and/or if the UI is updated (e.g. if you bound the model object to a text field/view), the model object will be updated automatically. But that’s not what your cellForRowAt is doing. It is just passing the value to the cell and the cell is just directly updating the controls. There is no data binding here. It’s a more traditional MVC pattern of simply programmatically updating labels/controls. \$\endgroup\$
    – Rob
    Commented Nov 8, 2022 at 16:29
  • \$\begingroup\$ This cellForRowAt part is exactly what was bothering me about this approach but I've seen a lot MVVM code snippets like this and still wasn't confident if its correct. And how can I achieve this said behavior? Because as I look at it, the vm.activities is bind with tableview. And as the tableview will reload the new activity data will be displayed(cellForRowAt). And in MVVM, isn't it that the Models and Views are not directly communicating with each other, thats the job of ViewModel, still learning this pattern. \$\endgroup\$
    – Joe
    Commented Nov 8, 2022 at 22:06
  • \$\begingroup\$ Yeah, I’ve seen this pattern, too (usually within the context of gradually easing new devs into the MVVM world), but it is incomplete, IMHO. Try modifying this so that instead of UILabel, each row had a UITextField. If you don’t bind the text field back to the view model, you’ll be stuck writing a bunch of event handlers to propagate the changes back to the view model. Compare that to a SwiftUI sort of implementation, where you bind the TextField to your view model: gist.github.com/robertmryan/9d9e6f8920c5492110b7d9e198c9c527 \$\endgroup\$
    – Rob
    Commented Nov 10, 2022 at 19:48
  • \$\begingroup\$ “And in MVVM, isn't it that the Models and Views are not directly communicating with each other, thats the job of ViewModel” … In MVVM, ideally you want to bind the UI control to the model objects in your VM, so that both (a) changes in the VM are reflected in the UI and (b) changes in the view automatically trigger updates to the VM. The table view implementation in the question accomplishes the former, but not the latter. Now, if you don’t need the latter, maybe you just declare success with the code in the question, but, strictly speaking, it is an incomplete MVVM implementation, IMHO. \$\endgroup\$
    – Rob
    Commented Nov 10, 2022 at 19:56

Your Answer

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

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