5

I've been working on a C# class library with the mindset that we will have new developers to work on it later. While I tried to keep the class/file name as descriptive as possible, the length is starting to get out of hand.

Here's a snapshot of what the project structure looks like:

MyCompanyName.ClassLibrary
  FlightPatternAnalysis
    IFlightPatternAnalysis.cs
    IFlightPatternAnalysisBuilder.cs
    DefaultFlightPatternAnalysis.cs
    DefaultFlightPatternAnalysisBuilderUsingJson.cs
    DefaultFlightPatternAnalysisBuilderUsingCsv.cs
    TriangulatedFlightPatternAnalysis.cs
    TriangulatedFlightPatternAnalysisBuilderUsingHourlyRecords.cs
    ...

And also extracted snapshots of some classes:

using System;
using System.Collections.Generic;
using System.Drawing;
using Newtonsoft.Json.Linq;

namespace MyCompanyName.ClassLibrary.FlightPatternAnalysis
{
    public interface IFlightPatternAnalysis
    {
        ...
        List<Point> GetTrackedCoordinations();
        DateTime CalculateEstimatedArrivalTime(Coord destination);
    }

    public interface IFlightPatternAnalysisBuilder
    {
        IFlightPatternAnalysis BuildFlightPatternAnalysis();
    }

    public class DefaultFlightPatternAnalysis : IFlightPatternAnalysis
    {
        private readonly List<TrackedRecord> _trackedRecords;

        public DefaultFlightPatternAnalysis(List<TrackedRecord> trackedRecords)
        {
            // Omitted.
        }

        ...

        public virtual List<Point> GetTrackedCoordinations()
        {
            // Omitted.
        }

        public virtual DateTime CalculateEstimatedArrivalTime(Coord destination)
        {
            // Omitted.
        }
    }

    public class DefaultFlightPatternAnalysisBuilderUsingJson : IFlightPatternAnalysisBuilder
    {
        private List<TrackedRecord> _trackedRecords;

        public DefaultFlightPatternAnalysisBuilderUsingJson WithJson(JObject jsonObject)
        {
            // Extraction and conversion, omitted.
            return this;
        }

        public virtual IFlightPatternAnalysis BuildFlightPatternAnalysis()
        {
            return new DefaultFlightPatternAnalysis(this._trackedRecords);
        }
    }
}

Builder pattern is just a choice of mine, I am hoping it will help with the code's readability. I was told that I could try abstract factory instead, but I have not research that, nor am I sure if it will help.

