Code should be reviewed, preferably by a different group of people who wrote the code, metrics are only useful for people to manage themselves. Upon review there should be immediate feedback to people who wrote it and if they continue to make the same errors, then you should eventually get rid of the person who wrote it. Making up stupid systems of control to "incentivize" people as a method of management is the stupidest thing rationalists ever cooked up. It destroys internal motivation and discipline.
That's what I had to do. My manager told me of this metric (set by the higher ups in a contract with the offshore company) along with "there's nothing we can do." I essentially became the code repository sheriff. Eventually I was spending too much of my time refactoring their code and not enough on my own. Then deadline creep set in for me and I had to just let it go.
>is the stupidest thing rationalists ever cooked up.
Oh, you don't like bonuses then? Unsolicited, forced code review doesn't have much value that I've found, unless your team is fairly junior. Senior guys know what good code looks like, even in a crunch. If you hold forced, unsolicited code reviews with senior developers, you are really just throwing away money and aggravating people.
It's just another half thought out process. A better incentive, in my opinion is: if you release a complete codebase with 0 medium and above defects by X date, you get a free week off, or something of significant value. A free, company branded desk clock doesn't cut it. (that's happened to me before).
If a company is trying to save money by hiring the cheapest offshore developers they can, they are still really just throwing away money and aggravating people. lol. That is a perfect example of a poor incentive: cut costs without any regard to the resulting cuts in quality. Anyone can make a turd cheaply, but that's rarely what businesses really want.
A reviewer soon understands that senior devs are producing good code and stops investing time on reviewing their code. If they are doing their job properly. But again it comes down to internal motivation and self management on their part.
Yes, sorry I should have been more explicit. Initial reviews have value (is this guy full of beans or does he know what he's doing), but constant reviews of senior developer code do not. If you've hired anyone you have to constantly do reviews for, you've probably hired the wrong person. I mean they should get what you are looking for after a month or two.
I know exactly zero developers senior enough to have gotten over writing bugs. I know I've written bugs that happened to pass all my tests only to blow up in somebody else's face, sometimes after surviving years of production use.
Code review would've at least had a chance to catch them early—so long as you treat it as critical analysis of program logic and not just a screen for generic goodness.
IMO if your code reviews are glorified manual style checking, you're doing it wrong.
>I know exactly zero developers senior enough to have gotten over writing bugs.
No one was arguing that. Code reviews aren't for bug discovery, though that can sometimes happen. It's a very inefficient way to discover bugs.
Developers should test their code for bugs before completing the task and submitting to QA. QA is a more thorough than development bug / unit testing and includes integration. Betas (if applicable) are the final source of pre-release bug discovery. Spending a bunch of time on code reviews to find bugs.
IMO code reviews should do two things:
1. Make sure the developer knows how to develop and is not being sloppy and not following whatever standards are set (comments, etc). They are essentially training wheels for new developers to the organization and junior developers.
2. Make sure the code represents what the developer thinks it does (sanity check).
You do need to make sure the changes follow the "philosophy" of whatever component is being changed. Otherwise you get things like duplicated functionality or APIs that don't have any cohesion. If the Senior engineer frequently works on this specific piece then its not an issue, but no matter how senior someone new to an area will not produce optimal code.
Product Managers get bonuses related inversely to hours of downtime (caused by bugs).
Engineers get bonuses related to number of features pushed out.
In theory, this would encourage engineers to ship as fast as possible whilst encouraging PMs to ensure quality over quantity wrt feature scope/volume. This way you'd have engineers begging to add features and PMs begging for tests.
What you cause is stress and tension between the two groups. They start to hate each other and blame each other. "That's not a bug - that feature was never in the spec"
The PMs will have an incentive to shift the blame around ("oh it worked fine when we tested it, it must be the fault of the database, not our bug"), while you'll only get "10x" engineers who turn your codebase into a flaming turd that's unmaintainable over the long term.
How do you check up on that reviewer? In you system, he has power to fire people, decide all standards+code style+architecture by himself and force that upon everyone else. All that with no accountability nor responsibility.
