Code Review: My first Powershell function

13 Dec 2023 🔖 powershell tips
💬 EN

I’m lucky to have enthusiastic, experienced PowerShell programmers at work. When I showed off my first attempt at writing a “hello world” function, they gave me an hour of their time to review and correct it.

Context

I’m working on writing an Azure DevOps Pipeline that can build Git-tracked PowerShell source code into “modules” and publish them into an internal corporate NuGet repository.

(Right now, developers are running build-and-publish commands on their own computers.)

Before I can write a CI/CD pipeline, I need to write some source code in PowerShell, so that I’ll have something to build.

Before

Here’s how my file tree started:

(root directory)
┣ .cicd/
┃ ┗ build-hello-world.yml
┣ src/
┃ ┗ all_my_modules/
┃   ┗ HelloWorld/
┃ ┃   ┣ Private/
┃ ┃ ┃ ┃ ┗ New-CatFact.ps1
┃ ┃   ┣ Public/
┃ ┃ ┃ ┃ ┣ Write-CatFact.ps1
┃ ┃ ┃ ┃ ┗ Write-Greeting.ps1
┃ ┃   ┣ HelloWorld.psd1
┃ ┃   ┗ README.md
┣ .gitignore
┗ README.md

And here are the contents of /src/all_my_modules/HelloWorld/Public/Write-Greeting.ps1 when I started:

function Write-Greeting {

    <#
    .Synopsis
        Greets you by name.
    .DESCRIPTION
        Says hello to you by name.
    .EXAMPLE
        Write-Greeting -Name 'Katie';

        This will write "Hello, Katie." to the success stream.
    .EXAMPLE
        'Katie', 'Abdul' | Write-Greeting;

        This will write "Hello, Katie." on one line and "Hello, Abdul." on another line of the success stream.
    .PARAMETER Name
        Name you would like to greet.  (Default is "world".)  Also accepts string arrays and greets them one at a time.
    .NOTES
        Author: Katie Kodes
        Date: 2024-01-11
    #>

    [CmdletBinding()]
    Param (
        [Parameter(
            Mandatory = $true, 
            ValueFromPipeline, 
            Position = 1, 
            HelpMessage = "The name whom you would like to greet"
        )]
        [ValidateNotNullOrWhiteSpace()]
        [string[]]$Name = 'World'
    ) # end Param

    Begin {
        $ReturnValue = [System.Collections.ArrayList]::new();
    } # end BEGIN

    Process {
        ForEach ($nm in $Name) {
            $ReturnValue.Add("Hello, $nm.") | Out-Null;
        }
    } # end PROCESS

    End {
        return $ReturnValue;
    } # end END

} #Write-Greeting

Things I did correctly

My colleagues were happy to see that:

1. I had used CmdletBinding(), [Parameter()] configurations, and [Validate...()] parameter configurations.
2. My function contained a BEGIN{...}, PROCESS{...}, and END block accompanying a ValueFromPipeline = $true parameter configuration.
3. My function was preceded by thorough comments.

Changes we made

Do not combine parameter default values with mandatory

The first thing my colleagues pointed out is that I shouldn’t do both of these at the same time:

1. Set the $Name parameter to be mandatory.
2. Give the $Name parameter a default value of “World.”

Indeed, I had noticed that when I tried to run Write-Greeting without a value for $Name, instead of seeing “Hello, World.” in my output, I saw the following error message:

Write-Greeting: Cannot validate argument on parameter 'Name'. The argument is null, empty, or an element of the argument collection contains a null value. Supply a collection that does not contain any null values and then try the command again.

I decided that I wanted a default value rather than an error message, so I simply deleted the line that said “Mandatory = $true.”

Speaking of which … my colleagues also told me that simply writing “Mandatory” is more conventional for newer versions of PowerShell.

Incorrect paramter position number

Position numbers start from 0, not 1. Oops.

Redundant parameter position number

There is no need to explicitly label the first parameter I define as “#0” and the second as “#1,” etc.

PowerShell already assumed that I mean to put them in that order.

Therefore, I can simply delete my Position = ... parameter configuration, unless I intend to guarantee developers using my function that they can trust that “$Name” will always be the first positional parameter of my function until I explicitly release a “major” new version of my function that properly documents that I have made a “breaking change.”

Use named parameters

As a rule of thumb, while my colleagues often invoke functions with positional parameters when experimenting at an interactive console (e.g. Write-Output 'Hello world'), they prefer to be extremely explicit when writing a script that some other programmer may have to read in 4 months (Write-Output -InputObject 'Hello world').

Don’t clog the pipeline

Consider a situation in which I want to be able to run my function in either of these styles:

Write-Greeting -Name @('Katie', 'Abdul') | ForEach-Object ToUpper

@('Katie', 'Abdul') | Write-Greeting | ForEach-Object ToUpper

