Controller to update a catalogue through an uploaded Excel file

Question

The goal of following MVC controller code is to achieve following workflow:

• User exports catalog (either with items of type Spread or of type Consumables) to Excel;
• User makes changes in exported Excel file;
• User uploads catalog to web;
• User preview his changes before committing to database.

Because catalog can be either List<Spread> or List<Consumable> I have every method/action written twice. What are the proper ways to avoid this situation and make code more clean, readable and maintainable?

public class PricebookBulkUpdateController : BaseController
{
    #region fields

    private readonly string _sessionImportedDenormalizedCatalogueItemsList;
    private readonly string _sessionImportedDenormalizedCatalogueType;

    private readonly CatalogueExportImportProcessor _exportImportProcessor;
    private readonly CatalogueComparator _comparator;
    private readonly ICatalogueServices _catalogueServices;

    private readonly Dictionary<int, string> _rigsDictionary;
    private readonly Dictionary<int, string> _vendorsDictionary;
    private readonly Dictionary<int, string> _unitsDictionary;

    private enum CatalogueType
    {
        Spreads,
        Consumables
    }

    #endregion

    public PricebookBulkUpdateController(ICatalogueServices catalogueServices, IBusinessServices businessServices)
    {
        var asposeLicFile = HttpContext.Server.MapPath("~/App_Data//") + "Aspose.Cells.lic";
        _exportImportProcessor = new CatalogueExportImportProcessor(asposeLicFile);
        _catalogueServices = catalogueServices;

        _rigsDictionary = businessServices.GetAllRigs().ToDictionary(x => x.Id, x => x.Name);
        _vendorsDictionary = businessServices.GetAllVendors().ToDictionary(x => x.Id, x => x.Name);
        _unitsDictionary = businessServices.GetAllUnitsOfMeasure().ToDictionary(x => x.Id, x => x.Name);

        _sessionImportedDenormalizedCatalogueItemsList = "ImportedDenormalizedCatalogueItemsList" +
                                                            _workContext.User.UserName;
        _sessionImportedDenormalizedCatalogueType = "SessionImportedDenormalizedCatalogueType" +
                                                    _workContext.User.UserName;

        _comparator = new CatalogueComparator(_rigsDictionary, _vendorsDictionary, _unitsDictionary);
    }

    public ActionResult Index()
    {
        return View();
    }

    public ActionResult PreviewSpreadItemsInDatabaseAndImportedExcelComparisonResults(int engineeringGroupId)
    {
        if (Session[_sessionImportedDenormalizedCatalogueItemsList] == null)
            throw new Exception("Unable to get imported results in preview method");

        var itemsInDb = _catalogueServices.GetSpreadCatalogue(engineeringGroupId);
        var itemsInExcel = (List<DenormalizedCatalogueItem>) Session[_sessionImportedDenormalizedCatalogueItemsList];

        var diff = _comparator.Compare(itemsInDb, itemsInExcel);

        ViewBag.Diff = diff;

        return PartialView("_PreviewComparisonPartial");
    }

    public ActionResult PreviewConsumablesItemsInDatabaseAndImportedExcelComparisonResults(int engineeringGroupId)
    {
        if (Session[_sessionImportedDenormalizedCatalogueItemsList] == null)
            throw new Exception("Unable to get imported results in preview method");

        var itemsInDb = _catalogueServices.GetConsumablesCatalogue(engineeringGroupId);
        var itemsInExcel = (List<DenormalizedCatalogueItem>) Session[_sessionImportedDenormalizedCatalogueItemsList];

        var diff = _comparator.Compare(itemsInDb, itemsInExcel);

        ViewBag.Diff = diff;

        return PartialView("_PreviewComparisonPartial");
    }

    public ActionResult CommitImportedCatalogueChangesToDatabase(int catalogueType, int engineeringGroupId)
    {
        var type = (CatalogueType) catalogueType;

        if (type == CatalogueType.Spreads)
        {
            CommitImportedSpreadCatalogueChangesToDatabase(engineeringGroupId);
        }
        if (type == CatalogueType.Consumables)
        {
            CommitImportedConsumablesCatalogueChangesToDatabase(engineeringGroupId);
        }

        Session[_sessionImportedDenormalizedCatalogueItemsList] = null;
        Session[_sessionImportedDenormalizedCatalogueType] = null;

        return RedirectToAction("Index");
    }

