When you track down and fix a regression—i.e. a bug that caused previously working code to stop working—version control makes it entirely possible to look up who committed the change that broke it.

Is it worth doing this? Is it constructive to point this out to the person that made the commit? Does the nature of the mistake (on the scale of simple inattentiveness to fundamental misunderstanding of the code they changed) change whether or not it's a good idea?

If it is a good idea to tell them, what are good ways to do it without causing offense or causing them to get defensive?

Assume, for the sake of argument, that the bug is sufficiently subtle that the CI server's automated tests can't pick it up.

Don't CC the whole team when you send him/her this email.
–
quant_devSep 27 '11 at 9:20

25

Of course tell him, diplomatically or/and with a joke. In the company I am working at we have a dashboard with every developer's name. Every time someone makes a repository-related mistake (forgot to commit something, forgot to tag, does not compile, etc.) that developer gains a "+". When he has "+++" he has to pay the breakfast for the next day. Strangely, since the system has been put in place there are less "mandatory" breakfasts :-)
–
JalaynSep 27 '11 at 9:30

25

@Jalayn - not with a joke - that just annoys people
–
MarkSep 27 '11 at 10:08

28

"Assume, for the sake of argument, that the bug is sufficiently subtle that the CI server's automated tests can't pick it up." Why not? Is this something that you don't have a test for? If it is, the first thing you should do is write a test (or a couple of tests) that fail now yet will pass when the bug is fixed. If it can't be tested, why not?
–
Thomas Owens♦Sep 27 '11 at 10:23

18

@Thomas Owens Because that's not the question I'm asking. :-P In an ideal world, no bugs would get into the system, because we'd write perfect code first time, and there would be an exhaustive suite of automated tests in case we didn't. This, however, isn't an ideal world, so I'm asking what you should do when a bug makes its way into your code.
–
ScottSep 27 '11 at 11:51

17 Answers
17

If you just approach them to tell them about a mistake they made then unless you are the best diplomat in the world its going to be difficult for it not to just sound like "Ha! - look at this mistake you made!". We are all human and criticism is difficult to take.

On the other hand unless the change is completely trivial and obviously wrong I normally find it benificial to talk to the person who committed the original change as part of my investigation just to make sure that I fully understand whats going on, hence the way I usually end up handling these situation is by going over to said person and having a conversation that goes a little like this:

Me: I'm working on this bug where ... summary of bug ... and I think I've tracked down the problem to a change you made. Can you remember what this change was for? / have you got some time to explain this change?

Then either:

Them: Sure, thats to handle ... situation I wasn't aware of ...

Or something along the lines of:

Them: Nope sorry I don't remember, looks wrong to me.

By going and investigating the change / bug together the original committer gets to learn from their mistakes without just feeling like they are being criticised*, and there is also a pretty good chance that you will learn something too.

If the original committer isn't around or is busy then you can alwasy just slog through and figure it all out yourself, I just normally find that talking to the person who originally made the change is quicker.

* Of course this is only going to work if you are genuinely interested in the other persons help. If you are just using this as a thinly disguised method of telling someone about a mistake they made then this is probably worse than just being open about it.

Be assertive not aggressive. Always favour saying something akin to "this piece of code is not working" vs "your code is not working". Criticise the code, not the person who wrote the code.

Better yet, if you can think of a solution, fix it and push to them -- assuming you have a distributed version control system. Then ask them if your fix is valid for the bug they were fixing. Overall, try to increase both your and their knowledge of programming. But do it without your ego getting in the way.

Of course, you should be willing to listen to other developers coming to you with the same problem and act how you would have wished they did.

+1. Personal favourite approach: "Before I go messing with this, was there a reason you did it this way?"
–
pdrSep 27 '11 at 9:39

67

+1. "Criticise the code, not the person who wrote the code."
–
c_makerSep 27 '11 at 10:51

11

+1, It is very similar advice to what my marriage counselor told my wife and I, when having a grievance against what your partner is doing, avoid the word YOU, it is too confrontational.
–
maple_shaft♦Sep 27 '11 at 11:11

