Take the 2-minute tour ×
Programmers Stack Exchange is a question and answer site for professional programmers interested in conceptual questions about software development. It's 100% free, no registration required.

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?

share|improve this question
6  
Could you add an example of what you consider as "clever", just so we're all on the same page? –  BlackJack Aug 14 '11 at 21:16
    
What is the hierarchy of the persons involved in this? –  user1249 Aug 14 '11 at 22:44
2  
What is the clever solution? I would not feel comfortable telling you how to deny the solution if there is a possibility it is actually superior to your own idea. –  jojo Aug 14 '11 at 23:16
    
Is the 'clever' code prematurely optimized, or prematurely generalized? Usually shortest code wins, for an appropriate measure of shortness (DRYness or tokens, not characters). –  kevin cline Aug 15 '11 at 6:29
2  
Offer an alternative. Otherwise you are really just an obstacle to productivity. It's difficult to provide "counter-arguments" to critique without an alternative approach. It's a bit like putting the burden of proof on the accused. You are asking them to defend against all possible counter-scenarios. –  NickC Aug 16 '11 at 0:17

8 Answers 8

up vote 9 down vote accepted

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.

share|improve this answer
    
+1 for the quote and the "challenge" –  Marjan Venema Aug 15 '11 at 6:27
    
Almost -1, debugging is not hard, unless your code is a mess and super fragile. The only problem with debugging is that a lot of developers have no clue how to use the debugging tools. Writing very resilient and error aware code is a LOT harder. –  Coder Aug 15 '11 at 15:39
    
I think that debugging is certainly harder. Think about it this way. If the code was initially written correctly, the debugging becomes a non-issue as testing it (unit and integration) finds no defects. –  tehnyit Aug 16 '11 at 6:34

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!)

share|improve this answer
3  
My test is "can a gradutae programmer (may 1-2 years out) maintain it?".... It can be clever, but must also be clear and concise. –  mattnz Aug 15 '11 at 5:23
    
@mattnz - If I were to use that test, half of the code I wrote over the years (or anyone else for that matter) would be considered bad. Graduate programmers (1-2 years out) are not all people take them for. Not saying that we should write bad code; just that that "test" makes not that much sense. –  Rook Aug 15 '11 at 23:43

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. :)

share|improve this answer

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.

share|improve this answer

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.

share|improve this answer

At your place (and I'm 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 it's for the best, as he too 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 writes 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)

share|improve this answer

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.
share|improve this answer

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.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.