Item — HN Reader

The first one doesn't belong on this list:

> Does the code work? Does it perform its intended function, the logic is correct etc.

I believe this concern lies with testing (automated or manual). I don't believe a code reviewer should even have to know about the requirements of the feature/bug, and should focus more on the design/quality vs. whether it works.

Mostly because it's almost impossible to know this by just reviewing the code, without having to run it -- and I certainly don't think a code reviewer should have to execute the code.

3 comments

Maybe I'm weird, but I think whether it works should be #1 concern over design/quality. I agree design/quality is a close #2 though.

Also, perhaps you need more practice reading code. So you are not so reliant on a "TEST PASS" output from a tool to know if a particular section of code is likely working.

I would consider works and design/quality to be 1a and 1b, and see them as linked concepts. If the design/quality is poor, it is much harder to prove it works in all cases.

Second, generally[1] code is read and edited many more times than it is written. Poorly designed, but working code today is setup to be broken when the first edit comes around.

[1] Some code is clearly 1 off and throwaway. This is not the code I'm talking about.

I certainly agree that whether it meets the requirements is important (debatable whether it's more important than design/quality).

I'm just saying it's not the concern of a code review.

Am I the only person who works in a place with very half arsed requirements? I have to take a guess a lot of the time as to what management actually want form what they say. Code quality matters 6 moths later when they say it isn't working (as expected) and I have to check what the code actually is doing.

Mostly because it's almost impossible to know this by just reviewing the code, without having to run it

If you believe that, then either

- The code is badly designed/obfuscated enough that its behaviour is not easy to see (something that should definitely raise a concern at a code review)

- The reviewer doesn't know enough to figure out what the code should do (which in and of itself is a huge WTF.)

In some sense, testing (especially the black-box type) is "dumb". It cannot discover failures at edge cases which could be seen from reading the code and thinking about its behaviour, something that should definitely happen at a code review. It is useful as a sanity check, but cannot replace reasoned inspection.

Agreed, it's very hard to catch errors in feature behaviour when code reviewing, if you check out the code and start testing then you are doing something else besides code review, sometimes is good for a big changeset, but most of the times the code review should be enough and the original author must have tested his code.

On the other hand, I have seen so much (tested) production code, that didn't do what was intended, but no one noticed in practice. Or stuff like people/documentation said it does a specific thing the code shows it didn't.