DaedTech

Stories about Software

By

The Wrong Thing more Efficiently is Still the Wrong Thing

Editorial Note: I originally wrote this post for the Infragistics blog.  Go check out the original post on their site and take a look around while you’re there to see some of the other authors posting interesting things.

Let’s say that, like many folks, one of your first tasks each morning is dealing with your email. You arrive at the office, grab yourself a coffee, and settle in to complete this ubiquitous modern task.

Naturally, the first thing you do is open your inbox. Then you open each unread email, select all, and copy it. Next, you open a new instance of Visual Studio, create a console project, add a text file to it, and paste the email text into that text file. By the end of doing this, you have somewhere between 2 and 20 instances of Visual Studio running, depending on how many unread emails you have. At this point, you’re ready to read the emails, so you alt-tab among the VS instances, closing each one after you’ve read the email.

This system works well for you, but there are two major drawbacks. First of all, it’s a real hassle to have to create a new project and a text file manually, and then open the text file. It’d be awesome if there were a way that you could make Visual Studio do this for you. And secondly, all of those instances of Visual Studio have a tendency to cause your machine to thrash, and they sometimes just crash.

ScaryComputer

What to do? I’ll come back to that.

New C# Features!

For the moment, I’d like to talk about the latest version of C# (6.0) and how it has introduced some exciting new features. I’ve personally enjoyed using the nameof feature that allows you to eliminate late binding between strings and names of code elements.  I’ve also found property initializers to be handy for eliminating boilerplate around backing fields that I don’t want.  But there’s one particular feature that I haven’t had occasion to use: the null conditional operator.  It’s this feature I’d like to discuss in more detail.

Consider the following code that takes a house and examines it to determine whether or not a prospective intruder would think someone was home.

public class SecurityAuditor
{
    public bool WouldAnIntruderThinkSomeoneWasHome(House house)
    {
        Light mostProminentLight = house.Floors[1].Rooms["Kitchen"].Lights["Overhead"];
        return mostProminentLight.IsOn;
    }
}

It’s a pretty simple concept. It considers a certain light in the house to be the “most prominent” and, if that light is on, it says that an intruder would think someone was home. I doubt I’m going to win a contract from ADT for this, but it’s easy enough to reason about.

But it’s also not at all defensive. Look at all of the stuff that can go wrong.  Objects null, arrays not initialized or sized properly, dictionaries missing keys… yikes.  To fix it, we’d need something like this:

public class SecurityAuditor
{
    public bool WouldAnIntruderThinkSomeoneWasHome(House house)
    {
        if (house != null && house.Floors != null && house.Floors.Length > 1 && house.Floors[1] != null
            && house.Floors[1].Rooms != null && house.Floors[1].Rooms.ContainsKey("Kitchen"))
        {
            Light mostProminentLight = house.Floors[1].Rooms["Kitchen"].Lights["Overhead"];
            return mostProminentLight.IsOn;
        }
        return false;
    }
}

Wow, that’s ugly. And we’re not even done. I just got tired of typing. There’s still all the checks around the room not being null and the lights. I also might have missed something. Fortunately, C# 6 lets us do something cool.

public class SecurityAuditor
{
    public bool WouldAnIntruderThinkSomeoneWasHome(House house)
    {
        Light mostProminentLight = house?.Floors?[1]?.Rooms?["Kitchen"]?.Lights?["Overhead"];
        return mostProminentLight.IsOn;
    }
}

