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. He will review my changes before they complete the end goal and make changes right away... and he's even broken my work once before. Other times, he has made unnecessary improvements to some of my code that is 3+ months old.

This annoys me for a few reasons:

I am not always given a chance to fix my mistakes.

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.

I don't always think his code is readable.

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

You don’t own nothin’

Michael answers (55 votes):
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:

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.

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 create a poisonous development environment.

Fix the process

Yannis Rizos ♦ answers (56 votes):
You and most of the responders 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) and an opportunity for social interaction. They can 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 groups 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. 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 favorite 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 can 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 to have 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.

24 Reader Comments

Definitely prefer answer number 2. Couldn't help but read the original question and think there are tools for that now! TFS and TF Services also have the Crucible like tools built in to the latest release. Surprised those haven't been implemented until now, given that it's a common SharePoint workflow for Office documents.

I resist (as painful as it can be) modifying junior developer's code or config without talking to them first. Its actually something I enjoy and the response tells me plenty about their future and mine.

1> time is money and unnecessary changes are wasteful2> save it for another day if its not critical; roadmap, discuss and plan.3> dont be petty, its not a democracy where I work - but its also not a contest to see how quickly I can chase off a resource that may have plenty of value. Performance, stability, accuracy are issues - everything else does not matter to me. I will address readability if it is very poor, but I'm not a style nazi.4> I encourage junior devs to thicken their skin and embrace the joy of code review. I sometimes wish more coders would take stabs at what I write. Most of the criticism I get is from non devs, and its politics or money oriented.

Communication is important, but pragmatic programmers shouldn't be offended by others improving their code, especially when it's 3+ months old.

"When you come across a stumbling block because the code doesn't quite fit anymore, or you notice two things that should really be merged, or anything else at all strikes you as being "wrong," don't hesitate to change it." -- Hunt, Andrew; Thomas, David (1999-10-20). The Pragmatic Programmer: From Journeyman to Master (Kindle Locations 3295-3296). Pearson Education. Kindle Edition.

The main problem is that coders are Primadonnas. And I do not even exclude myself. I think they had a survey once and fully 98%of all coders thought they were above average.

And the question doesn't sound like a coding or process question but more like a social problem. Looks like the older guy doesn't really respect you otherwise he would drop by and discuss major changes with you and you are Pissed of by this. Now I would suggest talking it out. No code review tool will fix that.

