24

Our dev team have agreed that all code contributed to the project must be reviewed. We branch off master and merge back for every unit of work (sometimes a whole feature, sometimes just work-in-progress that passes tests). However, as I'm becoming more productive I'm finding myself increasingly slowed down by having to wait (sometimes days) for code to be reviewed, then waiting for another review if there are suggested changes.

I'm working around it by moving on to new features while waiting for a review, but this means context switching, working off old code and more time spent merging branches. Our team is also small so I don't want to stress out or make my co-workers look bad by stacking up endless code reviews. I'm also at risk of making myself look bad by having large amounts of work in-progress/in-review.

They're also usually fairly small changes, so sometimes finding myself pushed to cram more features into a branch, but this feels like a bad idea?

I've not had much experience working as a team, so wondering what steps I can take to improve the productivity of myself and/or the entire team, or even encourage other developers to be more proactive in doing reviews? Would also be interesting to hear suggestions from people who have been on the other side of this.

  • 5
    Are you working in an agile group? Possibly start taking on less stories to leave time for the code reviews. Or everyone dedicates an hour every day to code reviews, etc – mkennedy Sep 2 '17 at 16:27
  • Yes - those are both good suggestions I could raise – J Thompson Sep 2 '17 at 16:29
  • Did all the team agreed to do the reviews? What are you achieving with the review that is not covered by your automated tests (you do have automated tests right?) – DarkCygnus Sep 2 '17 at 16:36
  • 4
    More...(guess I should write an answer)...does everyone's code reviews take a while, or is it mainly you? If the latter, try to figure out why. Can you add more commentary to help guide the review? – mkennedy Sep 2 '17 at 16:39
  • 3
    Never implied that, just asked for clarification – DarkCygnus Sep 2 '17 at 18:21
30

It seems that you are already quite productive with your work, as you say you are improving your development time and also are the one waiting for others to review you code so you can continue.

Also, I would not worry about having tasks on review making you look bad. Quite contrary, those pending reviews mean you are making good progress with your development tasks, and given the situation is not like you can rush or do the reviews all by yourself.

I will go even further and say that you should not be concerned about you doing your job making your coworkers look bad. Although it is noble for you to worry about your coworkers, it is their problem if they are taking too long to review the work you have done.

It may be you have few experience working as a team, but it seems you productivity is doing ok. Also, have in mind that it's probably not your job/responsibility to worry about team productivity and efficient reviews; that is something your lead or manager should be doing, so don't be tormented about having to make your team shine. In this case, setting an example (as you mentioned in a deleted comment) by being efficient in your reviews is a great way to encourage them to be more productive

Another important thing you mentioned (but also deleted the comment) is that Code Reviewing enables you to share knowledge with your team. The bugs you may find with the reviews are nothing compared to the insights, best practices, and knowledge domain your team gets when doing them. This will be more valuable on the long run that fixing some bugs a few days earlier.

Speaking of long runs, it also seems your team is just recently implementing this review strategy; surely when you get more used doing it your review productivity will improve significantly. It is expected of any learning curve to be harder at the beginning.

As mentioned in comments, if your team is including a new task that all team members must do (thus requiring time from your part) you should also balance your job burden accordingly, so you don't get overworked.

As a last suggestion, you could make better use of your time waiting for the reviews on your code by doing documentation, start to plan future tasks or do some other things that you can take out of the way in the meantime. Hope this helps, wish you luck.

  • 4
    Good answer - I also think it's worth mentioning that while code reviews may feel like they slow you down as you wait for them, but all that really matters is whether the team is getting things done (and with less introduced bugs) at the end of the sprint/week/month/project. – HorusKol Sep 2 '17 at 22:25
5

If you find yourself regularly waiting hours for a code review, this is normal, in my experience. You're doing the right thing by moving onto other tasks in the meantime, even if you have to do some annoying merges. As other answers have noted, smaller batches of code are more likely to get reviewed quickly (and the reviews are likely to be of better quality, as well).

If you find yourself regularly waiting days for a code review, then that starts to turn into a major productivity issue, especially if everyone else on the team also has to wait days for a review. Since you say this is a relatively new strategy for your team, I would suggest talking to your manager about how to encourage quicker turnaround. Maybe there could be a quick meeting where good review practices are discussed, both for the reviewer and the author. Occasionally my manager at work has to remind our team not to keep others waiting forever for code reviews, as well.

3

