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.

My company recently started doing formalized code reviews. The process goes like this: you submit into a github, request a pull request, code gets reviewed by approximately three people, then if all passes, your code goes in.

The process seems fair, however the three people who do the code reviews don't seem to be fair. I notice that when I put my code in for review, I get anywhere between 100-200 comments. The top number for me was 300 comments once. Of course you'd think it is big changes, but this can be very small changes as well with less than 50 lines of code (which includes unit tests). All comments are considered "must do" and without argument.

With that in mind, my main problem here is that it seems a bit excessive. I talked to the group and they told me basically that just because I had so many years of development in php doesn't mean I am an "developer." Of course this seems more hurtful than not. Also I notice that within the group, they don't appear to produce as much comments and most of the time they ignore or otherwise disregard other comments or suggestions rarely accepting it as a valid point even if something is broken.

So my question is if this is fair? Or common?

share|improve this question
3  
What kind of comments are you getting? Seems like a lot. Is it formatting comments? Coding? It's hard to answer without knowing more about the nature of the comments (and maybe exactly what in your code triggered the comments). –  MetalMikester Dec 19 '13 at 18:56
1  
Hey -- not sure if it's the right term, but it's mostly general "best practice" comments like renaming variables, moving functions, renaming functions upwards of 3-5 times, etc. We have phpcs installed so the formatting is correct. –  user1207047 Dec 19 '13 at 19:38
    
Also forgot to mention on this ticket, I am actually a level 3 developer at this company. I have php certification and have been going great for the past 8 years of being employed here. This has only been recently that this started to happen. So I mean one would like to think that after 8 years, you'd know something right? –  user1207047 Dec 19 '13 at 20:12
1  
"So I mean one would like to think that after 8 years, you'd know something right?" - Well, you'd be surprised... The things I see at work sometimes... –  MetalMikester Dec 20 '13 at 12:51
2  
show 2 more comments

8 Answers

up vote 15 down vote accepted

All comments are considered "must do" and without argument.

IMHO that's the real issue, since there is no priorization in that. When you get 100-300 comments, there must be some of them having a priority A (real bugs), some of them prio B (likely to lead to bugs later) and some of them prio C (everything else). Tell your colleagues that you are willing to respect all their wishes, but to make changes effective, and your time is limited, insist on a priorization. Then, start with fixing prio A comment first, and if you really have time for more after that, you can start with B (if you are lucky, your boss will understand that fixing prio B and C is not so important, and give you some more important tasks instead of wasting your time).

share|improve this answer
    
I have tried many times to ask for priority of comments. I get back something like "nice to have" and "required." Turns out a vast majority of them are "required." –  user1207047 Dec 19 '13 at 20:11
2  
I have seen it happen where a specific developer is given lots of action items from their reviews in order to prevent them from messing up code in other areas of the program. But that would be for an exceptionally poor developer who is "forced" onto the project and the lead can't get rid of them because of management decisions. –  Dunk Dec 19 '13 at 20:12
2  
You know @Dunk, I think you're right here. Your comment really hit home and I accepted this answer since I don't think I can accept a comment. I am an "outsider" to this group and realize now why the inner circle is getting better and quicker reviews and those outside aren't. I was "forced" onto this team by management, yes and we are "forced" to work together. So this sounds very reasonable and a logical explanation as to why it's harsher. That or I really stink at coding. Only way to figure that out is to go to another group/company and see for myself. –  user1207047 Dec 19 '13 at 20:54
4  
@user1207047: You shouldn't accept an answer because you like one of the comments underneath as it goes against the standards and purpose of the site (I think I'm sensing a pattern here). There is upvote comment functionality for that. –  webbiedave Dec 19 '13 at 22:10
add comment

Code reviews can be a divisive process.

You are at an important junction, though. Do a thoughtful analysis on their review. Are they identifying nit-picking issues, or highlighting serious flaws in your style and logic?

If it's the former, I'd recommend working towards a resolution ( new job, or new code review processes ).

If it's the latter, I recommend doing a lot of code-reading and studying to try to bring your code up to professional quality.

share|improve this answer
    