    private void CommitImportedSpreadCatalogueChangesToDatabase(int engineeringGroupId)
    {
        var itemsInDb = _catalogueServices.GetSpreadCatalogue(engineeringGroupId);
        var itemsInExcel = (List<DenormalizedCatalogueItem>) Session[_sessionImportedDenormalizedCatalogueItemsList];

        var diff = _comparator.Compare(itemsInDb, itemsInExcel);

        _catalogueServices.BulkDelete(diff[CatalogueComparator.State.Removed]);
        _catalogueServices.BulkInsert(diff[CatalogueComparator.State.Added]);
        _catalogueServices.BulkUpdate(diff[CatalogueComparator.State.Modified]);
    }

    private void CommitImportedConsumablesCatalogueChangesToDatabase(int engineeringGroupId)
    {
        var itemsInDb = _catalogueServices.GetSpreadCatalogue(engineeringGroupId);
        var itemsInExcel = (List<DenormalizedCatalogueItem>) Session[_sessionImportedDenormalizedCatalogueItemsList];

        var diff = _comparator.Compare(itemsInDb, itemsInExcel);

        _catalogueServices.BulkDelete(diff[CatalogueComparator.State.Removed]);
        _catalogueServices.BulkInsert(diff[CatalogueComparator.State.Added]);
        _catalogueServices.BulkUpdate(diff[CatalogueComparator.State.Modified]);
    }

    #region export and import to excel

    public FileResult ExportSpreadItemsCatalogueToExcel(int engineeringGroupId)
    {
        var spreads = _catalogueServices.GetSpreadCatalogue(engineeringGroupId);
        var stream = _exportImportProcessor.SaveToStream(spreads.ToDenormalizedCatalogueItemsList(_rigsDictionary));
        return File(stream, "application/vnd.openxmlformats-officedocument.spreadsheetml.sheet",
            $"WCET-SpreadCatalogue-On{$"{DateTime.Now:yyyyMMddhhmm}"}.xlsx");
    }

    public FileResult ExportConsumablesItemsCatalogueToExcel(int engineeringGroupId)
    {
        var items = _catalogueServices.GetConsumablesCatalogue(engineeringGroupId);
        var stream =
            _exportImportProcessor.SaveToStream(items.ToDenormalizedCatalogueItemsList(_vendorsDictionary,
                _unitsDictionary));
        return File(stream, "application/vnd.openxmlformats-officedocument.spreadsheetml.sheet",
            $"WCET-ConsumablesCatalogue-On{$"{DateTime.Now:yyyyMMddhhmm}"}.xlsx");
    }

    public ActionResult PostDenormalizedExcelCatalogue(HttpPostedFileBase file, int engineeringGroupId)
    {
        if (file == null)
            throw new Exception("Cannot get posted file in PostDenormalizedExcelCatalogue method");

        var fileExtension = Path.GetExtension(file?.FileName);
        if (fileExtension != null && (!fileExtension.ToLower().Contains("xls")))
            ModelState.AddModelError("model.file", "Error! Please select another excel file.");

        if (file.FileName.Contains("SpreadCatalogue"))
        {
            Session[_sessionImportedDenormalizedCatalogueType] = CatalogueType.Spreads;
        }
        else if (file.FileName.Contains("ConsumablesCatalogue"))
        {
            Session[_sessionImportedDenormalizedCatalogueType] = CatalogueType.Consumables;
        }
        else
        {
            ModelState.AddModelError("model.file", "Error! Please upload file with its original exported name.");
        }

        if (!ModelState.IsValid)
        {
            return
                Json(
                    new StructedJsonResult(NotifyType.ValidationError, null,
                        new JavaScriptSerializer().Serialize(ModelState.SerializeErrors())),
                    JsonRequestBehavior.AllowGet);
        }

        var dataTable = _exportImportProcessor.ReadFromStream(file.InputStream);
        var importedItems = dataTable.ToDenormalizedCatalogueItemsList();

        Session[_sessionImportedDenormalizedCatalogueItemsList] = importedItems;

        if ((CatalogueType) Session[_sessionImportedDenormalizedCatalogueType] == CatalogueType.Spreads)
        {
            return RedirectToAction("PreviewSpreadItemsInDatabaseAndImportedExcelComparisonResults",
                new {engineeringGroupId});
        }
        if ((CatalogueType) Session[_sessionImportedDenormalizedCatalogueType] == CatalogueType.Consumables)
        {
            return RedirectToAction("PreviewConsumablesItemsInDatabaseAndImportedExcelComparisonResults",
                new {engineeringGroupId});
        }

        throw new Exception(
            "Cannot identify type of catalogue in posted file in PostDenormalizedExcelCatalogue method");
    }

