I also include junior developers who don't know much about the application or environment in code reviews. It's a good way for them to learn the code base and see what type of defects the senior developers point out.

+1 to that one. Code Review should be Peer Review - it is not about a Senior reviewing a Junior it is about a developer reviewing the work of another one. The junior's questions may actually be as useful as advices (notably by forcing you to explain your choices).

I like to have one junior and one senior developer along with the software architect review another developers code. This way you get proper code review from the senior, and the junior learns and get exposure to other peoples code. But three people reviewing is actually already a too large team for review imo.

Hi, OP here. Interesting list. I think I would approve most of your points, but never worked (or created) such a "proper" environments in my various teams. Could you share the kind of checklist you use? Or give an idea of the kind of "checks" you have there?

Thanks!

See six steps to success section of my write-up of Dr. Holzmann's talk on how JPL builds reliable software. http://www.verticalsysadmin.com/making_robust_software/

The clever idea is to make your coding standard mechanically verifiable.

Thanks, nice list, summarize a good part of the best practices in a short form. Will quote.

Excellent, you are most welcome.

I need to write a shorter version, to make the data more broadly accessible.

Hi, OP. Thanks for the interesting post. Off the top of my head:

1. Variables declared or assigned, but never referenced again

2. Improper variable scope (local/global)

3. Reserved words (by language, framework, industry, app) used as variables

4. Improperly reused variable names

5. Improperly named variables (1 or 2 characters, contained in another variable name)

6. Too difficult to understand variable or function names

7. Function names must be Verb-Adjective-Noun

8. Variable names must be Adjective-Noun

9. Improper data type

10. Repeated (in one or more programs) code for which reusable code should be written. (Any future maintenance must only be done in one place.)

11. Fresh code rolled instead of using existing reusable code meant for that purpose.

12. Partially complete data base updates

13. Improper data base locking

14. Non-standard UI widgets

15. Version control problems (didn't start with current production version)

16. Not all changes documented with ticket#

17. Case/switch statements without fall-through clause (case 1)

18. Improperly structured if...then statements

19. if...then statements too long (according to published standard)

20. Improper indenting/spacing (according to published standard)

21. Comments don't match code

22. Comments inaccurate

23. Calling/using obsolete/deprecated code

24. Not backward compatible

25. Obviously works under only certain conditions (which will be caught in testing)

26. New code inserted in wrong place (if...then, case, function, etc.)

27. Obviously missing data base updates

28. Obviously missing API calls

29. What else?

Great list (especially "off the top of your head"). Thanks a lot for sharing. Several of those can actually be implemented in an automated tool (styles, short variables, spacing, unused variables even). What's your take on automated tools? (Disclaimer: my company built one - pulreview.com).

Are code reviews by non-programmers actually common? How can that possibly work?

I've seen 'code reviews' that were in fact functionality checks. So it's more like a 'check if this code works'.

And I can tell you: a web form capable of SQL-injections is still 'working code'. So it gets a positive 'code review'.

Sounds like there's no code being reviewed, how can it be called "code review"?

That's not a code review; that's either QA or UAT.

Bots that scan code for syntax/quality. While these are useful they should not qualify as a complete code review.