Code Smells

Welcome to the second edition of Code Smells! Periodically I’ll be posting about how to detect code smells and what they mean in terms of the big picture of your code. The previous installment can be found right here.

In computer programming, code smell is any symptom in the source code of a program that possibly indicates a deeper problem. Code smells are usually not bugs—they are not technically incorrect and don’t currently prevent the program from functioning. Instead, they indicate weaknesses in design that may be slowing down development or increasing the risk of bugs or failures in the future.

Onto the code smells!

The Stink List

Code Smell #4: (Thanks to reddit user fkaginstrom) You have an large number of parameters being passed in to your function call. Functions that take in a ton of parameters stink for a few reasons. How many is too many though? This is a topic that people have debated all over The Internet. This Stack Overflow answer even quotes an author saying to never have more than three parameters in a function. In my opinion? There’s no fixed number. It’s going to vary from situation to situation, project to project, class to class, and method to method. Putting a fixed number on it is sort of setting up a rule to be broken.

What can you do to avoid this kind of smell? This C#-based Stack Overflow thread has a bunch of great ideas. One simple solution is just to bundle things into logical groupings of data. An example (although, it’s potentially a poor example since it’s only two parameters) is x and y coordinates. You can bundle these into a custom point type and pass this into functions. Now a function that may have taken four pairs of coordinates is reduced from eight parameters down to four. This approach also introduces the dependency on your custom type for your function, but I’m just offering it up as an option. If you’re always passing around the same group of X pieces of data around, it may make sense to bundle them into a single container type.

A side effect of reducing the number of parameters your functions require is readability. It might seem minimal, but having functions with only a handful of parameters keeps them from becoming unwieldy and much easier to understand when scanning through code. Readability is sometimes overlooked by developers, but when you’re in a team (and most developers work in teams), it goes a long way.

Code Smell #5: (Thanks to reddit user fkaginstrom) Your class has a large number of methods. If we keep the Single Responsibility Principle in mind (which states that a class should have one reason to change), it’s a warning sign that we might be creeping in on violating it. How? If more and more methods keep getting added, more responsibilities/capabilities can sneak in. This MSDN blog article also highlights some examples of the Single Responsibility Principle. Essentially, as the methods within your class grow in numbers, your class becomes responsible for more types of things. If you later on want to use just one of those things in a different context, you’re now required to use one big heavy-weight type. Of course, this heavy-weight type comes with it’s own bundle of dependencies, setup requirements, and so on.

How do you avoid this? You can start by refactoring your monstrous type into multiple types. If your type has 12 methods that it defines, and they fall under three general categories of functionality, consider making three interfaces to group the functionality. Then you might consider adding three classes that stay true to these interfaces. The MSDN article I mentioned before does a good job of explaining how this kind o approach works.

Code Smell #6: Your single method has grown to hundreds of lines. This is one code smell I find that newer programmers introduce more frequently than experienced programmers. However, when you’re working on an enormous code-base, sometimes this type of thing sneaks right up on you. So what’s the problem with having one method do a ton of things? It’s a convenience, isn’t it? let’s say someone only has to call one method that can launch a rocket, play golf, and invest in the stock market while filming a block-buster movie. That’s power and ease of use, no?

This related to Code Smell #5, in my opinion. The convenience of being able to call a method that does all sorts of fancy things at once is the exact inverse of the problem you face when you want to test the method. If I just want to test that I can successfully start the burners in the rocket, I have no choice but to call the method that does everything. What makes this problem even worse is that once your code has been structured this way, breaking down big methods into smaller methods can prove to be a challenge. When you see how dependencies are passed down the call hierarchy, or where certain classes have knowledge of others, things become scary.

I’ll give one real life example of something I saw recently in a particular code base. A test had to be written to cover a problematic area of code that had been refactored. There needed to be some sort of verification in code that proved this section was behaving as expected under particular conditions. Great stuff. Except the section of code existed inside of a method that did the following:

UI interaction

Database read

Data processing

File read

Data processing 2

Database write

External disk operation* (This one was pretty specific to the project I’m describing, but it wasn’t just a simple file read/write)

File write

UI interaction

Where the highlighted “Data processing 2” is the section of the method that needed testing. How’s that for fun? In order to test this one section properly it required refactoring of all of the encompassing code so that we could test it as a unit.

Have your own code smells? Share them in the comments. Follow Dev Leader on social media outlets to see code smell updates as they come out!

Background

I thought this might be kind of fun (fun can also be read as “upsetting”), so I’m giving it a shot. It’s pretty frequent as programmers we go back and revisit some code and find ourselves shaking our heads at what we see. These code smells often don’t show their faces when they’re being created, so don’t beat yourself (or anyone else) up just yet. Common signs you’ve stumbled upon a code smell are when you find yourself saying:

How could that co-op have possibly coded this?! Blast those interns!

Or

What the heck was John thinking when he put this together?! Does he not have a brain?!

Or

No wonder we find so many bugs in this part of code! Look what Jane did!

