My name is Matt and I have a fat controller problem.
In truth I have limited experience of MVC and I'm trying to learn how to organise my code better so it's more testable and more maintainable. Here's my first effort at rewriting a bit of my existing application to make it happier and slimmer.
It's a controller which accepts a file from the view and validates it. If it's a zip file (which is allowed) it checks to see if it needs a password. If so, the user is redirected to another view to give the password. Otherwise the file is unzipped, and the contents validated.
Some notes on the code as presented:
- For brevity, I have not included code for Interfaces, DTOs or Views
- This is proof of concept code: it compiles but is otherwise untested
- It is reliant on the external libraries NLog and DotNetZip
I am as much interested in how this code is structured and organised along with MVC best practice as I am in the function and format of the code itself. I'm also trying to stick to SOLID principles as much as possible, and I'm really not sure I've done a great job.
I'm aware this is a fair amount of code, but I'm pretty desparate. I work alone and am trying to learn this stuff by experience: I have no-one to turn to but you for comments, so your help is much appreciated.
This is my controller class. BaseController ensures all controllers have a repository (Rep) and a logger which are injected via ninject.
public class HomeController : BaseController
{
private IValidationDomain _validDomain;
private IFileDomain _fileDomain;
// this is a big constructor, but I need to load this up somewhere to keep things mockable
public HomeController(IRepository rep, ILogger logger, IFileHelper fileHelper, IFileZipper fileZipper)
: base(rep, logger)
{
_fileDomain = new FileDomain(fileHelper, fileZipper);
_validDomain = new ValidationDomain(ModelState, logger, fileHelper, fileZipper);
}
public ActionResult NewJob(Job job)
{
// Job and FileQueue are DTOs
// Concerned this flow control is still too much "work" for the controller but seems unavoidable
if (_validDomain.IsValidJob(job, Request.Files))
{
FileQueue fq = new FileQueue(job, Request.Files[0]);
fq.Path = _fileDomain.SaveFile(Request.Files[0], job.JobId.ToString());
if (_fileDomain.IsZipFile(fq.Path) && _validDomain.IsValidZipFile(fq))
{
if (_fileDomain.ZipHasPassword(fq.Path))
{
return RedirectToAction("EnterPassword", new { jobId = job.JobId });
}
else
{
string unzipPath = _fileDomain.UnzipFile(fq.Path);
if(_validDomain.IsValidFile(job, unzipPath))
{
fq.Path = unzipPath;
}
}
}
Rep.Update(fq);
Rep.SaveChanges();
return RedirectToAction("FieldMapping", new { jobId = job.JobId });
}
string errors = string.Join(" ", ModelState.Values.SelectMany(v => v.Errors.Select(b => b.ErrorMessage)));
return RedirectToAction("Error", "Error", new { errorMessage = errors });
}
}
Here's my Service layer. I've called them "Domains" because in actuality we already have a "Service" project in the application which is an actual Windows service.
public class ValidationDomain : IValidationDomain
{
private ModelStateDictionary _modelState;
private ILogger _logger;
private IFileHelper _fileHelper;
private IFileZipper _fileZipper;
public ValidationDomain(ModelStateDictionary modelState, ILogger logger, IFileHelper fileHelper, IFileZipper fileZipper)
{
_modelState = modelState;
_logger = logger;
_fileZipper = fileZipper;
_fileHelper = fileHelper;
}
public bool IsValidFile(Job job, string path)
{
if (!_fileHelper.FileHasValidExtension(path))
{
_logger.Warn("Invalid fileExtension given for job {0}", job.JobId);
_modelState.AddModelError("ValidExtension", "Job must have a valid extension");
}
return _modelState.IsValid;
}
public bool IsValidJob(Job job, HttpFileCollectionBase files)
{
if (string.IsNullOrEmpty(job.Description))
{
_logger.Warn("Validation failed for job {0}", job.JobId);
_modelState.AddModelError("JobDescruption", "Job requires a description");
}
if (files.Count != 1)
{
_logger.Warn("File validation failed for job {0}", job.JobId);
_modelState.AddModelError("MultipleFiles", "Job must have exactly one file");
}
IsValidFile(job, files[0].FileName);
return _modelState.IsValid;
}
public bool IsValidZipFile(FileQueue fq)
{
if (!_fileZipper.ZipContainsSingleFile(fq.Path))
{
_logger.Warn("File {0} is a zip with multiple files", fq.FileId);
_modelState.AddModelError("MultipleZip", "Zip files must have exactly one file");
}
return _modelState.IsValid;
}
}
public class FileDomain : IFileDomain
{
private IFileHelper _fileHelper;
private IFileZipper _fileZipper;
private string _workingFolder = ConfigurationManager.AppSettings["WorkingFolder"];
public FileDomain(IFileHelper fileHelper, IFileZipper fileZipper)
{
_fileHelper = fileHelper;
_fileZipper = fileZipper;
}
// these two feel wrong - why not use a FileZipper directly in the controller?
public bool IsZipFile(string path)
{
return _fileHelper.FileHasZipExtension(path);
}
public bool ZipHasPassword(string path)
{
return _fileZipper.ZipHasPassword(path);
}
public string UnzipFile(string path)
{
string fileName = _fileZipper.UnzipFile(path);
string dir = _fileHelper.GetFolderFromPath(path);
return _fileHelper.GetSaveFilePath(dir, fileName);
}
public string SaveFile(HttpPostedFileBase file, string jobId)
{
string path = _fileHelper.GetSaveFilePath(string.Format(@"{0}\{1}\", _workingFolder, jobId), file.FileName);
file.SaveAs(path);
return path;
}
}
These in turn are dependent on a "business" layer. I've separated this out into another project because these are used elsewhere - such as the aforementioned Windows service.
public class FileHelper : IFileHelper
{
public List<string> ValidFileExtensions
{
get
{
List<string> extensions = new List<string>(GetAppSettings("ValidExtensions"));
extensions.AddRange(GetAppSettings("ZipExtensions"));
return extensions;
}
}
public List<string> ZipExtensions
{
get
{
return new List<string>(GetAppSettings("ZipExtensions"));
}
}
public string GetFolderFromPath(string path)
{
return Path.GetDirectoryName(path);
}
public bool FileHasValidExtension(string path)
{
return ValidFileExtensions.Any(item => path.EndsWith(item, StringComparison.OrdinalIgnoreCase));
}
public bool FileHasZipExtension(string path)
{
return ZipExtensions.Any(item => path.EndsWith(item, StringComparison.OrdinalIgnoreCase));
}
public string GetSaveFilePath(string folder, string fileName)
{
if (!Directory.Exists(folder))
{
Directory.CreateDirectory(folder);
}
return Path.Combine(folder, fileName);
}
private string[] GetAppSettings(string key)
{
string appSettingsValue = ConfigurationManager.AppSettings[key];
if (string.IsNullOrEmpty(appSettingsValue))
{
return new string[0];
}
return appSettingsValue.Split(',');
}
}
public class FileZipper : IFileZipper
{
public bool ZipHasPassword(string path)
{
return !ZipFile.CheckZipPassword(path, "");
}
public bool ZipContainsSingleFile(string path)
{
using (ZipFile zip = ZipFile.Read(path))
{
return zip.Count == 1;
}
}
public string UnzipFile(string path, string password = "")
{
using (ZipFile zip = ZipFile.Read(path))
{
zip.Password = password;
zip[0].Extract(ExtractExistingFileAction.OverwriteSilently);
return zip[0].FileName;
}
}
}