• [System.Collections.ArrayList]’s .Add() method is not a particularly slow operation.
• Nor is concatenating “Hello” to a name.

However:

1. Imagine that the “Hello, $nm.” command transforming “Katie” into “Hello, Katie.” takes 3 minutes to execute.
2. Also imagine that “Hello, $nm.” sometimes crashes.

If so:

1. It might be annoying to require "Hello, $nm." to run against both Katie and against Abdul before beginning the “ToUpper” transformation of “Hello, Katie.” into “HELLO, KATIE..”
   • I would not know if my script is working corrrectly until it finished processing all of the names.
2. My script might crash while transforming “Abdul” into “Hello, Abdul..”
   • It would be sad to lose all of my work before saving any output.
   • It would be better if I could have already saved “HELLO, KATIE.” to a safe place, like a file, before beginning to work on “Abdul.”

Therefore, my colleagues asked me to remove all use of a collection of strings from my function.

This isn’t idiomatic PowerShell:

Begin {
    $ReturnValue = [System.Collections.ArrayList]::new();
} # end BEGIN

Process {
    ForEach ($nm in $Name) {
        #Write-Host $Name.IndexOf($nm) # 0, 1 either way
        #Start-Sleep -Seconds 3 # 0, wait, 0/1, wait, HELLO K, HELLO A
        $ReturnValue.Add("Hello, $nm.") | Out-Null;
    }
} # end PROCESS

End {
    return $ReturnValue;
} # end END

Nor is this:

Begin {} # end BEGIN

Process {
    $ReturnValue = [System.Collections.ArrayList]::new();
    ForEach ($nm in $Name) {
        #Write-Host $Name.IndexOf($nm) # 0, 0 pipeline; 0, 1 non-pipeline
        #Start-Sleep -Seconds 3 # 0, wait, 0/1, wait, HELLO K, HELLO A
        $ReturnValue.Add("Hello, $nm.") | Out-Null;
    }
    return $ReturnValue;
} # end PROCESS

End {} # end END

Instead, my colleagues wanted to see me write my function like this:

Begin {} # end BEGIN

Process {
    ForEach ($nm in $Name) {
        #Write-Host $Name.IndexOf($nm) # 0, 0 pipeline; 0, 1 non-pipeline
        #Start-Sleep -Seconds 3 # 0, wait, HELLO K, 0/1, wait, HELLO A
        Write-Output -InputObject "Hello, $nm."
    }
} # end PROCESS

End {} # end END

Remove redundant parentheses

It’s not idiomatic PowerShell to use parentheses if you don’t have to.

Trust your space bar.

Remove redundant semicolons

It’s not idiomatic PowerShell to use a semicolon if you don’t have to.

Trust the line breaks.

Name the function New, not Write

After a little bit of debate, we decided that if the purpose of a function is to return data to the output stream with Write-Output, and if it doesn’t fetch it from anywhere special (just some simple transformations against data from the input parameters), it should start with New- instead of Write- or Get-.

After

I renamed some of my other functions after the Write vs. New discussions, so here is my new file tree:

(root directory)
┣ .cicd/
┃ ┗ build-hello-world.yml
┣ src/
┃ ┗ all_my_modules/
┃   ┗ HelloWorld/
┃ ┃   ┣ Private/
┃ ┃ ┃ ┃ ┗ New-CatFactArray.ps1
┃ ┃   ┣ Public/
┃ ┃ ┃ ┃ ┣ New-CatFact.ps1
┃ ┃ ┃ ┃ ┗ New-Greeting.ps1
┃ ┃   ┣ HelloWorld.psd1
┃ ┃   ┗ README.md
┣ .gitignore
┗ README.md

And here is the function I ended up with:

function New-Greeting {

    <#
    .Synopsis
        Greets you by name.
    .DESCRIPTION
        Says hello to you by name.
    .EXAMPLE
        New-Greeting -Name 'Katie'

        This will write "Hello, Katie." to the success stream.
    .EXAMPLE
        'Katie', 'Abdul' | New-Greeting

        This will write "Hello, Katie." on one line and "Hello, Abdul." on another line of the success stream.
    .PARAMETER Name
        Name you would like to greet.  (Default is "world".)  Also accepts string arrays and greets them one at a time.
    .NOTES
        Author: Katie Kodes
        Date: 2024-01-11
    #>

    [CmdletBinding()]
    Param (
        [Parameter(
            ValueFromPipeline, 
            HelpMessage = 'The name whom you would like to greet'
        )]
        [ValidateNotNullOrWhiteSpace()]
        [string[]]$Name = 'World'
    ) # end Param

    Begin {} # end BEGIN

    Process {
        ForEach ($nm in $Name) {
            Write-Output -InputObject "Hello, $nm."
        }
    } # end PROCESS

    End {} # end END

} #New-Greeting