My View Model looks like this:
public class ProductVM
{
public string SerialNumber { get; set; }
// Other properties left out for brevity
}
In the form that fills this model, users can supply the SerialNumber
of the Product or they can supply the ProductId
. The SerialNumber
is always 100 characters long and the ProductId
is always 50 characters long.
My current solution is to get the SerialNumber
from database if ProductId
is supplied, inside the Action method of the Controller, after checking if ModelState.IsValid
. From my view of thinking, this is bad because of the following reasons:
ModelState.IsValid
.What I want to know is:
Service Query
to fetch the needed data, but instead of querying by SerialNumber
, to query by ProductId
? (This solution looks to me like repetition of code for most of the part)Currently my Action method looks like this:
[HttpPost]
public ActionResult GenerateReport(ProductVM vm)
{
if(ModelState.IsValid == false)
{
vm.Rehydrate();
return View(vm);
}
// If Length is 100 then ProductId has been supplied, query database to find SerialNumber
if(vm.SerialNumber.Length == 100)
{
vm.SerialNumber = _db.GetSerialNumber(vm.SerialNumber);'
// This is the thing that I don't like.
// Multiple places where data validation of View Model is being done inside Controller.
if(string.IsNullOrEmpty(vm.SerialNumber))
{
ModelState.AddModelError("", "No Product has been found with the given ProductId!");
return View(vm);
}
}
var reportData = _productDomainService.GetReportData(vm.SerialNumber);
var report = _infrastructure.GenerateReport(reportData);
// Rest of code left out for brevity
}
My suggestion is to refactor the code to improve separation of concerns. While it's valid to use ModelState.IsValid
for standard validations such as Required
and MaxLength
, more complex validation that involves database interaction, such as verifying the SerialNumber
or ProductId
, should be handled in a dedicated service.
Here’s how you can refactor the code:
Create a service that handles the logic of transforming and validating the SerialNumber
and ProductId
. This will keep your controller focused on orchestrating actions rather than managing detailed validation.
Service Implementation Example:
public interface IProductService
{
string TransformProductIdentifier(string identifier, out string errorMessage);
}
public class ProductService : IProductService
{
private readonly YourDbContext _db;
public ProductService(YourDbContext db)
{
_db = db;
}
public string TransformProductIdentifier(string identifier, out string errorMessage)
{
if (identifier.Length == 100) // Assuming this is a ProductId
{
var serialNumber = _db.GetSerialNumber(identifier);
if (string.IsNullOrEmpty(serialNumber))
{
errorMessage = "No Product has been found with the given ProductId!";
return null;
}
errorMessage = null;
return serialNumber;
}
errorMessage = null;
return identifier; // Assume it is already a valid SerialNumber
}
}
Refactor the controller to delegate validation and transformation to the service. This approach adheres to the principle of Single Responsibility and keeps the controller simpler.
Controller Implementation Example:
public class ReportController : Controller
{
private readonly IProductService _productService;
private readonly IProductDomainService _productDomainService;
private readonly IInfrastructure _infrastructure;
public ReportController(IProductService productService,
IProductDomainService productDomainService,
IInfrastructure infrastructure)
{
_productService = productService;
_productDomainService = productDomainService;
_infrastructure = infrastructure;
}
[HttpPost]
public ActionResult GenerateReport(ProductVM vm)
{
if (!ModelState.IsValid)
{
vm.Rehydrate();
return View(vm);
}
// Use the service to transform and validate the identifier
var serialNumber =
_productService.TransformProductIdentifier(vm.SerialNumber, out var
errorMessage);
if (!string.IsNullOrEmpty(errorMessage))
{
ModelState.AddModelError("", errorMessage);
return View(vm);
}
vm.SerialNumber = serialNumber;
var reportData = _productDomainService.GetReportData(vm.SerialNumber);
var report = _infrastructure.GenerateReport(reportData);
//more code
}
}