How to tell someone they broke my code and ask them to (help) fix it?

Question

I started a new programming job recently and just wrapped up my first task. As part of it, I had to change a critical line in a file from ABC to XYZ.

For my second task, a coworker mentioned that it might be helpful to look at some code my teammate Sam recently worked on. He's been on the project for several years and sits in the same office as me. While searching for it, I noticed Sam had also recently edited that critical file from my first project. Out of curiosity, I looked closer... and found that the edit set that line back to ABC to resolve a minor display issue.

The ABC line will cause the entire application to crash in certain scenarios, which is why I was tasked to make that change in the first place. Obviously, this needs to be fixed!

I want to talk to Sam about this, because:

1. I don't know the best way to fix it such that both our problems are solved, and he would probably have some ideas since he's more experienced
2. I'd feel very awkward and rude not saying anything and surprising Sam in next week's meeting when I mention "I've been working on [same bug he just 'fixed']"
3. I want to explain why I did that, so he won't inadvertently introduce more bugs in the future. (Sam knows I had been working on this, so I'm not sure why I wasn't looped in on this in the first place, but that's beside the point now..)

I'm not sure how to approach Sam, though. Normally I would start a conversation like this by suggesting a fix I could do, but since I'm new to this project I don't have one. I don't want to sound accusatory or possessive over my code. Sam seems like a reasonable person, but given how busy he is, would likely be annoyed if he thought I was trying to dump this problem on him ("Hey your code is wrong, you need to redo it"). I'm also just worried about putting my foot in my mouth (has happened a few times lately, unfortunately) and giving a bad impression to my new coworkers.

More details

Our company has source control, bug tracking, and code reviews for every commit. Our commits are both already in the "alpha" code base (visible to developers and testers, but not released to the public), and linked to the respective bugs that they fix.

His commit message was like: "Bug: History doesn't show ABC. Fix: Line wasn't ABC, changed line." A comment at the top of XYZ file explains that it runs either ABC or DEF, because ABC doesn't always work. (The real names of the classes are more like "Thing" vs. "Thing Selector" if that helps explain it.) So this is why I suspect he (and the coworker who reviewed his change) didn't actually look into XYZ.

Unfortunately I only had tests to ensure XYZ functioned appropriately and not that this file was actually running XYZ (lesson learned there!).

Question

How can I approach Sam to let him know his "fix" breaks my code, and get his help on actually fixing it?

Answer 1

You are overthinking this a bit. It happens that software developers make mistakes. We all do. The best approach isn't to think of it as "this is your mistake" or "why did you mess up my code". I try (emphasis on try) to follow the approach called egoless programming.

So the best approach is the straightforward approach. Send Sam an email of the form "I noticed a problem in X code. I think it's going to crash under these circumstances. I'm not sure what the fix is. I'd like to go over it with you when you have some time so we can figure it out, or see if I'm wrong". Assuming Sam is a professional and not hyper-defensive, he'll be fine with this.

Unless the bug is critical or too hard to explain in email I tend to work this way so people can deal with the issue on their time.

Don't worry about whose fault it is. You both work for the same company. Bugs affect both of you. The customer isn't going to care who was responsible for the bug when the product crashes.

Every time something like this happens, I try to figure how how it happened, and how to prevent it from happening again. Did the comment entered on the commit make it clear what was being fixed? If the fix isn't obvious, should there be a comment in the code explaining what's going on? How about a unit test? Is this "the tip of the iceberg" and there are related bugs?

Answer 2

I think you are overthinking this. Simply tell Sam exactly what you have found by writing an email only to Sam(!):

Hello Sam, I saw that you changed the line XYZ back to ABC. The problem is that this line is likely responsible for a far nastier bug which I found and can cause a crash under certain circumstances (Insert details here). So we need something to fix the display issue and this bug. Could we talk about this?

I find Scohe001's reaction not sufficient because it does not mention the actual problem.

The normal reaction is that you both look at this and find a solution for both problems. It is important that you first mail Sam alone so you can both sort this out under you two, but that you have evidence in the rare case Sam tries to ignore/sabotage you. If you see that the line will cause unanticipated trouble (yeah, that happens often :/) it is likely that you both must escalate the situation to the lead developer (We have a little problem here...).

Are you sure that Sam overwrote the line? Normally you have a Version Control System (VCS) which informs each person about changes in the codebase and the first thing I do when I see changes is that I look up the reason in the commit message and if not sufficent ask the colleague what he did. The thing is if you do not update your current state repeatedly it is easy to assume that changes are (not) there when in fact they are (not).

Answer 3

I would email Sam with something like:

Hey Sam, I saw some changes you made recently in this commit (link the commit here if you can). I’m still trying to familiarize myself with our codebase and I was wondering if I could meet with you to discuss some questions I have.

I’m free (list 2-3 times you’re free. If you can, look at his calendar and match times when he’s free).

Thanks!

You’re right to fear ending up with your foot in your mouth—that’s definitely something that’s happened to me at new jobs more than I’d like to admit. I’ve found that an easy way to avoid it is to ask for an explanation of what’s happening instead of throwing accusations.

If Sam explains his change the same way you did, then this email gives you an excuse to follow up with “won’t this break in edge cases I, J and K?” on the pretense of learning more about the system. And whether you’re right or wrong, watching him think it through in real time and give you an answer probably will help you learn! Not to mention you can use this chance to ask him how he’d like you to handle any inconsistencies you find in his commits in the future.

Answer 4

In a reasonable environment, everyone's changes go through code review. If I changed your change back to the original, then code review is the right time to notify me. That's the time where you point out why your change was needed (which I might not have appreciated).

However, there is the issue that your change fixed a bug but caused a display problem that wasn't there before your fix. That should have been found when your code was reviewed but unfortunately wasn't. When you fix a problem, you shouldn't cause other problems. So I would reopen your bug, adding a comment that your solution fixed the big problem, but introduced a small one, and you should modify your solution to fix the big problem without causing a smaller one.