(and I am not taking sides depending on more details one or the other might be in the clear.

Where I work we are too lax on peer reviews. The sub-team I worked in for the first few years had a higher proportion of fellow young engineers, however, and we took it upon ourselves to ask each other for peer reviews before merging our code to the trunk.

When we do peer reviews, the dynamic is usually one of the following:- simple/obvious changes: reviewer looks over the implementer's changes and sends an email with comments/recommendations, or- complex/obscure changes: implementer and reviewer sit down together, and the implementer walks the reviewer through the changes, explaining them along the way.

Reviews are/were generally performed by whoever else (besides the implementer) had the most knowledge about the area being changed. At no time did we feel that it was appropriate to actually change someone else's code; we would just make recommendations, or even just comments.

We had a checklist at one time, but it was really only good as a self-check to make sure you were following our internal coding standard. It got tossed aside at some point, which didn't bother me too much since I was good at self-policing, but it probably did result in other people getting sloppier.

The second answer is actually off topic. It assumed the question is about "peer-review", but the question sounds more like proofreading - a senior programmer checking the code written by multiple junior programmers to make sure the code quality match his expectation. If that is the case, the first answer is right to the point.

I resist (as painful as it can be) modifying junior developer's code or config without talking to them first. Its actually something I enjoy and the response tells me plenty about their future and mine.

1> time is money and unnecessary changes are wasteful2> save it for another day if its not critical; roadmap, discuss and plan.3> dont be petty, its not a democracy where I work - but its also not a contest to see how quickly I can chase off a resource that may have plenty of value. Performance, stability, accuracy are issues - everything else does not matter to me. I will address readability if it is very poor, but I'm not a style nazi.4> I encourage junior devs to thicken their skin and embrace the joy of code review. I sometimes wish more coders would take stabs at what I write. Most of the criticism I get is from non devs, and its politics or money oriented.

I'd say your missing the biggest reason to tell someone why you're changing their code, so that they don't make that mistake again

If you're changing code that you know someone else at least feels responsible for if not "owns", and you have neither the courtesy to talk to them about it, nor the good habits to put comments in the code / repository / etc to explain the reason for the change, well, there's a word for people like that, and it isn't a polite one...

I find the 2nd answers statement about it being easier to change the process than the people to be a bit out of whack with where this junior developer finds himself. He's not in a position to change the process and unless his leadership is in a position to be open to changing the process they have likely defined ... he's going to have quite an uphill battle.

Getting management buy-in to integrating a code review tool into your development is just one part of the battle. Actually implementing it and getting the rest of the team to adapt to the new process is going to be a significant challenge for all but the smallest or tightest of teams. You're about to move their cheese, possibly give them more paperwork (generate a patch file, upload the patch file, file a code review, blah blah blah), or require more comprehensive changes to your development process.

Tread carefully, for changing a process is itself a process and patience and listening to your peers will go a long way in helping you reach a better development place.

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.

1: So what. If somebody else wants to fix your mistakes, then that is awesome! Less work for you.

2: If he's confused, that is his problem. Why concern yourself with it? If he thinks the best use of his time is to try and figure out how your code works... then that is his decision to make.

3: Readable code is subjective... and since you are not the "senior" colleague there isn't anything you can do about it except learn how to read his code style or hand in your resignation. Before handing in your resignation I would speak to your boss, tell him why you want to resign, so that he has a chance to do something about it (i.e. fire your colleague).

4: what does that have to do with anything?

Basically... I think you should not do anything at all about your "problem" because as far as I can tell you don't have any problem at all.

If there are bugs in the changes he made to your code, I would shoot him an email or file a bug asking him to fix it. Perhaps you could diplomatically say you weren't sure exactly why he wrote it the way he did (unreadable code style and all that), so you're hesitant to change things incase you make it worse.

It sounds like your colleague is sick of doing management work and when he sees some half-finished code that looks easy to fix he jumps at a chance to write some code. As long as there are no bugs in the code he's writing, then I don't see anything wrong with him doing that.

You do not "own" your code. No interesting project gets completed by one coder working alone, it's always a team effort and everyone on the team "owns" it and has a right to make changes.

In my experience it's the crybaby that is always at fault. He's the one creating the trouble. If you get upset a lot it's probably because you have issues, not because others are mistreating you. Everyone needs super thick skin, and needs to have an attitude of welcoming critiicism, and being taught things. If someone improves your code just thank them. It doesn't matter if they fixed it instead of asking you to fix it. If they know how to make a fix as soon as they see it, then why should they delay, and make you fix it yourself, and risk some crying when they confront you about it? On the other hand if someone breaks your code, tell them, and warn them, and if it happens repeatedly tell management there's a problem with the process...unless you're a junior developer. Junior developers really need to just go with the flow, no matter what the senior guys are 'doing to them'.

Yikes, where do you work? I want to know so I NEVER end up working there, it sounds like a toxic environment.

These are all great comments! As someone occasionally in the position of the "senior coder" I can relate! I attempt to be a mentor - not a dictator - to my juniors, but their are times when reviewing their code (usually when things aren't "working as designed") that I have a very hard time not getting my coding stick in, especially when they have resorted to overly complex code and data structures / classes for stuff that has high maintenance requirements. In a previous position, we used to reward software engineers more for lines-of-code removed than for lines-of-code added, while keeping the functionality intact, or enhanced! :-)

I have to disagree with the opinion of Yannis Rizos regarding this a failure of process and advocating better ways of doing code review as a solution. I do agree the process and measures he describes would have helped to reduce the probability of this problem occuring, but the root cause seems to me a personal conflict situation where one side does not seem to listen and the other side is potentially having problems voicing their concerns in a way the other side accepts.

What could help is to do some research into conflict resolution / conflict management techniques and see if this already enables you making your colleague understand yor concerns better. If you do not see a way to solve this by talking with your colleague one-on-one, try asking another person to act as a mediator. Ideally, somebody who is respected by both of you and knows how to deal with these kind of situations.

An option would be to attend a course on conflict management by somebody with a professional background in this area. This gives you a better idea on social dynamics, how things may go wrong and ways to deal with that. Depending on how the course is structured he / she could give you better tips on how to proceed in such a situation.

It's hard to judge without knowing the other side of the story, because it seems real odd to me that he makes the changes without good reason.

I'm a manager on projects and I've actually done that, which I don't consider correct, but there were logical reasons for it. - Slow work. This was the main reason, getting sick of waiting for a pretty basic piece of code to be completed. The OP talks about that in 4, saying that are no urgent deadlines ....really?;- Lack of improvement: the junior has repeatedly showed bad performance and has a poor track record accepting constructive criticism. So I didn't really bother giving feedback, waste of time.

My advice is for you to look into your own procedures, since you can't make a senior stop. If you could actually talk to him you wouldn't be here talking to strangers, which shows there is no trust.

The sooner you learn to remove your emotional attachment to the code that you write, the better you get at staying 'cool' (or aloof,perhaps) when others want to change things in your code. I view it as time earned that I don't have to spend maintaining the code and instead I can write even more new code.

I'd say your missing the biggest reason to tell someone why you're changing their code, so that they don't make that mistake again

Agreed. This is the main purpose of code reviews IMO - and why every true "code review" I have participated in was a verbal review involving the author and one or more reviewers. The author was then expected to make the suggested changes - but in their own style/time.

Moreover, a code review that doesn't involve the author being present/represented in the review gives the author no chance to explain what the code is doing and the "reviewer" may misunderstand the code, then break it when "fixing it".

I trust the developers under me. I just ask them to follow a coding style for method and stored proc names. As long as they run it through the profiler and lint we are good to go. Code Peer review starts when I hit F5 and I get compiler errors. Make them fix it then I act as dumb web site user.

C# is too young to have many really bad opportunities for gross code. We pretty much all have stolen code from MSDN.

The OP begs some questions:> I am not always given a chance to fix my mistakes.

How bad were your mistakes?

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

Was he really confused, or was it the other way around?

I've had many times where I thought my lead was confused about my code. It turns out he may have been confused about things specific to my code, but was a hell of a lot clearer about the objectives of the code in general. It's always a learning process.

> I don't always think his code is readable.

By whom?

> Deadlines are not an issue and his current workload doesn't require any work in my projects other than reviewing my code changes.

Sorry sorry. Just kidding. Start again. Honest, compassionate communication is the essence of relationships and, like it or not, you & the old fart have a relationship. As weird as it might feel, I'd just tell Mr. Lone Ranger how you *feel* (insulted, belittled, frustrated, confused) about his asynchronous changes.

If that doesn't work, trust in the process. There's a reason companies get rid of workers over 40 - they cost too much for health care, they get too much time off, they make too much salary and they can't remember schmidt. A Director or VP can hire two or three Indian grads for the price of one engineer with 20 yrs experience, and have enough left over for a self-directed "cost reduction" bonus equaling a year of payments on a new Tesla mod S.