12

So I started working for a large corp., one of those with 3 letters in the name, and they are trying to become Agile, but have tons of processes, which I don't feel are Agile.

The one that has me the most wound up are code reviews. My last job was with a startup that I would say is the most Agile development team I have seen, been on, and/or ever heard of.

Anyway, my argument is that Code Reviews are a waste of time in iterative or Agile development where the UX/UI is extreme/intense (think Apple/Steve Jobs perfection). Maybe someone here can help understand before they fire me?

Here is my development process and the one at my last startup... very Agile.

We do the early feature work to sort development task/todos. We would mock a couple versions up and present to users, team, and marketing to get feedback. We then do another mockup iteration to get one round in from the same stakeholders above. Then we divvy up the work and get started. We have milestones and dates to meet, but we keep plugging away. We have no code reviews during any of this. Several times during the weeks of our development we hold sesssions with the stakeholders again to see if they still agree features/functions/UX/UI are still a fit and on target.

As we approach the end of the 8 week iteration cycle QA starts testing, then it goes to alpha users, and finally to beta users. But during the alpha and beta developers are going over the new features and older features making iterative daily or hour changes to the UI to improve the UX. So a feature that was being developed this release, might end up being changed 3 more times the last four weeks to improve and perfect it or add a few tiny features (e.g. make the component a little slicker or smarter). Sometimes the changes might be superficial meaning no CRUD operations are changed or modified, but all UI only changes.

So with this type of development process, extreme Agile, wouldn't code reviews be a waste of time? Meaning if I had another developer or two review my code, but then that code changes 3 more times before it goes out the door, because of all the UI/UX improvements, are we not wasting our time for first 3 times they reviewed it the code as that code/component/UI was scrapped?

We never had many quality issues with this process and yes if a developer left all the knowledge walked out the door, but we always found smart developers to pick it up and takeover.

And yes, we have a lot of testers because they may have to retest things 3 or 4 times. Also please don't get hung up on asking why all the UI/UX changes...thats just how things are done... been then thats why the app wins tons of awards for UI/UX and the users will kill for the app. The thought process is if I can make a even a 2% improvement in something because I have an extra hour then do it. Users will be happier, which means more $ or users. And yes, our users are ok with the app changing continuously because thats how its been done since day one so they don't see it as bad or a negative.

Hope this post doesn't come off as pompous, but I just can't see how Code Reviews aren't wasteful. Maybe 2% of all our code in the code reviewed has bugs. Each release we might find 3 bugs via code review. So it ends up being 40 hours of code review per developer per release (4 x 40 = 160 hours) to find 3 to 5 bugs? Chances are 50% those 3 to 5 bugs would have been picked up by QA anyway. Wouldn't it be better to spend that 40 hour per developer adding a new feature or improving the existing ones?

Improve this question
5
  • @DeadMG: User Experience
    – Steven A. Lowe
    Commented May 19, 2011 at 21:06
  • 5
    @user25702: the process you describe does not sound like Agile, it sounds like RUP/spiral. Specifically "Several times during the weeks of our development we hold sesssions with the stakeholders again to see if they still agree features/functions/UX/UI are still a fit and on target." is anti-agile; features are frozen during an iteration to avoid the moving-target problems associated with RUP/spiral approaches. As to your nominal question, I don't see much value in code reviews here if and only if you are certain the bugs would have been found by QA.
    – Steven A. Lowe
    Commented May 19, 2011 at 21:24
  • 1
    8 week iterations is not agile and definitely not "extreme agile".
    – Martin Wickman
    Commented May 20, 2011 at 7:14
  • Some PMs think that iterations mean we have a couple short iterations in the beginning and a couple long iterations in the middle followed by as many short iterations at the end as needed. The problem is that this messes with the battle rhythm of software development and the ability to catch bugs early. The 8 week iteration would be one of those middle iterations. I do agree that this is not agile.
    – Berin Loritsch
    Commented May 20, 2011 at 13:23
  • If you want to argue code reviews away, then I recommend taking some stats. Document the time taken for the code reviews (in total man hours), the number of bugs/issues discovered in them, along with the severity of the problem. For my team it turned out we spent at least 16 man hours per review, found on average 2-3 bugs, all of which were cosmetic in nature. It was easy to argue for test-first methodology to replace peer reviews in the face of those numbers.
    – Berin Loritsch
    Commented May 20, 2011 at 13:27

7 Answers 7

Reset to default
13

There are a few things that code reviews can do for you, and some things they can't. The arguments in favor of code reviews:

  • Collective ownership
  • Find bugs (QC)
  • Enforce consistent style (QA)
  • Mentoring

Many agile processes tackle those in different ways:

  • Collective Ownership: everyone on the team is responsible for the project, which means everyone's eyes will be on the code at any given time.
  • Free to refactor: this takes code reviews to the next level, and allows anyone on the team to perform refactoring as necessary.
  • Unit tests (QC): unit tests are more efficient and less prone to human error then visual inspection. In fact, I have yet to find a more efficient means.
  • Pair programming (QA): takes care of mentoring and provides early refactoring advice as the code is written. This is also still a controversial topic, but I find it helps while ramping up a new developer. It's also a good time to enforce coding standards.

In essence there are other avenues of taking care of the potential gains you would normally have doing peer reviews. My personal experience with peer reviews is that they are very inefficient mechanisms and are not effective at finding bugs or larger design flaws. However, they do have their place in some teams, and on projects where agile is not feasible (for whatever reason), they are quite necessary.