Those question marks you see in there create a null conditional operator (“?.” and “?[“) that bake the “if(x == null)” check inline and return null if something along the way is null. By inserting these, we cover all of the defensive programming bases with the exception of checking those dictionaries for key presence. But hey, much better, right?

Well, it is and it isn’t. To understand why, let’s go back to the “Visual Studio as email client” scheme.

Problematic Coupling

What if I told you that I had the cure for what ails you as you check your email? Now, I know it’s pretty obvious how to handle this, but I wouldn’t be much of a blogger if I didn’t explain things. All you need to do is write a Visual Studio plugin that lets you, with a single keyboard shortcut, create an empty console app, create an “email.txt” file in that app, and dump the paste buffer to that text file. And then, to handle the crashing, you just need to buy more memory. Pretty sweet, right?

Yeah, maybe not. Maybe you’d tell me that this is like treating a compound fracture in your leg by shooting Novocaine into it until your entire lower body was numb, and then continuing about your business. And, you’d be right.

The essential problem here is that this method for checking email is stupid. But, it’s stupid in a specific way. It introduces problematic and needless coupling. There’s no reason to rely on Visual Studio when checking your email, and there’s no need to launch enough instances to thrash your machine. Just use Outlook or your browser for your email.

It’s actually the same thing with this code sample, as described by a principle known as the Law of Demeter. All I want to know in my method about an intruder is whether the house is prominently lit. I don’t need to know how floors, rooms, and lights all relate to each other to know if light is showing from the outside of the house. Why should I have to change my implementation if the homeowners decide they want to rename “kitchen” to “place where Ug get food?”

Here’s what that method ought to look like, removing problematic coupling.

public class SecurityAuditor
{
    public bool WouldAnIntruderThinkSomeoneWasHome(House house)
    {
        if(house == null)
            throw new ArgumentException(nameof(house));

        return house.IsLightVisibleFromDistance(25);
    }
}

Notice that I’m not using the spiffy new operator. I don’t need to, now that I’m no longer depending on a heavily coupled implementation in which I’m picking through the guts of someone else’s object graph.

If you find yourself using this new operator, it’s a smell. Note that I’m not saying the operator is bad, and I’m not saying that you shouldn’t use it. Like Novocaine, it has its place (e.g. numbing yourself for the pain of dealing with nasty legacy code). But, like Novocaine, if you find yourself relying on it, there’s likely a deeper problem.

17 Comments
Oldest
Newest Most Voted
Inline Feedbacks
View all comments
Pandichi Alin-Ionut
Pandichi Alin-Ionut
8 years ago

Now I’m curious what IsLightVisibleFromDistance looks like!

Erik Dietrich
8 years ago

Presumably, each step in the chain would expose a method hiding the details from its collaborators.

LamiaLove
LamiaLove
8 years ago
Reply to  Erik Dietrich

Oh, cool, so the house must implement 7545634 levels of function calls and use it’s own reasoning to tell others if they can see it. So, if the thief is blind or has eagle eyes, it’s the same, because the house will tell you if it is visible from a distance or not. Let’s say the writer of the security auditor thinks of another way to check if the house is burglar-friendly. What then? Does he tell the writer of the house to add a new method? So, including phone calls and e-mails in the programming process is way better… Read more »

Dries V
Dries V
8 years ago
Reply to  LamiaLove

Well I disagree. The complex functionality is broken up in less complex blocks and placed where it should be placed. The House could ask at each Floor what their maximal outside light intensity is and depending on the level of the floor the House could add a scale to calculate distance. And then every Floor will check for rooms with outside windows and ask for their light intensity. So when the EnergyAuditor needs to be sure that the house is not consuming too much power. And one of his criteria is that the light should not be visible at a… Read more »

Graham Corless
Graham Corless
8 years ago
Reply to  LamiaLove

“So, if the thief is blind or has eagle eyes, it’s the same, because the house will tell you if it is visible from a distance or not” “Let’s say the writer of the security auditor thinks of another way to check if the house is burglar-friendly. What then? Does he tell the writer of the house to add a new method?” Neither of these objections are solved by reverting to the original code. You’re still relying on interrogating the house object for information, and if the house class doesn’t expose the information you want, you’ll have to ask the… Read more »

Erik Dietrich
8 years ago
Reply to  LamiaLove

“Let’s say the writer of the security auditor thinks of another way to check if the house is burglar-friendly.”

It strikes me that if it falls to clients of an API to figure out how to implement the API’s behaviors, the API itself isn’t bringing much to the table. I mean, it took me all of 30 seconds to create that house memory map. So, 30 seconds, call it a day, ship it, and tell your clients, “alright, if you actually want this to do anything, that’s your problem?”

gavilanch
gavilanch
8 years ago

This is kinda fine IF you want to use the law of demeter, but for small enough apps that doesn’t really need those decoupling rules, it’s just easier to use the null conditional operator. Problems and smells are contextual, there are some contexts where this, as you present it, would be a code smell, but in smaller, trivial apps, this would be just fine.

Over-engineering is also a smell.

Erik Dietrich
8 years ago
Reply to  gavilanch

I wouldn’t dispute the context-dependent nature of design decisions like this. It’s not my style to be unilaterally prescriptive in any way.

Graham Corless
Graham Corless
8 years ago

I’m in complete agreement with everything you say here, Erik. You’ve expressed precisely my thoughts about the null conditional operator.
However, most opinions I’ve seen appear to think this is the best of the new C# features, so I imagine you’ll get a lot of people telling you that you’re wrong!

SomeoneFromDeepTundra
SomeoneFromDeepTundra
8 years ago
Reply to  Graham Corless

But Graham, you are saying this as your opinion would be the only one. As someone write above, over-engineering is also a smell and should be reconsidered. Example (as everyone loves it): house can be in special case, an apartment block. So now house-object writer should write new api methods to access each flat? And what if I find 3 other examples of using this one information (about lights in room)? Do I really need to call each time other team to add special method for me? And what if house writer is not in my company, even it’s not… Read more »

Graham Corless
Graham Corless
8 years ago

“you are saying this as your opinion would be the only one” No, I’m saying my opinion appears to be in line with Erik’s. And, in view of what I’ve read of OTHER people’s opinions, I I’m guessing that the caveats expressed by Erik are NOT what most others will think. They think an easier way to check for null references in deep structural hierarchies is cool. I think helping people avoid having to worry about such hierarchies via techniques such as good encapsulation, enforcing invariants etc. is even cooler. But I accept it’s not easy. It needs requires extra… Read more »

Erik Dietrich
8 years ago
Reply to  Graham Corless

Thanks!

I’ve been part of discussions along these lines a number of times over the years, and it seems as though there’s a subset of people (C transplants, I’m guessing) that REALLY enjoy them some memory map style structures. One of the nastiest such implementations I can recall was WPF app where the root object was a singleton and the GUI bound to it and all of its children. The result these incredibly elaborate eventing structures, property changed notifications, and null checking boilerplate.

I imagine the introduction of the new language feature leads to a doubling down on this. /shrug/

Tim Gray
Tim Gray
8 years ago

It looks like you are essentially just replacing the implementation specific way to get to the overhead kitchen light with a magic number 25. Your method still needs to know which light to check. Changing that number is just as likely as changing the name of the room. And if you add all those other steps in the chain, your readability and maintainability actually goes down. If the end user asks how the app determines if an intruder thinks you are home, the developer can look in one method in the null conditional example, or they can trace down through… Read more »

Erik Dietrich
8 years ago
Reply to  Tim Gray

As best I can recall (I originally wrote this a couple of months ago, I think) that method containing the value 25 was intended to demonstrate client code. So, 25 was the client’s determination, so to speak, just intended to demonstrate that you’d offer an abstraction of an arbitrary distance. The problem with generating toy code examples like this is the lack of context. If I were looking at something like this in an actual code base, my solution wouldn’t be to start throwing null conditional operators in there, but it also probably wouldn’t be exposing wrappers at every level,… Read more »

ByronSommardahl
ByronSommardahl
8 years ago

Another way to avoid the need for the null conditional operator is to enforce a rule that “nothing ever returns nulls”. Instead, the Null Object Pattern is a lot better. https://en.wikipedia.org/wiki/Null_Object_pattern — And I really appreciate your thoughts on this. I could care less about the new c# 6 features, personally. But your description of how we many times fix the wrong problem was SPOT ON.

Erik Dietrich
8 years ago

Definitely a good solution as well, as far as I’m concerned. I’ve long been leery of the null construct and a fan of the null object pattern. The difference between “I’m intentionally making the value here ‘nothing’ and ‘oops’ is far too often not clear.”

Graham Corless
Graham Corless
8 years ago

Yep. I 100% agree with this too. See alsoMark Seeman (and probably others) ideas around a Maybe – not dissimilar but subtly different from Java’s Option – the latter being a name I prefer, I must admit.

(Sorry, no link handy, you’ll probably find it easily if you google it)