How strict should code reviews be

Review your fellow developer code – The best way to improve your code

In the start of my career, when I was naive, I considered code review a time waster. Because, something my code got more than a week to be reviewed by my team-lead. But, now I have realized how important it is.

When you are on a project with strict timelines and you are handling pressure from each side, even an experienced developer can do a rookie mistake, and in the influence of pressure, sometime you cannot identify the mistake even after go-through many times.

To maintain organization standards and to have functionally accurate and clean code, every piece of code should be reviewed by other developers. It will be helpful for new developer, as when new project assign to him, he got organization maintained code so it become easy for him understand and work faster.

Code Review Check List

Following is the check list that I follow during my code review. This list shortens or widen slightly according to nature of any project

When you are on a project with strict timelines and you are handling pressure from each side, even an experienced developer can do a rookie mistake, and in the influence of pressure, sometime you cannot identify the mistake even after go-through many times.

  • General

    • Code should be easily extendable and manageable.
    • Code is in working condition without any error.
    • Code is doing its intended purpose.
    • Methods in a class support its functionality.
    • The routines in each source file shall have a common purpose or have interrelated functionality.
    • Each line will not be more than 80 characters long.
    • Don’t overload any function, unless there is a genuine reason.
    • No uncalled, unneeded, unreachable code should be in source code file. Small piece of useless code can be kept but in commented form.
    • Code should be easily extendable and manageable.
  • Conventions

    • Use camalCasing for variables.
    • Use PascalCasing for class, functions and namespaces.
    • Avoid liters and constants values as much as possible.
    • Every class should be in any namespace or sub namespace. No global namespace.
    • No tab-based indenting. Use 3 spaces relatively to indent any line.
    • All variable must be meaning full. One letter variables never help. Boolean should not like flag or check. They must have some meaning like $isUserAdmin or $recordFound.
    • File name and class name must be meaningful and reflect the purpose of creation.
    • User calendar control instead of allowing user to write date in a textbox.
    • When continuing lines of code on new lines, they are broken after a comma or an operator.
    • Wrapped lines of code are aligned with the beginning of the expression at the same level on the previous line.
    • Program statements are limited to one per line.
    • Parentheses are used to remove operator precedence ambiguity and to improve code readability.
  • Comments

    • Top of any file, class and function
      • Author.
      • Purpose.
      • Creation date.
      • Document version in which class introduced.
        • Input parameters, their type and purpose.
        • Output values, its type and purpose.
      • For class, there is a section of revision history, which contains the date, purpose and function names change.
    • Comments must be provided for complex peace of code. If any design pattern, algorithm, framework or library (like MVC) implemented, then it must be specified with the reason of usage.
    • Put comment on any loop or conditional clause if it is doing something important and complex.
    • Avoid spelling and grammatical mistakes in comments.
  • Exception Handling

    • Catch all errors and do proper handling.
    • If there is an unexpected error that you can't recover from, then log details, alerts the administrator, clean the system.
    • Meaningful error messages are used.
  • Modularization

    • Divide code into classes, functions and namespaces.
    • Do not write very long function. If any function is more than 80 executable lines, then probably it should be divided.
    • No duplication of code.
    • If you find you are indenting very deeply, you most likely aren't using subroutines when you should.
    • Do not reinvent functions and classes. If any code can be replaced by any library, replace it.
  • Security

    • Apply validation on input fields. Do not allow NULL on mandatory value, or value with wrong datatype. Check inputs from SQL injection.
    • Implement security by doing precautions for Cross-site request forgery and cross site scripting where needed.
    • Create multiple users with different privileges (optional).
    • Apply captcha to all forms which can be used by attackers like creation account, comments, contact us, quote request form, etc.
    • Encrypt/Decrypt password and critical configuration values.