Factory for report handlers

Question

Below is my try.

 //DAL
 //This is my interface 
 namespace iPublisher.DAL.Interfaces
 {
   public interface IReportHandler
   { 
       IEnumerable<DocumentMapper> FetchDocuments(int clientId, int pager);
    }
 }

//my concrete class
public class ReportHandler : IReportHandler
{
   public IEnumerable<DocumentMapper> FetchDocuments(int clientId, int pager)
   {
       //LINQ
   }
}

//Factory Interface 
namespace iPublisher.DAL.Interfaces
{
public interface IReportFactory
{
    IReportHandler Create(int factoryId);
}
}

//Factory class
namespace iPublisher.DAL.Factory
{
public class ReportFactory : IReportFactory
{
    private IReportHandler reportObj;

    public IReportHandler Create(int factoryId)
    {
        switch (factoryId)
        {
            case 1:
                reportObj = new ReportHandler();
                return reportObj;
            default:
                throw new ArgumentException("");
        }


     }
 }
}


//UI Layer

namespace iPublisher.Controllers
{
[Authorize]
[SessionExpire]
public class ReportsController : Controller
{
      ReportFactory factObj = new ReportFactory();

    public string GetAllDocuments(string url,int pager =0)
    {
        if (SessionInfo.IsAdmin)
        {
            var documentsDataJSON = GetDocumentIntoJson(SessionInfo.ClientID, pager);

            return documentsDataJSON;
        }
        else
        {
            return "Sorry!! You are not authorized to perform this action";
        }
    }

    private string GetDocumentIntoJson(int clientId, int pager)
    {

        IReportHandler reportObj = factObj.Create(1);

        var documents = reportObj.FetchDocumentsList(clientId, pager);

        string documentsDataJSON = JsonConvert.SerializeObject(documents);

        return documentsDataJSON;
    }



 }

}

Code Metrics still showing Maintainability Index to 60.

What I am missing here or how can I improve my implementation?

Answer 1

Not sure if that's going to improve the maintainability index, but

• You're misimplementing Inversion of Control. IoC is when your dependency (namely, IReportHandler) is given to you from outside the class. It isn't the case here. It defeats the whole purpose of having an abstract dependency instead of a concrete one.

  Read up on Inversion of Control and Dependency Injection and how they can improve code maintainability and testability. Don't use them unless it is an educated choice to do so.

• You're using a Factory where it has no benefit.

  Read up on the different kinds of factories (Factory from DDD, Factory method and Abstract Factory from Go4). Don't use Factories except where there's a clear advantage to doing so.

• The code is too complicated for what it does.

  Why the factoryId, switch statement and magic value 1?

  The url parameter is never used.

  Why the intermediate variables documentsDataJSON when you could just return <expression> and save one line of code for more clarity?

• Naming is awkward and violates the Principle of Least Surprise.

  Why would you expect a ReportHandler to Create a report, while ReportFactory creates a... ReportHandler?

  Why does FetchDocuments() not return a list of Documents, but a list of DocumentMappers?

Don't try to mimic code samples or game a maintainability analysis tool. Learn about the philosophy behind the big design principles/patterns and decide for yourself if, where and how to apply them.

Answer 2

Some observations:

• You shouldn't be creating the factory inside the class that uses it. That defeats its purpose! Instead, you should pass the factory to that class's constructor.

• You shouldn't be using a factoryId parameter to control creation of the factory. The thing using the factory shouldn't need to know what kind of factory to create - it makes it too strongly coupled. Instead, the code that passes the factory to the consuming class's constructor makes that decision.

So your ReportsController would ideally look more like this:

public class ReportsController : Controller
{
    public ReportsController(IReportHandlerFactory reportHandlerFactory)
    {
         if (reportHandlerFactory == null)
             throw new ArgumentNullException(nameof(reportHandlerFactory));

        _reportHandlerFactory = reportHandlerFactory;        
    }

    private readonly IReportHandlerFactory _reportHandlerFactory;
}

Then when the ReportsController class is created, you would pass to its constructor the appropriate implementation of IReportHandlerFactory.

However, in the case that your class must be created with a default constructor (which I believe is the case for a Controller), then instead of using a constructor parameter you would need a different approach.

How to do this varies depending on the specific framework, but see here for an example for ASP.Net MVC controllers.

This blog post may also be useful.