Dan is the founder of Tellspin, an on-call scheduler in Slack for DevOps and developers (https://tellspin.app). Helping workspaces reduce their contact footprint, resolve incidents faster, and regain deep focus.
Code smell is a way to describe code that hasn’t aged well and has the potential for a lot of issues.
It usually is the source of a lot of hot fixes or workarounds keeping it functional. My most common reflex is to rewrite it. However, if I’m not careful, I’ll waste an entire day and not improve anything.
After a decade of programming, here are my 7 steps to reduce code smell gradually.
Step 0: Admit there is a problem
I start to recognize my code is smelly when I start saying things like “that time only took an hour.”
I’m usually doing something simple, like adding another field to a form or another schedule for a customer. I quickly add in code because it feels like the easiest thing to do and ship the feature. There are so many other things on my plate, I don’t have time for this, I’ll say to myself.
By the 5th or 6th hour I’ve hacked the same spot, I realize, had I rewritten it sooner, I would have actually saved time.
Step 1: Identify spots to clean
Smelly code is so disorganized.
Is it really smelly or do I just not understand it? It’s very tempting to always default to a rewrite. If I write all the code, I’ll understand it. But who is to say the next person who looks at it will?
Similar to profiling code to identify the slowest spot, I work to identify the place that smells the most. Are there sections of the code that new devs are always struggling with? Are there frequent small changes that require touching lots of different files or methods?
Creating a list of smelly code helps identify which sections of code need the most attention.
Step 2: Pick the worst spot
Smelly code is like dirty dishes.
With a stack of dishes, I’ll plug my nose until I dispose of the rotting food that’s causing the stink. It was easy to blame the whole pile, but for the most part, all of the other dishes are fairly clean. They don’t need immediate attention. The rotting smell came from something I forgot to clean off when I was in a hurry.
When there is a piece of code that’s really rotten, it’s often hidden somewhere in the pile. Maybe an abstraction went too far, spreading a hundred lines of code across dozens of files.
I keep in mind that I need to fix the worst smell; most of the other code is good enough and doesn’t need my immediate attention.
Step 3: Resist the urge to do everything
Smelly code is never-ending.
Perhaps the hardest part of improving a code base is scoping it to one thing. It’s so liberating to finally get a chance to clean up, that I can easily take it too far. I’ll think, “While I’m at it, I might as well clean up this… oh! and that other thing needs fixing too.”
Resist! Do not do everything.
If I try to tackle everything, I’m not going to finish. Even more likely, it’s not going to pass code review. It’s better to do one piece at a time - ya know, like eating an elephant.
Step 4: Make sure it’s better
Smelly code has edge cases.
Inevitably, in the process of rewriting, I discover why the code was written that way in the first place. I might even stumble across a can of worms. At that point, I realize my not-so-dimwitted co-worker wasn’t as dumb as I thought (or even more likely, I discover I was the one who wrote the code originally 🤦♂️).
After learning all the edge cases, I’ll be tempted to walk away.
Step 5: Don’t immediately give up
Smelly code is messy to work with.
I’m frustrated imagining how far away the current code is from a better solution. I’ve got the code in my head, I know the edge cases, and I’ve got the context. It’s important not to give up as the solution may be right around the corner.
I keep thinking about it while I go for a walk. Maybe even take a break. Solutions often come to me while I’m on walks or in the shower.
Step 6: Use the co-worker bobblehead
Smelly code needs attention.
I steal my co-worker’s bobblehead and explain aloud what I’m doing. In the process, I figure out what I've missed or overlooked.
If a bobble head isn’t available, I resort to using my actual co-workers. (I’m checking my assumptions by walking them through what I’m thinking step by step.)
Step 7: Publish or throw in the towel
Smelly code can improve.
At the end of my steps I have a complete solution or I’m banging my head on the keyboard. If it’s the first, I push the change and take a breath of fresh air. If it’s the second, I commit it to a branch and plan to revisit another day. Sometimes we can’t have nice things.
Rinse and repeat
The depth I go into each step changes based on complexity or how critical the code is. Sometimes I can run through each of the steps in a few minutes, other times it’s spread out over a few weeks. It really depends on what I’m working on.
Running through these steps helps me gradually improve my code. There’s nothing better than finally getting a fix for some smelly code merged and into production. Sometimes we can have nice things.
Dan Willoughby is the founder of Tellspin, an on-call scheduler in Slack for DevOps and developers (https://tellspin.app). Helping workspaces reduce their contact footprint, resolve incidents faster, and regain deep focus.
Starved for top-level software engineering content? Need some good tips on how to manage your team? This article is inspired by Dev Interrupted - the go-to podcast for engineering leaders.
Dev Interrupted features expert guests from around the world to explore strategy and day-to-day topics ranging from dev team metrics to accelerating delivery. With new guests every week from Google to small startups, the Dev Interrupted Podcast is a fresh look at the world of software engineering and engineering management.
Pretty much everyone does code reviews. They’ve been around a long time. I remember back in my Borland days when the Chief Scientist would come in every morning and review all the code that had been checked into the Subversion(!) repository the previous day and send emails out to folks whose code wasn’t up to snuff. That’s old school.
Slightly less old school? Saving all the check-ins up until Friday for the Dev Leads and/or Dev Managers to review and approve. Both of these techniques leave a lot to be desired -- the main thing being a complete lack of interaction between the developer, the code, and the reviewer.
Code Reviews have a number of purposes. Probably the most important one is preserving the quality and integrity of the code in the repository. Even the two old-school ways above do that.
But almost as important is the learning opportunity that code reviews can provide. If the only feedback a developer gets from a code review is mistakes in formatting or other trivial things like that, then nobody learns and gets better. The old school ways above provide for few opportunities for a developer to increase their skills.
To provide learning opportunities, code reviews evolved into meetings where everyone looked at the code written that week and commented on it, criticized it, or otherwise ran it through the gauntlet. This did provide a learning opportunity for developers, but it took more time, as it was 100% synchronous and required all code to wait for the next scheduled meeting to be reviewed.
Now, almost no one is doing these old-school code reviews anymore. All the cool kids are doing pull requests. (Some folks call them “merge requests.”) Pull requests have a number of advantages over the previously mentioned methods, including:
- Being done completely asynchronously, but in public for all to see.
- No one needs to wait to review the code -- it can happen almost immediately after a pull request is issued.
- A history of all the comments stays with the code in a repository. This allows a developer to come back to the code a year later and see all the thought that went into writing it.
- Pull Requests can be tracked, monitored, and measured. A whole lot of good things can come out of that.
Should you do code reviews at all?
Interestingly, some say no, you shouldn’t.
Not only is Jessica Kerr a great speaker and a good Twitter follow, but she also has some interesting ideas about code reviews in her article of March 27 entitled "Those pesky pull request reviews." In fact, she doesn’t like pull requests, and argues that you should sidestep them by just working on a given task as a team, so that everyone sees everything as the work gets done.
She believes that pull requests work great for open source projects where a “team” is really a set of individuals coordinating work together. For true development teams, she believes that if a team all works together on a single task, everyone learns and understands the code, and thus there is no task switching between coding and doing pull requests because the pull requests are unnecessary.
Jessica’s idea is radical -- basically going beyond Pair Programming and moving into mob programming. Mob programming is the idea of having whole teams work together on projects in serial rather than individually in parallel. Mob programming can eliminate the need for pull requests by causing all of the communication and learning to take place during the coding phase, without any review.
Not a Fan
I’m having a hard time agreeing with her idea for a couple of reasons:
- The transaction costs are too high. It seems to me that having four people work on a project together makes for many communication channels, increases the likelihood of interruptions, and reduces the amount of code that will actually get written. It’s sort of a “Too many cooks spoil the broth” notion.
- It doesn’t capture the discussions and history that will remain long after the code is committed. One of the most important and powerful benefits of pull requests is the learning that can take place during and even long after code has been reviewed and deployed.
- Finally -- not all projects are conducive to multiple team members working together. Some are small and multiple people working together would be overkill. Some are esoteric and require the focus of one person. Some will match the team and can be worked on together. There’s no one size fits all solution for all projects.
- Finally, not doing pull requests pretty much eliminates all the benefits of metrics systems like LinearB. Tracking the progress of pull requests and code reviews through the pipeline is a critical process for knowing how your team is performing. Without that, you can measure things and if you can’t measure things, you can’t improve.
As part of a discussion about code reviews, Rob Kraft, one of the Development Leaders in our vibrant Dev Interrupted Discord Server (you should join!) made the following comment that I agree with:
I think that what Jessica needs is a good look at LinearB. 🙂
Let me address some of her more specific objections:
- “Let’s face it: nobody wants to review pull requests.” Well, I don’t think that is true. We here at LinearB see customers every day that are doing pull requests efficiently and effectively. Sure, pull requests can be hard and nobody wants to do them if you aren’t correctly incenting the team to create pull requests that are easy to review. No one likes a huge pull request. But through monitoring metrics like Pull Request Size, you can encourage your team to create small, easy-to-review pull requests. And voila! People don’t hate pull requests anymore.
- “They’re a social interaction minefield!” People complain that code reviews can cause strife on a team. Well, so can conversations during Mob Programming. I’m not sure that I see a distinction, And if doing a code review causes strife, then you have a cultural problem that no development methodology is going to solve.
- “We could blame the people. We could nag them more. We could even automate the nagging!” Well, if code reviews are small, concise, and easy to do, “automating the nagging” via our WorkerB product is usually more than enough to get the ball rolling and keep it rolling. Notifications and tracking of any reviews that do happen to languish keep things moving as well. LinearB customers have seen drastic improvements in code pipeline productivity as a result of this so-called “nagging”.
- “Maybe instead of trying to work a bit more together, we could work together.” Well sure, but if you do that, checking in code without a process of pull requests and code reviews, well, then you aren’t getting all the benefits listed above, nor those of a metrics tool that can show you what your Cycle Time is doing. And I don’t believe that mob programming will prevent the cultural problems that can arise from code reviews. People will be people whether in a mob programming environment or in an asynchronous code review process.
Okay -- so what rubber is hitting the road here?
If pull requests and code reviews are hard and people don’t want to do them, then you are doing them wrong. So the trick is to make them easy to do.
We here at LinearB see many, many customers improve their Cycle Time and their overall software development process by using and tracking pull requests. By combining metrics tracking around pull requests with tools like WorkerB, many, many development organizations have seen smaller pull requests, better reviews, shorter Cycle Times, and an overall sense that things are really humming.
Monitoring things like the size of pull requests, when pull requests are assigned, picked up, and commented on, as well as monitoring the depth of reviews that take place all create an environment of small, discrete, easy to review pull requests.
And of course, if you want to find out more about what our customers already know, you can book a free demo of LinearB.
In the end, while her ideas are intriguing and thought-provoking, I can’t say I agree with Jessica’s argument. There doesn’t seem to be any good reason not to do pull requests with code reviews.
If you haven’t already joined the best developer discord out there, WYD?
Look, I know we talk about it a lot but we love our developer discord community. With over 2000 members, the Dev Interrupted Discord Community is the best place for Engineering Leaders to engage in daily conversation. No salespeople allowed. Join the community >>