Waiting until end of project to remove commented out code, remove unused code, and resolve compiler warnings

Question

A co-worker of mine is working on project solo but would still like to have other team members review their code upon reaching certain milestones. Unfortunately, they also want to wait until the end of the project to remove commented out code, remove unused code, and resolve compiler warnings. Some of the commented out code and unused code was copied from other projects. (None of it is commented with a TODO to explain why it's there and/or may be needed in the future.) Their claim is that this is the way they work and it's easier to address cleanup at the end of the project.

I'm having difficulty expressing why I feel this is a bad idea. What arguments should I use with my co-worker?

Answer 1

What value is this commented-out code holding? What value is there in ignoring compiler warnings?

Commented-out code is generally not updated as the rest of the codebase is updated (most IDEs I've used will not refactor commented-out code). This makes the code less useful, as the design / architecture may have changed since that code was used (if it ever was), and potentially makes it a source of bugs if it is not uncommented and used very carefully. On top of that, if another developer comes in and tries to understand the code, they have a whole bunch of extra code in there they have to navigate and analyze and understand.

One common reason I've heard for keeping commented-out code is "well, what if I need to use it or refer back to it or my new code doesn't work and I need to revert"? This is what source control is for - it keeps your changes separate until they've been tested, and it maintains a history of every file and every commit, so you can easily refer to a previous version or revert specific changes / lines. Use your tools as they were intended to be used!

Compiler warnings exist for a reason - efficiency, clean code, possible sources of exceptions, to name a few. If you ignore those warnings, you are cluttering your code with un-needed lines of code. You are introducing possible sources of errors that aren't handled. Some of these are trivial to clean up at the end (e.g., just remove the unused variables), but others may require a substantial modification to the class / method / whathaveyou (e.g., exceptions that need to be handled but weren't).

If you as a team decide to ignore some compiler warnings (and if you do, make sure your team truly understands the reasoning for the warning in the first place, and your team's reasoning for ignoring it), then put that into your coding style / conventions and make sure your CI/CD build agent ignores them, too. Make sure your IDE ignores those specific warnings, too, so that you don't miss a warning your team isn't ignoring because you've got dozens of warnings that shouldn't have even appeared (some have configuration files you can share to facilitate this).

Answer 2

My initial though was "Sounds like a really short review". But that may not be helpful. Obviously you are experiencing some friction in your team. Bring it up in a meeting or stand-up. Do we want to do reviews on half-baked code to provide early feedback or do we demand that code that is offered for review meets some standards? After all, the developer is off-loading effort to the reviewer.

If the developer is working on something big it could be good to let others have a glimpse at the concept, to see where he is heading so he can be stopped early if necessary. Perhaps you should not call it a review and do it anyway. Or let him actively explain what he did and is planning, while the reviewer sits with him and looks at his screen. Either way, make it a team decision.

Answer 3

Working in a mess slows you down and hides things from you.

Cleaning up after every single line you write slows you down and hides things from you.

It's not easy to let go of hard won working code that proves how something works but doesn't happen to be useful. For that, I invented the junk drawer. Every project of mine has an unpublished junk folder that never gets checked in to source control. It's full of files that are full of little snips of code that refer back to where they came from that are there if I ever need them again (which I rarely do) that no one ever has to look at. It's a beautiful system that helps me let go of the past.

This keeps that noise away from others and lets us focus on what we're going to keep. Sadly it means no one ever sees the system. All I can do to popularize it is talk about it.

Anyway, everyone has there own way of working. But when you publish something for others to look at you're obliged to clean it up. You don't have to use my method. But you need to use some method. Just saying, "excuse the mess", doesn't cut it.

As for warnings, don't ignore them. Even the ones you don't care about. Because they clutter the ones you do care about. If you're sure you don't care about them then suppress them. Keep the build clean.

Now I say all that with the idea that this is a formal peer review. I'm a big believer in the informal peer review where you grab someone who happens to be walking by your desk and talk them into looking at some code on your screen. In those cases I'm a little more lax. What matters is if you can get them to focus on the issue you're finding troublesome.

What we're trying to prevent is something called bikeshedding.

Bikeshedding

"The act of wasting time on trivial details while important matters are inadequately attended".

By not cleaning up you're scattering bike sheds every where giving your reviewer something easy to complain about that isn't very useful to you.

You can stamp your foot and demand that reviewers be more useful or you can admit they are human and clean up the trivial problems.

And this is why I pick my early reviewers carefully.

Answer 4

Some developers prefer to develop a "quick and dirty" working prototype of a feature to verify the basic approach is sound. Only when they have the proof of concept running, they clean up, refactor, improve naming, resolve warnings, handle edge cases etc.

This is a perfectly valid way to develop. But it should be done per feature and in a feature branch. Messy, half-baked code should not be committed to the main development branch where other developers will stumble over it and waste time trying to make sense of it.

Waiting until "end of project" does not make much sense. This would mean no developers get the benefit of working with clean, readable code. So why even waste time on cleaning it up then?

Answer 5

The problem with leaving stuff until the end is that, by that time the project has ended. Basically you are saying these things wont get done.

However, I think the reason you are finding it difficult to explain your objections to things like commented code and warnings is that these things don't really matter as long as the code works.

They are meta concerns for teams and ongoing maintenance, adding features etc.

If the code is a single dev one off, then you can ignore them and concentrate on what they are really asking for help with. Performance? Edge cases? its unclear.

If the code needs to be merged in to se bigger project or supported by "the company" or whatever the presumably you have some rules about "code standards" which should include stuff like this so that when it comes up you dont have to argue about it. "It's just the rule"

If you do have these rules then the discussion is moot. You cant leave this untill the end because any change to the code will mean testing all over again, bug fixes etc. So features you think are "done" aren't done and you missed your deadline.

Answer 6

Warnings need to be fixed now. Because you need to have zero warnings or you will miss warnings that indicate real problems.

(I suppose downvoted by some real programmer who loves solving real problems).