Hey, good thoughts. From what I can gather some of them are indeed thoughtful analysis but most of them appear nit-picking such as moving functions or renaming functions. The problem is when they explain their thought process it indeed makes sense but among themselves they are not doing the same thing and making the same errors as I am. –  user1207047 Dec 19 '13 at 19:42
    
Even more so, the code review is so deep that I forget what I was doing and create more bugs fixing the app due to the excessive 100s of comments. For example, one time I was told to rewrite a large portion of the code. Prior to it the code was correct and functional. After the code reviews and nearly 150 comments, the original function and correctness was gone and tons of bugs got inserted. When I realized this and fixed them, I was basically told, "Yeah our code review process makes you an awesome programmer because now you're going back fixing it and it's easier to do." –  user1207047 Dec 19 '13 at 19:44
6  
@user:naming of methods/functions is important, it isn't necessarily nit-picking. If you do a poor job with naming then that can be annoying to your team. If you can't come up with a clear name then it's probably not a good function. You seem to be the "new" guy and the others apparently have a method to their madness that they've probably discussed many times before. Thus, the reason for less comments. I suggest you learn what they want and try to conform rather than butt heads. Earn some respect, then you'll be in a position to offer alternative ideas that will be met with an open mind. –  Dunk Dec 19 '13 at 20:06
1  
@user:It sounds like you are in need of coding/design standards. –  Dunk Dec 19 '13 at 20:52
1  
@user:All you can do is attempt to work within the system and demonstrate you are a team player. If you've done that. then either your perception is not correct, you are dealing with irrational people, they perceive your attitude to be contentious OR it is just plain nasty office politics. The only ones you have control over is your attitude/perception. If you are convinced you aren't in someway instigating the problem then I don't know why you would stay. Go find someplace that is enjoyable to work at because the people get along. If the same problems occur elsewhere then look in the mirror. –  Dunk Dec 19 '13 at 21:50
show 2 more comments

It seems by your comments that your colleagues are using the code review process to agree a methodology or polish the code. I have just started to do code reviews like you and I notice that sometimes we discuss a lot over things that are just implementation approaches or improvements. This is not bad at all as far as its reasonable (300 comments look too many for me, that must look like a reddit thread)

Maybe you need to agree some architectural decisions about the code before start implementing it or maybe is just agree about naming conventions, patterns and good practices so you all know what it is considered "good code".

If you are complying with your code standards as you say and the code works as intended, there shouldn't be so many comments, so either they are using your code as a forum or they are trolling you as it seems that you are pointing.

I would try to be critic with myself, would try to take part in the conversations and see the reason of all this comments and maybe talk with them about it in a constructive way to see why are they so unhappy with your code and if you can code in a way that makes everyone happy and the work don't get stuck in code review.

I just read your last comments, sometimes when you don't agree with the code you can go over it a hundred times and propose changes everywhere that don't make you happy because the real reason its that you would have done a different architectural decision and you just don't like that code, no matter how many times you refactor it. As I say above, maybe you need to agree the approach to the code beforehand so when you write it you know what they expect from it and therefore your code would be more reasonable to them.

share|improve this answer
1  
100% agree with the last paragraph:You should discuss your intended design before implementing. At least then you are starting with a supposedly acceptable framework. Then after implementing, it might be worth yet another shot at discussing the final design (not code). Then modify the code to match the outcome of the final design discussion. If after a couple of tries at that and it doesn't improve matters then that should make it obvious that the position is just a bad fit and you should start looking elsewhere. –  Dunk Dec 19 '13 at 20:59
add comment

From what you are saying, it seems to me that they might have a bias against php developers, and thus they are trying to find every single thing that is wrong with your code in order to prove their point.¹

Regarding the code review itself, I believe as you said already, that such a huge amount of minor comments is less helpful than a few good and valid criticisms. And although I have limited experience with respect to code reviews, the following technique has worked well for the teams I worked-in in the past.

  • First of all, a static code analyser should be used to identify most of the issues before the code review takes place. E.g.: running your code through Sonar, Lint or any other good code analyser should help you to get rid of most minor issues. Especially since your reviewers can define custom profiles to ensure everything from bracket placing, white-spaces, comments, proper variable naming and much more...
  • Second, I seems to work well if you divide the comments into different categories. For example two categories where one group includes small things that are you should take note of and apply in the future. And a second group for those comments that require an immediate modification of your code which would require another commit and subsequent review. Of course, the number of comments in latter group should be smaller.

