Archive for category Clean Code
I’m often asked about comments in code: when to do it, how to do it, what to put, etc. I’ve recently run into Steve’s post about when to comment your code, left a comment (!) there, and we got to expand our conversation in his podcast/screencast (link at the bottom). I’ve decided to create this post to consolidate the links and info shared during the interview, to make it easy for folks to find the material.
I remembered writing blog posts a couple of times over the years and it’s interesting to see how my opinion on this subject has changed over time.
The first post goes all the way back to 2005, with me asking “can you plesae put some comment on that Regular Expression?”. 15 years later, I still ask my smart friends to get me the RegEx I need, along some comments as to what each piece does!
In 2007, I was big into using Xml Comments, GhostDoct, and Documentor… I’m not anymore, as documented 10 years later with my post “XmlDoc Comments: Auto Generate and Hide the Clutter”. In nutshell, if we’re documenting a public API, yes, by all means let’s put in that documentation, but making it count: commenting the GetCustomers endpoint with “Gets the customers” doesn’t add any value to the effort!
My practice of making comments stick out as a sore thumb posted in 2010 still stands in 2020: I still set up all of my IDEs in that manner and it still produces exactly the outcome as I intended.
When I do want to drop a quick TODO comments in code (watch/listen to the podcast interview when it’s up to know why I might do that), I have templates on my IDEs to automate that: ReSharper in Visual Studio, User Snippet in VS Code, Live Template in RubyMine.
In regards to code that’s commented out, like so:
We should be using a source control system; if we ever want that code back, we have a way to bring it back. So just remove it!
Still using the example above, notice that each if-block is preceeded by a comment. Is it really necessary? How about removing the comment and extracting the expression into a method that tells us the question being asked?
There’s also the “narrator-style” comment:
Narrating every single line of code is very annoying (by the way, I think I wrote the code above many moons ago). If the comments were written initially as a placeholder for the steps that needed to be implemented, let’s make sure to get rid of it when we’re done.
Last but not least, some people say (I’ve said it myself) that comments should document “why” the code was written in such manner. I’d propose a variaton to that: how about documenting the why with some good specs (or tests, if that’s how you prefer)?
Now, I’m not refering to tests that look like this:
Such test doesn’t tell us the “why”; it tells us the “how”. I mean this kind of test (now you’ll see why spec fits better):
Summing it up, this is how I prefer to “comment” code:
- Given-When-Then specs
- Writing code English-first
If I do have a real need to drop an actual comment in code (“why do we have this query in the code that has to potential to perform badly”), I’ll probably drop a quick comment, with a link out to the issue tracker, where I’ll put more context about why the code was left like that, and where a Product Owner can decide when it’s appropriate to address the situation.
Any comments? 🙂
And for audio-only version:
A few weeks ago I mentioned I was starting to learn React. I’ve finally finished a few days ago the PluralSight course mentioned in that post. The course was good for me to get my feet wet in React. However, as I stumble across React code in the Real World, I just knew I’d continue to see Spanglish in Programming.
See this example:
As I read the code above, specifically the bit indicated with red arrows, this how it sounds in my mind:
“if page type is greater than or equals to this constant and… wait, does the thing following that && actually evaluate to a boolean?!”
I have no sympathy for code that mixes different language in the same file. It gets even worse when the code mixes different languages in the same function, method, etc.
That reminds of the day XML literals were added to VB, and of course, people start using it in the worst possible ways. The ability to mix different languages can be very powerful, but to me, there should some separation so that the brain doesn’t have to deal with so much context-switching.
Anyway, the example above doesn’t look like clean code to me. I’ve asked a fellow co-worker who’s more experienced in React, and he showed me how the code above could be cleaned up: simply extract the bits that create HTML into its own functions.
I believe every developer should always ask: “Is this readable? Is this clean? Is there anyway I can make it better?”.
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.