How do I communicate to a colleague that their code does nothing?

Question

Stay with me here. When I say the code "does nothing", I mean something like the following function:

void factorial(int n)
{
    int result = 1;
    for(int i = 1; i <= n; ++i)
    {
        result *= i;
    }
}

Sure, this function creates a local variable and computes a value, but it doesn't do anything to advance the program; there is no way to make use of the result of the computation, and it has no side-effects.

Keep in mind that this is a contrived example for the purposes of the question, but the code I refer to may be more complicated.

When I encounter such code, I often spend some extra time and go through it several times, because I worry that I might be missing something. I could just take the high road, or fix the bug and be done with it, but I worry that this slowdown would reflect badly on me and my performance.

So, should I take the high road, and assume the team understands, or should I communicate that the code doesn't do much? If the latter is the better option, how do I do that without being condescending or disrespectful?

If the code is in C and is nearly as simple as you show, compiling it with all warnings & debug info `gcc -Wall -Wextra -g` might be enough. Otherwise, you might profile the code for several test cases and notice that the function was not called (or did nothing). And I suggest speaking to your colleagues and asking "I got the impression that this code does nothing, could you explain me why it is here?" — Basile Starynkevitch, May 19 '15 at 08:48
@BasileStarynkevitch the example is just for illustrative purposes, actual code may not be that simple. For example it could not necessarily be a function, but a path of execution as part of a function which does not do anything, just performs some computation locally that has no impact on the final result. — AwesomeSauce, May 19 '15 at 08:50
What will this code do in the case that int n is int_max ? Take just a long time? Throw an exception? That's definitely side-effects in my book. — Pieter B, May 19 '15 at 09:17
*"How do I communicate to a colleague that ..."* perhaps start by asking the colleague? — rwong, May 19 '15 at 09:33
If you believe a piece of code to have no observable effects, why don't you ask the colleague that wrote the code to explain to you how it works? To ask it in a non-threatening way, phrase your question as if you are missing something. — Bart van Ingen Schenau, May 19 '15 at 09:49
If you are 100% sure the piece of code does nothing, remove it and wait if someone misses it ;-) — Doc Brown, May 19 '15 at 10:42
Obligatory? https://xkcd.com/1172/ : pretend that LongTimeUser4 is your colleague. — MonkeyZeus, May 19 '15 at 13:05
@PieterB taking a long time is not a side effect of functions, since that's not a change of state of the system or an interaction with external functions. Throwing an exception may be, but only if it is observed by a function in the call stack. — Dancrumb, May 19 '15 at 14:25
@BartvanIngenSchenau but what if it turns out it was you who wrote the code you don't understand? :-) — gbjbaanb, May 19 '15 at 15:39
@gbjbaanb: Then I ask myself what the code should do and fix the error. But that wasn't the question. The question was how to communicate. — Bart van Ingen Schenau, May 19 '15 at 16:30
In cryptography there is often "code that does nothing" except burn a specific number of CPU cycles doing an otherwise unnecessary computation, to avoid various timing and side-channel attacks. — Michael Hampton, May 19 '15 at 17:16
@MichaelHampton Surely it does something, or else an attacker would simply copy the algorithm and remove any useless code to speed up an attack. — rlms, May 19 '15 at 20:35
@sweeneyrod In that case you would somehow have to get the victim to run your modified code! — Michael Hampton, May 19 '15 at 20:40
I'd suggest either email or by talking in person. Email allows you to set out all your arguments, and a conversation in person while reviewing the code allows a dialogue. One piece of advice in either case, don't assume you're right as you seem to be doing in this question. — Ian Newson, May 20 '15 at 07:57
@DocBrown And that's when the race conditions from their other code they were preventing totally by accident start to kick in. — Erik Reppen, May 21 '15 at 01:03
@ErikReppen: ;-) if the code is so fragile you cannot even strip superfluous parts, imagine what can happen when you have to change something which is not superfluous. But seriously, it is always a good idea to do such refactorings only **when you are working on that part of the code base for some other reason**, so you have to test the changes either. — Doc Brown, May 21 '15 at 08:26
@DocBrown I certainly agree. Just having PTSD flashbacks to a wack-a-mole Java/Rails/.NET sandwich I was just supposed to be doing the UI for a couple years back. You know it's bad when people move all the business logic to templating because they don't trust what happens before then. Lost count at 18 layers and sub layers of JSPs, mixed with front end calls calling more content plugged into more JSPs. — Erik Reppen, May 21 '15 at 20:16
**Please do not use comments for extended discussion. If you would like to discuss further then please visit our chat room. Thank you.** — maple_shaft, May 22 '15 at 10:01

