Why I cannot understand this code?

Preface

Hi! My name is Krzysiek. I am a .NET developer. When writing code, I follow the rule, which assumes that the code should be as simple as possible and understandable for the rest of the team. I have seen many lines of code that could be write better, in a more understandable way. One time I thought: If such problems exist, why not share with other programmers your knowledge?

What I'm going to write is not new, but I think that it is always worth to pay attention to this (especially less experienced programmers) how important it is to write clean code. In my opinion, it is easier to learn by example than only by reading the theory, so I will focus on comparing two solutions to one problem.

Introduction

How often you wondering what this code is responsible? Why it not working or why it working good although it should not? How often you must use debugger to find code which do specific action?

I once heard such a story:

One developer analyze code other developer. He can’t understand what that code doing, so he asked the author about it. After a few minutes developer who wrote this code said: “On the day I wrote this code I and God knew what it was doing. Today only God knows”.

Sounds familiar? :) I think that many programmers have come across similar situations. Can such situations be avoided? In my opinion: yes, but you must start write code like as a story, which can be understand for person which is an expert (not technical, not developer) in the field for which the software is being created.

To understand this article you need to have at least basic knowledge of:

• C#
• LINQ
• Object-oriented programming

 

• Download SimpleReport.zip - 71.5 KB

 

Problem to solve

Suppose we need to create a module that will create simple reports. Our report have:

• name
• aggregate
• aggregate value

For example:

Company vs price

Company 1	12345
Company 2	45678
Company 3	10928

The report will be generated based on selected month and type of report.

Suppose we get the data to generate a report from a file. Row structure:

Month, Company name, City, Number of person, Number of days, Price.

We need to create four reports, but in the future there will be more reports, so our solution must be easy to maintain.

Ready? Let's go!

public class MyClass
{
        public Result Process(int type, int month, string name)
        {
            var list1 = GetDataForReport();
            var list2 = new List<string>();
            var r = new Result();
            if(type == 1)
            {
            r.Name = "Company vs price";
            r.Items = list1.Where(x => x.Month == month).GroupBy(x => x.CompanyName)
            .Select(x => new Item
            {
            ValueName = x.Key,
            Value = x.Sum(xx => xx.Price).ToString("F")
            }).ToList();
            }
            else if (type == 2)
            {
                r.Name = "Company vs city";
                r.Items = list1.Where(x => x.Month == month).GroupBy(x => x.CompanyName)
                .Select(x => new Item
                {
                    ValueName = x.Key,
                    Value = string.Join("; ", x.Select(xx => xx.City))
                }).ToList();
            }
            else if (type == 3)
            {
                r.Name = "City vs number of days";
                r.Items = list1.Where(x => x.Month == month).GroupBy(x => x.City)
                .Select(x => new Item
                {
                    ValueName = x.Key,
                    Value = x.Sum(xx => xx.NumberOfDays).ToString()
                }).ToList();
            }
            else if(type == 4)
            {
                r.Name = "Company vs number of person";
                r.Items = list1.Where(x => x.Month == month).GroupBy(x => x.CompanyName)
                .Select(x => new Item
                {
                    ValueName = x.Key,
                    Value = x.Sum(xx => xx.NumberOfPerson).ToString()
                }).ToList();
            }
            else
            {
                //r.Name = "";
                return null;
            }

            return r;
        }

        private IList<Row> GetDataForReport()
        {
            //... load data for example from excel file
        }
}

public class Result 
{     
    public string Name { get; set; }     
    public IList<Item> Items { get; set; } 
}

public class Item
{
    public string ValueName { get; set; }
    public string Value { get; set; }
}

public class Row
{
    public int Month { get; set; }
    public string CompanyName { get; set; }
    public string City { get; set; }
    public int NumberOfPerson { get; set; }
    public int NumberOfDays { get; set; }
    public decimal Price { get; set; }
}

We have written the code. We have made manual tests. Everything works as it should. Cool! We assign the task to code review and we are waiting for praise our work.

