Peer review

Hi all,

I’d like you opinion on the following function i created to log to SharePoint, can you understand everything i written and dose it make sense?

First of all, that param block needs to be laid out better. It’s impossible to read. Usually I do the following:

    [Alias('ShortName')] # optional
    [ValidateX('Validation')] # optional, recommended where possible; look up 'parameter validation' for more on that
    [ParamType] # e.g., string, object, hashtable, etc., etc.

    [Alias('ShortName')] # optional
    [ValidateX('Validation')] # optional, recommended where possible; look up 'parameter validation' for more on that
    [ParamType] # e.g., string, object, hashtable, etc., etc.

Grouping the parameters prevents the (very useful) extended attributes from cluttering too much, and the additional whitespace helps to be able to easily understand them.

Additionally, if you have parameters that are useable from any parameter set, you can simply… not specify either parameter set on them. Parameters that aren’t declared one way or the other will automatically be available from both sets (i.e., for most of those parameters, you can drop both parameter set declarations).

Finally (for parameters, heh), you want to declare the Mandatory property in the [Parameter()] attribute for those properties that need to be there for the function to work. If there are no properties the function can live without, they all need to be flagged as Mandatory.

The entire SplitBy-Capital function (which should be named differently; check ‘Get-Verb’ for PS’s approved verbs list) can be replaced with a simple case sensitive regex:

"StringSort" -creplace "(?<!^)([A-Z])",' $1'

(EDIT: remove the space between < and ! – this forum's HTML escaping is inserting a space for some reason where it shouldn't be)
To break it down for you:

'(?<! # negative lookbehind (don't match anything preceded by...)
^ # the start of the string (removes need for .Trim())
) # end lookbehind
([A-Z]) # group 1, match any single character A-Z in caps (needs -creplace to be case sensitive)
',' $1' # replace with a space, plus the first matched group (i.e., the same letter we matched on)

You also use += on arrays a lot. Instead of that, just assign a variable to the output of the loop. It’s quicker and less fragile:

#instead of this
$OutputString = @()
foreach ($a in ($InputString.ToCharArray()))
  if ([char]::IsUpper($a))
    $OutputString += " " + $a
    $OutputString += $a
return ($OutputString -Join '').Trim()
#try this:
$Output = -join (foreach ($a in ($InputString.ToCharArray()))
  if ([char]::IsUpper($a))
    " $a"
return $Output

Yes, powershell has weird shortcuts. Assigning a variable to the output of a loop is significantly quicker than adding to arrays. It uses PS’s native background pipeline/output stream logic, which afaik utilises ArrayList.

The other thing I’d recommend if you find yourself building arrays piece by piece is to use List[T], the generic resizeable list. See, the issue with arrays is that they’re a fixed size item. YOu can change what’s in them, but their size is constant. To add new items and expand the array, PS has to rebuild the array from scratch at the new size, and then populate it behind the scenes. Adds a ton of processing that doesn’t need to be there.

Generic list example:

$List = [System.Collections.Generic.List[string]]::new()

Use [object] or [PSobject] as the internal type if you want it to hold multiple types of items, otherwise using a specific type tends to lend itself to some performance improvements.

There’s more I could point out, but those are the main ones… OH and one last one. Don’t do this:

Write-NTSPLog -OverviewList "Automation Overview" -LogList "Automation Logs" -ProcessName $((Get-PSCallStack)[0].Command) `
							  -CurrentStatus InProgress -Result Success -ProcessOwner "CIMS Team" -ID $LoggingID -SkipCheck -Confirm:$false `
							  -ProcessObjectCount $(($ADGroupMembersUPN).Count) -ProcessObjectCurrentItem $AddingLoggingCount -TargetObject $($ADUser.UserPrincipalName) `
							  -CommandRun "Ran command : 'Set-MsolUser -UserPrincipalName $($ADUser.UserPrincipalName) -UsageLocation $($UsageLocation) -ErrorAction Stop'" -Step "Setting Region"

Yeah, my copy paste may or may not have butchered it, but backticks are VERY fragile. If you add a single space after them, the whole thing falls apart completely… among many other reasons. For long function calls, you should use splatting with hashtables. See ‘Get-Help about_Splatting’ for a good example on that.

Hi Joel,

Firstly, thank you for the suggestions. I’ve already made some of the changes, I like the function suggestions, it’s a good way to do it, we both get the same result but yours seems more elegant :slight_smile:

For the lists, that’s a good idea too, I guess this would speed up adding to the list thus the completion of the function, i’ll have a look at that. I used the hashtable type in the function with my string array as PowerShell has a habit of flattening them.

I’ve tidied up the param block as suggested as i can see what you mean, readability wise, i guess i was just focused on the other logic i did’t consider how others might find it difficult to read.

As for the back-ticks (`) I agree can be fragile, I guess it depends on who’s implementing it, the splatting idea is good, i’ll actually add that, I used splatting in the function itself so am familiar with the concept.

Thanks for taking the time to review this.


I can do another pass a bit later if you like, but I just noticed you’re using [System.String] as a parameter type – [string] is perfectly fine. :slight_smile:

I just like to be explicit :smiley:

Explicit is fine, but you don’t lend that same methodology to your other parameters. :slight_smile:

I tend to find that above all else, consistency in style and formatting tends to make it easiest to debug a lengthy script or function later!