72

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.
gnat
  • 21,442
  • 29
  • 112
  • 288
Jesslyn
  • 839
  • 7
  • 9
  • 20
    Do you use git, CVS, or TFS for your code repo? Just roll back his commits. He'll get it eventually :) –  Apr 03 '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 03 '13 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 Apr 03 '13 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 Apr 03 '13 at 21:20
  • I don't see any problems with developers changing your code as they see a need while doing there own development, regardless of who wrote the original code. Especially during development. Changing code on fielded systems is another matter entirely. The only caveat I would add is that at my place there's an unwritten rule that last person to modify also gets assigned the task of fixing whatever new problems are discovered with that file, whether their change caused the problem or not. – Dunk Apr 04 '13 at 13:46
  • 3
    @Dunk, in my question I stated that my colleague had no development assignments in the same project--he was only responsible for reviewing code. I believe one of the points of a code review is to involve the programmer whose code is being reviewed/changed. – Jesslyn Apr 04 '13 at 14:40
  • 1
    @Jesslyn-You also said he's 2nd in command. Maybe part of his role in that regard is to ensure that ALL the code is up to his expectations. Where I am currently working, while we are in development, we have an environment where everyone owns the code. People are free to change whatever they believe needs changing. Nobody is offended or takes it personally that somebody changed code that they may have written. The worst someone gets is jokingly ragged on for breaking something. The only problem I see in your situation is your choosing to be offended instead of just moving on to your next task. – Dunk Apr 04 '13 at 15:19
  • @Jesslyn That's not always how it works. Code reviews are a team activity and how exactly they are conducted highly depends on the team's structure, dynamics and... ahem... culture. There's no one size fits all solution, if you really want code reviews to be as effective as possible you have to sit down with your team, discuss, and decide what approach is best for you. – yannis Apr 04 '13 at 15:19
  • @Dunk, can you explain how your team does code reviews? Your team sounds very forgiving. You mentioned that the last developer to touch the code gets the next task, is that even when all they did was a small cosmetic change or only done minor changes in the project and are not considered the project expert? – Jesslyn Apr 04 '13 at 15:41
  • 1
    @Jesslyn-In the past, we had done code reviews as modules were initially completed. The problem is that once code is reviewed, it is considered locked down and can only be changed via Change Requests. This resulted in minor bugs/ugly code/other issues to remain in the codebase because the pain to fix was to high. Now we wait as long as possible to do code reviews, usually after a module is solid and works with other modules that are being developed. This gives the freedom for people to change code, fix minor bugs, cleanup code as they integrate. – Dunk Apr 04 '13 at 17:27
  • If a bug is discovered in a class that requires investigation then whoever modified the file last is usually told to investigate the issue. However, if the person is not even on your project so that you can't assign him/her the task of fixing the issue then that would complicate matters, which might be the part I missed. But even so, unless you have a formal Problem Report Tracking system and these changes are being tracked against that then I can't see how his changes could reflect negatively on you unless he is also complaining to your lead while making the changes. – Dunk Apr 04 '13 at 17:28
  • 1
    I would also add that once many people on the team get involved in hacking up the initial code, it invariably ends up being better than when it started and reusable in more situations. – Dunk Apr 04 '13 at 17:35
  • If I wanted to fix your code, I would do it, then push it as a separate branch and ask you to review it. Is the problem that this person fixed your code without consulting you, or that this person deployed the fixes without talking to you first? – user16764 Apr 04 '13 at 23:33
  • 3
    What sticks out to me here is "and he's not been responsive". Red Flag IMO - something not OK with that guy. – Vector Apr 04 '13 at 23:36
  • @hyde - nothing wrong with checking in unfinished code if it doesn't break the build and isn't callable by others. Just because code is checked in doesn't mean it's going to run, even if it's built. A lot depends on the context as well - are we dealing with a corporate evironment, a custom/botique software shop, an SAS provider etc. – Vector Apr 05 '13 at 01:53
  • @Mikey What's wrong with it is combination of that "if" you say, combined with there being a better way. Just because cutting trees with an axe used to be how it's done, modern chainsaw is still better, and a must if you do it professionally. Using a VCS systems which does not support easy branching and merging, or not using those features in team projects, is a lot like using an axe instead of a chainsaw, in my experience-based opinion (how to use them right is then a broad subject, and they can be used wrong, of course). – hyde Apr 05 '13 at 05:22
  • 1
    How can words "unnecessary" and "improvements" can be used in one sentence? Every code, you wrote, is not practically your. It's code of company, so every developer shares the same code. Do code reviews and every change should be agreed upon. – Adronius Apr 05 '13 at 06:26
  • Hey all, this discussion is getting a bit long, and comments aren't suitable for extended discussions. Jesslyn the better way to respond to comments asking for clarifications is to update your question instead of adding yet another comment. Comments are not that visible and it would be a shame for your clarifications to be lost in the noise. Everyone else please refrain from side discussions in comments (our [chat room](http://chat.stackexchange.com/rooms/21/the-whiteboard) would be the better place for those), only post comments if you want further clarifications from Jesslyn. – yannis Apr 05 '13 at 14:34
  • 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 03:18
  • 2
    I'm voting to close this question as off-topic because it's not about programming, it's about interacting with people in the workplace. – Ixrec Dec 21 '15 at 15:24
  • @Ixrec http://meta.workplace.stackexchange.com/questions/1767/curious-migration-to-programmers-se – Jesslyn Dec 22 '15 at 19:46

