Constructors should be simple

Whole day of yesterday I’ve spent debugging some “random” error that came up third time during this week. The issue did go away after some tribal dances around the code, but two other instances did not get resolved easier. The biggest problem was that only one of the developers on the team did get the issue, and everybody else were not affected. And the same happened in the new deployment. And the issue was quite major – the whole site did not load. Nothing beyond the login screen. And it was impossible to hook in the debugger, it was bypassing all the breakpoints.

I would’ve gone through the issue and try debugging it, by my machine was not affected by the problem. I could not reproduce the issue locally. But on my home machine the issue came up. Strange I through, but was glad it came up – means I can dig my teeth deep in the problem and debug it.

Not going to tell you all the details about debugging session, but the resolution is worth of your attention. It turned out that one of our controllers (we are talking about Asp.Net MVC4 application) had a constructor that did not just received the dependencies, as it should by DI rules, but also done some work. Here is the controller:

    public ShipmentsController(
        IQueryHandler<FindEntityQuery<Shipment>, Shipment> shipmentLocatorHandler, 
        IQueryHandler<AllEntitiesQuery<Shipment>, IEnumerable<Shipment>> shipmentsList, 
        IQueryHandler<FindMinorEquipmentForSiteQuery, IEnumerable<MinorEquipmentType>> findMinorEquipmentForSite, 
        IQueryHandler<FindEntityQuery<ShipmentItem>, ShipmentItem> findShipmentItem, 
        IQueryHandler<FindAssetsForSiteQuery, IEnumerable<Asset>> findAssetsForSite, 
        IQueryHandler<FindEntityQuery<Asset>, Asset> findAsset, 
        IQueryHandler<FindAssetsSerialNumbersAtSiteQuery, IEnumerable<string>> findSerialNumbers, 
        IQueryHandler<FindAssetsDistinguishingNumberAtSiteQuery, IEnumerable<string>> findDistinguishingNumbers, 
        IQueryHandler<FindFilteredAssetsForSiteQuery, IEnumerable<Asset>> findFilteredAssetsForSite, 
        IQueryHandler<ShipmentAssetsWithClashingMovementsQuery, ShipmentAssetMovementClashes> shipmentAssetsWithClashes, 
        IQueryHandler<FindShipmentExchangeRateQuery, ShipmentExchangeRate> shipmentExchangeRate, 
        IQueryHandler<FindEntityQuery<Tuple<int, bool>>, Tuple<int, bool>> shipmentSent, 
        IQueryHandler<FindShipmentFreeTextItemQuery, ShipmentsFreeTextItem> shipmentsFreeTextItem, 
        ICommandHandler<UpdateAssetInShipmentCommand> updateAssetInShipmentCommandHandler, 
        IFileList fileList, 
        IFileDownload fileDownload, 
        IConfiguration configuration)
    {
        this.shipmentLocatorHandler = shipmentLocatorHandler;
        this.shipmentsList = shipmentsList;
        this.findMinorEquipmentForSite = findMinorEquipmentForSite;
        this.findShipmentItem = findShipmentItem;
        this.findAssetsForSite = findAssetsForSite;
        this.findAsset = findAsset;
        this.findSerialNumbers = findSerialNumbers;
        this.findDistinguishingNumbers = findDistinguishingNumbers;
        this.findFilteredAssetsForSite = findFilteredAssetsForSite;
        this.shipmentAssetsWithClashes = shipmentAssetsWithClashes;
        this.shipmentExchangeRate = shipmentExchangeRate;
        this.shipmentSent = shipmentSent;
        this.shipmentsFreeTextItem = shipmentsFreeTextItem;
        this.updateAssetInShipmentCommandHandler = updateAssetInShipmentCommandHandler;

        var uploadContainer = configuration.GetUploadDirectory();
        var fileStorageLocationWithContainer = uploadContainer + "/Customisations/images/logo.png";
        var logoFile = fileList.LatestFilename(fileStorageLocationWithContainer);
        logoFileUrl = logoFile != null
                          ? fileDownload.DownloadFileUrl(logoFile, 1440)
                          : "/content/images/Logo-Faded-Edges-Small.png";
    }

What a bloater you might say. And you will be absolutely right!! The number of dependencies is unacceptable. But this is not the problem I found. Have a look on this part:

        var uploadContainer = configuration.GetUploadDirectory();
        var fileStorageLocationWithContainer = uploadContainer + "/Customisations/images/logo.png";
        var logoFile = fileList.LatestFilename(fileStorageLocationWithContainer);
        logoFileUrl = logoFile != null
                          ? fileDownload.DownloadFileUrl(logoFile, 1440)
                          : "/content/images/Logo-Faded-Edges-Small.png";

Here we do some work in constructor. We want to give some value to logoFileUrl field, so later on that can be used by controller actions. The code itself is not too bad, but it must not be done in controller.

What can go wrong?” you might ask. Simples: exceptions can be thrown, every second line above can throw an exception. And that exactly what happened in my case. fileList.LatestFilename was throwing exception, because it could not find Azure Blob Storage container.

How did this affect only some developers? Somebody did delete all the local containers, and all failed.

How did his affect the whole site? We are using MvcSitemapProvider and when it creates the menu it walks through all controllers and creates instances of them. And of course it could not create the specific controller, because an exception was thrown in the constructor. And SiteMapProvider did swallow the original exception and threw one of it’s own with no trace of the original problem.

So, kids. Keep your constructors simple, make sure the objects can always be created.

Now my task is to investigate how we can prevent these issue happening through tests or analysis on build server. Possibly I’ll have to create a test that through reflection finds all the controllers, and tell DI to create the instances of the controllers. That sounds like a huge integration test that might take a day to write and good couple minutes to execute. Alternatively I’ll look on FxCop analysis and try to get it to throw warnings on similar issues. Any other ideas?