Code review

We have feedback about our report module. Oh no.. Other programmer have problems to understand our code. He sent to us list of questions:

1. What it is "MyClass", "Result"?
2. What does the "Process" method do?
3. What does mean type "1", "2", "3" or "4"?
4. Can the "type" parameter accept the value "5"?
5. Can the "month" parameter accept the value "15"?
6. Is the "name" parameter needed?
7. What does mean "list1", "list2", "r"?
8. What is the purpose of the "list2".
9. "//r.Name = "";" - Why is the code commented?
10. The section in the first "if" has no indentation. The code is unreadable.
11. In many places in "Process" method the code is repeated.
12. The method is large. In the future there will be more reports so it will be even bigger.

Oh no! I don't remember what does mean type "1", "2", "3" or "4"! I have to check the documentation.

Oh, I wrote this code and I don't remember what this type means, so how someone can know about this who has not seen the documentation?

Exactly! Code should be understandable for human, not only for computer!

Let's start again only smarter

Let's take a look at the end user's perspective. I have the data I want to analyze. I want to choose the report and month and see the data table. 

In the first step, consider what structure reflects the report seen by the end user.

public class Report
{
    public string ReportName { get; set; }
    public IList<ReportItem> Items { get; set; }
}

public class ReportItem
{
    public string ValueName { get; set; }
    public string Value { get; set; }
}

We define a range of data visible to the user.

public enum ReportType
{
    CompanyVsPrice = 1,
    CompanyVsCity = 2,
    CityVsNumberOfDays = 3,
    CompanyVsNumberOfPerson = 4
}

public enum Month
{
    January = 1,
    February = 2,
    March = 3,
    April = 4,
    May = 5,
    June = 6,
    July = 7,
    August = 8,
    September = 9,
    October = 10,
    November = 11,
    December = 12
}

Now let's get to the point, how to present in the simple way the report generation in the code? 

public class Reports
{
    public Report GenerateReport(ReportType reportType, Month month)
    {
        var dataForReport = GetDataForReport();
        var reportFactory = new ReportFactory(dataForReport, reportType, month);
        var report = reportFactory.GetReport();

        return report;
    }


    private IList<Row> GetDataForReport()
    {
        //... load data for example from excel file
    }
}

Wow! It is really simple to understand. Based on the data, report type and month, the factory returns the report.

Now let's focus on how this factory works.

internal class ReportFactory
{
    private readonly IList<Row> _dataForReport;
    private readonly ReportType _reportType;
    private readonly Month _month;

    public ReportFactory(IList<Row> dataForReport, ReportType reportType, Month month)
    {
        _dataForReport = dataForReport ?? new List<Row>();
        _reportType = reportType;
        _month = month;
        Validate();
    }

    public Report GetReport()
    {
        var report = new Report();
        IReportDefinition reportDefinition = GetReportDefinition();
        report.ReportName = reportDefinition.ReportName;
        report.Items = reportDefinition.GetReportItems(_dataForReport, _month);

        return report;
    }

    private IReportDefinition GetReportDefinition()
    {
        switch (_reportType)
        {
            case ReportType.CompanyVsPrice:
                return new CompanyVsPriceReportDefinition();
            case ReportType.CompanyVsCity:
                return new CompanyVsCityReportDefinition();
            case ReportType.CityVsNumberOfDays:
                return new CityVsNumberOfDaysReportDefinition();
            case ReportType.CompanyVsNumberOfPerson:
                return new CompanyVsNumberOfPersonReportDefinition();
            default:
                throw new NotImplementedException(string.Format("Report for type '{0}' is not implemented!", _reportType));
        }
    }

    private void Validate()
    {
        if(!Enum.IsDefined(typeof(Month), _month))
        {
            throw new ArgumentOutOfRangeException("month");
        }
    }
}

The factory based on the type gets the definition of the report. Then the report definition processes the input and returns the report data. It is a very simple and readable factory.

