How to balance code quality vs strong developer personalities

Question

On code reviews at work I have been seeing code & patterns that I consider "clever" though not necessarily adding to the overall quality or maintainability of the code base. I point this out on my feedback and am unconvinced by the counterarguments. I am a bit concerned when this code makes into into the repo and later to production.

I want to maintain a cohesive team, so I dont want to create tension by being too vocal about my reservations. I also want to create a great product for our customers without being too leninent.

Traditionally, who has 'veto' power on what gets checked in and how?

How can code that works, but its too involved/clever be removed without stepping on toes?

Answer 1

I love this quote:

"Debugging is twice as hard as writing the code in the first place. Therefore, if you write the code as cleverly as possible, you are, by definition, not smart enough to debug it." – Brian W. Kernighan

On one side, this makes you weary of too clever code, since it will be hard to debug and extend later.

On the other hand, clever code that does work is a great way to learn. Probably for the whole team. What about encouraging the author to give a small informal talk about the code to their colleagues? Just be sure that it really works as intended and that it does makes sense in the project as a whole. You don't want to turn it into a pointless competition!

If you don't think it adds value, then place the counterchallenge asking: "How can you refactor this part (you have tests in place, right?) so that it's more readable?" Be sure to point that it's harder to make clever, readable code that to make impenetrable nuggets.

Answer 2

My advice is to play the idiot, when its time to do code reviews feel free to say you don't have a clue how it works (if your ego needs a massage, you can say you didn't have time to figure it out) and get the developer to explain it. When he's finished you can suggest he writes all that down as a comment for future maintenance, with the implied hint that its too convoluted to be considered 'good' code.

If its good code that's just a bit too complicated, most people will get the hint, without anything having been said about code quality or developer expertise.

PS. Obviously most of this code will be subjective anyway, so one person's impossibly-clever code might be a reasonable and maybe even industry-standard algorithm to another dev, so you can't accuse anyone directly of writing bad code (except when its fricking obvious like the contractor who copied a byte array into a stl list, passed it into an encryption routine and then converted it back into a byte array!)

Answer 3

My rule of thumb is to bend code quality guidelines in favor of strong developer personalities. I use it always when I don't have time to dig deep enough - such digging by the way is typically quite effort consuming (more on that below).

This rule is based on personal experience. I began (probably as any neophyte) with fanatically following guidelines and thoroughly fighting each and every deviation. Upon time I've gained sufficient skills and learned enough tricks to win such fights with relative ease - which in turn allowed to focus more on learning the overall impact of my "victories". And the impact as far as I can tell was rather negative - guys who "lost the fight" were suffering and became less productive - and, you know, the fact that their code was 200% compliant with quality guidelines did not compensate for that.

This discovery caused me to drop most of the fighting which in turn lead to having more time to analyze problematic cases. And I found that when I dig deep enough there is typically an interesting design problem somewhere behind, a subtle (or not too subtle) problem that was just hiding behind the personalities fighting.

• It's like, you know, like say I find 31K source file exceeding the recommended size limit which is say, 30K. My options are either to spend few minutes/hours fighting to force file owner to somehow squeeze that extra kilobyte out or to spend a day or two thinking and digging to find out that, say, there's some library API that can be used instead of all the code in that file (so that it can be just removed).

Such a discovery might sometimes be not much useful from end-user perspective (though sometimes it can have deep impact indeed) but have to admit the fun I get when cracking such a nut makes it well worth the effort anyway... and as an icing on the cake guidelines deviation also goes away without a fight. :)

Answer 4

Your organization should have a coding guidelines/standards document that's periodically updated with input from the development team. That document can spell out specifics, like: how to name variables, how to format code, and so on. The document should also explain the values that the organization expects programmers to adopt in writing code, including the relative importance of things like readability, maintainability, correctness, efficiency, and adherence to standards.

Code reviews should be conducted using that coding standards document. If the coding standards say that programmers should prefer readability to brevity when the two are in conflict, then you'll have some support in arguing against the "clever" code. If the standards don't say that and you think they should, then you can argue about it in the abstract at the coding standards meeting rather than trying to figure it out when somebody's ego is on the line.