3

+1, but I don't think the word "you" is confrontational. There needs to be a clear understanding of ownership. I have personally had people continually commit code that broke the build because they did not understand that they were the ones who caused it. I like @pdr's approach...this statement is non-confrontational yet it has the word "you" in it.
–
Tim ReddySep 27 '11 at 12:21

3

Sounds like you may be reintroducing a new bug. Their fix may have corrected a previous issue that you don't know about. Why not go to them and ask why they wrote the code in the way that they did. It might reveal that there is an odd language/design/vm quirk that it was covering. Going and showing them your ego ["heres how i can do better" won't help them]
–
monksySep 27 '11 at 14:09

Letting them know mistakes they make will help them become a better coder and reduce their chance of making mistakes in future. BUT do be polite and dont make a big deal of it, we all create bugs every so often. I find a polite email is a very non confrontational way of letting people know.

The "learning from mistakes" part is not universally true. The vast amount of bugs are things like missing validators for example. These are things that just happen, even to experienced developers. You won't learn much from it. That's why we need to have a decent QA.
–
FalconSep 27 '11 at 10:43

2

@Falcon The insight "we need to have a decent QA" is an example of learning from mistakes. You could carry on by thinking about "why don't we have QA / why did our QA miss this problem"
–
MarkJSep 27 '11 at 11:30

2

@Falcon "These are things that just happen" <--- this alone is the knowledge you obtain from repeated but trivial mistakes. Have you had an experience when you compile and things don't work, first thing you check your spelling and bang, within 10 seconds, bug is gone. You have the knowledge that "these are things that just happen", sometimes that's why you are able to debug in 10 seconds not 10 hours.
–
GaptonSep 27 '11 at 12:13

The constructive way is to find the bug, fix it and take actions to avoid similar bugs to arise in the future.

If it involves explaining people how not to introduce bugs, go for it.

Once, I worked in a team where the project manager never told a particular developer that he made a mistake: he organised a meeting with the whole team where he explained that a mistake was made and that a new process has been defined in order to suppress that kind of mistakes. That way, nobody was stigmatized.

+1 for "take actions to avoid similar bugs to arise in the future". That is the most important part, IMO.
–
Michael KjörlingSep 27 '11 at 12:39

1

The constructive way is to find the bug, fix it and take actions to avoid similar bugs to arise in the future. -> The question premise is you've already fixed the bug.
–
HugoSep 27 '11 at 16:12

1

Yes, but be careful about introducing new process. If you introduce too much process and call too many meetings, that slows the pace of development and poisons company-wide morale. I've seen too many shops overreact to one person's mistake. Only if the mistake is indicative of a broken process should new process be appropriate.
–
JacobSep 28 '11 at 17:39

Nobody should get defensive if you're tactful about it. An easy way to handle it is to ask them to double-check your change before you commit it back to the trunk (or whatever is relevant for your version control system). People will appreciate it if you save them a few minutes by fixing obvious errors, but they won't appreciate it if you fix something that wasn't broken and end up breaking their code. Giving them a chance to review your change tells them that you don't want to step on their toes and gives them an opportunity to object to your changes.

If it's a big change rather than just a typo, it's a good idea to give the author a heads up before you dig into trying to fix it. "Joe, I was merging my own stuff yesterday and found something that I'm not sure I understand. It looks like a bug, but I wanted to run it by you before I go messing with your code. Would you take a look with me?"

Your relationship with the author is a big factor. If you wouldn't mind the author fixing your code without telling you, and if you're pretty sure the feeling is mutual, then it might not be worth mentioning. If it's someone with more experience/seniority/status, you'll want to let them know that you're going to change their code. If it's someone with less, then consider whether it's the sort of thing they need to hear to avoid repeating the mistake or it might embarrass them needlessly.

Always remember that if you can find out who checked in the "bug", they can as easily find out who "fixed" their code. If you think they'd be upset/annoyed/embarrassed at finding out about your change after the fact, by all means tell them beforehand.

