Stories about Software


Methods Are Little Stories – Abstractions Are Important 6

If Then, If Then, If Then

Yesterday’s post where I included Grady Booch’s comment that clean code “reads like well written prose” made me think of something I’ve been contemplating. The other day I was looking at some code and I saw the following (obfuscated):

I automatically started refactoring this to the following:

and then:

To me, this keeps in line with Grady’s statement and is easy to reason about at every level. “If I need an umbrella, get it” and “I need an umbrella if it’s raining, I need to leave, and I’m parked in the street” are pieces of code so simple that one need not be a programmer to understand the logic. I think it’s hard to argue that this is less conversational than “If it’s raining then if I need to leave then if I am parked in the street then grab an umbrella.”

But does this matter? Am I just being fussy and shuffling around the code to no real benefit? Are there advantages to the “ifception” approach (thanks to Dan Martin for this term)? Why would someone prefer this style? These were the things that I found myself contemplating.

The Case For Ifception?

In order to understand possible advantages or reasons for this preference, I sought to figure out the motivation. My first thought was that someone would write code this way if they missed the week in discrete math/logic where DeMorgan’s laws and the rules of inference in Boolean Algebra were covered. However, I don’t like to assume incompetence or ignorance when the only evidence present is evidence only of a different preference than mine, so let’s dismiss that as a motivation.

The second thing that occurred to me was a lack of awareness or mistrust of the compiler short-circuiting and operations. To put it another way, they believe that all three conditions will be checked even if the first one fails, so the ifception is more efficient. But, again, this requires an assumption of ignorance, so let’s assume that the author understands conditional short-circuiting.

After that a slightly more valid motivation dawned on me (and one that doesn’t assume ignorance/incompetence) – the author loves debugger! That is, perhaps the code author likes it this way because he or she prefers to be able to step through the method and see the short circuiting or success in action.

As I poked around a little more, I found code in the same class of this form as well:

Two data points now seem to point to my conclusion. I believe the motivation here is Debugger Driven Development (DDD) — a term that I’ll use to describe the approach where you write production code specifically designed to be stepped through in the debugger. This is a rather pessimistic approach since it seems to say “when you’re dealing with my code you’re going to be in the debugger… a lot… seriously, I have no idea how or even if my code works.”

I will also allow for the possibility that someone might view these approaches as inherently more readable, but I can only imagine that’s the case as a result of familiarity. In other words, this style is only more readable if it’s what you expect to see — I doubt anyone not versed in programming would gravitate toward nested conditionals and/or return statements as approachable.

If anyone can think of an additional benefit that I’m missing, please let me know. Or, in other words:

Does It Really Matter?

So having tentatively identified a motivation for ifceptions (DDD), is this style preferable? Harmless? To be avoided? I actually wrestled with this for a while before forming my opinion. The style is very different from what I prefer and am used to, but I try very hard not to conflate “I’m not used to that” with “That’s bad”. Doing so is the height of arrogance and will greatly hinder one’s ability to learn.

That said, the conclusion I came to was that this should be avoided if possible. And the reason it should be avoided boils down to method level abstractions. A method should tell a story. The method “GrabUmbrellaIfNecessary” tells a story — it tells you that it’s going to figure out whether an umbrella is needed and grab it if so. As a client of that method, you’re going to take it at face value that it does what it advertises, but if you do decide to drill into the method, you’re expecting to see a concise implementation of what’s advertised.

In the factored example, that’s exactly what you see. What better captures “GrabUmbrellaIfNecessary” than a single if condition for “DoINeedAnUmbrella” followed by a “GrabUmbrella” for a true evaluation? But what about the ifception example? Well, I see that there’s a condition to see whether it’s raining or not and then a scoped block of code with another conditional. Oh, okay, if it’s raining, we’ll get in there and then we’ll see if I need to leave in which case we’ll get… another scoped block of code. Okay, okay, so now, we need to know where I’m parked and, what were we doing again? Oh, right, we’re seeing whether we need to get into another scoped block of code. Ah, okay, if we’re parked in the street, here’s the meat of the method – grab the umbrella!