I agree with the other post (that you shouldn't be overly concerned about this, and that it reflects well on you). But I would also add, that I can think of two ways you could maybe reduce the amount of "code review work" for your co-workers:

Make sure every change is as well tested as possible

Specifically, if you are getting stuck in a cycle of:

  1. New Feature
  2. Code Review
  3. Fix 1 to New Feature
  4. Code Review
  5. Fix 2 to New Feature
  6. Code Review

etc, etc.

then better testing will help you to optimise. Think about your ratio of new features to fixes to bugs (especially bugs that could have been found earlier in the cycle).

Minimise the amount of new / changed code per new feature

In other words, don't do in ten lines what you can do in two. This is good practice in any case, but it also makes life way easier for your colleagues.

Also, don't randomly refactor or reformat code, unless it is really necessary.

  • I would also suggest selecting/assigning work in different parts of the code base, so you are not dependent on changes in one branch to work on another. You may also may want to explore advanced features of your CM tool. If you can specify specific tags or versions of certain files, you may not have to actually do a merge. – pboss3010 Feb 13 at 16:17
2

Try packaging your work and making it easier to review. Estimate the time it will take to review the items.

If you have a lot of small independent items, package them into sets of code into logical chunks that can be reviewed within a specific period - like one hour. Schedule a meeting and send links to the repo items. Talk to the team and explain you are trying to clear the backlog through a series of short and targeted meetings.

From the other side, you can put everything together into a growing, single review request. But this is sort of pushing in the direction you don't seem to want to go. Every time you complete a new unit of code, add it to this single request, and update the total time. It may be that people just aren't thinking about the backlog growth. If you manager sees a code review request and it says, "This is a code review request for item X committed on {x/x/x} | {21 days ago}, item y committed on {x/x/x} | {20 days ago}, item... It will take approximately 40 hours to review these 200 items", it will drive home the point.

  • In addition: Code review is made easier by adding a quick description for what you've done (and why), what you've tested and what other things are relevant. This makes it easy to get started for the reviewer, and you'll find your reviews are picked up more readily. – bytepusher Feb 10 at 20:06
2

So I'm going to take the contrarian point of view here. I've worked with [REDACTED LARGE SOFTWARE COMPANY] for 6 years, and after going through the ramp-up phase, found my productivity plateauing at roughly 20% of what it was before (and after), due to mandatory code reviews occurring in different timezones.

The long-term productivity argument is, to put it mildly, nonsense. Sure, at the beginning of the learning curve you do get some insight on how things are done around here, and you learn to put your pride aside and get your brain reformatted in the llama. But after a year or two? You know the llama well enough, thank you very much, and much of the review comments start coming across as misguided if not outright ignorant. And yes, they become a drag on your productivity and sense of usefulness at work.

The coping strategies I found are:

  • If you have a say in whom reviews your code, use that power wisely. Getting someone to trust you so that your code reviews get rubberstamped promptly is good; scratching someone's itch (whatever it might be) to the point that they will assign Perforce OWNERShip of the code to you and leave your hands free to implement whatever evil plans you have, is better.
  • Use Git and private branches and stay as far ahead of the code reviewer(s) as you can comfortably manage. If your shop doesn't do Git, invest whatever amount of time it takes to install / build a gateway between the in-house VCS and Git.
  • Deal with it and plan for released code lagging days or weeks behind your Git tip, wasted time rebasing on top of the changes of other people with better connections or EQ as yourself, etc etc.
  • 1
    I've been programming since before code reviews became part of the chant. I find them a useful tool, but unfortunately the developer culture turns everything into a nail. Instead of developers requesting a review when needed, everything must be reviewed. It's grossly inefficient. – Tombo Feb 13 at 14:31
  • +1'd but I don't see what Git has to do with this. Every halfway-competent VCS has some kind of private branching or shelving or keeping feature, so just use whatever is provided. Git isn't a panacea, it's a source control system. – BittermanAndy May 10 at 10:34
1

Things that I would like to add from myself:

  • Remember that when someone spends too much time in code review may also mean that he gets it difficult to read through the code. I suppose, that you practice clean-coding. If not, make sure to write readable code using meaningful naming conventions and satisfactory code structure. Avoid tricky-hacky oneliners that can deceive your teammates.

  • Also teach your co-workers that if they don't clearly understand a line, it is a valid concern and should be mentioned as a comment in review.

  • Spend the extra time for helping others do their task more efficiently. Perform some pair-programming with your colleagues. Help them resolve issues that are slowing them down. You will see, that they eventually improve after some time.

  • Teach others, how to perform good code review. In my company, we usually add more than two people to a single review to allow them to learn the art of good review from themselves. In previous job, I noticed something like pair-code-review: More than two people were discussing face to face about concerns in someone else's code.

  • You can also spend some time to improve automation in your development process. If there are still some parts that require developers to do manually, think about writing scripts for it. It may be automatic deployment, continuous integration, box test environment. Someone mentioned about static analyzers, but I suggest to use them rather as an improvement, not to replace code reviews.

  • You can also spend more time to increase code coverage by tests.

Edit

  • Just another thing came into my mind: You can also use spare time to prepare a code review checklist. It is very useful mind tool to go through code review very quickly and not missing anything. It could help your colleagues a lot.
1

Our dev team have agreed that all code contributed to the project must be reviewed.

This is the crux of the problem, and given the organization size, it might be unavoidable. Your team needs to reevaluate this decision based on current outcome (choking productivity) because it's harder to read code than to write it. Based on that, the decision to code review everything is more than doubling everyone's workload and you need to factor that as a cost in your team's decision.

The way we do it in our (smaller) shop is:

Phase 1: First 3 months (probationary period), everything is reviewed. The developer learns how we do things, how the system is built, how to find existing code they should reuse (as opposed to rewrite / duplicate), naming conventions, formatting, etc. We gauge the developer's ability / talent. After this period the dev is either retained or let go.

Assuming the developer is retained, we switch to phase two:

Phase 2: Once the developer understands the way we code in a particular codebase, they self review. The developer has the option to request a review at any time, but they are not mandatory. They go back and look at their commit and make sure nothing is wonky. We trust the developer and they are accountable to be diligent in this step. Commits are available to anyone, so a manager or lead can continue to monitor commits or sample them. This cuts the cost in half. If a developer isn't diligent in this step, they get a talking to.

I've observed that most large companies typically do things, especially development, in a very inefficient manner. They have their reasons, but a lot of it is inertia and timidity. I don't like working at large companies because of that.

  • A lot of what you are describing here consists of a good plan for getting past developer familiarity and training issues which might weigh down a review process. But ultimately what you are describing is running without reviews - and that misses the 2nd set of eyes advantage. If there's a business decision to do so for speed and up-front cost reasons, fine, but "self review" is not a code review within the accepted meaning of the term. Conversely once you get to the point where your self review is workable, peer reviews should not take as much time as in the situation of the question. – Chris Stratton Feb 13 at 15:38
  • @ChrisStratton, every positive must be weighed against the cost. A 2nd set of eyes advantage is difficult to substantiate, particularly when you have unit tests, QA and UAT. I see it as useful when your development team is substandard, but if it's a strong team, the cost must seriously be considered. In the OP's case, it's halving productivity. If a manager implemented something that halved their team's productivity, they'd likely be fired for incompetence. Code reviews have their place, but doing it every time as dogma is insanity, particularly when you have other safety valves in place. – Tombo Feb 13 at 18:02
  • This is only true if you think about productivity in terms of output rather than in terms of utility. Code your team as a whole trusts and understands is easily worth twice what a lone proof of concept is, even a readable proof of concept that no one else has looked at is still not in the team's active knowledge. This gets forgotten, because today many startups are habitually in proof-of-concept mode - picking up the pieces of those ideas that seemed wise at the time, but are absurd in a second persons daylight viewing is left for later. – Chris Stratton Feb 13 at 18:09
  • @ChrisStratton, it's true regardless. For your argument to work, you need to be able to justify code reviews with halving productivity and also reconcile with the fact that you are paying for unit tests to be written, a QA team to test and UAT environments. The only way I can see that is if you have a substandard team (or you just don't trust their competence). Also, why just one code review? Why not hire an outside consultant to review code as well? At some point you get diminishing returns, and for me, that point is once the developer has proven he/she can write production quality code. – Tombo Feb 13 at 18:31
