This is something I should have perhaps written about a while back when it first got on my radar, but was never sure how to make a decent-sized article of, so didn't bother. However the situation has arisen again, and I figure it's probably worth raising, even if it's only short.
(ed: having finished it now, I don't think it's my finest-ever article, but... oh well).
A year or so ago we had a new dev who was reasonably good at PHP (certainly better then me, to damn them with faint praise), and not a bad programmer. However they had:
- very little experience in working within a team,
- very little experience with adhering to development standards other than their own,
- and very little experience with anyone making observations about their work.
We do rigorous code reviews at our joint, and on the whole the team members enjoy having eyeballs on their code as we all "get" that code is not "mine" or "yours", it's "ours", so we all need to be happy with it. We all have to have input into it, and to employ a tired cliche or two: two heads are better than one, and the same applies to "more eyes". Code review will reveal trivial stuff like unused or unVARed variables, typos and coding standard breaks; more significant issues like sub-par or expensive logic, missing test coverage; through to more theoretical considerations such as misuse (or lack of use ~) of design patterns.
A code review will also throw up more subjective stuff like "WTF is going on here?" This can be for a number of reasons:
- It could be because the reviewer is unaware of the context or the requirement of the code,
- they might not be aware of a technique,
- or the code might simply be:
- obfuscated,
- obtuse,
- less than ideal in design,
- or simply wrong.
We all write code that falls into one or more of these latter categories: shit happens. The thing is that code is collectively owned, so it kinda needs to regress to the mean slightly. "Smart" code ain't smart if they team can't maintain it, or it doesn't follow precedents already set in the code, or doesn't follow coding standard or other various "code collective" considerations. Having code that the team can maintain after it's written is the most important thing about collectively-own code.
So... code needs reviewing. That's a given. If yer not doing formal code review at your gig: you are, IMO, not conducting yerself professionally. If you work by yourself it's even more critical you find a way to get more eyes on yer code, cos working by yerself you're just in a locked-in cycle of kinda code inbreeding (if you'll permit that).
Another thing that less-experienced devs will do is to write code to feed their ego because they think they are clever. Now I use "less-experienced" in a measured way here. I don't mean "less experienced in coding", I mean "less-experienced in working as a team, and less-experienced at realising code is not all about me me me". And less experienced in understanding "no-one gives a shit how clever you think you are". There was a well-realised dev stereotype going around a decade or so ago - and seems to have died off these more enlightened days - of "Code Ninja". There was no such thing: there was just "Code Twats". You're not a "ninja" (FFS!) if the people you're coding with are going "um... WTF?" You're just a Code Twat.
If we combine people who aren't good team players, code almost as an act of hubris, and generally aren't quite so clever as they've lead themselves to believe, then code reviews can get... awkward.
Too many times with non-team-players (Code Twats) I've seen code reviews in which a team member has flagged up some "strategically challenging" (ahem) code and detailed why they thought there was a preferable approach. And the code's author just wouldn't have a bar of it. They liked their code, and had already decided they were "right", and anyone else's consideration was just mistaken basically cos they weren't clever enough to understand it. So refused to heed the review. More of the code reviewers pitched in, and said "well actually the review's kinda right: the code's not clear, and not the way we do things, even if it's syntactically correct and works". Nope. The author just got more defensive, started googling justifications of why they were right (because, after all, being the "victim" of a code review is an exercise in being right, right? Right?!). And the responses were getting longer and longer. And I suspect typed with heavier and heavier and stabbier and stabbier fingers.
The code review went around the houses for over 100 comments. There was about ten times more text in the comments than there was in the code under review. This is an egregious example, but it sticks in my mind. Also, btw, was not the only review that ended up that way with this individual. Who has thankfully left the company now. There's been similar reviews with other bods too, to a slightly lesser degree. It's not uncommon.
Ultimately the team lead stepped up and gave everyone a ballocking. Although I think perhaps the person under review shoulda been the only person on the receiving-end of said ballocking,because it was clear very quickly the team wasn't happy with the code, therefore the code needed rework. End-of. It's not their fault than the author wasn't having a bar of it. The one point the lead did have is that a couple of us (possibly mostly me) are supposed to be senior, so shouldn't've been so self-indulgent as to perpetuate the review cycle. We did, after all, have our own work to do. That said we were "senior" without any actual degree of seniority over the dev concerned, so we couldn't exactly go "Just. Do. What. Yer. Told" (which is all that needed to be said in this case, realistically). That was for the lead to do. And they shoulda done it earlier. (Said lead has also since left the company, but I look forward to the pint I am having with them this evening ;-)
What didn't occur to me until yesterday as I was reading more about code review failures / challenges, is that these devs who simply won't join the team and perpetuate these seemingly endless cycles of code review back and forth are actually "Code Bullies". It's just bullying. They're badgering their reviewers into submission, hoping they will just go "oh FFS, I don't care enough about this, OK have it your way". I have seen this happen. Hell, I have to admit I've engaged in it! (not my finest hour, and I've tried to learn from this, and I don't do it any more). The problem is I see the artefacts of these bullying tactics in our code every day. Bullying isn't just a temporally-limited thing; its impact has long-reaching consequences. Its victims continue to suffer (hey, if you were looking at some of this code, you'd not be thinking "suffering" was an overstatement ;-).
This situation needs knocking on the head. There's perhaps some guidelines which can help mitigate this:
- firstly the dev needs to actually consider the bigger picture, and it's not a competition, and no-one cares if they're "right" according to only a subset of considerations. This is difficult to control, but if it becomes symptomatic, it needs proactive attention.
- If the exchange between the dev and the reviewer goes into the third round, then they need to stop being keyboard warriors, and talk to each other. Odds-on they're sitting next to each other, or on the same campus. If they're not on the same campus, then there's this thing called a telephone. use it. Or, hey, Skype or something more modern than that. But switch to "interactive mode".
- If more than one reviewer are in accord: they're right and the dev is wrong. Perhaps not completely wrong, but the work needs rework anyhow. But there's a very good chance the dev is outright "wrong" according to more criteria than they might be "right" (which is generally just down to code twattery anyhow). Reject the pull request (or some equivalent thereof; end the code review anyhow) as a first step here.
- Make sure never to employ a passive-aggressive, sarcastic, supercilious or dismissive tones in your code review comments. You're just being a prick. Being a prick and then covering yerself by putting a winky at the end of the comment doesn't mean you're not a prick. It just demonstrates you know yer a prick, and act that way anyhow. Thus making you even more of a prick. This applies to both dev and reviewer, btw.
- If there's more than say ten comments on one code review point: the lead needs to step up. This ought to be either cos they're already watching the code review, or one of the participants raises it with them (which they should be completely prepared to do. That's what the lead is for!). Then the lead acts as sole arbiter, and whatever they say gets done. The first step is rejecting the pull request, if the decision is that there's rework to do.
- Most importantly: code bullies need to be admonished for their behaviour. Like I mean officially admonished, like any other sort of office bullying. It's just not bloody on. They are deleterious to morale, and they infect the codebase with their blight.
We can all do without this sort of anti-social (not to mention unprofessional) behaviour in our work place. Don't be part of the feedback loop, don't be a Code Twat, and seriously... call out Code Bullies when you see them.
Righto.
--
Adam
