ASP.NET C# MVC thin controller for file validation with service layer

Question

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;
        }
    }
}

Answer 1

Agreeing with the comments, your controller action doesn't look too fat to me, but maybe you could make it appear more terse like so:

public class HomeController : BaseController
{
    private IValidationDomain _validDomain;
    private IFileDomain _fileDomain;

    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)
    {
        if (_validDomain.IsValidJob(job, Request.Files))
        {
            var fq = new FileQueue(job, Request.Files[0]) 
            {
                Path = _fileDomain.SaveFile(Request.Files[0], job.JobId.ToString()) 
            };

            if (IsZipFile(fq))
            {
                ActionResult enterPassword;
                if (RequiresPassword(fq, job, out pwd))
                {
                    return enterPassword;
                }

                fq.Path = GetUnzipPath(fq);
            }

            Rep.Update(fq);
            Rep.SaveChanges();
            return RedirectToAction("FieldMapping", new { jobId = job.JobId });
        }

        var errors = ModelState.Values.SelectMany(v => v.Errors);
        var errorsStr = string.Join(" ", errors.Select(b => b.ErrorMessage)));
        return RedirectToAction("Error", "Error", new { errorMessage = errors });
    }

    private bool IsZipFile(FileQueue queue) 
    {
        return _fileDomain.IsZipFile(queue.Path) && 
               _validDomain.IsValidZipFile(queue);
    }

    private bool RequiresPassword(FileQueue queue, Job job, out ActionResult result)
    {
        result = null;
        if (_fileDomain.ZipHasPassword(queue.Path))
        {
            result = RedirectToAction("EnterPassword", new { jobId = job.JobId });
            return true;
        }               

        return false;   
    }

    private string GetUnzipPath(FileQueue queue)
    {
        var unzipPath = _fileDomain.UnzipFile(queue.Path);
        var isValid = _validDomain.IsValidFile(job, unzipPath);
        return isValid ? unzipPath : queue.Path;
    }
}

The added benefit is that the code becomes more self-documenting.

Edit: I just noticed your comment about 50 lines and calling out to private methods, but in this case, I don't think private methods are undesirable. Separating out concerns into more intuitively named code blocks is a GOOD thing, IMO.