7 Answers7

87

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.

Community
  • 1
yannis
  • 39,547
  • 40
  • 183
  • 216
  • 2
    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 http://smartbear.com/products/software-development/code-review at work – Ruan Mendes Apr 05 '13 at 01:57
81

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.

samthebrand
  • 368
  • 2
  • 12
  • 27
Michael
  • 2,979
  • 19
  • 13
  • 1
    My only concern with your answer is what if my team lead becomes worried that someone else "has to" improve my work? I doubt my current team leader would care, but I've had team leads in the past that would let such changes count against me--which doesn't seem fair if the improvements were unnecessary. – Jesslyn Apr 03 '13 at 16:22
  • 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 Apr 03 '13 at 16:26
  • So you're saying that I should just focus more on learning from my teammate and less on doubting myself and his incentive? This makes sense, as I cannot change my colleagues but only myself. – Jesslyn Apr 03 '13 at 16:31
  • 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 03 '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 03 '13 at 18:00
  • You are stating that "team ownership of code is great" as a fact, whereas it is a debatable statement and some team have a totally different view and organization. See e.g. point 7 in http://www.paulgraham.com/head.html – Giorgio Apr 04 '13 at 08:35
  • Giorgio - fair point, I realise that not all teams subscribe to team ownership of code but I think most teams would accept that other people reviewing your code and suggesting improvements is "a good thing" - there's definitely an etiquette to be considered though to avoid trampling all over someone elses work. – Michael Apr 04 '13 at 08:38
  • @Michael: Yes, also with individual ownership code reviews are well accepted, and the reviewer can also suggests changes, but the code owner has the responsibility and the last word about the change. Furthermore, with individual ownership, it is unthinkable that a developer starts playing (unfortunately, this is what happens sometimes) with a module they do not own. With team ownership the borders are less well-defined and this can be exploited to justify unneeded changes through team-ownership: You do not own the code and therefore I am free to improve it. – Giorgio Apr 04 '13 at 08:49
  • is it unreadable to just you and your mates or to other reviewers too? if he coding better then maybe you should take the trouble to see what he is doing and learn? places where he breaks the code functionality are not acceptable; that you need to address; maybe need more test suites to test that automatically and sooner? – tgkprog Apr 04 '13 at 09:14
  • 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
  • Also, the situation is symmetrical: did the one who changed the code try to understand the original version before changing it? Maybe they could have learned something from it? Maybe they have overlooked something? Sometimes there are changes that only serve to customize the code style according to the taste of one team member. And some times these changes affect (invalidate) the original design of the code (e.g. move business logic into the GUI layer). – Giorgio Apr 10 '13 at 17:34
  • 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
  • @Giorgio:I think your using Graham as a source falls flat, considering he is such a LISP proponent. Of course you don't want anyone else mucking with code when a typical line of code looks like (CAR (CAR (CDR (CAR (CDR list1 list2))))). Also his whole premise is that "Only the author can truly understand the code". IMO, If someone writes code that someone else can't understand then that means there's a problem with the code. – Dunk Apr 10 '13 at 18:44
  • 2
    I think time and currency is really critical to the ownership issue. Code that is being worked on right now by me is certainly owned by me. Code for a feature I finished two months ago and has been chugging along in production is the teams (especially if it breaks, tee-hee, but seriously it's gotta e fixed ASAP!). It's always a judgment call and relies on good developer relations. – Michael Durrant Apr 11 '13 at 03:03
  • "It's always a judgment call and relies on good developer relations.": I have upvoted your comment for this. – Giorgio Apr 11 '13 at 08:18
  • 1
    @Dunk: "typical line of code looks like ...": I am not a Lisp expert but AFAIK typical Lisp code does not look like that. Regarding "truly understanding" code: you take a statement to the opposite extreme and then conclude it is not valid. Code can be understandable and well written, but a lot of information (why a class is there and why it is connected to other classes in certain ways, how it can be safely extended, what the full implications of deleting a method are, and so on) is only in the head of the original author. By changing code arbitrarily you can invalidate much of this knowledge. – Giorgio Apr 11 '13 at 08:25
  • @Giorgio:I admit that I don't claim to be a LISP expert either, but I did have to do a fair amount of programming in LISP in Grad courses. I'm sure anybody else who had to do similar will vouch that my example is not unrealistic or extreme. Somebody who programs in LISP as their profession will hopefully have much better style. As for you implying that the author is the only one who can safely modify code...what do you think maintenance programming is all about? The original developer is seldom available yet they have to modify the code. Also, my experience has been that the more input... – Dunk Apr 11 '13 at 17:48
  • from other developers on code the better it turns out. If you make it easy for developers to give input (eg. let them modify the code to fit the needs) then more people will provide input. If they have to jump through hoops then they won't bother. They'll copy/paste/modify a new class instead. Once code has been formally reviewed it is a different matter. But while in development, making it easy for developers to make other people's code useful for their purposes pays off in much higher quality code and designs in the end. If your code needs all kinds of coupling knowledge then its bad code. – Dunk Apr 11 '13 at 17:52
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.

kevin cline
  • 33,608
  • 3
  • 71
  • 142
  • 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 Apr 04 '13 at 08: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.

user606723
  • 1,169
  • 9
  • 13
  • 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 Apr 04 '13 at 16:11
  • 1
    @Stargazer712, More confusing than having incomplete code in the branch? – user606723 Apr 04 '13 at 16:25
  • That is what shelfsets are for. Don't check in incomplete code. – riwalk Apr 04 '13 at 17:44
  • But if you're going to check in incomplete code... even thought you shouldn't.. it should probably have a comment. – user606723 Apr 04 '13 at 17:58
  • 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 Apr 04 '13 at 18:38
  • 1
    They are called TODOs, they get left in working code all the time – Ruan Mendes Apr 05 '13 at 01:59
  • @Stargazer: It really depends on what phase of development you are in. If you are modifying already fielded code and anything you check in is likely to be fielded, whether complete or not then you are correct. However, especially during development, sometimes the code does "enough" so that someone else who needs it can make use of it. Yet, that code may not do everything it is supposed to yet. Maybe you haven't done ALL the error checking/handling you plan on doing yet. A comment, with an easily searchable tag (ie TODO) helps ensure that the missing functionality isn't missed. – Dunk Apr 10 '13 at 18:38
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.

Vector
  • 3,180
  • 3
  • 22
  • 25
  • 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 04 '13 at 09: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 04 '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 04 '13 at 14:55
  • @Giorgio - yes, sometimes this sort of behavior is 'political' - I have seen it far too often. It undermines trust amongst team members: Instead of people learning from one another, they just worry about CYA and 'one-upmanship' - nobody profits from that. – Vector Apr 04 '13 at 15:47
  • 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 04 '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 04 '13 at 15:58
  • 1
    I didn't end up downvoting, but I did consider it for "A good team leader will take your side - a bad one won't". I really don't think it's a good idea to make that assumption. Thinking you have a really good case is one thing, thinking that anyone who dares disagree must be wrong is something else. I also didn't like "A good co-worker would call you over and point out the problem with your code to you", since it implies that anyone who communicates over email or has a different view of code ownership is a bad co-worker. – Michael Shaw Apr 04 '13 at 16:01
  • 1
    @Michael Shaw: I see your point. Even if one team member started to arbitrarily change someone else's code to undermine their position (which would be really a very bad behaviour) it is very difficult to judge if this is the case, especially because the original author of the code and the author of the change are the ones who know the code in detail, and an external observer can have a hard time understanding what is really going on (i.e. if changes to the code are necessary and well motivated or arbitrary and only aimed at disturbing someone else's work). – Giorgio Apr 04 '13 at 16:04
  • 1
    @Giorgio - it's philosophical concept: I'm not talking about legal 'ownership': simply that what YOU produce is 'YOURS' - a person naturally feels a personal connection to what they produce themselves. – Vector Apr 04 '13 at 16:59
  • 1
    @Michael Shaw: "A good team leader will take your side-a bad one won't". I really don't think it's a good idea to make that assumption." : Understood. I'm basing my statement on the scenario the OP detailed. If that's inaccurate, I agree - all bets are you off. 'it implies that anyone who communicates over email': you are taking it too literally. My point is, let your co-worker in on it - then everyone benefits - email is also fine. But tampering with a co-worker's code without letting them know doesn't pass the 'smell test' IMO, unless that is their assigned job: to be the 'end point' coder. – Vector Apr 04 '13 at 17:11
  • 1
    @Mikey: I agree with you on this: every person has a personal connection with what they produce. In fact, I have a much weaker connection with a project's code when using shared ownership. – Giorgio Apr 04 '13 at 17:29
  • 1
    @Giorgio - yes, that's the point. So based on what the OP Jesslyn described, her co-worker is not 'playing nice'- that sort of behavior is suspect IMO. PARTICULARLY since she approached him on the subject and was ignored. Not a good sign. – Vector Apr 04 '13 at 17: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.

gnasher729
  • 42,090
  • 4
  • 59
  • 119
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).

gnasher729
  • 42,090
  • 4
  • 59
  • 119
Sandeep
  • 169
  • 3