Skip to content

Instantly share code, notes, and snippets.

Embed
What would you like to do?
PHP Code Review Guidelines

Make sure these boxes are checked before submitting/approving the PR

General

  • The code works
  • The code is easy to understand
  • Follows coding conventions
  • Names are simple and if possible short
  • Names are spelt correctly
  • Names contain units where applicable
  • There are no usages of magic numbers
  • No hard coded constants that could possibly change in the future
  • All variables are in the smallest scope possible
  • There is no commented out code
  • There is no dead code (inaccessible at Runtime)
  • No code that can be replaced with library functions
  • Variables are not accidentally used with null values
  • Variables are immutable where possible
  • Code is not repeated or duplicated
  • No complex/long boolean expressions
  • No negatively named boolean variables
  • No empty blocks of code
  • Ideal data structures are used
  • Constructors do not accept null/none values
  • Catch clauses are fine grained and catch specific exceptions
  • Exceptions are not eaten if caught, unless explicitly documented otherwise
  • Files/Sockets and other resources are properly closed even when an exception occurs in using them
  • null is not returned from any method
  • == operator and === (and its inverse !==) are not mixed up
  • Floating point numbers are not compared for equality
  • Loops have a set length and correct termination conditions
  • Blocks of code inside loops are as small as possible
  • No methods with boolean parameters
  • No object exists longer than necessary
  • No memory leaks
  • Code is unit testable
  • Test cases are written wherever possible
  • Methods return early without compromising code readability
  • Performance is considered
  • Loop iteration and off by one are taken care of

Architecture

  • Design patterns if used are correctly applied
  • Law of Demeter is not violated
  • A class should have only a single responsibility (i.e. only one potential change in the software's specification should be able to affect the specification of the class)
  • Classes, modules, functions, etc. should be open for extension, but closed for modification
  • Objects in a program should be replaceable with instances of their subtypes without altering the correctness of that program
  • Many client-specific interfaces are better than one general-purpose interface.
  • Depend upon Abstractions. Do not depend upon concretions.

API

  • APIs and other public contracts check input values and fail fast
  • API checks for correct oauth scope / user permissions
  • Any API change should be reflected in the API documentation
  • APIs return correct status codes in responses

Logging

  • Logging should be easily discoverable
  • Required logs are present
  • Frivolous logs are absent
  • Debugging code is absent
  • No print_r, var_dump or similar calls exist
  • No stack traces are printed

Documentation

  • Comments should indicate WHY rather that WHAT the code is doing
  • All methods are commented in clear language.
  • Comments exist and describe rationale or reasons for decisions in code
  • All public methods/interfaces/contracts are commented describing usage
  • All edge cases are described in comments
  • All unusual behaviour or edge case handling is commented
  • Data structures and units of measurement are explained

Security

  • All data inputs are checked (for the correct type, length/size, format, and range)
  • Invalid parameter values handled such that exceptions are not thrown
  • No sensitive information is logged or visible in a stacktrace
@ahsyam

This comment has been minimized.

Copy link

@ahsyam ahsyam commented Nov 29, 2017

Great

@Vilisag

This comment has been minimized.

Copy link

@Vilisag Vilisag commented Mar 9, 2018

Great

@tristanbailey

This comment has been minimized.

Copy link

@tristanbailey tristanbailey commented Apr 22, 2018

Nice list. Could be maybe in simple to more detail order as some items likely to be rarer

@karthiiivgn

This comment has been minimized.

Copy link

@karthiiivgn karthiiivgn commented Jun 8, 2018

Good! but i need some more explanation for each of them

@samadfcibd

This comment has been minimized.

Copy link

@samadfcibd samadfcibd commented Mar 3, 2019

Good list. It's very helpful. Thanks

@dresh

This comment has been minimized.

Copy link

@dresh dresh commented Mar 6, 2019

Nice list. Could be maybe in simple to more detail order as some items likely to be rarer

yep

@ryanwhowe

This comment has been minimized.

Copy link

@ryanwhowe ryanwhowe commented May 1, 2019

Your Law of Demeter link is broken ... I used this one personally https://en.wikipedia.org/wiki/Law_of_Demeter

@cliche23

This comment has been minimized.

Copy link

@cliche23 cliche23 commented May 20, 2019

"There is an else block for every if clause even if it is empty"

why?

@karlodelarosa

This comment has been minimized.

Copy link

@karlodelarosa karlodelarosa commented Oct 24, 2019

"There is an else block for every if clause even if it is empty"

why?

Also my question

@Saneesh

This comment has been minimized.

Copy link

@Saneesh Saneesh commented Jan 21, 2020

"There is an else block for every if clause even if it is empty"
OR
"No empty blocks of code"

@nerandell

This comment has been minimized.

Copy link
Owner Author

@nerandell nerandell commented Jan 21, 2020

It has been some time since I took a look at this list. After thinking about it again, I agree that There is an else block for every if clause even if it is empty does not make a lot of sense and I myself never use it in practice. The idea behind it was that having an else block forces you to think if you should handle the else case. I however do not think that keeping that block of empty code in production adds any value. I have removed it from the list.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment