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

Question

This question already has answers here:

How can I tactfully suggest improvements to others' badly designed code during review? (16 answers)

Closed 5 years ago.

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?

Answer 1

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.

Answer 2

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.)

Answer 3

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.

Answer 4

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.

Answer 5

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.

Answer 6

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.

Answer 7

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.

Answer 8

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.