71

One of my teammates is a jack of all trades in our IT shop and I respect his insight.

However, sometimes he reviews my code (he's second in command to our team leader, so that's expected) without a heads up. So sometimes he reviews my changes before they complete the end goal and makes changes right away... and has even broken my work once.

Other times, he has made unnecessary improvements to some of my code that is 3+ months old.

This annoys me for a few reasons:

  1. I am not always given a chance to fix my mistakes
  2. He has not taken the time to ask me what I was trying to accomplish when he is confused, which could affect his testing or changes
  3. I don't always think his code is readable
  4. Deadlines are not an issue and his current workload doesn't require any work in my projects other than reviewing my code changes.

Anyways, I have told him in the past to please keep me posted if he sees something in my work that he wants to change so that I could take ownership of my code (maybe I should have said "shortcomings") and he's not been responsive.

I fear that I may come off as aggressive when I ask him to explain his changes to me.

He's just a quiet person who keeps to himself, but his actions continue. I don't want to banish him from making code changes (not like I could), because we are a team--but I want to do my part to help our team.

Added clarifications:

  • We share 1 development branch. I do not wait until all my changes complete a single task because I risk losing some significant work--so I make sure my changes build and do not break anything.
  • My concern is that my teammate doesn't explain the reason or purpose behind his changes. I don't think he should need my blessing, but if we disagree on an approach I thought it would be best to discuss the pros and cons and make a decision once we both understand what is going on.
  • I have not discussed this with our team lead yet as I would prefer to resolve personal disagreements without getting management involved unless it is necessary. Since my concern seemed more of personal issue than a threat to our work, I chose to not bother the team lead. I am working on code review process ideas--to help promote the benefits of more organized code reviews without making it all about my pet peeves.
21
  • 20
    Do you use git, CVS, or TFS for your code repo? Just roll back his commits. He'll get it eventually :)
    – MDMarra
    Commented Apr 3, 2013 at 16:26
  • 6
    In my org, all code changes are supposed to go through a review of some kind, and it's considered poor form to check in a change without noting in the changelist description who the reviewer was. Introducing that principle in your org might be a long-term solution to the problem of coworkers checking in changes to code you wrote without review.
    – Carolyn
    Commented Apr 3, 2013 at 18:51
  • 13
    Why do you have non-finished changes in a shared branch? That's a bad idea, and not just for the reason you have encountered.
    – hyde
    Commented Apr 3, 2013 at 19:43
  • 15
    Of course this depends on what VCS you use, but that might be something to look into, starting to use branches more. With personal branch, it's IMO great when you can commit (and push with DVCS) whenever you feel like it without worrying, and merge only when done with a part, or merge only partially when necessary (a good DVCS makes this pretty easy). Also works great with code review, being able to naturally do it and fix issues before merging.
    – hyde
    Commented Apr 3, 2013 at 21:20
  • 4
    @Jesslyn - Is your team lead at all concerned that your teammate is spending time making unnecessary improvements to old code? At the least, it seems inefficient for your teammate to be spending time making unnecessary changes as opposed to working higher priority tasks. In addition, if your teammate prefers to spend time "fixing" your code for you rather than empowering you to do it yourself, that seems pretty inefficient as well. Have you discussed any of these concerns with your team lead?
    – Cliff
    Commented Apr 10, 2013 at 3:18

7 Answers 7

79

I think most developers find themselves in this position at some point, and I hope that every developer who's felt victimized realizes how frustrating it will be when he or she becomes the senior and feels compelled to clean up code written by juniors.

For me, avoiding conflict in this situation comes down to two things:

  1. Courtesy. Talking to someone about his/her code allows a dev to know that you're interested and you can discuss it as grown up professionals.

  2. Forget about "code ownership" - the team owns the code. Other people wanting to make the changes is a good thing. If a senior dev makes changes that are "unreadable" or worse, then back them out. You don't need to be aggressive, just let an editor know that his/her changes didn't work, and you're more than happy to discuss your reversion.

Remember, team ownership of code is great and it cuts both ways. If you see something that doesn't make sense in someone else's code, then fix it. Being overly possessive and inadequately communicative is a surefire way to a create a poisonous development environment.

