So, for my first post here, I thought I'd try to garner some opinions. There's no wrong here.
I'm a senior developer, and a little over 6 months ago, I was asked to interview a woman - whom 3 of us agreed should be hired into the team. She was hired as a junior.
Fast forward 6 months, and she's working on small projects with lots of time investment from myself & others to start a Sprint. She then works for a full sprint having the usual standups etc, and has delivered a project within a single sprint.
My issue, is the code reviews.
I've always treated code reviews as "if it fits the requirements, it gets merged" and we don't have anything like style guides to worry about. If I can't work out quickly (based on the review content and the commit history) what a section of code is doing, I'll ask the author.
Unfortunately, code reviews with her are ALWAYS world war 3, everything is taken personally, and combative responses are the norm. Simple things like "did you consider X here, would have probably been a bit more efficient?" get a response of "no, and I'm not going to change it, this should have been discussed in requirements before the task was given to me." Consider here, I'm talking about things like static import use in Java, nothing complicated.
Yes, I still merge the code (because it obeys the functionality rule). But I can do without the arguments daily over the tiniest of things.
I could stop commenting, but that runs the risk of simply whitewashing her changes & allowing the defect rate to rise if I'm not paying attention.
I could take the hard line of "buck up or get another job" but I'd rather find a way to help her take on feedback healthier.
So, after that lengthy rant, what are your tips for reducing combative responses while helping juniors grow?
Top comments (4)
Too late but still gonna add my experience.
Sometimes its not about the attitude but rather about the environment a person is in. In one of my previous organization, code reviews were done only in hierarchy i.e. Sr Dev will review Junior Dev's code, Lead will review Sr. Dev's and so on. And somehow the review comments were taken in a negative sense. As if receiving a review comment meant that you didn't do proper coding in the first place and indirectly could affect your appraisals.
When I joined another org after that, I was surprised to see a 18 year experienced person asking me to review his code. I could certainly not find out errors or misses. But to start with he asked me to write a comment even if I have a question. Its really easy to discuss when you have the context in hand. It also helps you
Also the feedback and its related changes also take time and effort. You need to account for that. In one of the instances I got some feedback and I let the team know that the feature cannot be completed because the changes required for it will take some time. It was understood by all and that feature was move to next sprint. If at that instant I was made responsible for not delivering on time, I could have become a hater too:-P
One last thing is about the communication medium. Sometimes its really difficult to make out the tone of the statement. Many people get offended if they feel the statement is rude or impolite. Maybe in person you can convey your expression easily but in writing its a challenge.
In the end its up to the culture you are in. You can always move out and find a position where you fit in.
@z00md , firstly, thank you for a thoughtful response, and apologies that I missed it all that time ago.
Things in my situation have changed quite dramatically. For starters, the problematic person I was posting about has been made redundant (along with others, unrelated to my issues around code reviews). But also, I've moved from Senior Developer to Architect / Lead Developer / Dev Manager (as a consequence of the company restructuring).
The very first thing I set in stone is that anyone can read anyone's code. There's no need for someone more senior to approve it, so long as someone follows due diligence, I'm happy. I still write code when I can, and I deliberately assign the reviews to Junior Developers. Hopefully they'll spot something I haven't thought about, but worst case, they'll know what's changing and might get some ideas for their own work.
The next thing I set in stone, is that frankly, we used to have a "blame culture" - people spent far too long worrying about why something went wrong and it simply detracts time from just fixing the problem. So I've banned it. Even if the Board are asking "who screwed this up" - and it's something my team were responsible, my answer is "me, and this is what I'm going to do so we never have that problem again. I'm sorry." On top of this, as I keep reminding the team, there's no such thing as a "silly question."
As a result, the team is 3/5 the size it was, but the value we provide to the business is now well over 15x what we were doing per Sprint. In a few cases, we've turned around bug reports in the Live customer facing system, within 30 mins.
Re your point about tone, and communication style, I couldn't agree more. I've always preferred that I don't leave comments, I have questions, and when I have a question I prefer talking to the person. Lately I've let that slide a bit and been posting questions as comments - but there's nothing to stop me, during WFH pandemic situations, picking up the phone & talking to them. So thanks to your prompting, I'll be doing that.
On your point about changing delivery scope - I don't think the example you state would have been bumped to the next Sprint immediately (in our situation). We would have probably had you continue, and accept that it flows into the next Sprint (unless there was something higher priority/value that you could have done first). That said, delivery scope can, and should change when necessary - locked Sprints is a bad concept for Agile.
If you are managing her, you should consider pulling her aside at a time when no one is upset and explain the situation and concerns. Go over how code review is supposed to work. Make it clear that her attitude towards it needs improvement and that the goal of the review is to improve the code quality before it gets merged, not to blame or argue. Don’t make this a disciplinary meeting - it should always be clear what is expected before something like that happens.
You might want to consider having her do code reviews for yourself or others, or sit in on code reviews with others for her to see what’s expected.
Thanks for the reply, Tom.
While technically she doesn't report directly to me, the Dev Manager is the completely hands off type (which I prefer) and so it likely does fall on me to organise a quiet face to face. Thankfully, as she's not a direct report, it can't possibly be taken as a disciplinary either.
There's also things going on in the background that I won't detail (HR/medical issues), so I've been a little reluctant to take the bull by the horns on this issue.
However, my post was written while I was on vacation (lots of time for reflection on the beach...). I've come back & so far taken the "God Complex" approach of "you're a junior, I'm a senior, I have spoken & this is my reasons. Do as instructed."
I've only done that where I felt things were descending into a debate too much, and so far, HR haven't flagged anything in my direction. Hopefully once a trend sets in, I'll be able to back off a bit.