Notice that in the ifception reading, you see words like “scope” and “block”. I’m having to think of scoping rules, brackets, nested conditionals, control flow and other language constructs. Each of these things has exactly nothing to do with whether I should bring my umbrella or not and yet I’m thinking of them. If you look at the flattened early return method, a similar thing is happening:

If it’s not raining, then return. Okay, assuming we’re still in the method, if it’s not true that I need to leave, then return. Okay, now if I’m still in the method, if I’m not parked in the street then return. Okay, if I’m still in the method, then get the umbrella.

“Return”? “In the method”? What does that have to do with whether I need an umbrella and getting that umbrella? And why do I care about “not is raining” if I’m trying to figure out whether to use an umbrella? If it’s not not raining, do I need an umbrella? I think… ugh.

An easy argument to make at this point is “Erik, you’re a programmer and you should understand things like scope and early returns — don’t be lazy.” While this is true, it’s also true that I’m capable of squinting and making out tiny, bright yellow font against a white background. In neither case is that enjoyable or a good use of my time and effort when it’s possible simply to have more clarity and ease of reading.

Programmers are paid to solve problems by handling and abstracting away complexity. This applies to end-users and fellow programmers as well. It’s easy to lose sight of this and believe that knowledge of particular languages, frameworks, etc is the end goal, but that knowledge is simply a tool used in pursuit of the actual end goal: problem solving. If methods are written in such a way as to make them read like “well written prose”, I don’t have to focus on language syntax and details. I don’t have to be acutely aware that I’m in the middle of a method, figuring out scope and/or when to return. Instead, I can focus on the business logic of my problem domain and meeting user requirements.

Method writing techniques that make language syntax an afterthought are good abstractions. They hide the bare-metal, nitty-gritty details of writing C# (or any language/framework) as much as possible, exposing only enough to facilitate understanding of the problems being solved. And while it’s never going to be possible to avoid all scoping, returning and other such method housekeeping, you can certainly arrange your methods in such a way as to minimize and hide it, rather than to distract readers by calling attention to it.


Learning to Love The Linq .Any() Method

There is a Linq extension method that I managed to miss for a long time: Any(). It’s fairly natural to miss this, I believe, since the paradigm of collections tends to be fully ingrained in our way of doing things. That is, if I want to see if any members of a collection match a certain criteria, I do something like this:

I cycle through my collection of features looking for “Direct” and, if I find one, the boolean flag is flipped to true. Otherwise, it remains false. If I wanted to get a little more efficient, I could break if the flag got flipped or do this in a two-condition while loop.

This hearkens back to the days of storing values in simple arrays in older languages than C# and before the existence of IEnumerable, foreach() and other such constructs. For those of us who learned this way, iterate and check is quite natural.

But then, Linq came along and gave us the handy-dandy Where(Func<T, bool>) extension method:

This accomplishes the same thing, but as a one-liner. That in and of itself is nice, but it gets doubly nice when you consider that the short-circuit optimization is built in to the implementation of Where(). Where() returns an enumeration, which is more or less a collection (yes, yes, I understand that it’s not really, but that’s how we tend to regard it), so the natural seeming thing to do is query it for a count. I remained content with this for a long time, until I discovered Any().

I can achieve the same thing with:

This is a lot more semantically clear. I’m not really interested in an integer comparison — I want to know whether there are any features matching my criteria. It may not seem important, but cutting down on noise in the code is very important for promoting readability since, in the excellent words of Grady Booch, “clean code reads like well-written prose.” In other words, this code says “do any features exist where the id is equal to ‘direct’?” as opposed to “is the count of features with id equal to ‘direct’ greater than zero?” The former reads a lot more conversationally than the latter.

But, we have one more improvement to make here:

In terms of prose-like, conversational reading, this is pretty much still “do any features exist where the id is equal to ‘direct’?” The “where” is implied here, I would say. But, now the code is more succinct, and there is one fewer method call. Brevity is generally good.