18
  • 4
    I think that comes down to Point 1 - have a discussion with him. Counting the fact that code changes against the original developer is terrible management (I'm not saying it doesn't happen) and I'd argue that you need to petition for better metrics. Cross that bridge if and when it happens though.
    – Michael
    Commented Apr 3, 2013 at 16:26
  • 2
    I think that's what we should all focus on - in general, your teammates aren't out to make you look bad - don't spend time over analysing, just become better and a more valuable member of the team (I know that's easier said than done though!)
    – Michael
    Commented Apr 3, 2013 at 16:32
  • 7
    @Jesslyn, If he is doing it to you, chances are he is doing it to everyone. I doubt anyone is counting that against you. (If he isn't doing it to everyone, you might have a different problem)
    – user606723
    Commented Apr 3, 2013 at 18:00
  • 3
    @tgkprog: (1) Of course, getting feedback from others is very important and I have learned a few things from looking at others' code but (2) if you want to learn from each other you should suggest a change and discuss it together. Just changing random stuff because you think the code is better after the change is not the right approach. There are better approaches like code reviews using review tools.
    – Giorgio
    Commented Apr 10, 2013 at 17:31
  • 2
    yes i was thinking of times when i have fixed others code before a dead line at 11pm but even then would send a mail to team so they can check it too, including our QA ....
    – tgkprog
    Commented Apr 10, 2013 at 17:59
86

You and most of the answerers approach this as a communication issue between two colleagues, but I don't really think it is. What you describe sounds more like a horribly broken code review process than anything else.

First, you mention that your colleague is second in command and it's expected that he'll review your code. That's just wrong. By definition, peer code reviews are not hierarchical, and they are certainly not just about finding defects. They can also provide learning experiences (for everyone involved), an opportunity for social interaction, and prove a valuable tool for building collective code ownership. You should also review his code from time to time, learn from him and correct him when he's wrong (no one gets it right every time).

Furthermore, you mention that your colleague makes changes right away. That's also wrong, but of course you already know it; you wouldn't have asked this question if his gung ho approach wasn't a problem. However I think you are looking for a solution in the wrong place. To be perfectly honest, your colleague reminds me a bit of... me, and what worked for me in similar situations was a well-defined and solid review process and a set of awesome tools. You don't really want to stop your colleague from reviewing your code, and asking him to stop and talk to you before every little change is not really going to work. It might, for a while, but he'll soon reach a point where it will just get too annoying and you'll be back where you started, or worse: he'll just stop reviewing your code.

A key to a resolution here might be a peer code review tool. I usually avoid product recommendations, but for code reviews Atlassian's Crucible is really a life saver. What it does may seem very simple, and it is, but that doesn't mean it's not amazingly awesome. It hooks up to your repository and gives you the opportunity to review individual changesets, files or group of files. You don't get to change any code, instead you comment on everything that doesn't feel quite right. And if you absolutely must change someone else's code, you can simply leave a comment with the changeset explaining your changes. The introductory video at Crucible's product page is worth watching if you want more details. Crucible's pricing is not for everyone, but there are numerous freely available peer review tools. One I've worked with and enjoyed is Review Board and I'm sure you'll find a lot of others with a simple Google search.

Whatever tool you choose, it will completely change your process. No need to stop, get off your chair, interrupt the other person and discuss the changes; all you need to do is set some time off every week and go through the comments (once a week is just a suggestion. You know your schedule and daily routine better than I do). More importantly the core reviews are stored in a database somewhere and you can retrieve them at any time. They aren't ephemeral discussions around the water cooler. My favourite use case for old reviews is when introducing a new team member to our codebase. It's always nice when I can walk someone new through the codebase pointing out where exactly we were stuck, where we had differing opinions, etc.

Moving on, you mention that you don't always find this colleague's code readable. That lets me know that you don't have a common set of coding standards, and that's a bad thing. Again you may approach this as a people problem or you can approach this as a process problem, and again I would strongly suggest the latter. Get your team together and adopt a common coding style and set of standards as soon as possible. It doesn't really matter if you chose a set of standards that's common in your development ecosystem or you come up with your own. What really matters is for your standards to be consistent and that you stick to them. Lots and lots of tools out there can help you, but that's a whole different discussion. Just to get you started, a very simple thing to do is having a pre-commit hook run some kind of style formatter on your code. You can continue writing your code however you like and let the tool "fix it" automagically before anyone else sees it.

Lastly you mention in a comment that management does not believe individual dev branches are necessary. Well, there's a reason we call them "dev branches" and not "management branches." I'll stop here as there's no reason for the rant that's forming in my head to get out.

All that said, know that I don't doubt your colleague is (a bit) at fault here. That's not my point, my point is that your whole development process is also at fault, and that's something that's easier to fix. Arm yourself with the proper tools, explore the numerous formal and informal processes and pick those that fit your team. Soon you'll reach a point where you'll realize that most of your "people problems" don't exist anymore. And please don't listen to anyone (including yourself) that brings forth the "we're a small team, we don't need all that" excuse. A team of competent developers can set up the necessary tools in less than a week, automate everything that can be automated, and never look back again.

PS. "Code ownership" is a nebulous term, constantly debated, and it means different things to different people. You can find a brilliant collection of most of the differing (and sometimes antithetical) opinions on C2.

1
19

What is it about the process that makes you want to take responsibility for "your code" ? Do you have the sole responsibility to keep certain features working? Did the lead say "Michael, I want you to take responsibility for ..." ? Or is your responsibility implicit, in that the lead and the rest of the team look to you every time certain features are broken?

Either way, if you have the responsibility, then you need authority over the code. The next time the other fellow makes unilateral changes and the lead comes back to you to fix them, you should sit down with the lead and ask to have your authority and responsibility aligned.

1
  • 5
    +1 For pointing out an important fact: Either there is team ownership and (1) all are responsible (2) everybody can change the code, or there is individual ownership and (1) the module owner is responsible (2) every change must be approved by the module owner. Often confusion arises when a team member is expected to be responsible for a module, but a senior member feels entitled to make random changes to the code. In other words, one has to avoid the "responsibility without authority" situation.
    – Giorgio
    Commented Apr 4, 2013 at 8:38
4

Not that this will solve the whole situation, but you might try adding more comments to your source code.

  1. If code is not complete, it could be marked as such.
  2. If the purpose of a block of code is not self-documenting, then you should document it.

All and all, try to make lemonade instead wasting time sucking on lemons. As Michael said, in general, teammates aren't out to make you look bad. Try to learn from your mistakes and apply them to future revisions.

If you believe that his changes are having a negative impact, please voice this (diplomatically). If it were me, I would simply ask why specific changes were done and see if you can defend your original changes. Your senior co-workers are human too. It's quite possible that he missed something and/or is unaware of any negative impact he is providing.

7
  • 3
    Boy that could get confusing. Imagine--adding a comment stating, "This code is not complete," and then forgetting to remove it once you complete the code.
    – riwalk
    Commented Apr 4, 2013 at 16:11
  • 1
    @Stargazer712, More confusing than having incomplete code in the branch?
    – user606723
    Commented Apr 4, 2013 at 16:25
  • That is what shelfsets are for. Don't check in incomplete code.
    – riwalk
    Commented Apr 4, 2013 at 17:44
  • 2
    No. If you check in incomplete code that needs a comment to label it as such, then you're already in hell. Comments just take you to a different corner of hell.
    – riwalk
    Commented Apr 4, 2013 at 18:38
  • 1
    They are called TODOs, they get left in working code all the time Commented Apr 5, 2013 at 1:59
4

Everyone implicitely 'owns their own code', regardless of the politics, legalistics, or economics - it's the 'nature of things' - you naturally feel a personal connection to your own work.

If your co-worker is engaging in the behavior that you described and remains unresponsive when you ask for a heads up, that co-worker is discourteous, to say the least, and may be trying to undermine you (to say the worst...) - does NOT sound like a team player.

A good co-worker would touch base with you and point out the problem with your code to you - and let you fix/change it, or respond appropriately. I am very grateful that even when I was a newbee, my mentors always pointed out to me what I was doing wrong, explained why and let (or made) me fix it. That made me a better programmer and everyone benefited. And that's what I have always done when reviewing work done by others. Then you, (or whoever) actually learns something from your 'jack of all trades', and the code and the team all get better, including your teacher: Teaching helps understanding.

If it's at all possible, I would discuss the matter in private with the Team Leader. Based on your description of the situation, a good team leader will take your side - a bad one won't.... Obviously this requires caution - you will have to judge that for yourself.

12
  • 1
    Apart from the initial statement (there are teams who adopt team ownership and other teams that adopt individual ownership), I find the observations in this answer OK (+1 for this): I have also observed shared code ownership being used to undermine a team member's position within the team by arbitrarily modifying their code. So I do not understand the downvotes. It would be good if the downvoters cared to explain.
    – Giorgio
    Commented Apr 4, 2013 at 9:10
  • 1
    I think Mikey's answer is good explanation as to how do I stay a team player. Maybe this is too people focused? Like Yannis suggested, my team's development process sounds to be the real problem. Regardless I am curious as to why this was downvoted.
    – Jesslyn
    Commented Apr 4, 2013 at 11:11
  • 1
    @Jesslyn - I surmise I was downvoted because of my statement "Everyone implicitely 'owns their own code'". This is a philosophical question, not a policy question. :-)
    – Vector
    Commented Apr 4, 2013 at 14:55
  • 1
    @Mikey: "Everyone implicitely 'owns their own code'" This can be a matter of debate, of course. Programmers that are not self-employed normally sign a contract that says they do not own the source code. Apart from this, I think that the author of the code is the one that understands the code best, and if someone else changes the code, then neither the author nor the second developer really understand the code 100%.
    – Giorgio
    Commented Apr 4, 2013 at 15:57
  • 1
    @Mikey: Shared code ownership tries to ensure that enough team members understand the code well enough (while nobody really understand it really well). This is preferable to individual code ownership, where each module is (at least in principle) fully understood by one programmer, but there is the risk that this knowledge goes lost if that programmer quits.
    – Giorgio
    Commented Apr 4, 2013 at 15:58
1

If you write code, then I should review it.

If I change your code during the review, then the code is not the code anymore that I reviewed, but code that I changed. Therefore it needs to be reviewed. Probably by you.

If I commit your new code with my changes without someone reviewing my changes, then I have committed (1) an unreviewed change, and (2) the worst possible sin if code reviews are taken seriously.

0

I think you are handling it the right way for now - but there will soon be a tipping point where it distracts you to the extent you might not be happy coding this way at all.

If I were you, I would request a quick one-to-one with this person and explain my PoV calmly yet firmly. Team ownership of code etc is all fine but unless you give every developer enough space to put his work out, make mistakes and improve you will never build good code. This can be a friction area sooner rather than later.

(There is a totally different answer if this was on workplace.stackexchange. Figuring out the correct way to do code reviews is the easy bit. Convincing your co-worker to comply with this is a lot harder).

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