Programmers could learn a thing or two from bridge engineers

I was recently watching a documentary that described the engineering and design of bridges.  These bridges, some of them 80 years old, have been in constant use from the moment they were built.

There really was no cookie cutter recipe.  For the most part, each bridge was a custom design job that resulted from the engineering best practices at the time it was designed, the available materials, and the opinions of the engineers.  Sounds a lot like the software development.

One thing that didn’t sound like software at all was the attitude toward maintenance.  The bridge engineers all had respect for the original design and applied their skills to maintain it.  They would incorporate new techniques and materials where it supported the intent of the original designers, but there was no push to “refactor” the bridge.  As a result the bridges keep working, fulfilling their original purpose and providing value every day.

Imagine how this might be different if each new engineer on the project came in and said “This bridge design is crap.  I’m surprised it even works.  We know so much more about the right way to build a bridge now.  We’re going to have to gut this architecture and update it to implement current best practices.” 

How long do you think that bridge would remain in service?  Even worse, how reliable do you think that bridge would be after a new set of engineers came in and tried to implement a brand new architecture, producing a mish-mash of the old and the new.

It sounds like an obviously bad idea when talking about bridges, but for some reason we don’t give it a second thought in the software world.  We have a chronic lack of respect for those who came before us. We’re constantly rewriting code THAT IS WORKING for no better reason than we don’t like the design.

I can look back at my career and see multiple instances where I’ve been the chief refactor offender, rewriting code that was working, even entire application architectures because I thought they were written poorly.  It’s easy to do.  There’s no shortage of legitimately bad code or developers who rush to implement the latest tools and ideas without really understanding what they’re doing.

These days I’m a little older and a little wiser. I try to keep 2 important principles in mind

1.  Software is not an end unto itself. 
Our end customers don’t really care if our services are perfectly RESTful, or if we’ve properly implemented dependency injection throughout our app.  They care if it works.

2.  There’s always a reason the developer who came before me did things the way they did. 
Here’s a cycle I’ve gone through countless times.  I see code that works but it’s stupidly implemented, what was this idiot thinking.  It’s going to take me an extra hour but let me fix this nonsense.  Ok I’m pretty much finished and wait, this key use case doesn’t work.  I can fix it, but now the rest of my refactor is broken.  Well if I do this and this it will work, but…. oh… it pretty much works the same way it did to start with. 

Chaining the C# Null Coalescing Operator

This is something that came up in the comments last week.  I was refactoring some code and wound up with the  accessor method below that performs multiple null checks while trying to assign a value to _currentUser.

// CurrentUser

 private WebUser _currentUser;

 public WebUser CurrentUser

{

   get

   {

     if (_currentUser == null) _currentUser = GetWebUserFromSession();

     if (_currentUser == null) _currentUser = GetWebUserFromTrackingCookie();

     if (_currentUser == null) _currentUser = CreateNewWebUser();

     return _currentUser;

   }

}

Now this code is pretty clear, but a reader named Brian pointed out in a comment that we could shorten the code a bit by using the Null Coalescing operator “??”.  If you haven’t used the Null Coalescing operator yet, check it out.  It’s great for data access code where a method may return data or a null value.  The basic syntax is illustrated in this block of code from msdn:

   // y = x, unless x is null, in which case y = -1.
   int y = x ?? -1;

   // Assign i to return value of method, unless
   // return value is null, in which case assign
   // default value of int to i.
   int i = GetNullableInt() ?? default(int);

So, this is really cool, but there’s more. What Brian pointed out was that you can chain the ?? operator to do multiple null checks.  So, my block of code above can be rewritten like this:

// CurrentUser

 private WebUser _currentUser;

 public WebUser CurrentUser

{

   get

   {

     _currentUser = _currentUser ??

                    GetWebUserFromSession() ??

                    GetWebUserFromTrackingCookie() ??

                    CreateNewWebUser();

     return _currentUser;

   }

}

Note that normally the _currentUser assignment would fit all on one line but due to the width limitation of my blog I broke it up into multiple lines.  So, C# Null Coalescing operator chaining. I like it. I’ll be adding it to my toolbox.

 

Addendum:

Hey, you guys pretty much hated this change to the code. I got no blog comments but I received quite a few emails about this technique and the prevailing opinion seems to be that the original syntax with the multiple if blocks was clearer.  In fact, not a single person who emailed me liked the ?? method. So I took another look and I think I agree.  The original code does seem a little clearer to me and I don’t really think I was able to make my code any shorter or simpler by using ??.  So this may not have been the best example, but I do still think this is a very cool technique for the right situation. As always, I just need to make sure that I’m refactoring because the changes make the code cleaner, not just because they’re clever.

SOLID C# Code: Smaller Methods == Clean Code?

