MVC controllers are not very reusable. You cannot reuse codes from one controller to another (technically, you can but it’s ridiculous). This often leads to developers being sloppy and doing everything inside a controller. I have witnessed unending horror of controllers so heavily coded that I was hesitant to add another line of code worrying that it might blow right in front of me.
Fat Controllers
Consider saving a product category, let’s say you put it in a ProductCategory controller, under the Save action. This looks fine and harmless. However, let’s say you want to implement an import product feature that requires all product categories in a CSV to be automatically created if not found. You will end up duplicating the code for saving a product category.
A more sophisticated example would be the import CSV feature itself. Let’s say you implemented it in Product controller. Then it needs to be implemented on another controller, say Employee. This results to multiple versions of the CSV import which not only leads to buggy application but also to a testing predicament. Imagine hundreds of situations like this scattered all over your controllers.
Obese Models
Before I get into obese models, let me define what a model is: model is a layer not an object. This has been repeatedly said on many StackOverflow answers and it’s worth reiterating again. Often, developers confuse Data Transfer Objects (DTOs) as their Model (or even ViewModel — the whole DTO vs ViewModel vs Model is a lengthy, not to mention touchy topic so I’m not going to touch it with a ten-foot pole). I don’t blame developers here as even on Microsoft’s website, Model is defined as “objects”. This definition is not incorrect but rather limited.
A popular way to reduce controller bloat is to fatten the model instead: shoving business logics to models. While this solves the code reuse problem, it introduces another problem: fat model too often causes overlaps blurring the main purpose of the class. This happens when you need to process multiple entities, you are forced to do it in one model which results to god objects – unwieldy, monolithic models that are difficult to maintain. The problem stems from the fact that you abstracted the problem using the model instead of what problem actually constitutes. Take the import CSV example above. Using fat model, it’s almost deliberately telling you to create an Import method under your product model (if you’re importing products) even it’s obvious that the concern is potentially beyond products.
Fat Controllers – Service layer = Lean Models
Using a service layer compliments thin controllers and lean models. It is a set of classes that serves as a front-line of your model layer. The controller simply talks to the service layer, hiding all the intricacies of your data layer from controller at the same time fully encapsulates the domain. Consider the this action from a controller:
[HttpPost]
public JsonResult ImportProducts(IEnumerable files) {
return Json(new ImportCSVHelper<Product, IProductRepository>()
.Import(a => new Product {
Active = true,
Barcode = a[1],
Color = a[2],
DaysForReplacement = Convert.ToInt32(a[3]),
Discount = Convert.ToInt32(a[4]),
IsReordered = Convert.ToBoolean(a[5]),
ItemCategoryId = new ProductCategoryHelper().GetCategoryId(a[6], true),
ItemCode = a[7],
UnitOfMeasurementId = new UnitOfMeasurementHelper().GetUnitOfMeasurement(a[8], true)
//More field mappings here...
}, files.First()), JsonRequestBehavior.AllowGet);
} |
[HttpPost]
public JsonResult ImportProducts(IEnumerable files) {
return Json(new ImportCSVHelper<Product, IProductRepository>()
.Import(a => new Product {
Active = true,
Barcode = a[1],
Color = a[2],
DaysForReplacement = Convert.ToInt32(a[3]),
Discount = Convert.ToInt32(a[4]),
IsReordered = Convert.ToBoolean(a[5]),
ItemCategoryId = new ProductCategoryHelper().GetCategoryId(a[6], true),
ItemCode = a[7],
UnitOfMeasurementId = new UnitOfMeasurementHelper().GetUnitOfMeasurement(a[8], true)
//More field mappings here...
}, files.First()), JsonRequestBehavior.AllowGet);
}
There are few main things that I let my controller handle: return and/or route a view, populate the view model, and call codes from the service layer. Codes that do not fall on any of these categories are typically moved out. In the example above, I let the calling code in the controller define the mapping of the properties to the CSV fields. This way, my service class does not care how the mapping happens, it ‘s just concern is how to save the entities to the database.
public class ImportCSVHelper <T, TR> where T : BaseModel, new()
where TR : IBaseRepository {
public Tuple<int, int> Import(Func<string[], T> method, HttpPostedFileBase file) {
//Initialisation stuff
file.SaveAs(path);
File.ReadLines(path)
.ToList()
.ForEach(a => {
T t;
try {
var fields = a.Split(new[] { ',', ' ' }, StringSplitOptions.RemoveEmptyEntries);
t = CreateType(method, fields);
_repository.Add(t);
totalSuccess++;
} catch {
totalFailed++;
}
});
_repository.Save();
return new Tuple<int, int>(totalSuccess, totalFailed);
}
}
} |
public class ImportCSVHelper <T, TR> where T : BaseModel, new()
where TR : IBaseRepository {
public Tuple<int, int> Import(Func<string[], T> method, HttpPostedFileBase file) {
//Initialisation stuff
file.SaveAs(path);
File.ReadLines(path)
.ToList()
.ForEach(a => {
T t;
try {
var fields = a.Split(new[] { ',', ' ' }, StringSplitOptions.RemoveEmptyEntries);
t = CreateType(method, fields);
_repository.Add(t);
totalSuccess++;
} catch {
totalFailed++;
}
});
_repository.Save();
return new Tuple<int, int>(totalSuccess, totalFailed);
}
}
}
Service classes must be very flexible. In the example above, the ImportCSVHelper class accepts two generic parameters: the type we want to import and a repository interface. This might be a little too far but it perfectly illustrates the point I am trying to make: I can use the class to import CSV on any entity, on any controller. This pattern allows ultimate code reuse and eliminates the fat on both controllers and models by distributing the codes to its correct domain.