PowerShell Code Review Guidelines

I get asked to look at other people’s PowerShell code a lot at work, and I really enjoy it.  I also find myself looking at my “old” code from several years ago (back to 2007!) and think…there’s a lot of work to be done.

To that end, I’ve compiled a list of “PowerShell code review guidelines” to help keep the ideas flowing.

Before I show them, though, I have some ground rules to share.  BTW…I use function, script, and code somewhat interchangably, so please don’t get confused by that.

  1. The most important thing is whether the script solves the problem at hand.  In my book, if it does that, the rest is extra.
  2. Remember that everyone is at a different place in the “path to true PowerShell enlightenment”.  Don’t beat people over the head because they’re not using DSC, or Remoting, or Workflows, or whatever your favorite thing is.  That doesn’t mean you don’t mention those options, but the focus should be on the code in front of you.
  3. You can’t and shouldn’t try to solve all of the problems at once.  This goes right along with #2 above.  If the script is full of Write-Host output, and uses Read-Host in loops to validate parameters, you probably should deal with those and not worry so much that they haven’t used a lot of pipelines.

In my mind, a code review is an opportunity to help a scripter use best practices more consistently.  It is an opportunity to help them write more flexible, more maintainable, and more reliable code.

Most importantly, it’s not a humiliation session in how much better you are at PowerShell.  If you use them in this way, don’t be surprised if you don’t get a lot of return customers.

General

  1. Does the function or script accomplish the intended task?
  2. Is there any comment-based help including examples?
  3. Is the function or script “advanced” using [CmdletBinding()]?
  4. Are risk mitigation parameters supported if appropriate?
  5. Does the code use aliases for cmdlets?
  6. Does the script or function follow the Verb-Noun Convention?
  7. Is the verb in the approved list?
  8. Is it the correct verb?
  9. Are the noun(s) consistent?
  10. Is the function or script in a module with a discoverable name?
  11. Parameters

  12. Do the Parameters have specified types?
  13. Are the parameters named appropriately?
  14. Are parameters set as mandatory when appropriate?
  15. Is any declarative parameter validation used?
  16. Are arrays allowed when appropriate?
  17. Is pipeline input allowed and implemented properly?
  18. Are switch parameters used to flag options?
  19. Do parameters have appropriate default values?
  20. Are “use cases” divided into ParameterSets?
  21. Are named parameters used instead of positional parameters?
  22. Output

  23. Is the output in the form of objects?
  24. Is output produced in the PROCESS block if possible?
  25. Are format cmdlets used?
  26. Is write-verbose used to supply user-directed output messages?
  27. Is write-warning or write-error used for problem messages?
  28. Is write-debug used for developer-directed output messages?
  29. Flow

  30. Are filtering operations as far to the left in pipelines as possible?
  31. Are pipelines used appropriately in place of sequential logic?
  32. Are pipelines overly used (extremely long pipelines?)
  33. Comments

  34. Are comments used to explain logic (and not statements)?
  35. Is commented-out code present?
  36. Error Handling

  37. Are try/catch/finally used for terminating errors?
  38. Are errors signaled with write-error?
  39. Are terminating errors forced with –ErrorAction STOP?
  40. Are console apps checked using $LastExitCode
  41. Do write-error calls use the -TargetObject parameter?

Let me know what you think.

Should have it posted to github for revisions tomorrow sometime.

–Mike

2 Comments

  1. A few things:

    * I think that scripts or functions within scripts should have some sort of a `trap` block if they are lacking a `try/catch/finally` pattern for some reason. This gives them at least some level of exception coverage.

    * You might want to consider having scripts be unit-tested with something like Pester for additional QC.

    * A common pattern that I’ve seen in scripts is the “function that does everything”. These functions tend to be several hundred lines that really should be modularized.

    * Having comments that explain everything in the code can be dangerous since it is very easy to change functionality without updating those comments afterwards. Comments that explain WHY are, IMO, more useful than comments that explain WHAT. If I can’t figure out what’s happening in the code from reading it, then that usually means that it might be too complicated.

Comments are closed.