Ultimately, it does sometimes come down to a judgement call, and in those cases the final word should go to the person that's ultimately responsible for the code and/or product. That's usually someone like a senior developer, technical lead, project manager, or director of engineering. If you're the guy in charge and you feel that certain code isn't sufficiently maintainable, you shouldn't be afraid to say so. You can be diplomatic about that:

Sam, I'm impressed with your ingenuity here, but I'm concerned that it may be just a little too clever. I'll need you to be working on new development a year from now rather than maintaining this, and I'm concerned that whoever does have to maintain it may not fully comprehend its awesomeness. I know you hate to do it, but I'd appreciate it if you'd go back to the straightforward implementation that we discussed.

On the other hand, if you're not the guy in charge, then the best you can do is explain your position clearly and try to convince the rest of the team. If you're not getting support from the manager, then accept that it's not your call and move on.

Answer 5

At your place (and I, sometimes, am one of those smartasses) I wouldn't remove it, but ask personally to the witty/clever author to document it very well in comments, and if possible, to include some discussion on alternative and simpler writings he could have used, with examples.

I would underline this is for the best, because even him will probably not remember all the bits and bobs there are, in those lines, in two months time.

He will probably drop the smart code in favor of the simplest one, as soon as he's forced to write it, as an example.

Why would that work.

• You acknowledged you care about what he writes,
• you showed him respect by asking,
• by citing memory/focusing problems you devise and break down to him a scenario in which that code must be changed and cannot while he's still working for the company or the team

(without that last allusion this kind of request may be received as a try, on the company's side, to commoditize the programmer, making it interchangeable with any other code monkey at anytime)

Answer 6

In my experience, It's typically very difficult to get code that suffices all requirements out of the code base.

However, next time the code is to be maintained, you can easily argue for it's replacement as a future cost saving measure because the old code was harder to maintain.

As far as veto power, management obviously has that.
Sometimes their are teams or committees that are in charge of code quality, in which case they would have this power as well.

It also would be helpful if you give an example of a 'clever' pattern. Maybe you're just overreacting...

'clever' is almost never a bad thing in my book, but I can agree that there can be a slippery slope between clever and convoluted.

Answer 7

Like many other practices I think the answer is it depends.

• If it is a defect then it definitely needs to be fixed.
• If the code works you have to balance the value of insisting the the code be changed vs. the future cost of the clever code. While there are rules of thumb regarding the ratio between development work and maintenance work they will vary a lot from project to project. Be reasonable.
• Consider why are you unable to convince your team mate that the code needs to be simplified?
• You don't always have to get everything perfect. That would mean code reviews in an infinite loop and nothing checked in because nothing (except my code, haha) is perfect.
• My personal preferrence is for the author of the code to have the final say. Obviously if they are continously doing a very poor job then some other action has to be taken but as you say the negative value of forcing the developer to change the code may be higher than the gain made by fixing it if this is a very rare situation.

Answer 8

I can definitely relate to this question. I'm currently the technical lead for 2 teams and it's my job to ensure the code we produce is as readable and maintainable as possible. At the same time I've been known to produce some "clever" code and I know most of my teammates will have very hard time follow it.

Few observations I can offer:

1. You team needs one lead who will make the ultimate decision when there is a disagreement. In the last release I was on a team without leadership and it was atrocious. Everything became an argument, and if you have few people with strong personalities neither one of them will budge. If you do have a lead, regardless if which decision is chosen, everyone on the team MUST understand that what the lead says goes. That's why management made him the lead.

2. Keeping people happy is very important. Therefore, instead of pushing the entire team towards your viewpoint, just nudge them there. Explain SOLID principles, explain importance of small and cohesive classes/methods/interfaces and repeat these things every time you see them being violated (i.e. in a code review). At the same time, don't make them rewrite every single thing you don't like. At the end of the day, you need a balance between personal expression vs. following group standards. Hopefully, the two will converge as personal preferences shift towards how the group in general is operating.

3. I believe it is much more important to have clean, easy to understand class interfaces, than not to ever have any "clever" code. For example, we have a class that maintains a list of entries which are looked up 3 different ways. Currently it simply uses linear search for all lookups which works on a small scale, but because this class is at a very low level, I see it not scaling very well. I'm about to replace it with a different class, that uses Boost Intrusive containers. Each element will support being placed into each of the indexes simultaneously and all lookup will be done in O(sqrt(N)). Yes it will be much more complicated on the inside and a lot of people don't like Boost, but on the outside it will remain 3 methods: Add, Remove, Get. Bottom line is that people can write whatever code they want (within reason), but interfaces MUST not be clever.

4. Maintain the idea of code ownership. Although it is sometimes difficult to achieve as different people might need to add/modify some code. When the code is written, the original developer is the ultimate keeper of that code. That doesn't mean no one else can touch it. If others modify their code, that's fine, but at the end of the day original developer reviews it and remains being responsible for whatever goes in there. Whether that code is simple or clever, it is his baby. If/when, as you are predicting, bugs start piling up because of the design/coding decisions that were made, instead of simply fixing those bugs as they come in, sit down with that developer (who btw should be fixing all those bugs) and reflect on those decisions to see how they could have been made differently. Then give him a book on refactoring and see how you can "unclever" some of it.

Answer 9

I've spent many years leading and managing development teams. By nature, I am a bit OCD in terms of code and very black-and-white. I've learned through experience that picking your battles is one of the hardest skills to learn as a team lead. Yes, standards are important. Yes, readability and maintainability are incredibly important. Yes, we should all strive to write uniform, standards-compliant code. Developers are humans though... not code generation tools. We have personalities, opinions, we get bored, and we want to learn new things.

On code reviews at work I have been seeing code & patterns that I consider "clever" though not necessarily adding to the overall quality or maintainability of the code base.

OK... so they don't add, but do they detract? Are we talking just a matter of personal preference in coding styles, or is the code being written completely unnecessary (eg. using expression trees and reflection just because it is fun to use expression trees and reflection)? If it is the former, let it go. Part of the fun of being a developer is coming up with creative solutions to problems. Maybe (and most of us don't like to admit this), we sometimes feel intimidated by approaches we don't understand, and either don't want to ask or don't have the additional energy to learn the new approach.