    #endregion
}

Answer 1

You may consider the Strategy Pattern (or a form of the Strategy Pattern). You will still have every method written twice but they will be segregated to make it easier to maintain and unit test.

public interface ICatalog
{
    void Preview(int engineeringGroupId);
    void Commit(int engineeringGroupId);
    FileResult Export();
    ActionResult Post(HttpPostedFileBase file, int engineeringGroupId)

}

public class Spread : ICatalog
{
    public void Preview(int engineeringGroupId)
    {
        if (Session[_sessionImportedDenormalizedCatalogueItemsList] == null)
            throw new Exception("Unable to get imported results in preview method");

        var itemsInDb = _catalogueServices.GetSpreadCatalogue(engineeringGroupId);
        var itemsInExcel = (List<DenormalizedCatalogueItem>)Session[_sessionImportedDenormalizedCatalogueItemsList];

        var diff = _comparator.Compare(itemsInDb, itemsInExcel);

        ViewBag.Diff = diff;
    }

    public void Commit(int engineeringGroupId)
    {
        var itemsInDb = _catalogueServices.GetSpreadCatalogue(engineeringGroupId);
        var itemsInExcel = (List<DenormalizedCatalogueItem>)Session[_sessionImportedDenormalizedCatalogueItemsList];

        var diff = _comparator.Compare(itemsInDb, itemsInExcel);

        _catalogueServices.BulkDelete(diff[CatalogueComparator.State.Removed]);
        _catalogueServices.BulkInsert(diff[CatalogueComparator.State.Added]);
        _catalogueServices.BulkUpdate(diff[CatalogueComparator.State.Modified]);
    }

    public FileResult Export()
    {
        .
        .
        .
    }

    public ActionResult Post(HttpPostedFileBase file, int engineeringGroupId)
    {
        .
        .
        .
    }
}

public class Consumable : ICatalog
{
}

The Preview method in your controller...

public ActionResult Preview(int engineeringGroupId)
{
    // Not sure if you would always call both but you can always use an if statement if you want
    _spread.Preview(engineeringGroupId);
    _consumable.Preview(engineeringGroupId);
    return PartialView("_PreviewComparisonPartial");
}

You might also consider adding a method to the interface to identify which class to call. This will eliminate the if statements in your controller.

bool CanHandle(CatalogType type);

You would implement this in the Spread class like this...

public bool CanHandle(CatalogType type)
{
    return type == CatalogType.Spread;
}

Then in your controller you call the CanHandle to determine which set of methods to call.

You can also look more closely at the differences and only code the differences in the separate classes. For example, the Commit is very similar so instead of duplicating all the code the interface could do this...

string GetSessionVariable();

And in your Spread implementation...

public string GetSessionVariable()
{
    reutrn _sessionImportedDenomalizedCatalogueItemsList;
}

Then your Commit in your controller would read...

private void CommitImportedCatalogueChangesToDatabase(int engineeringGroupId)
{
    var itemsInDb = _catalogueServices.GetSpreadCatalogue(engineeringGroupId);
    var sessionVariable = _catalog.GetSessionVariable();   // _catalog would either be an instance of `Spread` or `Consumable`
    var itemsInExcel = (List<DenormalizedCatalogueItem>)Session[sessionVariable];

    var diff = _comparator.Compare(itemsInDb, itemsInExcel);

    _catalogueServices.BulkDelete(diff[CatalogueComparator.State.Removed]);
    _catalogueServices.BulkInsert(diff[CatalogueComparator.State.Added]);
    _catalogueServices.BulkUpdate(diff[CatalogueComparator.State.Modified]);
}

You may end up duplicating some code because I noticed some code is called in both cases. But I think this is an acceptable trade-off considering the maintainability of segregation.

Another thing to consider is whether the classes can be written so that they are not dependent on the Http "components", such as the Session variable. If you can keep those things out of the classes it will be easier to unit test.

There are lots of other refactoring's that can be done here but this should give you a good idea of where I am going with this suggestion.