Code reviews are an antipattern and should die in a fire. They slow down the development process without bringing any benefits.
Some say code reviews prevent bugs. They are wrong. Testing code and static analysis prevent bugs. Require all code to have unit tests and run them automatically. Run static analysis automatically. If you have developers who don't write tests, fire them. Code reviews only give you a false sense of security. You figure: since two people looked at it, it therefore must be good; forgetting that the reviewer just skimmed it over to get it out of their queue.
Some say code reviews enforce style. Assuming you care about style: why not just have your IDE or static analysis tool enforce it for you?
Some say code reviews help share knowledge. That may be true, but it should be unnecessary. Standups share knowledge. Wikis share knowledge. Comments share knowledge. Code should share knowledge. If you have code hitting trunk that every developer on your team can't understand easily and quickly, then you need to start finding new developers.
I can write you code that will pass all static analysis tests with full code coverage and complete dependency injection that still makes you taste your lunch in the back of your throat. I think any good programmer could.
> forgetting that the reviewer just skimmed it over to get it out of their queue.
There's a difference between cargo cult code reviews and an actual, formal code review.
There is a ~9 year old buffer overflow vulnerability in sendmail (crackaddr) that no static analyzer can identify even today (and they are specifically targeting it as a test case).
And what exactly is wrong with the code that passes all the tests and works? Its aesthetics offend you? Should the first priority of code be to work to just work?
Beauty is only a nice-to-have feature. If the pretty way is about as fast to produce as the "lunch in the back of your throat" model, sure lets go pretty. But if it's going to cost me 50% of my developer's time, I'll take the quick and dirty every time.
It's not so much about "beauty", but long-term maintainability of code makes development faster in the long run and reduces the murder rate within your dev team.
The wrong part here is in "passes all the tests and works" assumption. In my experience, static analysis, unit tests and reviews all help minimize errors (amongst their other benefits), ideally you'd make use of all three.
> Code reviews are an antipattern and should die in a fire. They slow down the development process without bringing any benefits.
Woah slow down there soldier. Code reviews are great for many reasons:
1) Introducing new technologies or techniques you used to solve a problem.
2) Letting your styles mesh with your team.
3) Focusing on whether the concern/problem has truely been addressed.
Also, unit testing isn't as widespread as you may think. Many of the projects I've worked on didn't have the infrastructure in place to even start unit testing. Also, ever try to unit test Javascript in the browser, it's possible but it sucks pretty hard.
If you are on a team of 2-3 hardcore devs, then you're probably more correct than not. Thats not usually the case.
1) Lunch and learns or brown bags or whatever your org prefers are better. Code reviews are only 1-to-1. Actual classes are more efficient.
2) Use tools to enforce style if you care. Personally, I think it's a waste of time.
3) If you have devs merging code to trunk that isn't fixing the bug and creating the feature requested, then that devs needs replacing.
RE unit testing: I realize it's not that widespread. But anyone developing software and not unit testing is doing it wrong. They should take all the time they waste on code reviews and instead spend it on building out their test suite.
And unit testing Javascript is not only possible, but easy. We do it with Jasmine. It works great. The key is to write good, testable JS instead of the mess of spaghetti that passes for Javascript code in most places.
>RE unit testing: I realize it's not that widespread. But anyone developing software and not unit testing is doing it wrong. They should take all the time they waste on code reviews and instead spend it on building out their test suite.
This is a very good point. If there are very basic things not being done (or done well) focusing on those things is more important than ceremonial code-reviews - and in the beginning it seems code-reviews are often only ceremonial and some shops never get past this stage. I've never seen them work really well aside from cases where they happen organically between two co-workers who like and respect each other.
I think that approach would work for a typical developer of a typical web app. But not all places are the same. You can't easily test many things. You can have correct tests that miss some cases. You can have incorrect tests matching incorrect code. You can have code fixing the result rather than the cause.
Basically unless you trust developers to never be wrong, I'd do reviews. And then if you do trust them, I'd still do reviews to make sure everyone learns what's happening outside of their area and where common utilities can prevent code duplication.
In short - there are things that cannot be realistically automated or simplified into compilation checks. If they don't occur in your environment - great. When they do, look again at code reviews.
For style, there are points of style that no automated tool that currently exists can enforce, such as 'good naming'. At the moment that looks like an AI-hard problem.
For your point about sharing knowledge, your argument works against all forms of sharing knowledge. You may as well say that wikis are unnecessary, because standups, code, comments, and code reviews share knowledge. What you have missed is that these different modes of knowledge sharing work better at sharing different kinds of knowledge, which is why you (should) want all of them.
Also, good developers are not really fungible. Developer X may be really good at making code flow, so that what it is trying to do is not obscured by how it is doing it, and developer Y might have a really good knowledge of algorithms and an eye for optimisation. In that case, having them review each other's code will always result both in better code and in (eventually) better developers.
As far as it goes, I have seen plenty of badly done, useless code reviews. I have also had really well done code reviews, where someone would email me an annotated diff of one of my commits, or where I would pair for a few hours with someone going over some thorny code I or they had committed and making it less thorny, or where we would go back and forth refactoring some piece of code while talking over the whys and wherefores. If I had only had bad code reviews I would be much more sympathetic to the point you are making, but as it is I just think that you have never been involved with a competently done code review.
That's all true but code reviews certainly share a lot of information and feedback specifically related to the context of the day by day development. Which certainly will increase your productivity by teaching you about mistakes and at the same time letting you evangelize someone else in your team/organization. You won't find the same amount of information for your project with as much frequency as you do on code reviews.
Yes. At Tingo.com, my team of 11 does not do code reviews. Our code base is big but not huge: 151k lines of Java and 125k lines of JS (not a great measure, but it gives some idea of magnitude). We do extensive testing and static analysis. We ship code everyday, and have been doing that for the last 10 months. We end up having to patch production to fix bugs pretty infrequently-- less then once a month.
There's a big, big gap between "Code reviews are an antipattern and should die in a fire. They slow down the development process without bringing any benefits." and "My startup does extensive testing and static analysis but no code reviews; we don't have very many bugs in production". I think you would have come across better saying something like this from the outset, and with a lot less attitude.
You've provided one uncontrolled anecdote that condradicts all the object research I've seen [1]. I don't find that very convincing. Not only does code review identify bugs, it does so much more effectively than any other method tested.
No, but neither is there quantitative evidence that code reviews help improve software quality. Code reviews are just always assumed to be good-- they're a basic part of the 'good software engineering' cargo cult.
I got rid of them here because I've personally never seen a beneficial affect at any of the places I've worked. Easy to spot bugs still made it to production. Crappy, inelegant code still made it to trunk.
Getting rid of code reviews and making developers personally responsible for what they commit has improved both of those aspects for us.
> No, but neither is there quantitative evidence that code reviews help improve software quality.
You might want to check out the book "Code Complete", which says in Chapter 24:
"...the average defect detection rate is only 25 percent for unit testing, 35 percent for function testing, and 45 percent for integration testing. In contrast, the average effectiveness of design and code inspections are 55 and 60 percent."
There is indeed quantitative evidence that code reviews help improve software quality. Steve McConnell's "Code Complete" refers in detail to such evidence.
Some say code reviews prevent bugs. They are wrong. Testing code and static analysis prevent bugs. Require all code to have unit tests and run them automatically. Run static analysis automatically. If you have developers who don't write tests, fire them. Code reviews only give you a false sense of security. You figure: since two people looked at it, it therefore must be good; forgetting that the reviewer just skimmed it over to get it out of their queue.
Some say code reviews enforce style. Assuming you care about style: why not just have your IDE or static analysis tool enforce it for you?
Some say code reviews help share knowledge. That may be true, but it should be unnecessary. Standups share knowledge. Wikis share knowledge. Comments share knowledge. Code should share knowledge. If you have code hitting trunk that every developer on your team can't understand easily and quickly, then you need to start finding new developers.