But it never truly hits home until you get one of these:

What is this crap?! This is by far the worst code I have ever seen. How cou–Oh. Wait. I did that.

Code is always a work in progress. If it’s not, it’s because you’re writing a one off script or your code doesn’t do much of anything. Our skills as programmers are always transforming as are our perspectives. You’re guaranteed to have one of these moments if you’re programming long enough and look back on your code that was once The Pinnacle of Awesome.

With that said, I’m hoping to share some code smells that come up as I see them in my own projects or when talking with friends/colleagues. You might be about to type up one of these code smells, so pay attention! I don’t know how frequently I’ll put one of these posts together, but I might as well start now. Every time I get a handful of code smells I’ll try to push something out to The Interwebz.

The Stink List

Code Smell #1: Your variable is named or prefixed with “temp”, “tmp”, or some variation of “temporary”. This is unnecessary. If you have a variable, by definition it’s something that’s temporary. Nothing in code lasts for forever. You’re just lengthening a variable name or not putting enough thought into a good name.

Code Smell #2: Your variable is one character long. The exception to this is probably for simple loops. You almost always see code that is iterating over a counting variable “i”. Maybe that’s not so bad. If you nest three loops and you have for i, j, and k, things can get messy. If you find you’re using single character names outside of loops… STOP. Just name your variable something that won’t be a puzzle for someone one day from now.

Code Smell #3: You prefix things as “New”, “First”, “Last”, or some other definitive/completely ambiguous position. If you have something that’s “Newest” now and then tomorrow a new one is made, you now have to go change all of your code that used “Newest”, because it’s not the newest now. Same with something like “old” or “new”. It’s the “old” one now, but what happens when your “new” one becomes old because of a third generation? Now you have two olds and a new. What the heck are you going to do? Pick a good name from the start.

Have your own code smells? Share them in the comments. Follow Dev Leader on social media outlets to see code smell updates as they come out!

Background

When you’re starting work on a new project or organizing a team to accomplish a goal, there’s often a foundation that needs to be established:

How is your team structured?

What software should we use to help us?

How do we set goals?

How do we measure our progress

… the list goes on.

It’s a common challenge that’s met by anyone organizing a team or setting off to work on something. So do you copy what worked for someone else by using a cookie cutter approach, or do you wing it and see what happens? My approach when faced with two extremes is usually to aim somewhere in the middle.

Cookie Cutters

Being a copy-cat and using cookie cutters has some benefits. If something worked for some all-star teams at big successful companies, then why re-invent the wheel? They’ve proven that they have a process that works!

Look at a successful company that’s the same size as yours. Look at a team that’s completed a project that has a lot of parallels to what you’re going to be working on. How did they structure their team? Did they split off into smaller sub teams? Did they have daily meetings to discuss progress? Weekly? Monthly? Are they using some sort of software to assist them in their work? Maybe it’s a ticket tracking software, or some CRM to aid with customer interaction. If it worked for them, why wouldn’t it work for you?

The answer to that is because you aren’t them, you’re not working on the same project, and despite all the parallels you might see, your environment is different.

From Scratch

Okay okay… So if we can’t copy people, then let’s just do it all from scratch. Start your project tomorrow by holding a meeting and seeing who wants to work on what. Let people just start tackling parts of the project. Have someone create some software that will help you in accomplishing your goal (after all, you don’t want to copy someone else and use some well-established software). And now that you’ve all set off on working on parts of the project, you should probably just meet whenever you need to. Probably not best to schedule anything, because you don’t even know if you need to meet!

So, that sounds pretty sketchy, right? Clearly doing everything from scratch doesn’t seem ideal… Why re-invent the wheel on things that have been proven to work? Where’s the happy medium?

The Happy Medium

The truth is there are aspects to tackling a challenge with a team that have been proven to work. There are processes out there that teams have used successfully, software that has improved team efficiency, and strategies for gauging progress that have been used effectively. So when do you copy and when do you work from scratch?

My personal recommendation is to evaluate your options from the start. Look at what other successful companies are doing. Do they use a waterfall approach to developing products, or are they agile? Are they using particular software products for tracking progress, managing projects, interacting with customers, and/or automating processes? Make a list of those too. How often do they meet with other team members? Why are they meeting at those intervals? What are the pros and cons?

After you come up with your options, start gauging how they might apply to you. Your customer requirements are set in stone for your enormous project? Maybe a waterfall approach is better than being agile. Everyone on your team has success using Git and bad experiences using subversion for their source code… So maybe you start with that. Maybe tasks are changing pretty frequently and it’d be helpful to have frequent updates from team members, so you meet briefly every day for updates. Look at your options and think of why certain ones might be good for you.

Start with something. Try it out. There’s no guarantee you’re going to be right the first time you try things out. Actually, it’s likely you’ll do it wrong. But so what? Find out what works. Find out what doesn’t. Then figure out why something didn’t work, and swap that process for something else. Swap that software for something that fits your needs better. Change what doesn’t work and you’ll converge to a rock solid foundation. But don’t fix what isn’t broken. If your daily meetings have been working well for everyone, then why bother arbitrarily switching them to weekly? If it works, then use it.