I’ve been using this for a while now, but I still encounter old code of mine where I wasn’t aware of this little gem and I encounter code written by others who aren’t aware of it. So, if you’re one of those people, be aware of it now and use in good health!



Quick Information/Overview

Pattern Type Structural
Applicable Language/Framework Agnostic OOP
Pattern Source Gang of Four
Difficulty Easy

Up Front Definitions

The Problem

Let’s say that you work on a web based piece of software that processes orders for customers in some sort of retail capacity and that there is a framework for a lot of existing operations with which you have to deal. You’re tasked with creating the UI for taking orders from customers. At your disposal, you have three handy functionalities: an order DAO for storing order information to the database, a receipt printer for printing receipts, and an email sender for sending confirmation emails. These things exist in different assemblies and your presentation layer assembly, fortunately, has references to all of them.

So, you write some code that looks like this:

Life is good. Your customer can now issue an order through your page and the view model that supports it. Making note of your success, your manager hands you a requirement to display recommendations to the screen following an order. Luckily, there is a module called RecommendationEngine that you can use for just such an occasion:

Excellent. Well, mostly, anyway. This class’s constructor is starting to get a little busy and it depends on sort of a hodgepodge of other assemblies. But, it’s probably nothing — adding that additional assembly reference for the recommendation engine was probably just an anomaly.

Besides, your manager is so impressed that it’s time to assign you to a different screen. As it turns out, there is the traditional order placement screen and a new express one for frequent customers. So, you need to write a new screen and ViewModel, which you do. Since you’ve already implemented the order execution logic, you just copy and paste it and the field declarations into the new ViewModel, initializing the fields in the constructor. You’re not thrilled about the copy and paste, but whatcha gonna do?

At this point, your manager points out a requirement that nobody had considered: users might want to access a virtual copy of their receipts later. You notice an assembly containing a class called ReceiptDao that sounds promising, and you add that:

Well, problem solved, though that dependency list is growing. The order logic now stores the receipt. You promote it and get a defect back from QA that the express ordering system doesn’t store virtual receipts the way the normal one does. Oops. Such is life when you copy and paste program — you forgot to change the other implementation. Ah, well, happens to the best of us.

But now, you get a series of assignments from the manager to add more functionality from other modules while simultaneously adding more places from which an order can be placed. That means adding a lot of code to the two implementations that already exist, bloating them, and then copying and pasting the resulting monstrosity to various other locations.

So, What to Do?

As always in this section, it’s important to identify where things went wrong. It’s debatable as to whether adding that fourth dependency was a problem or not — that constructor is getting pretty busy at this point and the dependency fan out is climbing. However, in and of itself, that may not be a problem.

It stops being debatable (as it always does) when they copying and pasting starts. That’s bad. Any time you find yourself doing that you’re failing at design. In this case, the most obvious fix is the one that will lead us to a better path. Specifically, we need to factor the duplicated code into a class. So, let’s introduce a class called OrderService:

Now, this service class can be used by any ViewModel client without duplicating logic. Here’s what the client now looks like:

The only duplication that might occur here is injecting and invoking the service (though that can be fixed among the ViewModels, but that’s beyond the scope of this post). We’ve eliminated the duplication that makes things like adding the receipt storage a pain point. Also, observe that if we put the service in a separate assembly from the ViewModels, we can now remove the assembly references to the various lower level services such as EmailSender, ReceiptPrinter, etc.

While this separation may seem like pushing our problems out a layer, there is a real benefit to doing this. Specifically, presentation layer classes should not be concerned with locating lower level components. They should be allowed to focus on marshaling data for presentation. Following the single responsibility principle suggests that presenting order confirmation to the user and navigating the mechanics of the subsystems required for an order are two separate responsibilities.

The five (and growing) dependencies that have been added to the constructor of the service to spare the ViewModel are still problematic in terms of good design. This could be fixed with further facades, if the application is large enough to support conceptual sub-layers. That is, there could be a general DAO facade and a general externalities facade that took care of the dao functions and email/receipts/recommendations respectively. This scheme would have the service knowing only about two collaborators instead of five while deepening the call stack hierarchy.

