Ingo has developed websites for more than 20 years. A creative web developer focused on creating and improving websites to make the web more accessible, sustainable, and user-friendly.
As everyone is now doing test-driven pair-programming all of the time, why would we even need code reviews anymore? </irony>
But seriously, there are many possible reasons to feel defensive or uncomfortable in a code review. Maybe you had to finish something in a hurry and hope to get it shipped before an important "deadline". Maybe you had to rewrite "your" code several times to meet peculiar formal code style requirements that your coworker isn't aware of. Maybe you're in a probabation period and anxious to make a good impression. I've had all of this, including "public" code reviews in a meeting room with a bunch of coworkers delegated to stare at our PR "to learn something" , but the most annoying code review situation was when I refactored some nice code to meet the change requests of the first review, only to defend those changes against the second code reviewer, reviewing my refinements and requesting to revert the changes back to my original draft.
Don't take it personally. You are not your code. If someone suggests a different way to do something, it is an opportunity to discuss. Maybe your way is better; maybe there's another way to approach the problem.
Keep in mind that if you are having your code PR'ed, it is being written for the team. The best communicators are the ones who can adapt what they're saying so that everybody regardless of skill level understands; the same is true with code.
You will not be the owner/sole maintainer of that code forever, even if you want to be (unless it's truly your own project, in which case you have the right to ignore the PR). Try to reach a compromise for everyone to agree.
Graduated in Digital Media M.Sc. now developing the next generation of educational software. Since a while I develop full stack in Javascript using Meteor. Love fitness and Muay Thai after work.
Depends on the reviewer's style of communication. If it's clear that the reviewer's intention is to help me mold a bulletproof piece of awesomeness then their review SHOULD always be as detailed as possible and should not hesitate of using the hardest criticism as possible.
However if it sounds as their intentions is simply to disregard my implementation, because it doesn't fit their taste then I may get defensive.
In the end it's all written and missing nuances of the spoken word, so both sides need to be careful with interpretations.
One of the most salient features of our Tech Hiring culture is that there is so much bullshit. Everyone knows this. Each of us contributes his share. But we tend to take the situation for granted.
I would say it depends on how much back and forth happens about what we did vs what the reviewer was expecting and how that is communicated.
This can be hard sometimes but depending on the company culture/standards, it's better to try to let go any personal attachments and just address the reviews and be done with all that since at the end of the day the code won't belong to us.
For work, I'm my own code reviewer (it's a small company).
As for contributing to OSS, I don't usually get defensive, no. I go in with the assumption that any PR I make might not get merged due to factors outside my control, but I'll argue the case for why it should get merged, unless something comes up during the discussion that changes my mind.
For nitpicks about code style etc I usually implement the requested changes without question.
There is one OSS project that's an exception, though, which has a weirdly insular and toxic community, especially one or two contributors who seem to take great delight in raising pointless objections to any attempt to approve the low quality of its code base... I won't name and shame, but such projects definitely exist. A lot depends on the community around specific projects.
Because it feels like your baby, the thing you have created is now being altered.
But to say it better, code changes all the time, next week the intern might have removed the code because the function specification was changed or whatever so just don't get to attached to your own code.
One of the most salient features of our Tech Hiring culture is that there is so much bullshit. Everyone knows this. Each of us contributes his share. But we tend to take the situation for granted.
It's our baby and we are encouraged to code with a perfectionnist mindset, have strong opinions on how to do it right, produce clean code, respect all good practices, remove that newline would offend our sense of beauty, ...
And then we submit a pull request and suddenly we need to be like an ego-less buddhist monk that see things with a beginner mind and welcome challenges from everyone.
Sometimes??? Most PR reviews degenerate into arguments (usually only within a team, not normally on open source stuff). Programming is a deeply personal thing, it's very difficult not to take review comments as a slight against you.
Two things to keep in mind when you're feeling defensive:
Collaboration can be tricky since we're all unique individuals with unique personalities
The nature of some PRs (ex: Architecture) require the solution to be challenged as the rest of the team will have to build upon the foundation the PR lays down.
One of the most salient features of our Tech Hiring culture is that there is so much bullshit. Everyone knows this. Each of us contributes his share. But we tend to take the situation for granted.
I like your second point, there are indeed nature PR of different natures and if we recognize that, we can explicitly invite people to challenge us hard for PR that will have lots of repercussion down the line, but not do it for all PRs which would be wasteful
The 1st one I ever had yes, I was a bit defensive but I decided to not engage till the next day after my team destroyed the PR and I cooled off. I feel like they had been waiting for the opportunity since forever. After that they no longer destroyed my PRs and were pretty cool about it.
But it's been a while since I've been on a team that actively reviews code this way lol
Software Manager, Agile Aficionado, Tech Dabbler, and Public Speaker. A lifelong learner, the prospect of something new and coffee are what get me up in the morning.
It's hard not to be, you've spent hours making a solution to the task you are given and think its ready and doing what it should, then you get critiqued on some point which then makes you feel you didn't do a good job. I've given constructive feedback which has at times riled someone's feathers because he thought he had solved just the problem he was given, but missed how the code was actually used. Took a lot of diplomacy to get past that.
When we still threw code over the wall to QA it used to be said in one place, "Developers code and give to QA, whose job is to tell Developers where they missed the mark."
One of the most salient features of our Tech Hiring culture is that there is so much bullshit. Everyone knows this. Each of us contributes his share. But we tend to take the situation for granted.
In my opinion, this is a dialogue where you can discuss. In the discussion, you can also protect yourself.
I have seen a PR with over 100 comments from reviewers on other teams. If the comments are objective and justified, then this is better than their absence.
I've been a professional C, Perl, PHP and Python developer.
I'm an ex-sysadmin from the late 20th century.
These days I do more Javascript and CSS and whatnot, and promote UX and accessibility.
Let's not all pretend that a good deal of individuals don't treat PR reviews as a complete ego driven exercise to enforce their power. At minimum a couple of people in every company I have ever worked at are a nightmare to work with. I have had a lot of PR's turn into countless back and forth's over something that wasn't even in the code or they wanted the name of a variable to be changed to something slightly different or they wanted the code a couple of lines up the page.
I feel this personality trait of people who don't like to ever admit when they are wrong or always want to be heard with a ego driven personality tend to gravitate towards software engineering roles. I have found this style of working to be rather toxic at times and puts a lot of stress on the Developers, it is one of many reasons why I have moved more into DevSecOps as the work is more black and white and less up to interpretation (Generally Speaking).
I guess you could say the problem is with the company/people and not the PR reviews themselves....but when this problem arises all too often, you can't help associate the two.
One of the most salient features of our Tech Hiring culture is that there is so much bullshit. Everyone knows this. Each of us contributes his share. But we tend to take the situation for granted.
The feedback you are getting here is that people are afraid to criticize you because the topic is so complex and unlike them you know so much about it :)
If so.eone has something to say, they have a reason to do so.
If you do not agree with their reason, then try and consider why they feel they way they do, and then try to communicate to them what you did, why you did it. If they are upset at you, that's on them, not you.
Always do you best, and as long as you do, there's nothing to be mad about
One of the most salient features of our Tech Hiring culture is that there is so much bullshit. Everyone knows this. Each of us contributes his share. But we tend to take the situation for granted.
One of the most salient features of our Tech Hiring culture is that there is so much bullshit. Everyone knows this. Each of us contributes his share. But we tend to take the situation for granted.
As everyone is now doing test-driven pair-programming all of the time, why would we even need code reviews anymore?
</irony>But seriously, there are many possible reasons to feel defensive or uncomfortable in a code review. Maybe you had to finish something in a hurry and hope to get it shipped before an important "deadline". Maybe you had to rewrite "your" code several times to meet peculiar formal code style requirements that your coworker isn't aware of. Maybe you're in a probabation period and anxious to make a good impression. I've had all of this, including "public" code reviews in a meeting room with a bunch of coworkers delegated to stare at our PR "to learn something" , but the most annoying code review situation was when I refactored some nice code to meet the change requests of the first review, only to defend those changes against the second code reviewer, reviewing my refinements and requesting to revert the changes back to my original draft.
I did at first; I don't anymore.
Don't take it personally. You are not your code. If someone suggests a different way to do something, it is an opportunity to discuss. Maybe your way is better; maybe there's another way to approach the problem.
Keep in mind that if you are having your code PR'ed, it is being written for the team. The best communicators are the ones who can adapt what they're saying so that everybody regardless of skill level understands; the same is true with code.
You will not be the owner/sole maintainer of that code forever, even if you want to be (unless it's truly your own project, in which case you have the right to ignore the PR). Try to reach a compromise for everyone to agree.
Depends on the reviewer's style of communication. If it's clear that the reviewer's intention is to help me mold a bulletproof piece of awesomeness then their review SHOULD always be as detailed as possible and should not hesitate of using the hardest criticism as possible.
However if it sounds as their intentions is simply to disregard my implementation, because it doesn't fit their taste then I may get defensive.
In the end it's all written and missing nuances of the spoken word, so both sides need to be careful with interpretations.
I agree.
When I have the feeling that written discussion is leading us nowhere very slowly, I jump to a slack call and share my screen. It usually helps a lot.
I would say it depends on how much back and forth happens about what we did vs what the reviewer was expecting and how that is communicated.
This can be hard sometimes but depending on the company culture/standards, it's better to try to let go any personal attachments and just address the reviews and be done with all that since at the end of the day the code won't belong to us.
For work, I'm my own code reviewer (it's a small company).
As for contributing to OSS, I don't usually get defensive, no. I go in with the assumption that any PR I make might not get merged due to factors outside my control, but I'll argue the case for why it should get merged, unless something comes up during the discussion that changes my mind.
For nitpicks about code style etc I usually implement the requested changes without question.
There is one OSS project that's an exception, though, which has a weirdly insular and toxic community, especially one or two contributors who seem to take great delight in raising pointless objections to any attempt to approve the low quality of its code base... I won't name and shame, but such projects definitely exist. A lot depends on the community around specific projects.
Because it feels like your baby, the thing you have created is now being altered.
But to say it better, code changes all the time, next week the intern might have removed the code because the function specification was changed or whatever so just don't get to attached to your own code.
It's our baby and we are encouraged to code with a perfectionnist mindset, have strong opinions on how to do it right, produce clean code, respect all good practices, remove that newline would offend our sense of beauty, ...
And then we submit a pull request and suddenly we need to be like an ego-less buddhist monk that see things with a beginner mind and welcome challenges from everyone.
It's a tough dance to master :P
Sometimes??? Most PR reviews degenerate into arguments (usually only within a team, not normally on open source stuff). Programming is a deeply personal thing, it's very difficult not to take review comments as a slight against you.
Two things to keep in mind when you're feeling defensive:
I like your second point, there are indeed nature PR of different natures and if we recognize that, we can explicitly invite people to challenge us hard for PR that will have lots of repercussion down the line, but not do it for all PRs which would be wasteful
The 1st one I ever had yes, I was a bit defensive but I decided to not engage till the next day after my team destroyed the PR and I cooled off. I feel like they had been waiting for the opportunity since forever. After that they no longer destroyed my PRs and were pretty cool about it.
But it's been a while since I've been on a team that actively reviews code this way lol
It's hard not to be, you've spent hours making a solution to the task you are given and think its ready and doing what it should, then you get critiqued on some point which then makes you feel you didn't do a good job. I've given constructive feedback which has at times riled someone's feathers because he thought he had solved just the problem he was given, but missed how the code was actually used. Took a lot of diplomacy to get past that.
When we still threw code over the wall to QA it used to be said in one place, "Developers code and give to QA, whose job is to tell Developers where they missed the mark."
That's so true, QA used to be the ones easing that tension!
Now we developers are both judge and defendant :)
In my opinion, this is a dialogue where you can discuss. In the discussion, you can also protect yourself.
I have seen a PR with over 100 comments from reviewers on other teams. If the comments are objective and justified, then this is better than their absence.
Let's not all pretend that a good deal of individuals don't treat PR reviews as a complete ego driven exercise to enforce their power. At minimum a couple of people in every company I have ever worked at are a nightmare to work with. I have had a lot of PR's turn into countless back and forth's over something that wasn't even in the code or they wanted the name of a variable to be changed to something slightly different or they wanted the code a couple of lines up the page.
I feel this personality trait of people who don't like to ever admit when they are wrong or always want to be heard with a ego driven personality tend to gravitate towards software engineering roles. I have found this style of working to be rather toxic at times and puts a lot of stress on the Developers, it is one of many reasons why I have moved more into DevSecOps as the work is more black and white and less up to interpretation (Generally Speaking).
I guess you could say the problem is with the company/people and not the PR reviews themselves....but when this problem arises all too often, you can't help associate the two.
Why should I feel defensive when the only feedback I get is "lgtm"?
The feedback you are getting here is that people are afraid to criticize you because the topic is so complex and unlike them you know so much about it :)
I get this sometimes, then something breaks. Usually means they didn't look at the PR
I did at first, but I don't anymore after making some mistakes at the production level. Now, I need it and am very dependent on it.
Try not to get defensive.
If so.eone has something to say, they have a reason to do so.
If you do not agree with their reason, then try and consider why they feel they way they do, and then try to communicate to them what you did, why you did it. If they are upset at you, that's on them, not you.
Always do you best, and as long as you do, there's nothing to be mad about
Why so serious?
What do you mean?
I don't, it's a learning opportunity. Besides the PR author and the reviewer are essentially in one boat.
Same boat yes, but often with strong conflicting opinions on whether to turn left or right :)