2015-November Scripting Games Wrap-Up

The November puzzle was all about cleaning up someone else's code, and came from community member Tim Curwick. Want to share your own puzzle for a future Games? Drop a line to admin@ via email!

Official Answer

Sometimes things break. Then we have to dive into the code and figure out what it does. Figuring out someone else’s code (or code you wrote long ago and long forgot) is a very important skill.

And sometimes, when you review old code, you find ample opportunities for improvement. Over time, scripts change, their environment changes, and PowerShell itself changes. Sometimes parts of a script have become irrelevant or inefficient or just weren’t very well done because you didn’t quite know what you were doing back then.

That was our challenge this month. We have a script that is part of a larger ecosystem that I had to understand. And then with that understanding, I had the opportunity to improve it. Because this script is part of a larger system, we can’t change in the input or the output, but we can change anything in between.

So let’s take a look.

param([string]$VMNameStr)
$VMs=@()
$VMNames=@()
if($VMNameStr.indexof(",") -gt 0)
{
$Trace="Found Comma..."
$VMs=$VMNameStr -split "," | %{$_.trim()}
$trace+="Length = $($VMs.length)"
$trace+=$VMs -is [array]
for($i=0;$i -lt $VMs.length;$i++){
if($VMs[$i] -gt ""){
set-variable -Name ("vmname" + $i) -value $VMs[$i]
$VMNames+=$VMs[$i]
}
}
}
else{$VMName0=$VMNameStr;$VMNames=$VMName0}
$VMNames

The $Trace variable is set, but never used. It appears to have been used at one time for some debug logging, but no longer serves any function. The Set-Variable line creates variables that are never used. These lines can be eliminated. (A few puzze entries preserved the creation of some of the unused variables. In fairness, if the script were dot sourced, these variables might be relevant to the calling script. But none of those entries (as of this writing) were consistent in this choice; they arbitrarily preserved some but not all of the variables.)

The only thing the script sends to the output stream is $VMNames. $VMNames is first defined as an array. If the input parameter $VMNameStr is a comma delimited list, string(s) are added to the array. If $VMNameStr is not a comma delimited list, $VMNames is redefined as the original input string.

In the latter case, I would probably consider it a bug to not trim that string, but if nothing but white space were input, the output would change from white space to an empty string if I trimmed it, and I don’t know if that would break the error handling in the calling script, or some other script in the system. So I am going to preserve that "bug" for compatibility.

If the input has a comma, it is split at the commas, and each resulting string is trimmed. Each trimmed result that is a non-empty string is added to the array. (A common mistake amongst the entries was to filter out the empty strings before trimming them. This would fail to filter out strings with just white space.)

The original scripter checked for commas in the input string using the .IndexOf() method. We are going to use .Contains() as a more concise, faster, more readable option.

If ( $VMNameStr.Contains( "," ) ) {

To split a string, sometimes the -Split operator is preferable, but since we are going to be chaining a bunch of string methods, we’ll use the .Split() string method.

If ( $VMNameStr.Contains( "," ) ) { $VMNameStr.Split(',')

In newer versions of PowerShell, we can then trim the white space from all of the elements of the array simultaneously.

If ( $VMNameStr.Contains( "," ) ) { $VMNameStr.Split(',').Trim()

And then we filter out any empty results with PowerShell’s .Where{} method. The .Where method is always faster than Where-Object, because there is no pipeline overhead, and it is more concise and readable without resorting to aliases and positional parameters. If a string is converted to Boolean, non-empty strings convert to $True and empty strings convert to $False, so we don’t have to compare the string to anything to determine if they are empty or not.

If ( $VMNameStr.Contains( "," ) ) { $VMNameStr.Split(',').Trim().Where{ $_ } }

And if the input string does not contain commas, we just return the input string.

If ( $VMNameStr.Contains( "," ) ) { $VMNameStr.Split(',').Trim().Where{ $_ } } Else { $VMNameStr }

Then we add a comment to make it easier to figure out next time, and the final result looks like this.

For PowerShell 4.0 and up.

Param( [string]$VMNameStr )
#  If the input string is comma delimited
#    Split it into an array, trim the results, and filter out empty results
#  Note: if the input string in not comma delimited, it will be returned as is, untrimmed
If ( $VMNameStr.Contains( "," ) ) { $VMNameStr.Split(',').Trim().Where{ $_ } } Else { $VMNameStr }

For PowerShell 2.0 or 3.0 compatibility, we use ForEach-Object and Where-Object commands instead of .ForEach() and .Where{} methods.

Param( [string]$VMNameStr )
# If the input string is comma delimited
# Split it into an array, trim the results, and filter out empty results
# Note: if the input string in not comma delimited, it will be returned as is, untrimmed
If ( $VMNameStr.Contains( "," ) ) { $VMNameStr.Split(',') | ForEach { $_.Trim() } | Where { $_ } } Else { $VMNameStr }

Community Highlights

If you haven't browsed through some of the submissions from November, you definitely ought to. A lot of folks took real care to clean up the code and explain what they did - it's an incredible useful exercise, since modding someone else's code is perhaps one of the more difficult and frequent exercises you'll do in the real world.

User brianburke did what is perhaps the most thorough writeup, cleaning up and reformatting the original as well as providing a corrected bit of code. User Jake Ballard may be the "winner" from the month's Games, as he also provided a complete "testing" script to run against the corrected code - great idea! User Keithlr2 also has a notable entry, where he discusses the assumptions he made and the tests he wrote for the code. Again, that's such a good way to see the process involved here, that he may also be a "winner" this month.

Posted in:
About the Author

PowerShell.org Announcer

This is the official account for PowerShell.org and sponsor announcements.

One Comment

  1. Ok, I get the answer.
    But shouldn't the article also remind that storing coma separated values into a string, to later split and trim, isn't real Powershell?
    I feel like a penguin...