Another approach might be to consolidate some of these assemblies or classes at lower layers, if possible. It might also be reasonable, depending on the situation, simply to leave this service class alone, having abstracted this sub-system out of the presentation layer.

A More Official Explanation

DoFactory defines Facade as a pattern that aims to:

Provide a unified interface to a set of interfaces in a subsystem. Façade defines a higher-level interface that makes the subsystem easier to use.

This is pretty straightforward to understand and it constitutes one of the most basic and fundamental patterns of programming. Facades provide a layer of abstraction, hiding low level details from client code in favor of broader, easier to understand interactions. I have blogged extensively about the subject of abstraction, if you’d like a more in-depth treatment of the concept.

In general, facades form the back bone of solid architecture in code bases by virtue of hiding conceptually difficult details behind conceptually simple points of access. The crux of this pattern is hiding and managing complexity.

Other Quick Examples

Other incarnations of the Facade Pattern include:

  1. The iconic example of mortgage applications where lenders, banks, inspectors, etc are all necessary to complete the single process.
  2. A unified or single class that you write to hide an ugly third-party API or series of APIs.
  3. In a very real sense, the structure of a class is a bit of a facade since implementation details (private fields and members) are hidden behind the facade of the public API.

A Good Fit – When to Use

Facade is good any time you want to hide details of a process from client code. This can apply, as I mentioned, at the assembly/module level and it can even apply at the class and method level. If you practice good software design this is a pattern that you will find yourself using constantly. It is the essence of providing good abstractions.

Square Peg, Round Hole – When Not to Use

The main usage anti-pattern of a facade is providing a layer or “wrapper” that really does nothing or fails to simplify things for clients. There can be a fine line between organizing code and hiding details and introducing pointless layering, so it is important to consider whether the facade that you’re adding is either (1) making things easier or (2) hiding details that should be hidden. If it’s doing neither of these things, it probably shouldn’t exist.

So What? Why is this Better?

Generally speaking, facades can help eliminate redundant logic and they can simplify interaction with a system. These are two of the most important considerations in all of software. Redundancy is the bane of maintainability and abstraction is the backbone of understandable systems. Well placed facades (such as layered architecture, for instance) provide both in one shot. The end result is that your code will be well factored and decoupled, easy to maintain, and easy to reason about.


Faking Tomcat Support in IntelliJ IDEA Community Version

A while back, I blogged about my switch from Eclipse to the community (free) version of IntelliJ IDEA. After that, I went on a number of trips out of town, felt out the job market a bit, and generally engaged in a series of activities that have kept me too busy to work on my home automation. In the last week, however, that’s changed and I’ve revisited it.

The first thing that I decided to do was play with Ant a bit to create an environment where I could build and run the application locally. I’m in a bit of a catch-22 with IntelliJ at the moment. I might be interested in purchasing the Ultimate Edition, but not before I take it out for a few months and kick the tires. But, in order to do that without pain, I kind of need the server support for what I’m doing, which is only available in Ultimate. So, I’m faking it. I created the following Ant build.xml:

The other build targets are somewhat standard, if memory serves (I was never particularly adept with Ant, and now I’m several years removed from using it). But, the interesting one is the “war” target, which I’m probably going to rename here shortly. This first compiles the project into a deployable WAR file and plops it into the local Tomcat webapps directory, which is where the it needs to be for me to run it locally in a browser.