Furthermore, I have to say that my first real code reviews also contained more comments than I originally expected. However I never regarded this as a bad thing. If you continue to learn from their comments² and are willing to apply those newly learned techniques / best practices in your future code submissions, the comments should become less. It sure was the case for me ;-)

¹ In my experience, this happens a lot as many programmers claim that php is the most evil programming language, having the most inexperienced programmers using it. I distance myself from this statement as I believe that great software can be written in any language!

² Assuming that although the comments are excessive, some value is in them

share|improve this answer
    
I wholeheartedly agree with what you said. It's a learning experience and one should learn. However, this has been going on for long enough to a point where it just doesn't seem like that is the case. Either I'm getting dumber or something else is going on. I suppose if each pull request is generating 100s of comments then either you're very wrong all the time, or there is something else involved here that isn't coinciding with what they're claiming they're trying to do. Either they have to say, "Okay let's stop and learn" or get to the point. At least that's how I'm seeing it. –  user1207047 Dec 19 '13 at 20:02
1  
@user1207047 After reading your responses to the other answers, it seems to me as you already know the answer to your own question.. :-) It seems quite clear that something is fishy with your code reviews. Maybe its time to speak with a superior or to request a transfer to another team? –  Jérôme Dec 19 '13 at 20:48
add comment

Is it common for anyone to get 100+ comments in their code reviews on a routine basis? I would say not. Is it common for people whose code quality "leaves a lot to be desired" to get a lot of comments, absolutely.

However, it also depends on the "rules" of the code review process. EVERYBODY has their own ideas on how something should have been done. If your code review process allows comments to be of the form "You should do it this way instead of that way", then you'll likely get LOTS of comments even for adequate code. If your process is intended to find "defects" then the number of comments should be much smaller.

In my experience, reviews that allow "suggestions" for alternative methods are time wasters. Those "suggestions" should be handled one on one outside the review process. Defect reviews are more useful as they get people to focus on bugs instead of "why didn't you do it like I would have done it?". It also is more useful because there's no denying a bug if someone finds one. Thus, there's no hurt feelings but likely gratitude instead.

UPDATE: With all that said, some code is just plain bad, even if defect free. In that case, the review comment should be a single comment that says something like. "This code needs to be cleaned up. Please postpone the review until the code is discussed with [your name here]." In that case, further review of the code should stop until the comment is rectified.

UPDATE2: @User:Do you discuss your code/design with one of them while you are developing it so you can implement what they are looking for before you get to far doing it your way? Are you changing anything about how you are developing code based on their suggestions or keep thinking your way is fine? Are you learning anything from their comments?

When I am the sw lead on a project, it is my job to be responsible for ALL work products. If I approve a work product then I am claiming the product is acceptable. I want to have a reputation for building quality products. Thus, I have expectations and won't accept less than satisfactory. At the same time I try to teach and explain the reasons for my preferences. Those preferences may not always be ideal (particularly in the eyes of others), but most of those preferences have come from experience. Usually a reaction to avoid repeating the bad ones. Thus, there are a few of my personal "sticklers" that are necessary to get my approval, regardless of pushback.

On the other side, you need to learn the expectations that are necessary to get your work products approved. You can disagree, but since you don't appear to have the authority to over-rule then learn what is expected. I doubt that the team is trying to make you fail. As that makes them look bad also. In that regard, just demonstrate that you are eager to learn (even if you aren't), take what they say and do your best to adapt to their preferences and you'll likely see them back off quite a bit. Maybe find the one that you can at least tolerate and see if they'll do a bit of hand-holding to teach you their ways. Who knows, in the process you may learn something that really could take your skills to the next level.

share|improve this answer
    