Also, fixing the bug isn't your only option. You can always just report the bug in your issue tracker. Tact is again required here -- reporting the bug makes it more visible to the entire team, but it also gives the author a chance to fix his or her own mistake. Reporting is the best option if you're not certain about the best way to fix the problem or if you just don't have time to fix it.

I like the "I don't quite understand this, can you explain to me how it works?" approach. If it's intentional (and recent), then the original programmer should be able to explain pretty well how the code works. If it's a bug, chances are good that in explaining what the code does, s/he will spot the mistake and in the middle of the explanation you will hear an "oops". Either way, anyone would be hard pressed to feel like they are having a finger pointed at them for a possible error.
–
Michael KjörlingSep 27 '11 at 12:38

3

+1 for "it looks like a bug, but I wanted to run it by you before I go messing with your code."
–
Russell BorogoveSep 27 '11 at 16:41

I could only add a technique I learned from a manager once when I would make a mistake.

I was the middle-aged consultant with the Ph.D. and she was the young manager without, so there could have been a perceived prestige gradient.
At any rate, she had clearly had experience with this situation and knew how to handle it.

She mentioned to me in an almost apologetic tone that there seemed to be a problem, and would I have time to look into it?

I think there's a deeper issue underlying this question. Yes, the submitter should certainly be made aware of the consequences of their change, so that they can understand what happened and not do the same thing again. However, the context of your question indicates that you prepared and submitted a fix without the original submitter's knowledge that they even caused a problem. Therein lies the deeper issue: why doesn't the submitter already know about the regression and why didn't they fix it themselves? The situation you described may indicate a lack of accountability or vigilance on the part of the original submitter, which is a potential concern with respect to their overall performance and motivation.

My software engineering experience has taught me to own all of my code changes, not just projects I'm responsible for, all the way to production, which includes being aware of their impact, including on your build system and (obviously) product behavior.

If someone's change has caused a problem, it doesn't mean the person is a bad engineer, but usually they should be responsible for and involved in fixing whatever has gone wrong. Even if they are not "at fault," e.g. their code exposed an underlying bug that has existed in the codebase for years, they should be one of the first people to be aware of a problem with their change. Even if the original submitter isn't the right person to fix the bug, they should be closely connected to the life cycle of their change.

Good traction on your question! Everyone's told you what do. Should you tell? YES! Anytime the question asks "should I communicate more?", the answer is almost always YES!

But to add something different: your premise is flawed.

A co-worker made a commit that didn't break CI, but lead you to
discover a problem.

Congrats! You found a new bug, not a regression. Seriously, do you manually test every scenario and line of code not covered by automated (or standardised manual) testing when you commit?

By all means, get your colleague involved in the fix, with tests to ensure it can't happen again. You are both heroes! But if you let slip any blame in word or action, you are responsible for perpetuating one of the worst organisational diseases: accountability without responsibility.

If you really need to find a villian, think about the guy who committed the original code that broke, and left a trap for your unsuspecting buddy (obviously without sufficient test coverage). Hopefully that wasn't you!

If someone gets offended when you said him he made a mistake, it means he thinks he is the wisest on the earth and makes no mistake, and when criticized, he gets the feeling, as we said in Poland, that 'crown is falling from his head'.

So you shouldn't hesitate to say that someone has made a mistake. It is normal. Everyone makes mistakes, even the best! Only those who make nothing make no mistakes ;)

It's all in how you tell the person they made a mistake. I make mistakes all the time and will be happy for someone to point them out so I can improve but if you come up and tell me "Dude, your last commit totally broke the code. Why can't you be better at checking your mistakes?" I'm of course going to be offended.
–
The JugSep 27 '11 at 14:09

In addition to what others have said, make sure it's ACTUALLY their commit that caused a bug. Certainly don't blame someone else for your own mistake. No matter how tactfully you approach them, you're still going to piss them off if you've blamed them for something they didn't do. (Speaking as someone who has been blamed for other peoples' bugs constantly; one time someone came up to me and said I did something utterly stupid and I brought up the commit log and found that the last person to touch that line of code was the person who was blaming me. Somehow he still seemed to think it was my fault because I'd written the line originally.)