I’m a big fan of Robert Martin.  His book on Agile Patterns in C# is still one of the three most important programming books I’ve ever read.  If you listen to Uncle Bob for any amount of time, it won’t be long before you start hearing terms like SOLID Principles and Clean Code.  These are concepts that are widely known, but still a bit tough to define.  What exactly is clean code?  What does SOLID code look like?

We’ll I’m not smart enough to provide a definition that’s any better than what’s already out there, but I can point at an example and say “that smells pretty SOLID”.  So, below is an example of some code that I wrote for an ASP.Net MVC application.  CurrentUser is a property that wraps an instance of my WebUser class, which represents, you guessed it, the currently logged in user for my web app. When the CurrentUser property returns a user, there are four possible places that it might have to check to find that user:

1. There may already be a WebUser instance in the private member _currentUser,
2. There may be a WebUser instance stored in the Session,
3. There may be a TrackingCookie in the HTTP Request that can be used to get an existing WebUser,
4. We may have none of the above, in which case we have to create a new WebUser.

So, with that said, let’s take a look at my first version of this code.

// CurrentUser

private WebUser _currentUser;

public WebUser CurrentUser

{

    get

    {

        // Do we already have the CurrentUser?

        if (_currentUser == null)

        {

            // Try to get the user from Session

            Object obj  = HttpContext.Current.Session["__currentUser"];

            if (obj != null)

            {

                _currentUser = (WebUser)obj;

                return _currentUser;

            }

            // Try to get the user from a TrackingCookie

            SecurityHelper secHelper = new SecurityHelper();

            WebUserRepository rep = new WebUserRepository();

            if (secHelper.TrackingGuid != Guid.Empty)

            {                      

                _currentUser = rep.GetWebUserByTrackingGuid(secHelper.TrackingGuid);

                if (_currentUser != null) return _currentUser;

            }

            // If we still don't have a user then we need to create a new

            // WebUser with a new TrackingGuid.

            WebUserFactory factory = new WebUserFactory();

            _currentUser = factory.CreateWebUser();

        }

        return _currentUser;

    }

}

 

 

Hmmmmmm. Not too horrible, not too long and ungainly, but it definitely has some code smell (the bad kind).  First, I’m not thrilled about the multiple returns nested in conditional blocks. Also, the code is doing a number of different things that I felt needed comments to explain.  Now I love comments, and I’m a firm believer that when in doubt, comment.  But I’ve also come to realize that it is possible to design code in such a way that it can be just as clear, and just as understandable, without the need for comments. 

So how do we reach this promised land of understandability? The main technique that I’ve been using is simply to take my big blocks of code that do several different things, and break them up into several separate, clearly named methods, and then just call them from my main block. Here’s how I applied this technique to my CurrentUser property.

 

 

// CurrentUser

 private WebUser _currentUser;

 public WebUser CurrentUser

{

   get

   {

     if (_currentUser == null) _currentUser = GetWebUserFromSession();

     if (_currentUser == null) _currentUser = GetWebUserFromTrackingCookie();

     if (_currentUser == null) _currentUser = CreateNewWebUser();

     return _currentUser;

   }

}

 

 

// GetWebUserFromSession

private WebUser GetWebUserFromSession()

{

     Object obj = HttpContext.Current.Session["__currentUser"];

     return obj == null ? null : (WebUser)obj;

}

 

// GetWebUserFromTrackingCookie

private WebUser GetWebUserFromTrackingCookie()

{

     SecurityHelper secHelper = new SecurityHelper();

     WebUserRepository rep = new WebUserRepository();

     if (secHelper.TrackingGuid == Guid.Empty)

         return null;

     else

         return rep.GetWebUserByTrackingGuid(secHelper.TrackingGuid);

}

 

// CreateNewWebUser

private WebUser CreateNewWebUser()

{

     WebUserFactory factory = new WebUserFactory();

     return factory.CreateWebUser();

}

 

Now I realize that in this second version I wrote more code than the first, and I do a couple of extra null checks in my main property code, but look at how easily understandable everything is.  I mean really. There’s not a single comment, but can you imagine anybody not understanding exactly what’s going on in this block?

   get

   {

     if (_currentUser == null) _currentUser = GetWebUserFromSession();

     if (_currentUser == null)  _currentUser = GetWebUserFromTrackingCookie();

     if (_currentUser == null)  _currentUser = CreateNewWebUser();

     return _currentUser;

   }

Plus, because I factored out methods like GetWebUserFromSession() and GetWebUserFromTrackingCookie() I can now use those methods in other parts of my class without having to rewrite the functionality.  So overall, I think this version smells much more SOLID.  What do you think?  If anyone has ideas or favorite techniques for how to get more SOLID, please leave a comment.