4 ms·

I've been criticized as being too harsh in code review before, and I'll try this. I'm not sure how to apply it in all cases though. Frequently the issues I see

by yoklov 12y ago

I've been criticized as being too harsh in code review before, and I'll try this. I'm not sure how to apply it in all cases though. Frequently the issues I see are coding standard violations... use of exceptions, bad use of virtual or function pointers, ... How do you say 'we dont use throw here' as a question?

jeanie_conner 12y ago
You can ask a leading question or series of questions about what bad thing(s) might happen down the road as a result of what they did - which could hopefully lead to them having a greater understanding of the 'why' of the best practice, and less likely to repeat it in the future.
- yoklov 12y ago
  Well, the biggest why of exceptions is that we want to be able to compile with exceptions turned off. And there are lots of reasons why not to use virtual functions or function pointers, but aside from 'it kills performance if used too much', they're fairly subtle. But really, the issue is typically with something like this the solution is 'rewrite it', which is brutal no matter how you phrase it. Something like a period of pair programming/mentorship might ease the transition (we only do code reviews on new hires), but it's tough to justify. If they really need dynamic dispatch, we prefer using an enum because of serialization, efficient sorting by type, code readability (this is debatable), etc., but really even this isn't what we want, because most of the time we don't need dynamic dispatch. Doing this wrong in one place is unlikely to be a huge deal, but if we can't trust them to write code that performs well... it's bad.
ZoFreX 12y ago
You could leave a top-level comment: "Have you checked all this code matches our coding standards?" If they say yes you could start getting more specific - "does this bit of code match our coding standards for exceptions?"
- yoklov 12y ago
  The first is probably the right approach, as the second comes off as unnecessarily condescending to me.

by yoklov 12y ago

jeanie_conner 12y ago

You can ask a leading question or series of questions about what bad thing(s) might happen down the road as a result of what they did - which could hopefully lead to them having a greater understanding of the 'why' of the best practice, and less likely to repeat it in the future.

yoklov 12y ago
Well, the biggest why of exceptions is that we want to be able to compile with exceptions turned off. And there are lots of reasons why not to use virtual functions or function pointers, but aside from 'it kills performance if used too much', they're fairly subtle. But really, the issue is typically with something like this the solution is 'rewrite it', which is brutal no matter how you phrase it. Something like a period of pair programming/mentorship might ease the transition (we only do code reviews on new hires), but it's tough to justify. If they really need dynamic dispatch, we prefer using an enum because of serialization, efficient sorting by type, code readability (this is debatable), etc., but really even this isn't what we want, because most of the time we don't need dynamic dispatch. Doing this wrong in one place is unlikely to be a huge deal, but if we can't trust them to write code that performs well... it's bad.

ZoFreX 12y ago

You could leave a top-level comment: "Have you checked all this code matches our coding standards?" If they say yes you could start getting more specific - "does this bit of code match our coding standards for exceptions?"

yoklov 12y ago
The first is probably the right approach, as the second comes off as unnecessarily condescending to me.