Click here to Skip to main content
13,195,379 members (60,801 online)
Click here to Skip to main content
Add your own
alternative version

Stats

18.5K views
42 downloads
17 bookmarked
Posted 17 Jun 2017

Why I cannot understand this code?

, 28 Jun 2017
Rate this:
Please Sign up or sign in to vote.
"The code that a computer can understand can be written by anyone. A good programmer writes code that people can understand."

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

 

 

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 112345
Company 245678
Company 310928

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!

License

This article, along with any associated source code and files, is licensed under The Code Project Open License (CPOL)

Share

About the Author

No Biography provided

You may also be interested in...

Pro
Pro

Comments and Discussions

 
QuestionYes and no Pin
InvisibleMedia31-Jul-17 23:09
memberInvisibleMedia31-Jul-17 23:09 
QuestionEasy to read? Pin
Member 1174085530-Jun-17 5:14
memberMember 1174085530-Jun-17 5:14 
QuestionMonth but no year? Pin
Paulo Zemek29-Jun-17 10:10
professionalPaulo Zemek29-Jun-17 10:10 
AnswerRe: Month but no year? Pin
Krzysiek Wiśniewski29-Jun-17 19:38
memberKrzysiek Wiśniewski29-Jun-17 19:38 
QuestionComments in code Pin
Ciprian Beldi28-Jun-17 1:11
memberCiprian Beldi28-Jun-17 1:11 
AnswerRe: Comments in code Pin
kw67728-Jun-17 20:56
memberkw67728-Jun-17 20:56 
GeneralRe: Comments in code Pin
Jason Barden1-Jul-17 8:42
professionalJason Barden1-Jul-17 8:42 
QuestionCode block formatting Pin
Wendelius27-Jun-17 20:44
mvpWendelius27-Jun-17 20:44 
AnswerRe: Code block formatting Pin
kw67728-Jun-17 20:21
memberkw67728-Jun-17 20:21 
GeneralRe: Code block formatting Pin
Wendelius28-Jun-17 20:32
mvpWendelius28-Jun-17 20:32 
GeneralRe: Code block formatting Pin
Krzysiek Wiśniewski28-Jun-17 22:16
memberKrzysiek Wiśniewski28-Jun-17 22:16 
GeneralRe: Code block formatting Pin
Wendelius29-Jun-17 4:56
mvpWendelius29-Jun-17 4:56 
AnswerToo complicated Pin
Mr.PoorEnglish24-Jun-17 22:21
memberMr.PoorEnglish24-Jun-17 22:21 
GeneralRe: Too complicated Pin
kw67725-Jun-17 19:38
memberkw67725-Jun-17 19:38 
GeneralRe: Too complicated Pin
Mr.PoorEnglish26-Jun-17 6:03
memberMr.PoorEnglish26-Jun-17 6:03 
GeneralRe: Too complicated Pin
kw67727-Jun-17 20:20
memberkw67727-Jun-17 20:20 
GeneralRe: Too complicated Pin
Mr.PoorEnglish28-Jun-17 9:11
memberMr.PoorEnglish28-Jun-17 9:11 
GeneralRe: Too complicated Pin
kw67728-Jun-17 21:12
memberkw67728-Jun-17 21:12 
QuestionI don't understand too Pin
Member 1327518822-Jun-17 23:08
memberMember 1327518822-Jun-17 23:08 
AnswerRe: I don't understand too Pin
kw67725-Jun-17 19:43
memberkw67725-Jun-17 19:43 
QuestionMonth 15. Pin
Paulo Zemek22-Jun-17 7:45
professionalPaulo Zemek22-Jun-17 7:45 
AnswerRe: Month 15. Pin
kw67722-Jun-17 20:18
memberkw67722-Jun-17 20:18 
PraiseTo the point Pin
SteveHolle22-Jun-17 5:51
memberSteveHolle22-Jun-17 5:51 
GeneralRe: To the point Pin
kw67722-Jun-17 20:50
memberkw67722-Jun-17 20:50 
QuestionLore of the Ancient Masters Pin
YrthWyndAndFyre20-Jun-17 8:10
memberYrthWyndAndFyre20-Jun-17 8:10 
AnswerRe: Lore of the Ancient Masters Pin
kw67720-Jun-17 19:11
memberkw67720-Jun-17 19:11 
AnswerRe: Lore of the Ancient Masters Pin
David A. Gray28-Jun-17 6:46
memberDavid A. Gray28-Jun-17 6:46 
GeneralRe: Lore of the Ancient Masters Pin
kw67728-Jun-17 20:27
memberkw67728-Jun-17 20:27 
GeneralRe: Lore of the Ancient Masters Pin
David A. Gray29-Jun-17 7:39
memberDavid A. Gray29-Jun-17 7:39 
PraiseVery nice article... Pin
Destiny77719-Jun-17 12:27
memberDestiny77719-Jun-17 12:27 
GeneralRe: Very nice article... Pin
kw67719-Jun-17 19:08
memberkw67719-Jun-17 19:08 
GeneralMy vote of 4 Pin
Andy Peralta19-Jun-17 1:59
memberAndy Peralta19-Jun-17 1:59 
GeneralRe: My vote of 4 Pin
kw67719-Jun-17 19:09
memberkw67719-Jun-17 19:09 
QuestionThank for your advice. Pin
ThomaLuke18-Jun-17 4:43
professionalThomaLuke18-Jun-17 4:43 
AnswerRe: Thank for your advice. Pin
kw67718-Jun-17 20:16
memberkw67718-Jun-17 20:16 
QuestionGood start, a few suggestions Pin
Marc Clifton17-Jun-17 8:20
protectorMarc Clifton17-Jun-17 8:20 
AnswerRe: Good start, a few suggestions Pin
kw67718-Jun-17 19:51
memberkw67718-Jun-17 19:51 
GeneralRe: Good start, a few suggestions Pin
Southmountain24-Jun-17 7:29
memberSouthmountain24-Jun-17 7:29 
GeneralRe: Good start, a few suggestions Pin
kw67725-Jun-17 19:44
memberkw67725-Jun-17 19:44 

General General    News News    Suggestion Suggestion    Question Question    Bug Bug    Answer Answer    Joke Joke    Praise Praise    Rant Rant    Admin Admin   

Use Ctrl+Left/Right to switch messages, Ctrl+Up/Down to switch threads, Ctrl+Shift+Left/Right to switch pages.

Permalink | Advertise | Privacy | Terms of Use | Mobile
Web03 | 2.8.171019.1 | Last Updated 29 Jun 2017
Article Copyright 2017 by Krzysiek Wiśniewski
Everything else Copyright © CodeProject, 1999-2017
Layout: fixed | fluid