Why don't I see a single answer here that reflects the top voted comment on the question??

Yes, absolutely tell them about it, but don't do it in front of the entire team

Approach the developer 1:1 and point out the bug. Don't make a big deal of it. I always thought that pointing out the error in front of the entire team was a bad idea. It might work for some developers, but its not for everyone and can have a negative effect. Remember, we've all been in their shoes at some point or another, and as the 2nd top voted answer says, you learn from your mistakes

I usually find it works best when you start with a compliment and then get to the error... something like "the fix you implemented works great, BUT it seems to have broken x,y,z", or "thanks for doing a,b,c, BUT it seems to be causing x,y,z"

Longer answer: My last job was at an Agile company that used TDD with CI tools to ensure that what was in our SVN repo was good, working code at all times. When something was committed, our TeamCity server got a copy, compiled, and ran unit tests. It also ran integration tests hourly. If something was committed that caused the CI to fail, everyone got an e-mail stating the build had broken based on a commit by a particular person.

That didn't always catch everything; woe to us, we didn't enforce code coverage, and even if something was covered by unit or integration tests, they might not exercise that code sufficiently. When that happened, whoever got the task of fixing the known issue (if QA caught it) or defect (if, dun-dun-dun, the clients did), would run a "blame" (shows who last modified each line of a code file) and determine the culprit.

Calling someone out for checking in broken code is not necessarily a bad thing. They have failed to do their job properly, and either they or someone else had to go back and fix the mistake. This happens all the time; how big a deal it should be depends on how easy the fix was, whether the mistake indicates the person didn't even compile or run the code in question, and the overall corporate culture. What's important in the whole thing is that something is learned by the person who made the mistake; if the build breaks due to the same guy over and over again, there's a deeper issue with that person which must be addressed. Builds breaking all the time indicate an issue with the team's communication or knowledge of the process.

Yes. Ask the person to review the fix you made to the code. Sometimes I have found that someone else's bug was actually a tricky part of the code with some other unseen consequences if the bug was simply fixed.

How certain are you that it was real a bug, and how certain are you that your fix is correct?

If the problem was minor - a typo/thinko/cut & paste bug - and the breaker is a busy peer, and you're confident in your assessment of the problem, you probably don't need to bring it to their attention. (e.g. foo.x = bar.x; foo.y = bar.y, foo.z = bar.y).

In most other cases, it's a good idea to mention the problem. In non-serious cases, you don't need to interrupt what they're doing; wait and do it over lunch or when running into them in the break room.

If the nature of the error indicates a serious misunderstanding (of the implementation platform, the local policies, or the project spec), though, bring it up ASAP.

If you aren't certain of your assessment, ask for them to review your fix, especially if it's not in code that you're very familiar with. (I strongly recommend your dev team adopt a 'code buddy' policy where all changes are reviewed by one other person prior to checkin, anyway.)

They may make the same mistake in other places because they don't understand that it is causing a problem. Not only that but there will be extra unnecessary time to repeatedly fix the same mistake. You can't learn from mistakes you are unaware you nade.

Second, they think they are doing a better job than they are. When people are not made aware of their problems they can hardly be blamed for thinking they are doing well when they aren't. Even when the problem is a careless mistake, people make fewer of them when they are aware that the mistakes are noticed.

Next if someone doesn't look up who did it, how will you know if you have a particular problem employee who is either always careless or has basic misunderstandings of the product? Would a responsible person want that to continue in a team he or she is associated on?

If you fix and move on without discussing it are you sure, you fixed it correctly? Sometimes it is tests that need to change when a requirement changes. If it is something other than a fairly minor typo, can you really be sure either one of you has the correct solution? You might be breaking his code in return without consulting.

The pros

People don't get embarrassed or annoyed with you for pointing out their mistakes.

I guess I come down strongly on the side of telling them but doing it nicely and privately. There is no need for public humiliation. If the person repeatedly makes the same mistakes or is making critical mistakes that show a lack of understanding, then the supervisor needs to be made aware as well.