How to update this class/method to improve Code Metrics

Question

This was my Initial Method

public String GetAllDocuments(string url,int pager =0)
{
    if (SessionInfo.IsAdmin)
    {
        ReportHandler dal = new ReportHandler();
        var documents = dal.FetchDocumentsList(SessionInfo.ClientID, pager);
        string documentsDataJSON = JsonConvert.SerializeObject(documents);

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

Visual Studio shows the following Code Metrics:-

Member: GetAllDocuments(string, int) : string
Maintainability Index: 67
Cyclomatic Complexity: 2
Class Coupling: 7
Lines of Code: 7

So in order to improve it, I modified my method as below:-

public String GetAllDocuments(string url,int pager =0)
{
    ReportHandler dal = new ReportHandler();
    var documents = dal.FetchDocumentsList(SessionInfo.ClientID, pager);
    //moved the JSON Conversion to Separate class
    string documentsDataJSON = JsonHandler<T>.ConvertToJSON(documents);
    return documentsDataJSON;
}

But still it shows the Code Metrics as

Member: GetAllDocuments(string, int) : string
Maintainability Index: 72
Cyclomatic Complexity: 1
Class Coupling: 5
Lines of Code: 5

I can't submit this, unless the Maintainability Index is 90+.

What else can I do to improve Code Metrics.

Also, I am thinking for such small things creating separate method/class isn't it overhead

Answer 1

If you interested in just make Maintainability Index better read how this is calculated: http://www.projectcodemeter.com/cost_estimation/help/GL_maintainability.htm

You can see that it depends mostly on number of lines of code.

If want good design, stop using concrete implementations, use one of SOLID principles - Dependency Inversion. For example, instead of using:

ReportHandler dal = new ReportHandler();

You shoud use something like:

IReportHandler dal = reportHandlerFactory.GetReportHandler();
// where reportHandlerFactory is IReportHandlerFactory which is also 
// make dependency on interface but not on concrete class 

Or even better is to inject such things using DI container.

Answer 2

You should often encapsulate any creation of objects in a factory interface, an instance of which you pass to the constructor of the class that wants to use it.

Thus your class would have a constructor and field like so:

public MyClass(IReportHandlerFactory reportHandlerFactory)
{
     if (reportHandlerFactory == null)
         throw new ArgumentNullException(nameof(reportHandlerFactory));

     _reportHandlerFactory = reportHandlerFactory;
}

private readonly IReportHandlerFactory _reportHandlerFactory;

This is known as dependency injection, specifically constructor injection.

To do this effectively you would probably also want to create an interface for your ReportHandler class. Then the factory interface would go something like this:

public interface IReportHandlerFactory
{
    IReportHandler Create();
}

And your report handler interface like this:

public interface IReportHandler
{
    IEnumerable<Document> FetchDocumentsList(Guid clientID, int pager);
}

...and so on. Hopefully you get the idea.

You should also split up your method, maybe something like this:

public String GetAllDocuments(string url,int pager =0)
{
    if (SessionInfo.IsAdmin)
    {
        return documentsData(_reportHandlerFactory, SessionInfo.ClientID, page);
    }
    else
    {
        return "Sorry!! You are not authorized to perform this action";
    }
}

private static String documentsData(
    IReportHandlerFactory reportHandlerFactory, 
    Guid clientID,
    int pager)
{
    IReportHandler dal = reportHandlerFactory.Create();

    var documents = dal.FetchDocumentsList(clientID, pager);
    string documentsDataJSON = JsonConvert.SerializeObject(documents);

    return documentsDataJSON;
}

NOTE: To be honest, this question is really a better fit for https://codereview.stackexchange.com/

I highly recommend the following books on this subject:

Clean Code: A Handbook of Agile Software Craftsmanship (Robert C. Martin)

and

Dependency Injection in .NET (Mark Seemann)

or Mark Seemann's blog.

Answer 3

Take a maintainability index as a tool, not a goal.

There's no point in increasing the maintainability index if you don't realize why your code wasn't maintainable in the first place. You'll just keep moving code around to satisfy the index, without ever understanding what you're doing.

Your question shouldn't be

How to update this class/method to improve Code Metrics?

but

How to improve the maintainability of this class?

I see a number of problems with it right now :

• Dependencies are implicit, i.e. you're new'ing things up directly inside the method. This makes the code less flexible, composable and readable.

  Passing the ReportHandler dependency in to GetAllDocuments() or the class constructor instead would be better.

• It is poorly testable. If ReportHandler were an interface (or an abstract class) instead, you could substitute it with a fake report handler in tests for GetAllDocuments() for improved performance and testing. Note that you do not have to use a Factory to do that, a simple interface with one real implementation and a test implementation are good enough.

• Parameter url is not used.

• SessionInfo.IsAdmin is a kind of magic shorthand that is prone to the same range of problems as the above. If you're in a controller, it's no big deal but if you're supposed to be in a business layer, this will hamper testability and maintainability.