score 83 · Accepted Answer · edited May 19 '15 at 16:03

83

The easiest way is to ask from a point of not understanding and ask their opinion. It's no good to say "your code sucks", it's quite another to say "erm, I saw this and I really can't understand what it's doing, or why it's doing it. What do you think?" or similar - you could say what your concern is and ask them to explain it to you, or ask them just if you are right.

This way, you bring them into being part of the solution and typically will realize the problem and fix it themselves (or you can offer to do it for them).

I'm sure you've written code like that yourself, you just don't see it until someone points it out and you have a d'oh moment. Treat such code not as a huge, glaring error but as just another slight cock up that we all make now and then and you'll be good.

edited May 19 '15 at 16:03

Canadian Luke

327
2
15

answered May 19 '15 at 10:12

gbjbaanb

48,354
6
102
172

That's what I do. The code might of been useful in the past but no one notice it isn't used anymore or written when prototyping. – the_lotus May 19 '15 at 13:03
15

Oh, but "your code sucks!" is so much more fulfilling... – Neil May 19 '15 at 14:36
12

@Neil only if you have an unfulfilled and ultimately futile and lonely life that thrives on the misfortune and errors of others :-) – gbjbaanb May 19 '15 at 15:42
6

@gbjbaanb Ouch. Totally unnecessary burn, man. – Neil May 19 '15 at 15:43
5

@Neil oh, not you, of course... (please note the smiley) – gbjbaanb May 19 '15 at 15:46
3

I encourage people to tell me my code sucks. I either can agree with them and possibly fix it or prove that it does work and bask in the glory of knowing there are unlimited ways to enjoy a moment like this. – JeffO May 19 '15 at 18:57
@gbjbaanb - your comment sucks! – Daniel Earwicker May 19 '15 at 19:51
Yeah, but sometimes that's when their eyes glaze over the color of the kool aid they've been snorting and they get really excited and start man-splaining some concept that they clearly don't understand as if it's supposed to justify a construct that is either 100% redundant or actually DNGN. – Erik Reppen May 20 '15 at 02:28

score 14 · Answer 2 · answered May 19 '15 at 09:20

The purpose of all code is ultimately to affect the world outside the program in some way. Nobody cares about the content of memory cells or CPU registers; they only care about the printed page or the information displayed on a screen. If code doesn't input/output anything, and doesnt change the state of the program in any way that could eventually cause any I/O, it is by definition superfluous.

But how to see this? The easiest way is to have unit testing. This code cannot satisfy any test that check for actual factorial values to be computed, since the only value that is computed never makes it outside the function. Simply write up a test case that codifes what the code should be doing, and it becomes trivial to demonstrate that it doesn't. (Admittedly this isn't the primary reason why people usually advocate test-driven development, but it works really well for this purpose, too.)

score 11 · Answer 3 · edited Apr 13 '17 at 12:48

Stay with me here. When I say the code "does nothing", I mean something like the following function

The correct term for this type of source code is incomplete. You could say to another programmer "Upon reviewing your source code I found a function that is incomplete. Can you clarify its intent?"

Incomplete work makes it's way into production products all the time. There are many possible reasons for this to happen.