Agreed, and you'd hear no argument from me on those grounds. However, the process isn't quite like that. They say it is, and in most cases it appears that only people outside of these three groups are under heavier scrutiny than themselves. They claim others are bad developers but they are the only "developers" on team. –  user1207047 Dec 19 '13 at 20:00
    
One thing however is that if you can't understand the code, or the developer reinvented the wheel instead of using an existing method, or if his method have a cyclomatic complexity of 50, then that is definitely a case for a comment, even if there is no bug. Hard to read code and duplication is a liability, even if its not a bug. That is why I never hesitate to point that a variable is poorly named, or that the solution introduce a temporal coupling that make understanding the code harder. Technical debt must be managed. –  Laurent Bourgault-Roy Dec 19 '13 at 21:19
1  
@Laurent:I know what you are saying and in many ways agree. However, that opens a can of worms that tends to snowball. If your company has the funds and schedule to allow for code reviews to take up a significant part of the effort then it's fine (like medical equipment/aircraft projects). But most projects don't have the luxury. Thus, limiting the scope of review comments is very helpful. To offset your concerns, it is the job of the sw lead to oversee developers and their work. They should know who to monitor most closely and get those problems corrected before code review. –  Dunk Dec 19 '13 at 21:38
    
We'll have to agree to disagree here :) . Technical debt is something that you will have to pay sooner or later (and the more you wait, the more interest you pay). You won't save any penny be delaying cleanup. If you don't take the time to clean it up now, then the next change may cost you double the amount of time because you will have a hard time understanding the code. I work with an 8 years old code base and development had slowed to a grinding halt because of quality issues. We now have an official "internal quality is non-negociable" rule. I can attest that it saved us! –  Laurent Bourgault-Roy Dec 19 '13 at 21:57
    
I reread your comment and I realize that maybe we have a different outlook due to our methodologie. I work on an Agile Team where there is no lead. Since we are all equal and all responsible for code quality, we must all monitor each others. And code review are done every 3-4 hours before each integration. So cleaning a big pull-request is a few hour if we are very nazi or if we did a refactor that impacted an old and crufty part of the software. Hence why I see code quality comment as a "no biggy". –  Laurent Bourgault-Roy Dec 19 '13 at 22:24
add comment

Some important differences with our team inspection process:

  • the basis of an inspection is a checklist, compiled by the whole team.
  • Focus is defects (present and future), not style for the sake of style.
  • the 3 inspectors (including the author) sit together to run over the comments. Only comments with majority vote remain.
share|improve this answer
add comment

For 50 LOC 300 comments seems to be a bit excessive and - wow - 3 reviewers for every pull request? Your company must have a lot of resources.

From my experience for a useful code review process there must be some rules and/or guidelines:

  • Priority of comments
  • Classification of comments (Bug, Code Style, etc.)
  • Agreed architecture / design to follow
  • Agreed code style

If you don't get a priority from the reviewers ask your responsible project manager / team leader; the responsible person for the costs should have an opinion about priorities.

If you have a defined architecture, a common understanding what design patterns you use in your project and an agreed code style, then the review comments should be only about "real issues" like security issues, unintentional bugs, corner cases not covered by the specified architecture, etc.

If your development team has not agreed on "taste issues" (e.g. should a member variable start with "m_"), then every reviewer will force you to follow his style, which is just a waste of time/money.

share|improve this answer
add comment

This really sounds to me like a communication problem. You have an expectation that your code isn't bad enough to deserve 300 comments. The reviewers seem to think you need a lot of feedback. Arguing back and forth in an asynchronous fashion is going to waste a lot of time. Heck, writing 300 comments is a TREMENDOUS waste of time. If these are not all defects then it's possible as a new team member that you don't know the team's style yet. That's normal and something that should be learned to accelerate the whole team.

My suggestion is to save time. Accelerate the feedback. I would:

  • Do more interim reviews to avoid making the same mistake and generating the same comment 50 times
  • Sit with a reviewer as they review your code so you can talk about the issues as they come up, thus avoiding documenting 300 "issues" that will be wiped away in the next commit
  • Pair with one of these "real" developers for some time as you write the code to see what they would do differently

People may argue against pairing because "it will take longer" but that's obviously not an issue here.

share|improve this answer
add comment

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.