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.

Project Managers/Technical Leads sometimes tend to be over enthusiastic when it comes to software.

But during code reviews if instead of functionality of the code the only complain one hears is about formatting/spacing and similar trivial things, when there are far better things to discuss (Among other things I have noticed the sometimes during the so called "reviews" suggestions are made that implementation needs a re-write just because it doesnt use the most happening technology/buzzword)

How do fellow programmers deal with such scenarios? or is this just a one off? (or is the fault entirely on me )If you have similar experience and what you did to overcome it? Feel free to share.

share|improve this question
11  
Formatting consistency IS important though. –  Htbaa Mar 1 '11 at 12:19
    
There is indent application on linux atleast which does C style indentation. I sometimes wonder if the PL knows about it and is acting ignorant and just say/shouting stuff to show who is the boss. shakes head –  Simon Mar 1 '11 at 12:24
4  
Simon, it sounds to me like your mind is made up and you don't want to be confused with the facts. –  George Marian Mar 1 '11 at 13:04
    
Formatting consistency ISN'T important though. –  JohnFx Mar 1 '11 at 13:24
    
@all: My apologies if this came across a disguised rant (in a way it was), I have learned my lesson and not sure if my inexperience dealing with all the corporate BS shows through. Just one of those off days. ( I would say the wording of my question is much softer to what I really had in mind ) –  Simon Mar 1 '11 at 15:40

7 Answers 7

Getting the formatting/spacing and such "trivial things", are an important prerequisite to discussing more important items. If the code doesn't follow your coding standards it will mark it harder for others to following for a review.

As far as re-write, I don't think anything should be re-written just because a new technology exists. However, if a new technology exist that is useful, it is much easier to re-write the code in the review stage than after the product is in production.

I would suggest you try to get the code formatted as nice as possible before the review so that it is not a distraction. Secondly, if your lead wants to use a specific method/technology try to learn why that decision was made. Your lead/manager should have more experience than you do and therefore might know of some headaches that the new technology will save later on down the road.

share|improve this answer
1  
Prince Arthas is correct - try to put yourself in the other shoes and you'll at least understand where the other person is coming from. This is extremely hard for some people, but its kind of necessary. –  Sergio Mar 1 '11 at 12:33
    
I find it weird when there are applications that are capable of doing indentation instead of suggestions to use them why all the fuss about it. Please use so and so for formatting end of, instead of a hour or so of code review. Come to think about it I feel there are ulterior motives. –  Simon Mar 1 '11 at 12:53
5  
@Simon, looking at your comments and judging by your apparent tone, it seems you aren't really asking this question from learning viewpoint, but instead have personal conflicts with a project lead. –  jzd Mar 1 '11 at 13:04
2  
@simon, if you used those programs before the code review as you should have it wouldn't be an issue. The reviewer doen;t know you are going to do it later, code review is supposed to be code you think is ready to go live, not code you are going to format at some mythical later date. –  HLGEM Mar 1 '11 at 14:31

Code consistency is extremely important. Make sure you follow the guidelines set at your company to the T. If everyone is coding with the same guidelines in mind, suddenly the whole project feels cohesive.

Try to ask your manager why he wants to use the latest and greatest technology? Is it to please the buzzword hungry boss of his? Will this new technology help you complete this quicker?

Also try to keep this in mind whenever you're coding:

Leave the campground cleaner than your found it.

Have this present in your mind and suddenly coding becomes fun and you just feel good while coding because you know you are doing a good job, professionally.

share|improve this answer

Assuming that the coding standards aren't too onerous/strict, then they should certainly be followed, even if you are not used to them. Give them a chance, you never know what may happen. (I used to prefer the K&R style for brackets, until I had to use the Allman style at my last job. I now prefer the Allman style, as I believe it is more readable.)

If there is a problem w/ the coding standards (e.g. way too complicated, strict, what-have-you) then that must be brought up with the leads and managers. You'll likely need evidence to support this position.