Now let's focus on report definitions.
Report definition has:

• report name
• aggregate function
• data processing function

internal interface IReportDefinition
{
    string ReportName { get; }
    Func<Row, string> Aggregate { get; }
    IList<ReportItem> GetReportItems(IList<Row> dataForReport, Month month);
}

internal class CompanyVsPriceReportDefinition : BaseReportDefinition, IReportDefinition
{
    public string ReportName { get { return "Company vs price"; } }

    public IList<ReportItem> GetReportItems(IList<Row> dataForReport, Month month)
    {
        var reportItems = base.GetReportBaseConditions(dataForReport, month)
        .Select(reportItem => new ReportItem
        {
            ValueName = reportItem.Key,
            Value = base.GetPriceAsString(reportItem.Sum(reportValue => reportValue.Price))
        }).ToList();

        return reportItems;
    }
}

internal class CompanyVsCityReportDefinition : BaseReportDefinition, IReportDefinition
{
    public string ReportName { get { return "Company vs city"; } }

    public IList<ReportItem> GetReportItems(IList<Row> dataForReport, Month month)
    {
        var reportItems = base.GetReportBaseConditions(dataForReport, month)
            .Select(reportItem => new ReportItem
            {
                ValueName = reportItem.Key,
                Value = base.GetAsString(reportItem.Select(reportValue => reportValue.City).ToList())
            }).ToList();

        return reportItems;
    }
}

internal class CityVsNumberOfDaysReportDefinition : BaseReportDefinition, IReportDefinition
{
    public string ReportName { get { return "City vs number of days"; } }

    public override Func<Row, string> Aggregate { get { return reportItem => reportItem.City; } }

    public IList<ReportItem> GetReportItems(IList<Row> dataForReport, Month month)
    {
        var reportItems = base.GetReportBaseConditions(dataForReport, month)
            .Select(reportItem => new ReportItem
            {
                ValueName = reportItem.Key,
                Value = reportItem.Sum(reportValue => reportValue.NumberOfDays).ToString()
            }).ToList();

        return reportItems;
    }
}

internal class CompanyVsNumberOfPersonReportDefinition : BaseReportDefinition, IReportDefinition
{
    public string ReportName { get { return "Company vs number of person"; } }

    public IList<ReportItem> GetReportItems(IList<Row> dataForReport, Month month)
    {
        var reportItems = base.GetReportBaseConditions(dataForReport, month)
            .Select(reportItem => new ReportItem
            {
                ValueName = reportItem.Key,
                Value = reportItem.Sum(reportValue => reportValue.NumberOfPerson).ToString()
            }).ToList();

        return reportItems;
    }
}

internal abstract class BaseReportDefinition
{
    public virtual Func<Row, string> Aggregate { get { return reportItem => reportItem.CompanyName; } }

    protected IEnumerable<IGrouping<string, Row>> GetReportBaseConditions(IList<Row> dataForReport, Month month)
    {
        return dataForReport.Where(reportItem => reportItem.Month == (int)month).GroupBy(Aggregate);
    }

    protected string GetPriceAsString(decimal price)
    {
        return price.ToString("F");
    }

    protected string GetAsString(IList<string> strings)
    {
        return string.Join("; ", strings);
    }
}

Report definition is really simple. Each report have to implement interface "IReportDefinition". Inheriting from the "BaseReportDefinition" class give us access to methods that simplify the report generation. Adding a new report comes down to adding a new type of "ReportType", creating a new definition file and informing the factory in the "GetReportDefinition" method about this type.

Summary

Writing clean code is very important. In the future can save a lot of time and nerves. The language you write your code with should look like it was made for the problem. Code should be minimal. There should not be redundant code that does nothing. Variable, methods names should unambiguously indicate their destination. Each constant should have a name that uniquely identifies its destiny. Reading your code should be easy and pleasant. 

Remember: "The code that a computer can understand can be written by anyone. A good programmer writes code that people can understand."

Have a nice day!