Intro
Even though all software developers should take part in code reviewing their peer’s code, I find most developers I work with aren’t that interested, so ultimately it comes down to a few high-ranking developers, plus me.
Recently, we have been rejecting a lot of bad code from inexperienced developers. It’s fine being inexperienced as long as you have a team to support you and help you eventually produce an effective solution. I think what happens is – there’s not enough seniors (or seniors that are actually good), so the code gets into their Project Branch. Then, when it is time to merge in for the Main release, experienced developers review the code and reject it.
I often specifically get asked by the Software Delivery managers to review code to ensure it is in a good state for the release. We had a group conversation where we were discussing the current bad state we are in. Software Delivery managers had asked Darren, Dave and me to review a project.
Participants
Darren, Dave – Principal Developers
Mark, Michelle – Software Delivery Manager
Me – the silent observer
I’ve given the developers names that start with D, and the managers names that start with M, so it might be easier to follow.
The Conversation
Darren 08:36
Whole lot of Nope in this Code Review.
Mark 10:52
Judging by your review comments, this doesn’t look to be something that can be resolved in the next couple of days. Does their implementation need to step back and restructure?
This might need another call
Have any of you guys been involved from a high level view for this Project?
Darren 10:55
The changes to the Database are totally inappropriate, and possibly way off the mark of what is needed.
Changes to the Database filing are non-trivial, if people don’t understand it, they need to reach out for help, not just smash code in.
Our product is going to turn into a legacy codebase very quickly if this behaviour carries on.
Dave 11:11
We do keep getting Code Reviews to Main with unworkable solutions. It’s highly frustrating, and a big waste of our time. Most of the time, the code or even the work item has not been reviewed by an expert-level person before a review to Main is requested. On occasion, someone gave correct advice in the first instance which was then ignored.
We need to fix this, and have help from you guys raising the problem.
Darren 11:14
The solutions feel like a very reactive approach to programming… Very little thought for design.
Mark 13:02
So have any of you guys been approached about this Project before this review was raised?
Because I think this ties in with the whole idea of needing an Subject Matter Expert during the construction of the project rather than at the very end – something that we’re trying to drive forward
Dave 13:07
only about 2 days ago 😖
All our Subject Matter Experts are on the New product 😛
Darren 13:08
I reviewed the previous incarnation of it, without the concerning the Database changes.
“All our Subject Matter Experts are on the New product” Even so, I might have made time to discuss this with someone if asked.
Dave 13:09
yeah same
My point is that the day-to-day design and building of the projects is led by inexperienced devs. The developers are not being mentored while developing the code – but instead, at the end of their code writing; which should be something a lead developer is doing
It’s good to have the view throughout the dev process, or at least the start, as well as quality checks at the end – which should be a straight-forward “yeah, you’ve not done anything crazy, it meets the coding standards and is what we agreed”.
Mark 14:43
Yeah. This is all the good stuff which me and Michelle are trying to address as well. Trying to have a Subject Matter Expert assigned to each project during construction just for that overarching view, and to give a steer when needed.
I.e. a month ago this could have been caught at the start by saying “don’t make these changes within the Database itself” haha.
Darren 14:51
To be fair, you don’t need a Subject Matter Expert to tell someone not to dump a load of copy and pasted code from one module into another. Just someone with some experience in developing software to a higher standard than “Hello World”.
Michelle 14:52
“Hello world” just triggered some terrible school flashbacks.
Dave 14:52
Agreed, but as a business, we need Seniors to point out when people are constantly doing silly things to their line manager.
Michelle 14:57
It’s the experience aspect that is the biggest challenge, if there was a better ratio of experienced people to inexperienced people areas where skills are not good enough, they would start to improve (or at least that’s the theory).
Everything just seems stretched way too thinly at every turn. Hell, it isn’t just development that has the problem.
Darren 15:00
I’d also add: knowing anything about the teams working on the code would help.
I can’t know everyone, but I don’t even know what the teams are, what they are working on, who the seniors / leads are to discuss with.
E.g. if someone has a problem with code my team is doing, then I’d expect to be brought into the loop (either to explain our position, or to understand the other sides)
Dave 15:06
Who do we talk to, to at least pass the buck on the issue and raise it?
Michelle 15:11
I think it is more for the Development Director. It’s something me and Mark have brought in fashion everytime we come across an example. Difficulty has always been what to suggest to fix it within the limitations of what we got
Dave 15:21
Certain business types don’t understand the value of good engineers and will simply get the cheapest money can buy, in the hope the decent engineers will act as quality gates and stay with the company.
It must be soul-destroying, working away in an office across the globe, with ‘experts’ in the UK saying if your work can or can’t join the rest of the product. It’s not the right thing to do.
They need infrastructure mirroring ours, and separate projects would be a great way to achieve it
Darren 15:25
Last I knew, the other office still have Development and Testing as separate departments
Dave 15:25
This just reinforces the stereotype that Ben bangs on about: the company is ruled from the UK. Normal companies have different product sectors at different locations, because it mitigates these problems
Michelle 15:27
All valid points. What Dave is saying are points raised verbatim by more than a few people who have been in this world for a while. As for soul destroying: 100% agree it has to happen to catch this stuff; but each time it’s caught it will knock you back that little bit more
Just needs some kind of sensible structure that is adhered to. But then again – if no one is made to follow something, who will?
Dave 15:30
Yeah, for me, it’s responsibility, if you don’t have to pay for your actions, then it drives bad behaviour. But since we maintain our Product, having to pick up the bugs, see the customers complain on Social Media, see the error reports on live and backlogs grow, we’re a lot more invested
it is feeling more and more like, knock this feature out as quick as possible, throw it over the fence at UK law-enforcement, see if it goes through
👮
With responsibility you’re driven to get the right structure in place, you want to place experts/leads with the teams and have them monitor each day, because it soon becomes a very long task to get fixed last-minute. Top-down approach, drives the 9-5 job behaviour, lack of interest and minimum viable work ethic.
Darren 15:32
+1
Which goes back to my point about who the teams actually are. And we really need to do proper ownership. I still own large bits of our Product, because I care about them!
Dave 15:34
Same. Developing and code reviewing isn’t my job, really. However, I don’t want to see the product damaged and go to waste, because it helps people, genuinely.
Darren 15:34
Would be so much nicer to say “I don’t look after this any more, just do what you want and hope nobody gets sued” 😣
And it isn’t any one change that is unsafe, it is the poor quality, lack of adherence to existing designs, just piling code in, coupling stuff together, changes in the wrong place, etc that lead someone else to create a unsafe bug later – usually without anyone batting an eyelid.
OK, some changes are totally unsafe too – a few weeks ago, I have to point out that you cannot calculate an age in months by doing ageInYears * 12
😖
Dave 15:48
So, if I were to write some general comments without mentioning names, and send them to the Development Director, expressing that we’re at our wits-end, would anyone be prepared to back it up or be a part of the revolution?
Darren 15:50
Feel free to add my name to give clout to this
Dave 15:50
sure, I will of course send to you first to check
Summary
- Not enough people willing, or have the ability to review code. – I always find this a weird one; given that I reckon 90% of a developer’s job is reading existing code. If you cannot review code, then can you be a good developer?
- People aren’t putting enough thought into the architecture, or adhering to well-known standards like SOLID Principles – This is the kind of behaviour I saw in developers I’ve written about like Colin and Derek.
- The ratio of Juniors to Seniors is far too high, which not only is a problem now, it is a problem in the future when the Juniors haven’t progressed enough.
- The rapid hiring of staff has meant you don’t know who people are, what they are working on, and what their level of experience.
- Hiring cheap staff has caused some resentment/uncertainty with UK staff. Pay seems frozen and more jobs are moving abroad. It wouldn’t be that bad if the staff we hire are cheaper and are good, but they often just appear to be cheap.
- It would be better to split the offices into separate products so people feel like they have full ownership. There’s other problems too like time-zone differences and general communication problems.
Stranger Coding Tings 