Why I cannot understand this code?

• Download SimpleReport.zip - 14.8 KB

Introduction

Please first refer to the original-article (and note it's exorbitant rating) - before examining my enhancements of it.

I tried to annoy the originals author with my statement: "Your solution is not a good example for easy-understand-coding".

His clever reply was, to invite me to present a 'better version' - from my point of view.

Ok - I'll try.

The Application

There is assumed some Data, which is to query in several ways, and the query-results - short summaries - are to display on the console.

See yourself

The original codes contained two versions - a perfect template to me, to add my third version.

To make it simple: I offer Version2 and Version3 to you - as they are. And you can inspect the listings and decide for yourself, which version you find easier to understand.

Please note: I show the file-contents as they are, without omissions, and especially without any additional explainations.
This articles topic is not to explain code - it is about making it as self-explainatory as possible.
In real-life there are also no additional explainations, and you must get by with the code as it is.

You propably will be right, when you find technical and architectural drawbacks - in several respects.
But as said: The topic is not "What is done?", but it is: "Can you understand, what is done?"

My version

File Datamodel.cs:

  1  /* The Datamodel is so small, that I didn't introduce a specific namespace. Moreover I associated
  2   * the different ways, the datamodel can be queried, as part of the Datamodel.
  3   * I know: Using enums to distinguish different Query-Types is questionable - but so was the template */
  4  using System.ComponentModel;
  5  
  6  namespace SimpleReport.Version3 {
  7  
  8     public enum ReportType {
  9        [Description("Company vs price")]
 10        CompanyVsPrice = 1,
 11        [Description("Company vs city")]
 12        CompanyVsCity,
 13        [Description("Company vs number of days")]
 14        CityVsNumberOfDays,
 15        [Description("Company vs number of person")]
 16        CompanyVsNumberOfPerson
 17     }
 18  
 19     /// <summary> Datamodel-class - only one </summary>
 20     public class CompanyInfo {
 21        public Months Month { get; set; } // improved Datamodel: Month-Property with appropriate Datatype
 22        public string CompanyName { get; set; }
 23        public string City { get; set; }
 24        public int NumberOfPerson { get; set; }
 25        public int NumberOfDays { get; set; }
 26        public decimal Price { get; set; }
 27     }
 28  }

File Report.cs:

 29  /* Two special Datatypes define a special understanding of "reporting": create groups of strings */
 30  using System;
 31  using System.Collections.Generic;
 32  
 33  namespace SimpleReport.Version3 {
 34  
 35     /// <summary> represents a mostly summarizing report about a set of Data </summary>
 36     public class Report {
 37        public string Name { get; set; }
 38        public List<ReportEntry> Entries { get; set; }
 39     }
 40  
 41     public class ReportEntry {
 42        public string Caption { get; set; }
 43        public string Value { get; set; }
 44     }
 45  }

File Repository.cs:

 46  /* A usually Repositories Job is to provide Data, in appropriate Datatypes.
 47  This Repository only provides kind of 'metadata' - namely several types of Reports.*/
 48  using System;
 49  using System.Collections.Generic;
 50  using System.Linq;
 51  
 52  namespace SimpleReport.Version3 {
 53     class Repository {
 54  
 55        // the database-Mock, from which we 'query'
 56        private static List<CompanyInfo> _CompanyInfos = new List<CompanyInfo>
 57              {
 58                  new CompanyInfo { City = "City 1", CompanyName = "Company 1", Month = Months.January, NumberOfDays = 5, NumberOfPerson = 30, Price = 23045 },
 59                  new CompanyInfo { City = "City 1", CompanyName = "Company 1", Month = Months.February, NumberOfDays = 4, NumberOfPerson = 50, Price = 10000 },
 60                  new CompanyInfo { City = "City 1", CompanyName = "Company 1", Month = Months.March, NumberOfDays = 8, NumberOfPerson = 70, Price = 55000 },
 61                  new CompanyInfo { City = "City 2", CompanyName = "Company 1", Month = Months.April, NumberOfDays = 3, NumberOfPerson = 3, Price = 12000 },
 62                  new CompanyInfo { City = "City 2", CompanyName = "Company 2", Month = Months.January, NumberOfDays = 2, NumberOfPerson = 12, Price = 15000 },
 63                  new CompanyInfo { City = "City 2", CompanyName = "Company 2", Month = Months.June, NumberOfDays = 2, NumberOfPerson = 11, Price = 16000 },
 64                  new CompanyInfo { City = "City 3", CompanyName = "Company 1", Month = Months.July, NumberOfDays = 8, NumberOfPerson = 20, Price = 33000 },
 65                  new CompanyInfo { City = "City 3", CompanyName = "Company 3", Month = Months.August, NumberOfDays = 5, NumberOfPerson = 10, Price = 9000 },
 66                  new CompanyInfo { City = "City 1", CompanyName = "Company 1", Month = Months.September, NumberOfDays = 3, NumberOfPerson = 20, Price = 22000 },
 67                  new CompanyInfo { City = "City 1", CompanyName = "Company 1", Month = Months.October, NumberOfDays = 8, NumberOfPerson = 30, Price = 55000 },
 68              };
 69  
 70        public static Report GetCompanyInfoReport(ReportType reportType, Months month) {
 71           var rep = new Report() { Name = Util.GetEnumDescription(reportType) };
 72           var monthData = _CompanyInfos.Where(x => x.Month == month); // all queries select 1 month
 73           IEnumerable<ReportEntry> itms = null;
 74           switch (reportType) { // the queries differ in Grouping and Selecting
 75           case ReportType.CompanyVsPrice:
 76              itms = from inf in monthData group inf by inf.CompanyName into g
 77                     select new ReportEntry { Caption = g.Key, Value = g.Sum(i => i.Price).ToString("F") };
 78              break;
 79           case ReportType.CompanyVsCity:
 80              itms = from inf in monthData group inf by inf.CompanyName into g
 81                     select new ReportEntry { Caption = g.Key, Value = string.Join("; ", g.Select(i => i.City)) };
 82              break;
 83           case ReportType.CityVsNumberOfDays:
 84              itms = from inf in monthData group inf by inf.City into g
 85                     select new ReportEntry { Caption = g.Key, Value = g.Sum(i => i.NumberOfDays).ToString() };
 86              break;
 87           case ReportType.CompanyVsNumberOfPerson:
 88              itms = from inf in monthData group inf by inf.CompanyName into g
 89                     select new ReportEntry { Caption = g.Key, Value = g.Sum(i => i.NumberOfPerson).ToString() };
 90              break;
 91           default:
 92              throw new ArgumentException("unknown reportType");
 93           }
 94           rep.Entries = itms.ToList();
 95           return rep;
 96        }
 97     }
 98  }

File Util.cs:

 99  /* general useful things */
100  using System;
101  using System.Collections.Generic;
102  using System.ComponentModel;
103  using System.Linq;
104  using System.Reflection;
105  
106  namespace SimpleReport.Version3 {
107  
108     public enum Months { January = 1, February, March, April, May, June, July, August, September, October, November, December }
109  
110     static class Util {
111  
112        /// <summary> gets the DescriptionAttribute-Value of an enum-value - if present. Otherwise value.ToString() </summary>
113        public static string GetEnumDescription(Enum value) {
114           var fi = value.GetType().GetField(value.ToString());
115           var attr = ((DescriptionAttribute[])fi.GetCustomAttributes(typeof(DescriptionAttribute), false));
116           return attr.Length == 0 ? value.ToString() : attr[0].Description;
117        }
118     }
119  }

The Original-Version

File BaseReportDefinition.cs:

  1  using System;
  2  using System.Collections.Generic;
  3  using System.Linq;
  4  
  5  namespace SimpleReport.Version2 {
  6     internal abstract class BaseReportDefinition {
  7        public virtual Func<Row, string> Aggregate { get { return reportItem => reportItem.CompanyName; } }
  8  
  9        protected IEnumerable<IGrouping<string, Row>> GetReportBaseConditions(IList<Row> dataForReport, Month month) {
 10           return dataForReport.Where(reportItem => reportItem.Month == (int)month).GroupBy(Aggregate);
 11        }
 12  
 13        protected string GetPriceAsString(decimal price) {
 14           return price.ToString("F");
 15        }
 16  
 17        protected string GetAsString(IList<string> strings) {
 18           return string.Join("; ", strings);
 19        }
 20     }
 21  }

File CityVsNumberOfDaysReportDefinition.cs:

 22  using System;
 23  using System.Collections.Generic;
 24  using System.Linq;
 25  
 26  namespace SimpleReport.Version2 {
 27  
 28     internal class CityVsNumberOfDaysReportDefinition : BaseReportDefinition, IReportDefinition {
 29        public string ReportName { get { return "City vs number of days"; } }
 30  
 31        public override Func<Row, string> Aggregate { get { return reportItem => reportItem.City; } }
 32  
 33        public IList<ReportItem> GetReportItems(IList<Row> dataForReport, Month month) {
 34           var reportItems = base.GetReportBaseConditions(dataForReport, month)
 35               .Select(reportItem => new ReportItem {
 36                  ValueName = reportItem.Key,
 37                  Value = reportItem.Sum(reportValue => reportValue.NumberOfDays).ToString()
 38               }).ToList();
 39  
 40           return reportItems;
 41        }
 42     }
 43  }

File CompanyVsCityReportDefinition.cs:

 44  using System.Collections.Generic;
 45  using System.Linq;
 46  
 47  namespace SimpleReport.Version2 {
 48     internal class CompanyVsCityReportDefinition : BaseReportDefinition, IReportDefinition {
 49        public string ReportName { get { return "Company vs city"; } }
 50  
 51        public IList<ReportItem> GetReportItems(IList<Row> dataForReport, Month month) {
 52           var reportItems = base.GetReportBaseConditions(dataForReport, month)
 53               .Select(reportItem => new ReportItem {
 54                  ValueName = reportItem.Key,
 55                  Value = base.GetAsString(reportItem.Select(reportValue => reportValue.City).ToList())
 56               }).ToList();
 57  
 58           return reportItems;
 59        }
 60     }
 61  }

File CompanyVsNumberOfPersonReportDefinition.cs:

 62  using System.Collections.Generic;
 63  using System.Linq;
 64  
 65  namespace SimpleReport.Version2 {
 66     internal class CompanyVsNumberOfPersonReportDefinition : BaseReportDefinition, IReportDefinition {
 67        public string ReportName { get { return "Company vs number of person"; } }
 68  
 69        public IList<ReportItem> GetReportItems(IList<Row> dataForReport, Month month) {
 70           var reportItems = base.GetReportBaseConditions(dataForReport, month)
 71               .Select(reportItem => new ReportItem {
 72                  ValueName = reportItem.Key,
 73                  Value = reportItem.Sum(reportValue => reportValue.NumberOfPerson).ToString()
 74               }).ToList();
 75  
 76           return reportItems;
 77        }
 78     }
 79  }

File CompanyVsPriceReportDefinition.cs:

 80  using System.Collections.Generic;
 81  using System.Linq;
 82  
 83  namespace SimpleReport.Version2 {
 84     internal class CompanyVsPriceReportDefinition : BaseReportDefinition, IReportDefinition {
 85        public string ReportName { get { return "Company vs price"; } }
 86  
 87        public IList<ReportItem> GetReportItems(IList<Row> dataForReport, Month month) {
 88           var reportItems = base.GetReportBaseConditions(dataForReport, month)
 89           .Select(reportItem => new ReportItem {
 90              ValueName = reportItem.Key,
 91              Value = base.GetPriceAsString(reportItem.Sum(reportValue => reportValue.Price))
 92           }).ToList();
 93  
 94           return reportItems;
 95        }
 96     }
 97  }

File IReportDefinition.cs:

 98  using System;
 99  using System.Collections.Generic;
100  
101  namespace SimpleReport.Version2 {
102     internal interface IReportDefinition {
103        string ReportName { get; }
104        Func<Row, string> Aggregate { get; }
105        IList<ReportItem> GetReportItems(IList<Row> dataForReport, Month month);
106     }
107  }

File Month.cs:

108  namespace SimpleReport.Version2 {
109     public enum Month {
110        January = 1,
111        February = 2,
112        March = 3,
113        April = 4,
114        May = 5,
115        June = 6,
116        July = 7,
117        August = 8,
118        September = 9,
119        October = 10,
120        November = 11,
121        December = 12
122     }
123  }

File Report.cs:

124  using System.Collections.Generic;
125  
126  namespace SimpleReport.Version2 {
127     public class Report {
128        public string ReportName { get; set; }
129        public IList<ReportItem> Items { get; set; }
130     }
131  }

File ReportFactory.cs:

132  using System;
133  using System.Collections.Generic;
134  
135  namespace SimpleReport.Version2 {
136     internal class ReportFactory {
137        private readonly IList<Row> _dataForReport;
138        private readonly ReportType _reportType;
139        private readonly Month _month;
140  
141        public ReportFactory(IList<Row> dataForReport, ReportType reportType, Month month) {
142           _dataForReport = dataForReport ?? new List<Row>();
143           _reportType = reportType;
144           _month = month;
145           Validate();
146        }
147  
148        public Report GetReport() {
149           var report = new Report();
150           IReportDefinition reportDefinition = GetReportDefinition();
151           report.ReportName = reportDefinition.ReportName;
152           report.Items = reportDefinition.GetReportItems(_dataForReport, _month);
153  
154           return report;
155        }
156  
157        private IReportDefinition GetReportDefinition() {
158           switch (_reportType) {
159           case ReportType.CompanyVsPrice:
160              return new CompanyVsPriceReportDefinition();
161           case ReportType.CompanyVsCity:
162              return new CompanyVsCityReportDefinition();
163           case ReportType.CityVsNumberOfDays:
164              return new CityVsNumberOfDaysReportDefinition();
165           case ReportType.CompanyVsNumberOfPerson:
166              return new CompanyVsNumberOfPersonReportDefinition();
167           default:
168              throw new NotImplementedException(string.Format("Report for type '{0}' is not implemented!", _reportType));
169           }
170        }
171  
172        private void Validate() {
173           if (!Enum.IsDefined(typeof(Month), _month)) {
174              throw new ArgumentOutOfRangeException("month");
175           }
176        }
177     }
178  }

File ReportItem.cs:

179  namespace SimpleReport.Version2 {
180     public class ReportItem {
181        public string ValueName { get; set; }
182        public string Value { get; set; }
183     }
184  }

File Reports.cs:

185  using System.Collections.Generic;
186  
187  namespace SimpleReport.Version2 {
188     public class Reports {
189        public Report GenerateReport(ReportType reportType, Month month) {
190           var dataForReport = GetDataForReport();
191           var reportFactory = new ReportFactory(dataForReport, reportType, month);
192           var report = reportFactory.GetReport();
193  
194           return report;
195        }
196  
197  
198        private IList<Row> GetDataForReport() {
199           //... load data for example from excel file
200           return new List<Row>
201              {
202                  new Row { City = "City 1", CompanyName = "Company 1", Month = 1, NumberOfDays = 5, NumberOfPerson = 30, Price = 23045 },
203                  new Row { City = "City 1", CompanyName = "Company 1", Month = 2, NumberOfDays = 4, NumberOfPerson = 50, Price = 10000 },
204                  new Row { City = "City 1", CompanyName = "Company 1", Month = 3, NumberOfDays = 8, NumberOfPerson = 70, Price = 55000 },
205                  new Row { City = "City 2", CompanyName = "Company 1", Month = 4, NumberOfDays = 3, NumberOfPerson = 3, Price = 12000 },
206                  new Row { City = "City 2", CompanyName = "Company 2", Month = 1, NumberOfDays = 2, NumberOfPerson = 12, Price = 15000 },
207                  new Row { City = "City 2", CompanyName = "Company 2", Month = 6, NumberOfDays = 2, NumberOfPerson = 11, Price = 16000 },
208                  new Row { City = "City 3", CompanyName = "Company 1", Month = 7, NumberOfDays = 8, NumberOfPerson = 20, Price = 33000 },
209                  new Row { City = "City 3", CompanyName = "Company 3", Month = 8, NumberOfDays = 5, NumberOfPerson = 10, Price = 9000 },
210                  new Row { City = "City 1", CompanyName = "Company 1", Month = 9, NumberOfDays = 3, NumberOfPerson = 20, Price = 22000 },
211                  new Row { City = "City 1", CompanyName = "Company 1", Month = 10, NumberOfDays = 8, NumberOfPerson = 30, Price = 55000 },
212              };
213        }
214     }
215  }

File ReportType.cs:

216  namespace SimpleReport.Version2 {
217     public enum ReportType {
218        CompanyVsPrice = 1,
219        CompanyVsCity = 2,
220        CityVsNumberOfDays = 3,
221        CompanyVsNumberOfPerson = 4
222     }
223  }

File Row.cs:

224  namespace SimpleReport.Version2 {
225     public class Row {
226        public int Month { get; set; }
227        public string CompanyName { get; set; }
228        public string City { get; set; }
229        public int NumberOfPerson { get; set; }
230        public int NumberOfDays { get; set; }
231        public decimal Price { get; set; }
232     }
233  }

Usage (all versions)

File Program.cs:

  1  using SimpleReport;
  2  using v1 = SimpleReport.Version1;
  3  using v2 = SimpleReport.Version2;
  4  using v3 = SimpleReport.Version3;
  5  using System;
  6  
  7  namespace SimpleReport {
  8     class Program {
  9        static void Main(string[] args) {
 10  
 11           Console.WriteLine("version 1:");
 12           var myClass = new v1.MyClass();
 13           var result = myClass.Process(1, 1, "name 1");
 14           foreach (var item in result.Items) {
 15              Console.WriteLine(string.Format("{0}:\t{1}", item.ValueName, item.Value));
 16           }
 17  
 18           Console.WriteLine("\nversion 2:");
 19           var reportFactory = new v2.Reports();
 20           var v2report = reportFactory.GenerateReport(v2.ReportType.CompanyVsPrice, v2.Month.January);
 21           foreach (var item in v2report.Items) {
 22              Console.WriteLine(string.Format("{0}:\t{1}", item.ValueName, item.Value));
 23           }
 24  
 25           Console.WriteLine("\nversion 3:");
 26           var v3Report = v3.Repository.GetCompanyInfoReport(v3.ReportType.CompanyVsPrice, v3.Months.January);
 27           foreach (var entry in v3Report.Entries) {
 28              Console.WriteLine(string.Format("{0}:\t{1}", entry.Caption, entry.Value));
 29           }
 30  
 31           Console.ReadKey();
 32        }
 33     }
 34  }

Discussion

You see: We compare 13 Files of overall 233 lines of code (original-Version) with 4 Files of 119 lines of code, including several File-Header-Comments, which explain the concerns of the File (my-Version).

In my Version it's obviously, where the data come from: of course from the Repository.

And where my Datamodel is is obviously as well: in the Datamodel-File.

And my Datamodel-Class is named CompanyInfo - because that is what it is (it is not a "Row")

In the original i stepped through all Files searching the data - finally found it in a Class named Reports. (why named "Reports"??)

And Original-Querying the data is spread over 7 other Files, with following hard-to-read-Names:
BaseReportDefinition, CityVsNumberOfDaysReportDefinition, CompanyVsCityReportDefinition, CompanyVsNumberOfPersonReportDefinition, CompanyVsPriceReportDefinition, IReportDefinition, ReportFactory

Note: These 8 Files - in my version they are only one - my Repository. And a specific query is one single line.

But I know, many of you will now downrate me, and disagree, when I postulate:
"The shorter version, with less Files, with rare but meaningful commentation, with better naming, without Interface, baseclass, derived classes, factory and other hoo-haa-stuff - it is easier to understand."

(Ok - I do not know that, but it's my pessimistic expectation.)

This is strange, because I totally agree with the originals authors opinion:

Quote:

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. 

Let me pressume to give some simple advices for better approaching to these "golden goals":

• Try recognize appropriate wellknown patterns, like "Datamodel", "Repository", "Utility", and use these terms to give your solution structure.
• Keep it simple as long as possible: Eg. just naming a File "Repository" will give good orientation to readers (as long as the File actually contains Repository-Concerns).
• When things become more complex, you can adapt the structure: eg. introduce meaningful named folders, namespaces, or complete projects to manage those specific concerns.
  As said: when it becomes complex - not earlier!
• Keep Files in human-friendly dimensions:
  Thoughtful split Files of more than 300 lines into several smaller ones, each with its own identificable specific concern.
  On the other hand - when classes are smaller than 30 lines - feel free to store several of them into one File - as long as the classes are of fairly similar topic.
  Because hundreds of Files with nearby no code inside is human-unfriendly as well as one Monster-File.
• Don't apply all language-features you've ever heard from to your code.
  If you can get by without Interfaces, baseclasses, factories and stuff - please do get by without.
• But apply useful language-features, consequently
  Especial to avoid repeating code  base-classes can be very helpful - that can be seen as the main-concern of baseclasses.
  On the other hand - interfaces are code which does nothing - by design. So before introducing one, stop and think about it, whether you really need it.
  If there is only one class, implementing the interface, it is mostly dispensable - you can take the class itself instead.
  Same if there is a baseclass present, which can do the job.
  And same as above: When things become complex, Interfaces may become helpful. Then (not earlier) don't hesitate to introduce them.