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.
- The most important thing is whether the script solves the problem at hand. In my book, if it does that, the rest is extra.
- 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.
- 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.
- Does the function or script accomplish the intended task?
- Is there any comment-based help including examples?
- Is the function or script “advanced” using [CmdletBinding()]?
- Are risk mitigation parameters supported if appropriate?
- Does the code use aliases for cmdlets?
- Does the script or function follow the Verb-Noun Convention?
- Is the verb in the approved list?
- Is it the correct verb?
- Are the noun(s) consistent?
- Is the function or script in a module with a discoverable name?
- Do the Parameters have specified types?
- Are the parameters named appropriately?
- Are parameters set as mandatory when appropriate?
- Is any declarative parameter validation used?
- Are arrays allowed when appropriate?
- Is pipeline input allowed and implemented properly?
- Are switch parameters used to flag options?
- Do parameters have appropriate default values?
- Are “use cases” divided into ParameterSets?
- Are named parameters used instead of positional parameters?
- Is the output in the form of objects?
- Is output produced in the PROCESS block if possible?
- Are format cmdlets used?
- Is write-verbose used to supply user-directed output messages?
- Is write-warning or write-error used for problem messages?
- Is write-debug used for developer-directed output messages?
- Are filtering operations as far to the left in pipelines as possible?
- Are pipelines used appropriately in place of sequential logic?
- Are pipelines overly used (extremely long pipelines?)
- Are comments used to explain logic (and not statements)?
- Is commented-out code present?
- Are try/catch/finally used for terminating errors?
- Are errors signaled with write-error?
- Are terminating errors forced with –ErrorAction STOP?
- Are console apps checked using $LastExitCode
- Do write-error calls use the -TargetObject parameter?
Let me know what you think.
Should have it posted to github for revisions tomorrow sometime.