5 ms·

Proper code review provides much more benefit than cost and is indispensible for quality software. Improper code review is a waste of time (or worse). Proper c

by edw519 13y ago

Proper code review provides much more benefit than cost and is indispensible for quality software. Improper code review is a waste of time (or worse). Proper code review is done: - by another programmer - by someone with some knowledge of the application - by someone with some knowledge of the environment - against some code standard - against standard requirements (APIs, database, etc.) - with a checklist - uniformly (no matter who is doing it) - until it's right Improper code review is done: - by a non-programmer - fully automated - as a "rubber stamp" on an approval checklist - against the standard du jour - according to the whims of the reviewer - with a deadline for the next step - as a replacement for testing

jchendy 13y ago
Are code reviews by non-programmers actually common? How can that possibly work?
- ohwp 13y ago
  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'.
  - masklinn 13y ago
    Sounds like there's no code being reviewed, how can it be called "code review"?
  - trevor_oke 13y ago
    That's not a code review; that's either QA or UAT.
- ryanto 13y ago
  Bots that scan code for syntax/quality. While these are useful they should not qualify as a complete code review.
nradov 13y ago
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.
- martinvanaken 13y ago
  +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).
- kabouseng 13y ago
  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.
martinvanaken 13y ago
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!
- atsaloli 13y ago
  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/ http://www.verticalsysadmin.com/making_robust_software/ The clever idea is to make your coding standard mechanically verifiable.
  - martinvanaken 13y ago
    Thanks, nice list, summarize a good part of the best practices in a short form. Will quote.
    - atsaloli 13y ago
      Excellent, you are most welcome. I need to write a shorter version, to make the data more broadly accessible.
- edw519 13y ago
  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?

by edw519 13y ago

jchendy 13y ago

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

ohwp 13y ago
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'.
- masklinn 13y ago
  Sounds like there's no code being reviewed, how can it be called "code review"?
- trevor_oke 13y ago
  That's not a code review; that's either QA or UAT.
ryanto 13y ago
Bots that scan code for syntax/quality. While these are useful they should not qualify as a complete code review.

nradov 13y ago

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.

martinvanaken 13y ago
+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).
kabouseng 13y ago
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.

martinvanaken 13y ago

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!

atsaloli 13y ago
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/ http://www.verticalsysadmin.com/making_robust_software/ The clever idea is to make your coding standard mechanically verifiable.
- martinvanaken 13y ago
  Thanks, nice list, summarize a good part of the best practices in a short form. Will quote.
  - atsaloli 13y ago
    Excellent, you are most welcome. I need to write a shorter version, to make the data more broadly accessible.
edw519 13y ago
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?