Now, when creativity leads to unnecessary code and completely unjustifiable complexity, then by all means be vocal and make your case. Being a team player is important, but so is being (and holding others) accountable. Code reviews are about accountability as much as quality assurance and learning. You are going to step on some toes, but if you feel like you have a strong argument why effort (money) should be spent rewriting working code AND an ego should be bruised in the process AND you want to risk squashing someone's enthusiasm for their craft, then you shouldn't shy away from putting it on the table. If you are the team lead, this is your job. Be aware of the impact, and do it. If you aren't a team lead and don't have the authority, then put it to the team to decide.

The best way to instill accountability in your team is by encouraging others to hold you accountable. If you keep an open mind and don't shut people down when they suggest improvements to your code, you may find they are more receptive to your suggestions.

Answer 10

From personal experience, when this happens I would ask for permission to be a volunteer adversarial unit tester to write tests in order to torture the unfamiliar implementation.

I will try very hard to truly understand how the code works, and also try to probe it in many different ways.

Note that this is a time commitment. It may be hours, or tens of hours, or even a good part of a week. Whether it is justified depends on whether the code's complexity is commensurate with the requirement's complexity.

If I didn't prevail, i.e. if the code survives the many tests, I would convince myself that maybe the implementation is truly more enlightened than I am, and I will support its inclusion into the main product.

Depending on the source control methodology, the code may have to be provisionally committed into the source control system (perhaps in a branch) before this testing phase can begin.

My answer is colored by my experience of using OpenCV. There are algorithms that are way more sophisticated than any programmers can implement on their own; not even PhD's - maybe the top few percents of PhDs. If the code is that good, you have to trust that, acting against your own instincts and prejudice, even if you do not have effective means of quantifying that level of trust.