Similarly, you can politely ask them to support their positions, especially in the case of a requested rewrite that doesn't use their favorite hot new technology, pattern, or whatever. However, you should not come at this as an antagonist with a closed mind. To you it may just seem like a buzzword, but maybe there is a good reason that you're not seeing or understanding.

It may also be useful to bring up the question of cost. Is the time necessary for the rewrite justifiable in the face of the deficiencies (perceived or actual) of the existing version?

I cannot over stress the need to keep an open mind. (And, as Htbaa states in his comment, consistency is important.)

share|improve this answer

What part of the project manager/technical lead is your boss and has the right to determine what the software will be like whether you agree or not, do you not get? You sound like a problem employee to me and I bet your boss thinks so, too.

How do you deal with the scenarios above? You fix what they want fixed.

share|improve this answer

I'd say in just about every situation - try to find the common ground. Ideally at least, the technical lead wants quality code to be created in a timely manner. Anything that makes the code better, or the process faster is a win. Sometimes it just has to get boiled up to that level.

Code reviews pose an extra challenge - they can be expensive in both the time for the attendees to prep and attend, and in the interruption to flow (the point where you're really humming in development tasks).

In this particular scenario - I would avoid saying to the manager that the formatting work is "trivial". As others have pointed out - it's not trivial - consistent, easy to read code helps everyone out in the long run. BUT - most formatting work is not debatable. It's usually that someone found a problem to pretty clear coding guideline. As you say - the meeting could bette be spent on items where consensus is needed and were discusion is required.

I'd suggest the following:

  • Do your absolute best to go through the coding guidelines and submit code that is already formatted well. Unless your guideines are nebulous, you should be able to submit well-formatted code without a review.
  • Ask reviewers to markup the code for format BEFORE the meeting and hand you their markups in the meeting.
  • Don't invite discussion on the little stuff, accept and move on - just say you'll review the markups and make the updates and then start asking questions about the hard stuff.

There's a tricky point of trust here - you have to make sure the changes get in. If it's too trivial to warrant discussion, then should be so easy to change that you can update the code in an hour or two. If people come to believe that their markups aren't getting updated in the code, then they will feel the need to voice it in the meeting and there you are... back on formatting again.

share|improve this answer

You're experiencing the pain of bikeshedding. The way to fix this is to help the bikeshedders break the habit by finding ways that they can constructively contribute — the idea is not to inflame or incite them.

For things that are easily bikeshedded (formatting, spacing, etc.) get everyone to agree on the team style and use a formatting tool to fix it up before you do a code review. No sense wasting time reviewing code that can be fixed up by automated tools. If you give ground on this front, you'll earn some credibility you'll need for the next step, which is...

Come up with a code review checklist. Automate, with static analysis tools, checking for the types of bugs that static analysis tools are good at identifying. Then, identify the common classes of bugs that remain and make them a code review checklist. This will give the would-be bikeshedders a constructive way to contribute to a code review and they'll be less inclined to generate random permutation requests. Win-win!

Don't forget to periodically review your checklist. Over time, your team will become keenly aware of certain classes of bugs and get better at not writing those types. At that point, it's better to replace that class on your checklist with a class that you're still writing.

share|improve this answer

I found a simple solution that has worked for me for years. I begin each review with one action item called FIX REDLINES. You quickly skip over punctuation, grammar, formatting, coding standards comments in the meetings with hardly mention, if even a mention at all. Just agree to incorporate everyone's comments offline (no matter how dumb you think they may be) because it usually takes more time to argue about it than to just go ahead and do it. When you go to incorporate the comments offline and it turns out that it really isn't trivial or it is just plain wrong then it's worth the discussion. If it's just a matter of you think it's better go ahead and incorporate anyways because your arguing about it is just wasting everyone's time and in the end it probably doesn't matter anyways other than to massage your ego.

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.