DaedTech

Stories about Software

Jan
30

By

Write Once, Confuse Everywhere

Category: Anti Patterns Tags: , , , |

Not too long ago, someone asked me to take a look at a relatively simple application designed with the notion of having some future flexibility in mind. I poked around a bit to see what was where and found a reasonably simple and straightforward application. Like any, it had its warts, but there were no serious surprises–no whiplash-inducing double-takes or WTFs. One thing that I did notice, however, was that there were a bunch of classes that inherited from GUI components and had no implementations at all. So instead of using a TextBox, you might use a “DoNothingTextBox” with class definition DoNothingTextBox : TextBox and no implementation. (It was not actually called “DoNothingTextBox”–that’s just for illustration purposes.)

ConfusedI puzzled over the purpose of these things for a while until I inspected a few more and saw one with some code in it. I saw the purpose then immediately: hooks for future functionality. So if, for example, it were later decided at some point that all TextBoxes should disallow typing of non-alphanumeric characters, the behavior could be implemented in one place and work everywhere.

On the one hand, this does seem like a clever bit of future-proofing. If experience tells you that a common need will be to make every instance of X do Y, then it stands to reason you should make it easy and minimally invasive to touch every X. On the other hand, you’re quite possibly running afoul of YAGNI and definitely running afoul of the Open/Closed Principle by creating classes that you may never use with the specific intention of modifying them later. Also to consider is that this creates a weird flipping of what’s commonly expected with inheritance; rather than creating a derived class to extend existing functionality when needed, you speculatively create an ancestor for that purpose.

Is this a problem? Well, let’s think about how we would actually achieve this “change everything.” With the normal pattern of abstracting common functionality into a base class for reuse, the mechanism for the reuse is derived classes deferring to parent’s centralized implementation. For instance:

public class Bird
{
public virtual void Reproduce()
{
Console.Write("I laid an egg!");
}
}

public class Ostrich : Bird
{
public override void Reproduce()
{
base.Reproduce();
Console.Write("... and it was extremely large as far as bird eggs go.");
}
}

public class Parrot : Bird
{

}

Notice that since all birds reproduce by laying eggs, that functionality has been abstracted into a base class where it can be used everywhere and need not be duplicated. If necessary, it can be extended as in the case of Ostrich, or even overridden (which Ostrich could do by omitting the base class call), but the default is what Parrot does: simply use the common Reproduce() method. The bird classes have a default behavior that they can extend or opt out of, assuming that the base class is defined and established at the time that another bird class extends it.

Aha! This is an interesting distinction. The most common scenario for inheritance schemes is one where (1) the base class is defined first or (2) some duplicated piece of functionality across inheritors is moved up into a base class because it’s identical. In either case, the common functionality predates the inheritance relationship. Birds lay eggs, and part of deciding to be a bird (in the sense of writing a class that derives from Bird) is that default egg-laying behavior.

But let’s go back to our speculative text boxes. What does that look like?



public class DoNothingTextBox : TextBox
{

}

Now let’s say that time goes by and the developers all diligently stick to the architectural ‘pattern’ of using DoNothingTextBox everywhere. Life is good. But one day, some project management stakeholder tells one of the developers more on the UI side of things that all of the text boxes in the application should be green. The developer ponders that for a bit and then says, “I know!”

public class DoNothingTextBox : TextBox
{
public DoNothingTextBox() : base()
{
BackColor = Color.Green;
}
}

“Done and done.” He builds, observes that all text boxes are now green, checks in his changes, and takes off for the day to celebrate a job well done. Meanwhile, a handful of other developers on the team are updating to the latest version of the code to get an unrelated change they need. Each of them pulls in the latest, develops for a bit, launches the app to check out their changes, and, “what the… why is this text box green–how did I manage that?” Their troubleshooting progression probably starts with rolling back various changes. Then it winds its way into the version history of CSS files and styling mechanisms; stumbles into looking at the ASP markup and functionality of the various collaborators and controls on the web control/page; and, only after a great deal of frustration, cursing, and hair-tearing, winds up in the dusty, old, forgotten, formerly-empty DoNothing class.