1

We had this problem at a previous company I worked at. Every single code change was required to go through a Pull Request (PR) and code review with at least two approvals from experts in the domain (usually our own team members sufficed but certain people's approvals were weighted more heavily than others) before it could be merged into master. Here are some things we found helped speed up the review process:

  • Try to have as few changes as possible in each PR to make them quicker to review.
  • Make the PR description as detailed as possible with regards to what you're changing and why. Any UI changes should have before and after screenshots. This gives the reviewers context about the change so they can more easily identify things that may not be intended.
  • New PRs are posted to our team Slack channel to increase visibility. If working in another team's domain, the PR is posted in their Slack channel as well.
  • If working in another team's domain, solicit help from that team in doing that work to begin with so the PR isn't a surprise. You're also much less likely to have done things in a way that they'd rather you didn't.
  • Each team member spends 15-30 minutes each morning and afternoon reviewing any outstanding PRs. We usually did this first thing in the morning and when we got back from lunch so we were less likely to forget.
  • If a PR has gone more than 24 hours without being reviewed, verbally remind the team.

Between all of those, we found a nice balance between writing and reviewing code, and thus were able to deliver code at a pretty consistent rate.

  • Another thing that can help is keeping the PR as separate patches optimized for reviewing efficiency to be rolled up for final merging. As a contrived example, if you moved and changed a function, having the move and the change in separate commits makes reviewing simpler. Reviewing a change that doesn't change any code is easy. And now the code change is smaller. – David Schwartz Feb 14 at 2:05
0

While this question is tagged Software Industry, I think it would best to step back a bit and look at it from an industry neutral point of view.

Your companies process for the work you do involves an external quality review step. Your productivity is thus measured not by the time it takes for your task to go from start to finish, it is measured by the time spent between start and ready for review (in total, if more work is required to pass the quality control review, then that extra work gets added on to your total).

So your personal productivity is not at issue, this external review does not slow you down.

Now there are two caveats to be aware of, one that is directly your responsibility, and one from a company efficiency point of view.
1. First the personal, you obviously want to have as little rework as possible. Some is expected, possibly even desirable, a lot is a problem.
2. Secondly, and this is more industry specific, you want to make it as easy as possible for your work to be reviewed. If the QA takes longer because you didn't take the extra steps necessary to ensure that testing was easy and clear, that can be laid at your feet, particularly if your collegues do take steps to help the testers. So making it easily testable can be considered part of making it ready for review.

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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