MVC - data calculations best practice - viewmodel vs. controller

Question

rI need some advice on where to run a calculation on data.

I have a viewmodel that contains all the fields that I need for my calculation and I created the following for one of my calculations:

public class CommissionVM
{
public int? LoanAmountLock { get; set; } // from loan table    
public decimal BranchRev { get; set; }   // from revenue table
public decimal BranchProcessFee { get; set; }   // from revenue table

public decimal BranchGrossTotal
    {
        get
        {               

            return Convert.ToDecimal(LoanAmountLock * (BranchRev/ 100) + BranchProcessFee);
        }
    }

}

I tried to use the Model.BranchGrossTotal in my view, but it is returning 0. I think I have an order-of-operations problem. The values LoanAmountLock, BranchRev, and BranchProcessFee are returned as the results of a query:

public ActionResult Reconcile(int? id, string RevenueLoanType)
    {
var model = new CommissionVM()
        {

            Loan = db.Loan.FirstOrDefault(a => a.id == id ),                
            BranchRevenues = db.BranchRevenues.FirstOrDefault(a => a.RevenueLoanType == RevenueLoanType),  
        };
return View(model);
}

I originally was able to get these calculations to work by doing all the math in the controller after I populate the viewmodel with the query, but there will be about 10 calculations, and from what I understand, I shouldn't clutter up my controller with business logic.

What is the best solution for this? Do I need to create another class for the calculations? If so, how do I populate that class with my data and use it in my controller?

EDIT: I am not sure how to set up the business classes and use them in the controller. Can anyone point me in the direction of a tutorial?

Answer 1

You should not do the calculation in your controller nor in your view model. You should do it in the Business Layer. Think about View Models are really simple classes that contain data to be displayed to the user, that's it.

Regarding the calculation, you should convert one of the terms to decimal, not the result of the calculation. If you divide integers, you get an integer.

You could create a class and call it CommissionService for example. That class should call your Data Layer to get the data, do any extra calculation and return the data (maybe a DTO) to the controller. The controller should create View Models based on the DTO and send them to the view.

Read these articles:

1) https://msdn.microsoft.com/en-us/library/hh404093.aspx

2) http://www.asp.net/mvc/overview/older-versions-1/models-%28data%29/validating-with-a-service-layer-cs

3) http://blog.diatomenterprises.com/asp-net-mvc-business-logic-as-a-separate-layer/

4) http://sampathloku.blogspot.com.ar/2012/10/how-to-use-viewmodel-with-aspnet-mvc.html

Answer 2

I don't like calculations on my view models -- you can't reuse the calculation easily elsewhere and it is harder to test and debug. Create separate classes to do business logic.

Your business logic classes can either return your view models or return values you use to populate those models. The trade-off is ease of use with reusability.

I generally favor returning the value rather than a big object so my services are more reusable.

Controller

public class BranchController : Controller
{
    private IBusinessService service;

    public BranchController()
    {
        this.service = new BusinessService(db);
    }

    public ActionResult Reconcile(int? id, string RevenueLoanType)
    {
        var model = new CommissionVM
        {
            BranchGrossTotal = this.service.GetBranchGrossTotal(id, RevenueLoanType),
            ...
        };

        return View(model);
    }
}

Service

You can make any number of these and your controllers would use them as needed. If you need a query you should pass the DbContext instance or you may have problems with related entities on separate contexts.

public interface IBusinessService
{
    decimal GetBranchGrossTotal(int id, string revenueLoanType);
}

public class BusinessService : IBusinessService
{
    private DbContext db;

    public BusinessService(DbContext db)
    {
        this.db = db;
    }

    public decimal GetBranchGrossTotal(int id, string revenueLoanType)
    {
        var branch = db.Branch.First(b => b.Id == id);
        // do stuff
        return total;
    }
}

You could fully-populate and return a view model in your GetBranchGrossTotal() if you choose.

Answer 3

First of all, the properties you are assigning to your CommissionVM on your controller do not match the ones declared on your model. You assign Loan and BranchRevenues, when you have only LoanAmountLock and BranchRevs available on your model. Please notice that the Loan property is an object itself, and the LoanAmountLock must be retrieved from this object (Loan.LoanAmountLock). The same happens with the BranchRevenues object. You should assign the BranchRevs to the respective property of the BranchRevenues object as needed. If you do not do this, then the values will default to 0 and when trying to calculate the BranchGrossTotal it will obviously be 0.

Another reason, assuming that you are correctly populating your model properties, is that the FirstOrDefault method, renders null values because there is no such entity. This will result also in the BranchGrossTotal to be 0.

You are right that you do not need to clutter your controller neither with calculations nor with db access. I would create a business class ComissionBusiness and instantiate it at the top of your controller. This class would have a method which performs all calculations. You should move the Reconcile method to your new business class method and call it on the reconcile action. Something like (excuse the lack of syntax)

public MyController : Controller {

public ComissionBusiness comissionBusiness;

public MyController(){
comissionBusiness  = new ComissionBusiness();
}

public ActionResult Reconcile(int? id, string RevenueLoanType)
    {
var model = comissionBusiness.Reconcile(id, revenueLoanType);
return View(model);
}
}