9

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?

  • Did your change make it into the codebase or is it yet to be merged? – jcm Sep 2 '18 at 0:21
  • @jcm Both sets of changes are in the "alpha" codebase now. That is, it's visible to developers and a limited set of user testers, but not the general public. – Em C Sep 2 '18 at 0:33
  • 5
    As a side note, since it sounds like you don't have a test suite that would have raised an alert about this problem, there's an additional communication problem that should usually be handled by including an inline comment explaining the (apparently nonobvious) problem. – chrylis -on strike- Sep 2 '18 at 7:57
  • It looks like your changes had undesirable side effects, so you didn't fix the issue properly. You can ask Sam help to fix the issue (and maybe apologize for breaking things). – Guillaume Sep 2 '18 at 14:36
  • 1
    @Mast yep, I added some more details in an edit – Em C Sep 2 '18 at 18:08
6

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?

  • It I have never been in a company that likes fixed for xyz bug comments, that is what version control and commit comments are for – WendyG Sep 2 '18 at 10:55
  • 1
    @WendyG Sure, you put that information in the commit. But documenting why a non-obvious line of code exists is exactly what should be commented. I'm assuming in this case it wasn't clear from reading the code that the change had anything to do with a crashing bug. Sam should have checked the commit to see why the change was made, but putting in a comment prevents any misunderstanding. – DaveG Sep 2 '18 at 11:33
  • I was imagining you know the clutter line // bug 1245 all by itself. – WendyG Sep 2 '18 at 18:30
15

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

  • 100% sure - I found the change because I was looking at a list of his recent commits. The commit literally only changed that line, with a message like "Bug: List doesn't show 'ABC'. Fix: Line was XYZ, changed to ABC". It does indeed fix the display in the list, but I've built the latest version of the project and verified it will crash as I expected. – Em C Sep 2 '18 at 0:45
  • @EmC Well, then you simply talk with him after the mail. Good that you have the build so you can demonstrate it. As said, you both can think over how both problems can be solved and if not (or if you are unsure about side effects), escalate to the lead. – Thorsten S. Sep 2 '18 at 1:15
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.

  • Thanks for the answer! Is there a reason you suggest email over just talking in person? (I just realized it wasn't clear either is a possibility so that's edited in now.) – Em C Sep 1 '18 at 20:44
  • @EmC it sounds like part of the reason you’re worried about talking to him is because he’s a busy person. Going through the “official channels” with email and a calendar invite when you settle on a time will allow him to work you into his schedule and will hopefully help you feel less like you’re “annoying him.” Not to mention an email like this would give him a chance to look over the commit and remind himself of the code in question. – scohe001 Sep 1 '18 at 21:10
  • Depending on the office culture, an in-person meeting might be overkill. – David Z Sep 1 '18 at 23:55
  • 3
    Instead of saying “won’t this break in edge cases I, J and K?” (because you already know it will) why not just say you changed the code to XYZ to address I, J, and K, and then ask if there's anything you're missing? – jcm Sep 2 '18 at 0:22
0

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.

  • 1
    The OP says that he doesn't know how to fix the display problem, which is why he needs to work with Sam. – DaveG Sep 1 '18 at 23:22
  • It seems that the display problem wasn't there without OP's change. So he introduced it. Sam removed the code that fixed one bug and introduced another bug. How to fix both bugs simultaneously is OP's job, not Sam's. – gnasher729 Sep 2 '18 at 16:59

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.