Any suggestions?

  • Are there XXXFlightPlan classes that are separate from the XXXFlightPlanAnalysys classes? If not, consider ditching the "Analysis" – user949300 Aug 11 at 6:00
  • @user949300: first, the name was FlightPattern, not FlightPlan. Second, I would expect a class named like FlightPatternAnalysis to have methods taking an object of type FlightPattern and do some analysis with it, so for the sake of having a reasonable example for this question, lets assume FlightPattern exists. – Doc Brown Aug 11 at 6:29
  • @user949300 @DocBrown: I do not have FlightPattern class right now. However, you are correct that FlightPattern might be created later to replace the List<TrackedRecord>. (Edit: but I don't think DefaultFlightPatternAnalysisBuilderUsingFlightPattern.cs is any shorter.) – penpen Aug 11 at 14:28
  • 4
    long names are long – D. Ben Knoble Aug 11 at 15:57
  • Namespaces, method signatures, class relations. all give context that allow names to be shorter. Words like "calculate", "sum" add nothing. Everyone says use "Get.." when returning a value - but the return type is not void! Try acronyms but be consistent & don't overload; "FP" doesn't need explanation I bet. Understanding often does not require proper grammar, consider removing prepositions - "using" for example. Change names multiple times and see how each feels while actively coding, IMO, forced encoding of structure, types, etc. becomes technical debt sooner or later. – radarbob Aug 12 at 22:28
8

In general, (far) shallower inheritance hierarchies. Instead of overriding all of these behaviors, consider injecting them. All of these variations can then be simple static functions (since they take arguments and contain per-instance state). It also makes the FromJson or FromCsv sort of factory methods more discoverable.

This sort of approach should simplify the code and let you restrict some of the implementation details to a single class. It also allows you to be more flexible when future needs require new behaviors to be injected. The lengthy names are just a code smell, and should get cleaned up as a side effect of making the design better.

  • Yes, this sounds very sensible. – Robert Harvey Aug 11 at 17:28
  • Just wondering, would it be better to use a helper class then? (Say, DefaultFlightPatternAnalysisHelper.cs with methods like public static DefaultFlightPatternAnalysis CreateFromJson(JObject jsonObject).) Or that's harder to read? – penpen Aug 11 at 19:12
  • @penpen - why not just put those on FlightPatternAnalysis? – Telastyn Aug 11 at 19:18
  • @Telastyn: both works for me, just wondering if there's a preference from other developers. – penpen Aug 11 at 19:21
  • @penpen - not a fan of helper classes, particularly in C# given visual studio’s excellent discoverability features. – Telastyn Aug 11 at 19:43
6

Telastyn's recommendation is a good one in general. However, I think what you have shown in your example can be still a reasonable inheritance hierarchy, and changing it may not be the right solution for this case.

But there are some well-known, simpler techniques to deal with the shown problem without a complete redesign of the class hierarchy, and often it is more suitable to start with these:

  1. Live with the fact that those class names get a little bit lengthy. All modern development environments provide "auto completion" features which take a lot of the burden of typing all those characters from the developer. And standard screens in most working environments today don't have a "80 characters per line" limit.

  2. Leave out parts which are self-explaining or just for binding purposes. For example, why is the class name DefaultFlightPatternAnalysis when there is no class FlightPatternAnalysis at all? It should be mostly self-explaining that FlightPatternAnalysis is the "default" for this type, and TriangulatedFlightPatternAnalysis is a specialization. Or leave out the word "Using" - which leads to FlightPatternAnalysisBuilderJson instead of DefaultFlightPatternAnalysisBuilderUsingJson - that is not really less readable, since you probably don't have other classes like FlightPatternAnalysisBuilder{Some other fill word here}Json in your code base.

  3. You may also consider to introduce - carefully - abbreviations which are a compromise between name length and readability. You may use these abbreviations only for helper classes with very long names, like your builder classes, in case they are used rarely in the code base. But beware, don't abbreviate to the point where names become cryptic, and try to be consistent - try to stick to the same abbreviation style within the same context.

  • Thanks for point 2. To elaborate, I used the "Default" prefix as workaround for same namespace and class conflict. – penpen Aug 11 at 18:52
  • Also, after reading all the answers & comments, I think I should create a FlightPattern class as user949300 suggested, refactor the existing Analysis classes to Analyser classes, then create new Analysis classes. That said, I am now uncertain what will happen to the specialization - I wonder if I am overkilling it? – penpen Aug 11 at 18:58
  • 1
    @penpen: if this is overdesign or not depends - primarily what code / responsibilities you will have in these different classes, and if it helps you to manage, maintain and evolve the code, or if it makes things more complicate. As a rough indicator, it makes a difference if each of these classes will contain 5 lines of functional code in average, or 500 lines. – Doc Brown Aug 11 at 19:35
4

Flight pattern analysis seems to be a big thing in you business logic. It is so common that replacing it with Fpa won't hurt. The people in the company are probably using the abbreviation already anyway. The class documentation header can spell it out once.

If you must have "UsingSomeFileFormat" descendants (which I doubt, you should probably use separate adapter classes instead), the Using part could be omitted, keeping just the format identifying suffix.

Consider using a namespace for the repeated domain identifier and just call your class Analysis (or Analyser if that would be more appropriate). Your namespace could be Aviation.FlightPatternAnalysis and your base class could be Analyser.

  • Thanks, I will start with the removal of "Using". Initially I also considered to use just DefaultAnalysis.cs and DefaultAnalysisBuilderUsingJson.cs since they are under FlightPatternAnalysis namespace, but then I got worried because there are more "analysis" classes under different namespaces. – penpen Aug 11 at 16:02
  • @penpen Default is a good start but try not to let yourself become attached. Default and Impl are both names used when you simply don't know what else to call this concrete implementation because right now it's the only one you have. Often once you know what the second one will be you have a much better idea what to name the first. – candied_orange Aug 13 at 6:36
  • I really dislike these kinds of abbreviations in identifier names. They really hurt readability; that's why we no longer have Hungarian. – Robert Harvey Aug 14 at 20:36
2

Think about the redundancy you are seeing in all of these names. "FlightPatternAnalysis" is repeated over and over and over. Due to the repetition, it does not provide any value, and actually makes the API harder to comprehend. I work on a project where the name of the project is used so often that you can't find where any code you need to change actually lives. Imagine 80 projects with the name "FlightPatternAnalysis" repeated in the project name. It's an extreme case to drive home the point that repeating words so many times is at best noise, and at worst actively harmful to productivity.

Many times we overlook perfectly good tools to help us organize our code: Name Spaces. You are even using one, but you can use it more effectively.

Consider for example, you have the namespace FlightPattern, that namespace is part of your name.

FlightPattern.IAnalysis
FlightPattern.IAnalysisBuilder

Now, all your implementations are still in the namespace FlightPatter, so you can simplify the different options:

DefaultAnalysis
DefaultBuilderFromJson
DefaultBuilderFromCsv

TriangulatedAnalysis
TriangulatedBuilderUsingHourlyRecords

If you need to disambiguate them from other types of analysis, you can use the relative namespace:

FlightPattern.DefaultAnalysis

Or just be fluent

If you were to rethink the way you consume or split work, using a fluent API, a lot of the naming would be hidden from users. This can make the API easier to consume.

var defaultAnalysis = Analyze.FromJson(myJObject);
var tringulatedAnalysis = Analyze.UsingHourlyRecords(trackedRecords);

The fact that the static class Analyze creates a TriangulatedFlightPatternAnalysisBuilderUsingHourlyRecords class to in turn generate the TriangulagedFlightPatternAnalysis object is invisible to the consumer of the API. They have an IFlightPatternAnalysis object at the end of it, and that's enough.

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.