I submit that the problem here is a subtle but profound one. As I’ve mentioned before, inheritance, by its nature, extends and sometimes modifies existing functionality. But this framework for building out and expanding software relies upon the fact that base classes are inherently more fixed and stable than child inheritors and that the relationship between child classes and base classes is well-defined and fixed at the time of inheritance/extension. To put it more concretely, OO developers will intuitively understand a how to use a “base” bird that lays eggs–they won’t intuitively understand how to use a “base” bird that does nothing and then later, spontaneously turns every bird on earth green. Changes to a base class are violent and confusing when they alter the behavior of inheritors while leaving the inheritors untouched.

So I’d encourage you to be extremely careful with using speculative inheritance structures. The instinct to create designs where potential sweeping changes can be implemented easily is an excellent one, but not all responses to that instinct are equally beneficial. Making a one line code change is certainly quick and creates a minimum of code upheaval for the moment, but once all of the time, effort, and hacking around during troubleshooting by other developers is factored in, the return isn’t quite so good anymore. Preserve that instinct, but channel it into better design decisions. It’s just as important to consider how broadly confusing or unintuitive a change will be as it is to consider how many lines of code it takes and how many files need to be touched.

Dec
14

By

Scoping And Accessibility Quirks in C#

Category: .NET Tags: , , |

As I mentioned recently, I’ve taken to using an inheritance scheme in my approach to unit testing. Because of the mechanics of this scheme, making a class under test internal this morning brought to light two relatively obscure properties of scoping and visibility in C# that you might not be aware of:

  1. Internal can be “less visible” than protected.
  2. Private isn’t always private.

Let me explain by showing the situation in which I found myself. As part of an open source project I’m working on at the moment to allow SQL-like querying of Autotask data through its API, I’ve been writing a set of tests on a class called “SqlQuery” in which I take a SQL statement and parse out the parts I’m interested in:

[TestClass]
public class SqlQueryTest
{
    protected SqlQuery Target { get; set; }

    [TestInitialize]
    public void BeforeEachTest()
    {
        Target = new SqlQuery("SELECT id FROM Account");
    }

    [TestClass]
    public class Columns : SqlQueryTest
    {
        [TestMethod, Owner("ebd"), TestCategory("Proven"), TestCategory("Unit")]
        public void Contains_One_Element_For_One_Selected_Column()
        {
            Assert.AreEqual(1, Target.Columns.Count());
        }
...

Up until now the class under test, SqlQuery, has been public, but I realize that this is an abstraction that only matters in the actual lower layer assembly rather than at the GUI level, so I made it internal and added an InternalsVisibleTo to the properties of the assembly under test. With that in place, I downgraded the SqlQuery class to internal and was momentarily surprised by a compiler error of “Inconsistent accessibility: property type ‘AutotaskQueryService.SqlQuery’ is less accessible than property ‘AutotaskQueryServiceTest.SqlQueryTest.Target'”.

KoalaWat

On its face, this seems crazy — “internal” is less accessible than “protected”? But when you think about it, this actually makes sense. “Internal” means “nobody outside of this assembly can see it” and protected means “nobody except for this class and its inheritors can see it.” So what happens if I create a third assembly and declare a class in it that inherits from SqlQueryTest? This class has no visibility to the assembly under test and its internals, but it would have visibility to Target. Hence the strange-seeming but quite correct compiler error. One way to get rid of this error is to make SqlQueryTest internal, and that actually compiled and all tests ran, but I don’t like that solution in the event that I want tests in that class and not just its nested children. I decided on another option: making Target private.

If you look at the code snippet above, are you now thinking “but that won’t compile!” After all “Columns” inherits from SqlQueryTest and uses Target and I’ve now just made Target private, so Columns will lose access to it. Well, no, as it turns out. The private scoping in a class means that only the things between the {} of the class can see it. Our nested class here happens to be one of those things. So the scoping trumps the hierarchy in this instance. This can easily be confirmed by changing Target to static and removing the inheritance relationship, which also compiles. The nested class, even when not deriving from the outer class, can access private static members of the outer class.

In the end, my solution here is simple. I make the Target private and move on. But I thought I’d take the opportunity to point out these interesting facets of C# that you probably don’t run across very often.