Why I cannot understand this code?
4.34/5 (22 votes)
This is an alternative for "Why I cannot understand this code?"
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:
/* The Datamodel is so small, that I didn't introduce a specific namespace. Moreover I associated
* the different ways, the datamodel can be queried, as part of the Datamodel.
* I know: Using enums to distinguish different Query-Types is questionable - but so was the template */
using System.ComponentModel;
namespace SimpleReport.Version3 {
public enum ReportType {
[Description("Company vs price")]
CompanyVsPrice = 1,
[Description("Company vs city")]
CompanyVsCity,
[Description("Company vs number of days")]
CityVsNumberOfDays,
[Description("Company vs number of person")]
CompanyVsNumberOfPerson
}
/// <summary> Datamodel-class - only one </summary>
public class CompanyInfo {
public Months Month { get; set; } // improved Datamodel: Month-Property with appropriate Datatype
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; }
}
}
File Report.cs:
/* Two special Datatypes define a special understanding of "reporting": create groups of strings */
using System;
using System.Collections.Generic;
namespace SimpleReport.Version3 {
/// <summary> represents a mostly summarizing report about a set of Data </summary>
public class Report {
public string Name { get; set; }
public List<ReportEntry> Entries { get; set; }
}
public class ReportEntry {
public string Caption { get; set; }
public string Value { get; set; }
}
}
File Repository.cs:
/* A usually Repositories Job is to provide Data, in appropriate Datatypes.
This Repository only provides kind of 'metadata' - namely several types of Reports.*/
using System;
using System.Collections.Generic;
using System.Linq;
namespace SimpleReport.Version3 {
class Repository {
// the database-Mock, from which we 'query'
private static List<CompanyInfo> _CompanyInfos = new List<CompanyInfo>
{
new CompanyInfo { City = "City 1", CompanyName = "Company 1", Month = Months.January, NumberOfDays = 5, NumberOfPerson = 30, Price = 23045 },
new CompanyInfo { City = "City 1", CompanyName = "Company 1", Month = Months.February, NumberOfDays = 4, NumberOfPerson = 50, Price = 10000 },
new CompanyInfo { City = "City 1", CompanyName = "Company 1", Month = Months.March, NumberOfDays = 8, NumberOfPerson = 70, Price = 55000 },
new CompanyInfo { City = "City 2", CompanyName = "Company 1", Month = Months.April, NumberOfDays = 3, NumberOfPerson = 3, Price = 12000 },
new CompanyInfo { City = "City 2", CompanyName = "Company 2", Month = Months.January, NumberOfDays = 2, NumberOfPerson = 12, Price = 15000 },
new CompanyInfo { City = "City 2", CompanyName = "Company 2", Month = Months.June, NumberOfDays = 2, NumberOfPerson = 11, Price = 16000 },
new CompanyInfo { City = "City 3", CompanyName = "Company 1", Month = Months.July, NumberOfDays = 8, NumberOfPerson = 20, Price = 33000 },
new CompanyInfo { City = "City 3", CompanyName = "Company 3", Month = Months.August, NumberOfDays = 5, NumberOfPerson = 10, Price = 9000 },
new CompanyInfo { City = "City 1", CompanyName = "Company 1", Month = Months.September, NumberOfDays = 3, NumberOfPerson = 20, Price = 22000 },
new CompanyInfo { City = "City 1", CompanyName = "Company 1", Month = Months.October, NumberOfDays = 8, NumberOfPerson = 30, Price = 55000 },
};
public static Report GetCompanyInfoReport(ReportType reportType, Months month) {
var rep = new Report() { Name = Util.GetEnumDescription(reportType) };
var monthData = _CompanyInfos.Where(x => x.Month == month); // all queries select 1 month
IEnumerable<ReportEntry> itms = null;
switch (reportType) { // the queries differ in Grouping and Selecting
case ReportType.CompanyVsPrice:
itms = from inf in monthData group inf by inf.CompanyName into g
select new ReportEntry { Caption = g.Key, Value = g.Sum(i => i.Price).ToString("F") };
break;
case ReportType.CompanyVsCity:
itms = from inf in monthData group inf by inf.CompanyName into g
select new ReportEntry { Caption = g.Key, Value = string.Join("; ", g.Select(i => i.City)) };
break;
case ReportType.CityVsNumberOfDays:
itms = from inf in monthData group inf by inf.City into g
select new ReportEntry { Caption = g.Key, Value = g.Sum(i => i.NumberOfDays).ToString() };
break;
case ReportType.CompanyVsNumberOfPerson:
itms = from inf in monthData group inf by inf.CompanyName into g
select new ReportEntry { Caption = g.Key, Value = g.Sum(i => i.NumberOfPerson).ToString() };
break;
default:
throw new ArgumentException("unknown reportType");
}
rep.Entries = itms.ToList();
return rep;
}
}
}
File Util.cs:
/* general useful things */
using System;
using System.Collections.Generic;
using System.ComponentModel;
using System.Linq;
using System.Reflection;
namespace SimpleReport.Version3 {
public enum Months { January = 1, February, March, April, May, June, July, August, September, October, November, December }
static class Util {
/// <summary> gets the DescriptionAttribute-Value of an enum-value - if present. Otherwise value.ToString() </summary>
public static string GetEnumDescription(Enum value) {
var fi = value.GetType().GetField(value.ToString());
var attr = ((DescriptionAttribute[])fi.GetCustomAttributes(typeof(DescriptionAttribute), false));
return attr.Length == 0 ? value.ToString() : attr[0].Description;
}
}
}
The Original-Version
File BaseReportDefinition.cs:
using System;
using System.Collections.Generic;
using System.Linq;
namespace SimpleReport.Version2 {
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);
}
}
}
File CityVsNumberOfDaysReportDefinition.cs:
using System;
using System.Collections.Generic;
using System.Linq;
namespace SimpleReport.Version2 {
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;
}
}
}
File CompanyVsCityReportDefinition.cs:
using System.Collections.Generic;
using System.Linq;
namespace SimpleReport.Version2 {
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;
}
}
}
File CompanyVsNumberOfPersonReportDefinition.cs:
using System.Collections.Generic;
using System.Linq;
namespace SimpleReport.Version2 {
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;
}
}
}
File CompanyVsPriceReportDefinition.cs:
using System.Collections.Generic;
using System.Linq;
namespace SimpleReport.Version2 {
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;
}
}
}
File IReportDefinition.cs:
using System;
using System.Collections.Generic;
namespace SimpleReport.Version2 {
internal interface IReportDefinition {
string ReportName { get; }
Func<Row, string> Aggregate { get; }
IList<ReportItem> GetReportItems(IList<Row> dataForReport, Month month);
}
}
File Month.cs:
namespace SimpleReport.Version2 {
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
}
}
File Report.cs:
using System.Collections.Generic;
namespace SimpleReport.Version2 {
public class Report {
public string ReportName { get; set; }
public IList<ReportItem> Items { get; set; }
}
}
File ReportFactory.cs:
using System;
using System.Collections.Generic;
namespace SimpleReport.Version2 {
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");
}
}
}
}
File ReportItem.cs:
namespace SimpleReport.Version2 {
public class ReportItem {
public string ValueName { get; set; }
public string Value { get; set; }
}
}
File Reports.cs:
using System.Collections.Generic;
namespace SimpleReport.Version2 {
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
return new List<Row>
{
new Row { City = "City 1", CompanyName = "Company 1", Month = 1, NumberOfDays = 5, NumberOfPerson = 30, Price = 23045 },
new Row { City = "City 1", CompanyName = "Company 1", Month = 2, NumberOfDays = 4, NumberOfPerson = 50, Price = 10000 },
new Row { City = "City 1", CompanyName = "Company 1", Month = 3, NumberOfDays = 8, NumberOfPerson = 70, Price = 55000 },
new Row { City = "City 2", CompanyName = "Company 1", Month = 4, NumberOfDays = 3, NumberOfPerson = 3, Price = 12000 },
new Row { City = "City 2", CompanyName = "Company 2", Month = 1, NumberOfDays = 2, NumberOfPerson = 12, Price = 15000 },
new Row { City = "City 2", CompanyName = "Company 2", Month = 6, NumberOfDays = 2, NumberOfPerson = 11, Price = 16000 },
new Row { City = "City 3", CompanyName = "Company 1", Month = 7, NumberOfDays = 8, NumberOfPerson = 20, Price = 33000 },
new Row { City = "City 3", CompanyName = "Company 3", Month = 8, NumberOfDays = 5, NumberOfPerson = 10, Price = 9000 },
new Row { City = "City 1", CompanyName = "Company 1", Month = 9, NumberOfDays = 3, NumberOfPerson = 20, Price = 22000 },
new Row { City = "City 1", CompanyName = "Company 1", Month = 10, NumberOfDays = 8, NumberOfPerson = 30, Price = 55000 },
};
}
}
}
File ReportType.cs:
namespace SimpleReport.Version2 {
public enum ReportType {
CompanyVsPrice = 1,
CompanyVsCity = 2,
CityVsNumberOfDays = 3,
CompanyVsNumberOfPerson = 4
}
}
File Row.cs:
namespace SimpleReport.Version2 {
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; }
}
}
Usage (all versions)
File Program.cs:
using SimpleReport;
using v1 = SimpleReport.Version1;
using v2 = SimpleReport.Version2;
using v3 = SimpleReport.Version3;
using System;
namespace SimpleReport {
class Program {
static void Main(string[] args) {
Console.WriteLine("version 1:");
var myClass = new v1.MyClass();
var result = myClass.Process(1, 1, "name 1");
foreach (var item in result.Items) {
Console.WriteLine(string.Format("{0}:\t{1}", item.ValueName, item.Value));
}
Console.WriteLine("\nversion 2:");
var reportFactory = new v2.Reports();
var v2report = reportFactory.GenerateReport(v2.ReportType.CompanyVsPrice, v2.Month.January);
foreach (var item in v2report.Items) {
Console.WriteLine(string.Format("{0}:\t{1}", item.ValueName, item.Value));
}
Console.WriteLine("\nversion 3:");
var v3Report = v3.Repository.GetCompanyInfoReport(v3.ReportType.CompanyVsPrice, v3.Months.January);
foreach (var entry in v3Report.Entries) {
Console.WriteLine(string.Format("{0}:\t{1}", entry.Caption, entry.Value));
}
Console.ReadKey();
}
}
}
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.