Let’s Look At Bad Code

One of the best ways to learn how to program well is to look at examples of ‘bad’ code and understand why they are problematic. That’s what we’ll be doing today.

A few years ago, I came across today’s example of not-so-great programming. Since I value my clients’ confidentiality, what you’ll see is a mere reproduction of the questionable parts, as I recall them.

OK, here we go:

The listing is of an ASP.NET WebApi controller action.

CustomerDataAccess represents a controller-wide reference providing access to a customer database.

GetCustomerById() is a method in a base controller class. Here is the definition for it:

If I recall correctly, in the original system, the method to retrieve the customer, GetCustomerById(), was called many times from the controllers.

As you can see, the customer’s id comes into the controller action as a parameter. Or it could have come from HTTP header values or whatever—it doesn’t matter. The important thing is that we have access to an identifier for the customer. And, even though the id is of type string, it contains either an integer or a Guid, a Globally Unique Identifier. Why? I remember the system was in the process of being migrated from database integer identifiers to Guid identifiers.

To retrieve the customer from the database via CustomerDataAccess, a programmer chose to pass in the two data access method delegates, one to get by integer and another to get by Guid. Inside method GetCustomerById(), one of the two delegates will be called depending on whether the identifier is an integer or a Guid. 

Does this code feel a bit strange? I hope it does. It’s an odd implementation. It’s problematic in at least two ways:

1. What if we had to include a third type of id besides integer and Guid? Wouldn’t that mean passing in another data retrieval method? Yes, it would. Such a modification would alter the signature of GetCustomerById()—if there are 97 calls to GetCustomerById(), we would need to make changes in 97 places! Ouch! 
2. What is this data retrieval detail doing in the web layer? There isn’t anything web. Should we be doing this data retrieval-specific work in the controllers? No, of course not.

OK. What would be a better design? 

How about this approach: 

Let’s have a new method, GetById(), on CustomerDataAccess which retrieves a customer by string id. We then push all the parsing code that currently lives in GetCustomerById() into GetById(). Internally, CustomerDataAccess.GetById() will still call GetByIntId() and GetByGuidId() but these methods, hopefully, no longer need to be exposed. 

A benefit of this new approach will be that changes to how we parse the identify values no longer affects the WebApi controllers. The web controllers are isolated from changes to how the data layer identifies entities: