Isn't this the point of code review? When I click accept on a code review, I am saying that I have looked over the change and believe that it is correct and okay. If I just arrive at the conclusion by saying "Joe wrote this, and I trust Joe" the there is no point in me reviewing it.

When it is unreadable and hard to see whether it works it is different thing, but then the complain in review should be some specific variant of "hard to read".

I review mostly for architectural compliance and "bad idioms" or code smells. There is difference between not like I would write it and badly written mess and many programmers confuse them.

It is not a matter of if the reviewer "suspects there is a bug", but rather a matter of if the reviewer is convinced that there is not a bug.

If the reviewer needs to have someone explain why every minor code change is correct, they are either lazy or incompetent (or the code is badly written). Further, it is generally a bad idea to go to the person who wrote the code to explain why it is correct, as you are then much more likely to make the same mistake the original author made. However, being "okay" often goes beyond correctness, and into business decisions. Often times, these business are not documented (or if they are documented, it is in some management document that is not linked to from the code), so in order to review it, the reviewer has to ask the author what the bussiness consideration that led to the change was.

It is similar with being petty in code review - sometimes people do it because they think it makes them look like better coders.

As for checking business case, yes sometimes you have to do it. But there is also such a thing as doing it too often and second guessing every requirement that you see during code review. This has tricle down effect on analysts/managers/or even customers (he got pissed after a while of this - true story) who then have to litigate and defend every detail and solve programmers conflicts over insignificant details.

Code reviews occur far too late in the development process for an uninvolved developer to provide a good, timely review of “bigger picture” concerns like business consideration (or software design) simply because the reviewing developer needs time to come up to speed on why decisions were made. In these cases it’s better to have the reviewing developer involved in a review of business requirements before the code is written. The code review can then stay focused on code.

This ^ I just had this discussion last week in a PR, if the comment is a matter of opinion, the whole team should agree and put a rule in the linter or style guide. If not every one is free to have their own preferences.

A code review ensures that the non-functional quality of the code is high. I.e. that the code is understandable/maintainable by someone other than the programmer himself, that there are no anti-patterns or dangerous-but-correct usages of language features, that the implementation fits to the intent of the specification etc.

Occasionally a reviewer will spot a bug. Occasionally there will be a false positive that turns out not to be a bug.

You really want to deliberately discard this bug-finding opportunity? Why?

Even if it's a false positive, doesn't that indicate that something bears commenting?

And most often of all, while explaining what they've done, the reviewee will say "and this does... oh crap, wait" and fix the mistake themselves. Among all their other virtues, a code reviewer is a level 2 debugging duck. (Although all of my code review experience is with buddy check-in style reviewing rather than sending it off to be reviewed. I can understand endless nitpicky questioning being annoying if you're doing it offline.)

That is, go over the diff and add commentary explaining to the reviewer why you made particular changes (not explaining what the changes do, that should be in comments in the source).

Isn't the code review generally pretty late, i.e. just prior to release? At that point, the code should be passing all unit tests and I'd expect obvious bugs to be pretty unlikely. Non-obvious bugs generally won't be spotted in a code review setting.

Also, lots of software houses don't have formal testing of all new code. Not unusual with GUI code, for instance.

Also, passing tests is meaningless if the tests are worthless or incomplete. So, those alone can't be used as a metric of code sanity; Meaning obvious bugs are still possible.

Tests and reviews are just to reduce the number of released bugs. and hopefully increase maintainability.

Also, what's obvious to one developer might not be to another.

> A code review ensures that [...] the implementation fits to the intent of the specification.

I don't see how both of these can be true.

> Functional correctness is typically "proven" by tests.

You can (and, in my view, should) review tests, too.

Pedantic is the wrong approach to technical debt though. Almost every aspect of technical debt is about trade-offs.

If what you're doing appears pedantic it should come accompanied by an explanation of why the trade off the developer took is not ideal.

>I also ask a lot of pretty dumb questions during a review because I want to make sure that my understanding of the requirements matches the understanding the other engineer had.

Your code should really come accompanied by tests which you can read and use to understand what the precise interpretation of those requirements was.

That is not objective fact across projects. That means either the process or culture is bad.

Obviously, we do weight how much time it takes to fix it vs how much of the debt it actually is. We would not had that much independence in these decisions if we did not.

You get told to quickly write something as a Proof of Concept. Then get told, it works so put it in the code and reelase it. We will rewrite it later.

You never get to rewrite it later.

Over time the code base goes to hell. Every single time

The really big mess tend to be emergent - when features and code base grew too much for original architecture over time. You cant prevent that one by detailed code review and it takes a lot of effort to fix it.

But then we had 5% unit test coverage on a 5million+ LOC code base, they were terrified that refactoring (with no tests to back it up) might break things.

We were writing medical software and they were very risk adverse (obviously), so if i refactored a method I would have to raise that and would mean test cases would have to be written and manually run to ensure I hadn't broken anything.

So refactoring hardly happened (The code was an absolute mess), when i left they were working on a project to fix it but the timescales for it were 5 to 10 years.

As a corollary, when I ask a question in code review, I usually don't want it to be answered there - I'd prefer it to be answered in the code, if need be using a comment.

This is why I hate implicit assumptions. If I need some special knowledge or some special assumption that is no where in the immediate vicinity of the code, then maybe your approach to the problem is flawed. Sure it'll work but good luck to future coders working with it

This discussion makes me wonder about more tightly connecting the code with past reviews. What if when reading some code, the user had not only access to the revision history, but also the review history around the code in question? Something along the lines of storing reviews in revision control metadata.

when posting the review, you try to eliminate as much of the cruft as possible.

when reviewing code, sit down, you're actually going to read it for understanding.

your mutual goal is to improve code and product quality. if after a few passes something seems wrong, or unclear you raise it. now the two of you get to have a pleasant discussion about what the right thing to do is. maybe some rework. maybe just a comment about unhanded error conditions. maybe nothing.

if someone raises an issue that not important either way, just change it to save time. and don't raise something unless you think its important. so, if you both think its important, you can have a discussion and reach consensus.

i think thats the real problem, that somewhere along the way the consensus culture got lost. that was a really important thing.

I would take the position that written code standards enforced by humans should not be allowed. Either a given standard is important, in which case you enforce it automatically, or it isn't, in which case you drop it. That frees up code review to focus on more important issues.

Write a document which describes how you want code to be reviewed, share it with the team and then lobby for everybody to agree upon it / sign it during retrospectives?

I think it's better to develop a strong rapport with members on your team and only join teams with strong shared values about what constitutes good code, but in lieu of that you could do worse than to draw up a shared written agreement about what constitutes good code (and iterate upon it).

or may be we should find a job where constructive criticism is part of the culture and you then leave the current job! Relatively easier to find where the change exists than investing effort to bring about a change.

Personally I find that non-idiomatic code can be a bit distracting when I'm trying to evaluate the code for its overall approach and architecture.

This has nothing to do with trust or any other personal matter. Everybody makes mistakes, everybody might misinterpret code they use and not every programmer has a complete understanding of the scope of the changes they are making. This happens for both senior and junior engineers. (Obviously one would expect this happening much less for seniors)

The only way minimising the amount of future issues is to have thorough, rigorous and pedantic code reviews. Another major benefit for this approach to code reviews is having more people on the team understand deeply the component you coded.

That’s funny; I thought you were going the exact opposite direction with that. I wish my code reviewers were pedantic and obsessed with correctness!

The few time’s I’ve had the pleasure of working with them (usually open source) were the times I’ve learned the most about programming in the shortest time, by far. Especially about readability and quality of comments.

If it's not immediately clear to a reviewer that your change is correct, then either your code isn't clear, you're not writing tests, your colleagues don't have sufficient understanding of the code base, or some (non-trivial) linear combination of the above.

The first two cases are something you can work on. If people don't understand your code, then try to write code that is more clear. If your colleagues are as talented as you say, then they should be able to understand your code without explanation. If you're not writing tests, then well, maybe there is just no hope.

If your team does not have sufficient understanding of the code base, then do communal code-review sessions where you pick parts of the system and read through it together. Or organize knowledge share sessions.

Ultimately, as Lenin said, 'trust is good, but control is better'. The whole point of code reviews is that you want to get extra eyes on your code. Trying to bypass that by asking your colleagues to blindly trust you is undermining that process and ultimately undermining the quality of your system.

That said, in the end, correctness is important and justifying and communicating your change to others is the purpose of a change request. If you aren't following define standards and idioms then you should get on board. If they aren't defined then they should be defined and reviewed to get everyone on the same page. If everyone is using code reviews to grandstand or show off their knowledge that's a cultural problem that needs to be addressed.