Take the 2-minute tour ×
Programmers Stack Exchange is a question and answer site for professional programmers interested in conceptual questions about software development. It's 100% free, no registration required.

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.
share|improve this question

migrated from workplace.stackexchange.com Apr 3 '13 at 18:20

This question came from our site for members of the workforce navigating the professional setting.

18  
Do you use git, CVS, or TFS for your code repo? Just roll back his commits. He'll get it eventually :) –  MDMarra Apr 3 '13 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 Apr 3 '13 at 18:51
10  
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 Apr 3 '13 at 19:43
14  
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 Apr 3 '13 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 Apr 10 '13 at 3:18

7 Answers 7

up vote 69 down vote accepted

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.

share|improve this answer
3  
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 Apr 3 '13 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 Apr 3 '13 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 Apr 3 '13 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 Apr 10 '13 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 Apr 10 '13 at 17:59

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.

share|improve this answer
6  
+1 and more if i could. Great answer that addresses the best ways to improve such a process. –  Waldfee Apr 4 '13 at 8:19
1  
Yes the OP needs a code review tool, that way you can review the code together, it's not just someone changing code because they feel like it. We just started using smartbear.com/products/software-development/code-review at work –  Juan Mendes Apr 5 '13 at 1:57

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.

share|improve this answer
4  
+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 Apr 4 '13 at 8:38

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.

share|improve this answer
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. –  Stargazer712 Apr 4 '13 at 16:11
1  
@Stargazer712, More confusing than having incomplete code in the branch? –  user606723 Apr 4 '13 at 16:25
    
That is what shelfsets are for. Don't check in incomplete code. –  Stargazer712 Apr 4 '13 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. –  Stargazer712 Apr 4 '13 at 18:38
1  
They are called TODOs, they get left in working code all the time –  Juan Mendes Apr 5 '13 at 1:59

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.

share|improve this answer
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 Apr 4 '13 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 Apr 4 '13 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 Apr 4 '13 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 Apr 4 '13 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 Apr 4 '13 at 15:58

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.

share|improve this answer

I don't see why it's a problem.

It's better for the workplace if multiple people take responsibility for any given section of code. What happens when you're the only one who knows how to work on a certain part, and you leave?

If this person is breaking code that you made work, the source history should bear that out. If someone says "Hey, the stuff you worked on is broken!", just gently say, "That's weird. I thought it was working. Can we look at the history to double-check what happened?" If this person is habitually breaking your code, it'll all be there in the history.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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