Summary

Regardless of your approach to getting your challenge completed, I think one thing is important: Have flexibility in your foundation until you find what works. Don’t use certain things because other people say to. Use what you think might work best after you’ve evaluated your options, and then once you’ve had it in place for some time, change what doesn’t work. Use the cookie cutters as a source of information and inspiration for why you might want to try something, but don’t let your entire foundation be built from one big cookie cutter. Use lots of little cookie cutters to make your foundation for overcoming your challenge the best it can be for your team and not someone else’s.

Background

This probably sounds really nit-picky or OCD, but I think it’s an issue worth addressing. Excessive nesting of logic within code can make things nightmarish to read. Even a few of years ago I never thought anything of this. I mean, how much could it really affect someone reading it? He/she must be a complete newb to not be able to read my logic. Fast forward to a co-op placement where this was more closely moderated by my managers, and I began to pay more attention to it…

Why?

Alright, so all that you know so far about my opinion on this is that excessive nesting bothers me. So far, my mission is accomplished. Everything else is just extra. The first issue with excessive nesting is that it actually makes logic hard to follow. If you’re doing code reviews or revisiting your old code, large methods that have lots of nested if statements and loops actually become a tangled mess of logical workflows. You don’t need to believe me yet, but I’m hoping by the end of this you might change your mind.

The next thing, and it’s related, is that it makes refactoring code quite tricky. If you have lot’s of deeply nested if statements, switching up the behaviour of a function even a little bit could have your mind warping with how to tackle all the logical branches. Have fun. Remember that one monolithic function that nobody wanted to go back and refactor? Well, it turns out you need to pass in another parameter now and handle it in all of your separate logical paths. Hold back the tears when you’re trying to recall the logic once you’re 10+ levels deep into nested if statements.

Another key point I’d like to mention is that, in my opinion, the larger the vertical separation between a conditional check and it’s bodies (i.e. the if block and the else block) the more difficult it becomes to read the code. Of course, this may not be a law or an all-the-time thing, but it’s certainly a decent guideline. Think about it though. If you have an enormous block of code for your if statement body, by the time you finish understanding that, you have to go back up to the if statement condition and invert the whole thing to beign to understand what your else block does.

The Offender

Let’s have a look at some real offensive code. Who knows what it does really… Well, nobody does. Why? Because it’s completely contrived to illustrate my point. And that’s that. Behold the horror!

Pretty filthy, right? In all honesty, anyone who has worked in production code is guaranteed to have seen code that nests much much deeper… veering right off the developer’s window (and some of us code with multiple monitors). It’s a scary world out there, and this example doesn’t even begin to illustrate how bad it can get. I mean, this particular example actually fit on my narrow blog window.

That’s so much prettier. So what’d I do there? A handful of techniques:

Invert logical blocks if they can reduce your nesting. For condition1, I had an if/else block where DoThis(thing) resided in the bottom else block… farrrrr farrrr away from the check itself. I simply inverted this check and moved the else block up. Of course, I then had to put a continue statement there to go back up to the next iteration.

For condition2, by simply placing a continue right after the method call in the body, I was able to completely eliminate the else block and reduce nesting by a whole level. This works well with if/else blocks with returns too.

Next up was a whole pile of combinations for checking when I’m not going to be calling DoThis(thing). That reduced nesting by a bajillion levels, approximately.

The final block there for condition8 was still necessary. Of course, it could have be written to be the inverse check (so, if NOT condition8) with a continue inside the block, followed by DoThis(thing) outside of the if block. To me this would have been a bit overkill.

Did You Catch That?

Something extremely important to remember when changing logical flows like this is that the order you check your conditions is EXTREMELY important. Notice how in my refactored version the condition checks are still in the same order that they originally appeared? This is on purpose.

Consider if I move condition8 up to the if statement that tests condition1 and say if NOT condition1, OR condition8. Now this is technically not equivalent to the initial implementation. Why? Because the initial implementation says that for one of the logical paths that call DoThis(thing) the following must be met:

condition1 = true

conditon2 = false

conditon3 = false

condition4 = false

condition5 = true

conditon6 = false

condition7 = false

condition8 = true

Thus, by combining the condition8 check with the condition1 check, how have I guaranteed all those other conditions? Additionally, how do I know that skipping those condition checks (i.e. pretend they are method calls) has not altered state elsewhere in the class? This optimization actually may not make the code incorrect in certain situations (because it really depends what those conditions are), but it’s important to note that the checks would not be equivalent to the original. It’s just something to pay attention to, but who knows, you may even find that you can optimize some of those checks away depending on your situation!

I work as a team lead of software engineering at Magnet Forensics (http://www.magnetforensics.com). I'm into powerlifting, bodybuilding, and blogging about leadership/development topics over at http://www.devleader.ca.