Improve this answer
4
  • 3
    There seems to be some misinformation in the current answer. Collective ownership does not mean "all eyes on all code all the time." Refactoring has nothing to do with defect detection. Unit tests and inspection serve different purposes and in fact can each uncover different kinds of defects (examples in other answers). Pair programming, while a form of review, is not a true replacement for e.g. Fagan inspection. Your personal experience seems atypical, especially concerning design errors -- what kind of reviews did you do? How did you measure efficiency for the reviews?
    – Michael
    Commented Sep 25, 2011 at 20:23
  • 1
    Time performing review vs. defects found and their severity. We compared that with the same metrics against unit testing. Issues discovered during code review were almost always code formatting related, and they took longer to perform. The same time spent doing unit tests uncovered real problems and took no longer to prepare and do.
    – Berin Loritsch
    Commented Feb 6, 2015 at 12:03
  • "Collective Ownership": In my experience this is often an illusion: reviewers often nitpick at small details and do not see the big picture in code written by others. Then, when it comes to modifying that code, they do not really understand it and they either (1) do not dare to change it, or (2) they extensively rewrite it so that they can understand it. Approach (2) often has two side-effects: (A) they introduce bugs, and (B) the original developer does not understand the code any more.
    – Giorgio
    Commented Jun 25, 2017 at 11:42
  • Point B shows that often what happens is not collective ownership but individual ownership shifting from one developer to another all the time. In this way, each team member roughly knows what the code does and how it is organized, but no one really understands it deeply. A true collective code ownership would require much more time and discussion about the code to get a common understanding, but often this time is simply not available.
    – Giorgio
    Commented Jun 25, 2017 at 11:44
13

Are code reviews only for finding bugs though? You seem to think that is true and I don't.

I'd argue that code reviews can be more about collective ownership of the code, ensuring that knowledge is in multiple heads, and preparing others to inherit the code which could be for new features as well as for bugs. I like code reviews as a way of having a bit of a check and balance to the system as you never know when someone may have an idea of where something may be re-written to maintain the conventions.

Improve this answer
7

Code reviews and tests often don't catch the same sorts of bugs, and bugs caught by code review are likely to be easier to fix, since the location of the bug is known.

You can't know if code is bug-free just because it passes testing with none recorded. "Testing can only prove the presence of bugs, not the absence." (Dijkstra?)

Code review also keeps the code style the same, and is capable of finding places where the code is not good but happens to work for now. It saves maintenance costs down the road.

Also, the demands of a large corporation and a startup are different. Startups normally fail, and have to move fast. Large corporations get much more value out of doing things right rather than as fast as possible. You may well prefer working at startups than large companies, but that's not a reason to try to impose startup strategies where they don't fit.

Improve this answer
4

Pair programming is the XP answer to code reviews. Essentially, every line of code is reviewed as it is written. It's code reviews taken to the extreme.

Improve this answer
1
  • 8
    I would argue strongly with this. Sure, it's being reviewed by two people, but those people are generally on the same page as the code is being written. A code review is someone with a completely different state of mind looking at your code and finding "doh! Forgot about handling that case" kinds of issues -- XP really doesn't help with that.
    – Billy ONeal
    Commented May 19, 2011 at 20:29
3

Do your code reviews only ever turn up UI/UX changes? I would argue that's not a code review, it's a usability test. Code reviews are much more about turning up the problems that the users/testers/business/whatever never see, because they're in the code. The clue is right there in the name.

Now I will agree with you that there is a line to be drawn somewhere. Do you review 4 iterations of the same UI change? Or do you go through 4 iterations of that, with each one potentially making the code less maintainable? I would say try and measure both approaches and find the right balance for your team, but don't abandon code reviews completely.

Even if a code review never turns up a problem, it has a benefit that you rarely notice until it's not there: every piece of code is looked at by two developers, so two developers know what the change was and what it was intended to achieve. So one of them falls sick the next day and is off for a week, the other can pick up any urgent work they were doing.

Improve this answer
1

I tend to agree that collective code ownership and pairing along with TDD and CI are the Agile antidotes to formal code review sessions.

Even under UP/Spiral I wasnt a big fan of a specific process step being "code review" because is seemed to me the kinds of issues it's likely to find are found later than they could be if the same energy were instead invested in some upfront collaboration and some simple automation.

I felt that because there was: - some shared review of the design (usually expressed in UML at least on a whiteboard) meant large scale design issues or poor use of APIs, etc. get caught before lots of code is written. - FxCop, CheckStyle, FindBugs (or similar) running along with automated continuous integration builds to catch naming, stylistic, visibility, code duplication, etc.

We were able to fail earlier and get feedback quicker than a downstream code review habit would have produced.

I'm not saying it's a waste of time to sit down and have a look at your codebase once in a while, but making code review be a gating step to calling something done seems like it creates lots of work in progress that could have been avoided with better inspection/collaboration upstream.

Improve this answer
0

One of the primary goals I expect from code reviews is to increase the ease of code maintenance. Code reviews should be helping everyone write clear code reasonably compliant with good coding standards. Most code gets a lot of maintenance especially in big companies. Payback for maintainable code should begin before the code is released, and continue on afterwards.

Code reviews in and of themselves should never result in code changes. If the code review indicates that changes are required, then implementing the change will result in a change in the code.

Code state may change as a result of the review, but that should be mostly irrelevant to the issues you mention.

If code review is resulting in multiple code changes then something is broken in your development process. Given the number of testers you have there may be a throw it over the wall and let the testers find the problem mentality.

Things should be going to the testers in completed state. As much of the testing as possible should be automated, so that testers aren't retesting the same stuff time after time.

UI/UX does require some testing time, but having design/development experts on the front end should reduce that. It also requires a face in front of the screen. However, in all the applications I have worked with, it was a relatively small portion of the code.

Cost to implement changes (including bug fixes) generally goes up for every stage it goes through. Finding bugs in development is generally cheaper than fixing them after testing find them.

Improve this answer

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