Archive for category Clean Code
In October of 2009 I did a presentation on Clean Code for the HDNUG. I just found a video recording of that and decided to make it available to those who care. Enjoy!
It’s usually not easy to find good examples that explain the Liskov Substitution Principle (LSP) to developers. But really, how hard could it be? The definition is so simple:
“What is wanted here is something like the following substitution property: If for each object o1 of type S there is an object o2 of type T such that for all programs P defined in terms of T, the behavior of P is unchanged when o1 is substituted for o2 then S is a subtype of T.”
Say what? I don’t know about you, but that kind of definition usually flies way over my head. Some people use the Rectangle versus Square example to explain the principle, which is a good one, but that doesn’t necessarily relate to things we normally do.
I like Uncle Bob’s definition a lot better:
“Functions that use pointers or references to base classes must be able to use objects of derived classes without knowing it.”
Recently we’ve run across a violation to that principle in a project. We have an interface defined like so:
It’s a very small interface that represents resources that can be loaded in memory, and persisted afterwards in case there were changes to it.
Let’s pretend we have the following implementations of that interface:
The actual implementation of those methods doesn’t matter here; just assume that the real implementation loads and persists application and user settings.
Somewhere in the application we have some way to retrieve a list of instances of implementations of that interface, kind of like this:
Some place else, we have a method that takes in a list of those objects, and call Persist on them:
And somewhere else we may use those methods, like so:
Everything works great, until a new class is added to the system in order to handle, let’s say, some “special settings”:
It looks like the Load method does whatever stuff it’s supposed to do in order to handle loading these special settings. The Persist method, on the other hand, throws a NotImplementedException. As it turns out, those settings are meant to be read-only, therefore, the Persist method can’t really do anything.
The system is told to load the new class along with the other ones that implement that same interface:
Now when we run the app everything should still work fine, until we hit the code that tries to persist all of those loaded resources, at which point we get a big and fat “NotImplementedException”.
One (horrible) way to address this would be to change the SaveAll method:
If the specific resource being processed is of type SpecialSettings, we skip that one. Brilliant! Well, maybe not. Let’s look back at a simplified definition of the Liskov Substitution Principle:
“An object should be substitutable by its base class (or interface).”
Looking at the SaveAll method it should be clear that “SpecialSettings” is NOT substitutable by its “IPersistedResource” interface; if we call Persist on it, the app blows up, so we need change the method to take that one problem into consideration. One could say “well, let’s change the Persist method on that class so it won’t throw an exception anymore”. Hmm, having a method on a class that when called won’t do what its name implies is just bad… really, really bad.
Write this down: anytime you see code that takes in some sort of baseclass or interface and then performs a check such as “if (someObject is SomeType)”, there’s a very good chance that that’s an LSP violation. I’ve done that, and I know so have you, let’s be honest.
Another great definition for LSP comes from this motivational poster that the folks at Los Techies put together:
So what’s the fix?
The fix here is to tailor the interface based on what each client needs (Interface Segregation Principle, or ISP). The LoadAll method (which is one client of those classes) is really only concerned about the “Load” capability, whereas the “SaveAll” method (another client) is only concerned about the “Persist” capability. In other words, these is what those clients need:
The SaveAll takes in something tailored to its needs, IPersistResource’s, and the same goes for LoadAll, which only cares about ILoadResource’s (in the real app, the actual instantiation of these classes happen somewhere else). This is what the granular new interfaces look like:
Yup, it’s pretty much the former “IPersistedResource” split up into two separate interfaces, tailored to their client needs. Both the UserSettings and ApplicationSettings classes can implement these two interfaces, whereas the SpecialSettings class would only implement ILoadResource; this way, it isn’t forced to implement interface members it can’t handle.
Very often people ask what’s the most appropriate number of members in an interface. In the real world example I gave here, the original interface had only 2 members; one could say that was small enough, but as it turns out, it wasn’t. The IPersistedResource interface was doing too much (both loading *and* persisting stuff) based on the clients that use its implementers. In the end, two interfaces with a single method on them fit the bill a lot better. Interfaces with single responsibility? Yup, Single Responsibility Principle (SRP); as with design patterns, sometimes SOLID principles go hand in hand together.
Also check out A Good Example of Open-Closed Principle!
The other day I was pairing with my buddy and co-worker Alan Stevens. We were reviewing some code, and at one point he said: “Claudio, those comments are ugly!”. At first I thought he meant how comments were worded, but then I realized he meant the way the comments were formatted:
You can see in the snapshot above that something really stands out, right?
Comments (which include code that’s commented out, but *not* XML Comments) are formatted in my environment in a very nasty way, and that’s on purpose. The thing is so ugly, it forces me to read it and find out what it’s all about:
- It may be a TODO, so I want to make sure it won’t be forgotten (very often, a TODO is dropped in code to indicate some Technical Debt);
- It may be code that’s commented out that should have been removed in the first place, instead of be sitting there as waste, polluting our code base with useless, distracting things. “But what if we ever need that code back?”, one might ask. Well, hello-oh, it’s 2010; people better be using some sort of Source Control System!!
- It may be the means of a developer to explain some poorly written code; most times a simple “rename variable” or “extract method” refactoring can easily remove the need of the crappy comment;
- In very rare situations, the comment is actually useful in explaining the intent of the code, so it’s a good thing that it stands out.
It comes a time when code may be tainted with so much useless comment that the mind just starts to ignore the warnings. What do I do? Just change the coloring to some other new ugly combination.
How about you: do you have similar approach regarding how you see comments in the code? Do you configure your environment so to have the editor visually indicate anything you deem important?
I’ve been doing a lot of reading about how to fight code smells and how to write more cohesive code. One of the tools that I use to help me finding out code smells is DevExpress‘ Metrics window (it comes with RefactorPro, but I know they also provide that plug-in for free).
I always have that window open in VS, and as I work through source code I can "see" the smell whenever I notice the graph bars going into the yellow or red areas.
Inspired by the modern amusement parks, I’ve been thinking about developing a device to push that experience even further. You know when we go to those movie theaters where they try to make you really get into the movie, and they do so by spraying water on you and things like that?
So, I’m thinking about creating this device that one attaches to the computer, and as the developer "walks" into smelly code, a bad smell pours out of the device, right into the developer’s face. Wouldn’t that be a nice 3-D experience? Of course we could also have the device sprays some nice fragrance in case the code smells right. 🙂
I’m thinking about naming this device as Smell# (pronounced Smell Sharp):
Imagine people getting used to the other developers’ bad smell: "Hey, I’m pretty sure Joe has been here… this code smells bad just like him", or "look, this can only be Tina’s code; I could smell that perfume from miles away!".
Of course, some people would get really upset about having to feel somebody else’s bad smell. However, this should eventually help with pushing everybody into taking care of their smells. Nobody would like to go into a meeting with other developers and hear something like "hey dude, what have you been eating? Nobody can stand you smell… everybody avoids your code just like we’re avoiding a plague…".
Ok, that’s the idea. I haven’t put a patent on it yet, so if you decide to go ahead and build this, please make sure mail me the checks. Thanks. 🙂
All joking aside, I remember working on this project years ago where I was maintaining somebody else’s code. The guy used to use to name his variables after bad words. Oh boy, you could find some real nasty variable names in there. Imagine the user getting an error message that reads something like "Penis not found" (I’m keep it mild so to not get this post categorized as PG-13 or something…).
I mean, the days when developers would use some cryptic names for variables, methods, properties, classes, etc., are (or should be) long gone. Applications are not a black box anymore, where the end-users wouldn’t ever see the underlying code for it. Nowadays, programming is much less of a black art, and most of the time the customer you’re building an application for will be looking at your code, so you better write it as professionally as you can. No bad words, no in-house funny remarks, nothing like that is allowed.
I feel like this is a topic I’ll be posting more about it, just because I’ve been feeling really strong about it.