With that in place, I’ve added the next line, which starts the web server. In the way I invoke it, a command window pops up, and I can stop the server by closing it. Once the server is started, I launch chrome to the URL where the application will be. This works, but I’d like to refine it a bit. Here are some things that I think should happen:

  1. First of all, DRY isn’t just for code. I’m defining “Daedalus.war” and “http://localhost:8080/Daedalus” and if I decide to change the deployable name, there are two failure points. I’m going to correct that.
  2. And, as long as I’m changing the “destfile” property of the war file, I might as well define a descriptive constant for the Tomcat webapps directory.
  3. Secondly, “browser” isn’t a particularly good property name for Chrome, so I’m going to rename it.
  4. Along those same lines, I’m going to create a property for Internet Explorer as well
  5. I’m also going to create build targets for chrome and internet explorer so that I can choose which one to run without modifying the build.xml file.
  6. Finally, I’m going to decouple the building of the War from the starting of the server and decouple both of those from opening a browser. That way I won’t have to repeat myself in the internet explorer and chrome versions.

Here’s what it looks like now:

So, I’m relatively pleased with this for the time being. Two small caveats with this approach, though. The first is that I don’t really like duplicating the “http://localhost:8080” as the server’s localhost, but I’m unlikely ever to change this, and if I abstract that out to a constant, it fails for some reason that I haven’t seriously looked into (and realistically, probably won’t for now). The second is that I start the server, but you have to manually stop it. That’s kind of ick, but I’ll live with it for now. If/when I put together a better scheme, I’ll post it.

But, with this you get support for Tomcat with IntelliJ IDEA community edition. It’s obviously not nearly as snazzy as having it integrated to the IDE, but if you’re in my boat, this is nice. I’ve automated most of the stuff that I’d rely on the IDE to do, and I’ve done it without spending a few hundred dollars I’m not yet sure I can justify. Cheers if this helps you.


You Need CodeRush


The other day, I was chatting with some developers and one of them pulled me over to show me the following code:

I did a double take and then smirked a bit, pointing out that this was probably dead code. I asked the person who was showing me this to do a “find all references” and there actually was one:

So, we then backed it up a step and discovered that this was an event handler for an event declared in XAML (this whole thing was in a code-behind file). So, every time some event occurred (some sort of navigation thing), the code would cast the sender as a page, call a method with that page, and then return if the page was null, and also return if the page wasn’t null.

How Did It Come To This?

Looking at the SetLayout method, I quickly developed a hypothesis as to how the code wound up this way. The “SetLayout” method probably checked the page for null as a precondition prior to performing some sort of action on Page. At some point, that action became undesirable and was removed from the code, but whoever removed it didn’t realize that the precondition check was now meaningless, as was the whole chain of code above it for the event handler. This was confirmed correct by a quick glance through source control.

Here’s what this looks like in my IDE:

Do you see the dashes on the right, next to the scroll bar? Those are the issues in the CodeRush “issues list”. Do you see the squiggly underline of “SetLayout” (hard to see at this resolution, but you can zoom in or open the image in a new tab if you want)? That means there’s an issue here. In the interests of full disclosure, this is actually to tell me that the method could be static (it operates on no instance member of the class) rather than to tell me that the method has no effect on anything, but the important thing is that it’s underlined. It draws my attention to the method instead of encouraging me to skip over it while looking for something else here. And, as soon as this method catches anyone’s attention, it will obviously be deleted.

The Value of Productivity Tools

In general, the Code Rush issues list is a great companion. It alerts me to dead code, undisposed disposables, redundant statements, and plenty of other things. If you pause any time it tells you about an issue, you will become better at programming and you will start to recognize more problems. It isn’t necessarily superior to using, say, Style Cop or any other particular tool, but it is very convenient in that it paints the problems right inline with the IDE. They’re quite noticeable.

Code Rush’s competitor, Resharper does this as well. I plug for Code Rush because I use it and I love it, but if you get Resharper, that’s fine — just get something. These productivity tools are game changers. When looking at this code in someone else’s IDE, there was no squiggly and no immediately obvious problem if you’re casually scrolling through the class. But in my IDE, my eyes are drawn right to it, allowing and in fact demanding that I fix it. This sort of boy-scouting makes you better at writing your own code and better at fixing other people’s.

I forget what the license costs, but your employer can afford it. Heck, if they’re too cheap, you can afford it. Because what you can’t afford is to have code checked in under your name that looks like the code in this post. That’s no good, and it’s avoidable.