1
\$\begingroup\$

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;
        }
    }
}
\$\endgroup\$
  • 2
    \$\begingroup\$ FWIW I glanced at your controller and it doesn't look fat to me. It's just routing to views and calling into your service. That's exactly what a controller should be doing IMO. \$\endgroup\$ – RubberDuck Jun 16 '16 at 10:02
  • \$\begingroup\$ @RubberDuck Thanks. Before I started work on this, that controller action was 50 lines long and called out to several private methods in the class 0_o \$\endgroup\$ – Matt Thrower Jun 16 '16 at 10:19
1
\$\begingroup\$

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.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.