The developer pushed these changes by mistake.
The developer needed to mock functionality until it can be finished later.
The developer was unable to finish the work, but pushed it anyway.

When I encounter such code, I often spend some extra time and go through it several times, because I worry that I might be missing something. I could just take the high road, or fix the bug and be done with it, but I worry that this slowdown would reflect badly on me and my performance.

Your self-confidence and work efficiency has nothing to do with work produced by other people. The processes, source code quality, quality assurance and management style of a business are outside your control (unless it's part of your job to change them). Try to keep those things separate from how you perceive your own performance and value.

So, should I take the high road, and assume the team understands, or should I communicate that the code doesn't do much? If the latter is the better option, how do I do that without being condescending or disrespectful?

What you're asking here has nothing to do with computer programming.

I would ask this on the https://workplace.stackexchange.com/

You will likely get answers on how to work effectively in a team.

Put 100% of your effort into your work. No one can fault you if you've tried your best. Expecting more than what you are able to do is just foolish management. Trying to work quickly just makes you fail faster.
Communicate constructively with your team members. Speak up and express yourself.
Listen to what your team members have to say.
Don't be a lone wolf. Help the team fix problems.
Openly share with your team members. Share your ideas, thoughts and concerns. Don't wait until someone asks. Make an effort to share.
Offer to help. If you find incomplete work. Offer to finish it.
Be more flexible. Rather than complain about problems. Be thoughtful of why those problems exist, and accept them. Most people stressed out on a project do so over things they can't change.
Always be respectful, supportive and honest with fellow team members. You might not get the same in return, but you'll learn the value of those traits.

score 1 · Answer 4 · answered May 19 '15 at 08:50

1

If nothing exits or is referenced outside of the scope of the system that the code represents, it has no use. You just have to draw a line around the entire system and challenge the obstinate dev to point out where it's actually crossed.

answered May 19 '15 at 08:50

Erik Reppen

6,243
31
34

score 1 · Answer 5 · answered May 19 '15 at 10:35

Ask the dev to walk you through the code in code-review session, and ask what his thought process was around it. See if he recognises the superfluous behaviour. If he doesn't, ask him specifically about it. Maybe a bit more roundabout, but not as confrontational.

Steve Jessop · Answer 6 · 2015-05-19T14:25:37.383

I worry that this slowdown would reflect badly on me and my performance.

If improving the code you find reflects badly on you, then you have a much bigger problem in your team than exactly how you raise the issues you find! OK, if it's not broken you don't have to fix it, but for genuine redundant code it's probably a good idea to simplify by removing it. Time estimates should be based on the practical reality of understanding code as you go and fixing issues you find. If you're in crisis mode and can't do that, then the answer would be "put your head down and hope for the best", but that should be rare.

Redundant code isn't the worst problem you could find, but it indicates an error in the thinking of whoever wrote it (or modified it to become redundant). Therefore it deserves at least as much attention as any other potential refactor you discover in code you visit, and perhaps more.

should I take the high road, and assume the team understands

No. You're part of the team, you don't understand, so not everyone understands. Don't assume there's a higher mystery everyone else is privy to. Much more likely, nobody would want it to be like that it and everyone else either hasn't noticed it or hasn't got around to improving it. Or you're wrong. In the latter case you'll benefit from an explanation of your mistake, in the former case someone else will benefit from dealing with it.

should I communicate that the code doesn't do much?

Depending how your team feels about code "ownership", it might be that the correct thing to do is just to fix it and (if the change doesn't speak for itself) discuss it when your changes are reviewed. Alternatively there might be someone in the team who's the obvious person to talk to about it before going ahead with a change, either the author or someone else with particular responsibility. When you do talk to someone, be it the owner or a code reviewer, all that's left is to decide what to say.

If you're comfortable shooting straight from the hip:

"This code does nothing"

"You forgot to return the result, shall I fix it?"

"This void function has no side-effects, what's it for?"

Naturally this might be considered rude, it really depends on your relationship with the person. So you can be more diplomatic:

"What does this code do?"

"Is the result needed anywhere or can we skip this part?"

"This looks wrong, so I'm not sure I've understood everything"

"I'm terribly sorry to trouble you, but I haven't been able to understand these few lines of code. It seems to me as if this function doesn't do anything, so I was wondering why that is. Quite likely I've misunderstood it, or I suppose it might be there to support some future direction, in which case I was wondering if I could usefully add some comments to explain what".

Naturally, the last example runs a fine line between obsequiously polite and just plain condescending. Some people sound good talking like that, others sound like sarcastic gits.

Personally, I usually go with "Is this supposed to not return anything?". I'm expecting the answer "no". It's intentionally somewhat impolite/joking, because with my current colleagues I can be, but I wouldn't accuse anyone of anything beyond an oversight. And if the answer is "yes" with a reason, then fair enough. Another common option for me is "I think this should return something". Both are things that I'd be happy to have said to me.

score 1 · Answer 7 · answered May 19 '15 at 15:27

1

I would've preferred to post this as a comment alas I'm short on rep.

In my world, if I can't directly trace a piece of a code to a documented requirement or derived requirement then I flag it for a team review which occurs every two weeks or as needed.

    // REVIEW: 05/19/2015
    // Can't find the need for this code. Perhaps it's been orphaned? 
    // Need to identify the requirement associated or remove it.
    public static bool WouldYouLikeToDance(this Person you, Person dancePartner)
    {
        // determine minimum acceptable attractiveness
        int minAcceptable = you.SelfWorthAndRespect - (you.CountDrinksConsumed() * 0.10);

        return (minAcceptable <= dancePartner.Attractiveness);           
    }

I find it's best not to confront them directly, just anonymously identify concerns and expect they would do the same for you. During the group review, you can weigh in with the team as a control to ensure you're not erroneously identifying something valid. The overall goal is code quality and as a side effect, improving the skill set of each developer and the team as a whole.

answered May 19 '15 at 15:27

Code.Combustion

51
2

1

How could you possibly engage in a code review if you don't want to be directly confronted with your work? – JeffO May 19 '15 at 19:00
My team schedules a code review at the end of each sprint/iteration,etc or when a certain milestone or has been reached. We have two defined REVIEW tags, REVIEW_R for dead/pointless code for requirement validation and REVIEW_Q for code quality concerns. Either myself (lead) or the project manager will copy all REVIEW_* comments from the task list in visual studio. The REVIEW_Q list is dumped into a powerpoint presentation along with the code from each. – Code.Combustion May 20 '15 at 17:38
The REVIEW_R list is sent to our requirements team (which consists of a few developers, the requirements manager and a tester/QA/functional). They either concur with the observation of the REVIEW_R comment or they identify the requirement that this code fulfills. The updated REVIEW_R list, is added to the bottom of the powerpoint presentation. Finally, the development team hashes it out in a meeting and resulting changes are made. It becomes a group review, not just developerA telling developerB their code sucks. YMMV – Code.Combustion May 20 '15 at 17:38

score 0 · Answer 8 · answered May 19 '15 at 23:41

If this code is presented during code review, it seems you have little problem; just provide feedback that you are not clear what it does and ask for a unit test.

If it's not during code review, then you might want to think not just about how to rectify the immediate issue, but look for a deeper solution in terms of process change.

Any of the following might pick this up:

Suitable warning levels at compile time, coupled with a zero warning requirement on code.
A code review process
Sufficient unit testing coverage.
Commenting standards

Now, the sheer fact that you have this problem suggests to me that maybe some of your team members are not great at reading code. Unfortunate, but in the real world so very common.

I'd note that above practices are useful in even a very strong team (commenting is debatable).

However, the useful thing about the above practices that are particularly useful in a weaker team; they provide a framework to help improve code quality without it getting personal.

How do I communicate to a colleague that their code does nothing?

8 Answers8