Book Review: Windows Powershell 2.0 Best Practices, Ed Wilson

Last week I watched a TV program called Michelin Stars, The Madness of Perfection it talked about the pressure Chefs put themselves under - sometimes tragically so. The presenter was a restaurant critic and in talking to one of his fellow critics they talked about a problem – not exclusively a Michelin one – where every plate was a supreme accomplishment and yet they didn’t want to go to those restaurants.

That’s quite a good analogy for how I look at Ed Wilson’s “PowerShell 2.0 best practices.”. It doesn’t deserve a ranting torrent directed against it, because there are plenty of people who will get a lot from it; but I’m not one of them and it left me frustrated.

My first frustration is the title. A book on best practices should be a slim volume, “Always do this, Try to do that, Avoid so-and-so”. The list of things for PowerShell can’t be enormous, yet this book is big enough to stun an Ox. At over 700 pages it’s the biggest book on PowerShell I’ve met so far. It is padded out with tables and lists which really don’t need to be there - quite early on I came on a 3 page table which lists all the properties of WIN32_processManagement object where it adds no value.  A lot of the rest is tips and good ideas – useful stuff, but not helping me to tell good practice from bad.  For example two chapters - 65 pages - cover Active directory using ADSI – PowerShell 2.0 introduced cmdlets but these get the briefest of mentions. What I wanted was something to say either – “You’re mad if you don’t ensure you can use the AD cmdlets” or “In these situations you’re better using ADSI because …” .

The second frustration is with the likely reader: if you’re writing for people who know PowerShell and telling them about best practice, I think it’s inevitable they will try to pick holes in the code examples. It would seem that Ed writes scripts to do a task rather than add commands to the PowerShell environment – which is most of  what I do – so he and I will have different approaches in places. Although he talks about the importance of making scripts readable, he will produce something with many short lines, where you can’t just understand a section on it’s own. It’s probably easiest to show an example. Given a need to read some data from a file and process it, the book shows something which is structured like this

 Function TestPath {...}            
Function SetConnectionString   {...}            
Function ReadData  {...}            
Function CloseAdoConnection   {...}                        
$FilePath = "C:\BestPractices\excel.xls"             

The problem with this is you have to start at the bottom with TestPath and then go up to where the function is defined. Then back to bottom to see the next line is SetConnectionString , and then up to where that is defined. Since “up” takes you to a different page / screenful of code, it’s bad for readability.  TestPath does a check and exits the script if the file isn’t found – here’s the code: 

 Function TestPath($FilePath)             
 If(Test-Path -path $FilePath)             
    if($verbose) { "$filePath found" }             
  } #end if             
   Write-Host -foregroundcolor red "$filePath not found."             
  } #end else             
} #end TestPath            

Notice that a “verbose” response doesn’t go to the verbose channel but becomes output of the function which is a way to introduce some interesting bugs,  I would get rid of the function and write

 $FilePath = "C:\BestPractices\excel.xls"             
If   ( Test-Path -path $FilePath)             
     { write-verbose "$filepath found"             
ELSE { Write-Host -foregroundcolor red "$filePath not found."}             

Notice there is a CloseAdoConnection but not an open connection ?  That’s because SetConnectionString opens the connection: Like the other functions it doesn’t return a result – the connection which it opens is left as a variable for ReadData (which doesn’t just read data in this example but creates AD objects) and CloseAdoConnection to use.  Here’s what appears in the book.

 Function SetConnectionString()             
 $strFileName = $FilePath             
 $strSheetName = 'Excel$'             
 $strProvider = "Provider=Microsoft.Jet.OLEDB.4.0"             
 $strDataSource = "Data Source = $strFileName"             
 $strExtend = "Extended Properties=Excel 8.0"             
 $strQuery = "Select * from [$strSheetName]"             
} #end SetConnectionString            
Function NewAdoConnection()             
 $Script:objConn = New-Object System.Data.OleDb.OleDbConnection(`
 $sqlCommand = New-Object System.Data.OleDb.OleDbCommand($strQuery)             
 $sqlCommand.Connection = $objConn             
 $Script:DataReader = $sqlCommand.ExecuteReader()             
} #end NewAdoConnection  

This seems to going out of its way to avoid passing parameters and returning results: but that means you can’t see when SetConnectionString is called that it relies on the $FilePath variable. The functions don’t follow proper naming conventions meaning they can only really be used inside their script (not in a module). Although I’ve seen Ed protest that he is a PowerShell scripter now, but the way the functions are declared with the redundant () after them suggests his VB habits die hard – PowerShell does allow you to declare parameters in brackets, but the convention is to use the Param statement. The VB convention of putting STR in front of strings is followed in some places and not others; but naming isn’t the thing I find horrible with variables and scopes here. SetConnectionString() sets a bunch of variables within its own scope, and then calls NewAdoConnection() – which inherits that scope and relies on those variables. If you decided to move NewAdoConnection() between SetConnectionString and ReadData it would fail.  NewAdoConnection()  doesn’t return a result but sets a variable for something else in the script to use, but it has to use the script scope to do it.  The job could be done in 6 lines rather than 18

 $objConn = New-Object System.Data.OleDb.OleDbConnection(`
   "Provider=Microsoft.Jet.OLEDB.4.0;Data Source=" + $strFileName +"; Extended Properties=Excel 8.0" )             
$sqlCommand = New-Object System.Data.OleDb.OleDbCommand("Select * from [Excel$]")             
$sqlCommand.Connection = $objConn             
$DataReader = $sqlCommand.ExecuteReader()

There is a danger of getting into “PowerShell Golf” – chasing the fewest [key]strokes to reach the target – which can produce something intricate but impossible to understand and maintain. But here I think the shorter version is clearer and easier.  There’s a case to be made for keeping New-AdoDataReader as a function, but it should be reusable, and this isn’t. It should take the parts of the connection string as parameters and return a result, and this doesn’t

I don’t like the habit of never writing a constant, but always assigning it to a variable first and then using the variable (it’s even worse here with $filePath being put in a new variable $strFileName ) and again you can write code which uses no variables but at the price of making it incomprehensible. In this case if you looking through the ReadData function trying to work out what it does, you have where find where another variable was set, and to find out how it was set you have to find where other variables are set. 

This “variablitis” copes up in many places , and Ed has written that the constraints of the 75 character page width in a book can cause this, but I spotted this example

 $os = [environment]::osversion             
$os | Get-Member -MemberType property             

Writing it properly would only take 60 characters.

 [environment]::osversion | Get-Member -MemberType property

Once I started seeing these things I kept tripping over them. For example, Powershell supports for loops in the style of C like this .

 For($i = 0 ;$i -le 10; $i++)   {doSomethingWith  $i}

But people who work in PowerShell would more commonly write 

 1..10 | ForEach {doSomethingWith  $_}

Again one can argue this “PowerShell golf”, but is it easier to read “1..10” or “Set i to 0, while I is less than 10, increment i” . I choked when I saw it here.

 Function Get-MoreHelp             
 # .help Get-MoreHelp Get-Command Get-Process             
 For($i = 0 ;$i -le $args.count ; $i++)             
  Get-Help $args[$i] -full |   more             
 } #end for             
} #end Get-MoreHelp             

There isn’t really any justification for writing 

 For($i = 0 ;$i -le $args.count ; $i++)  { doSomethingWith $Array[$i]}

Instead of

 $Array | foreach { doSomethingWith $_ }            

But the bigger issue for me using args instead of named parameters is generally discouraged and doing so to get the user passes a list of items as separate un-named parameters rather than doing what all PowerShell’s built in cmdlets do and writing a comma separated list, well it’s just wrong.

Ultimately this was what stopped me getting value from the book – there were so many departures from accepted good practice that I lost faith in it as a book on best practice. If MS Press or Ed himself had used a title like “Improve your PowerShell” I would have had a lot less of a problem with it. There is good content, and lots of contributions from people with proven expertise. But you can’t blindly order copies for a team who work with PowerShell and tell them to use it as a Style guide. On the other hand if you’re in a bookshop and see it on the shelf I’d recommend having a thumb through it (I look up something I’ve been trying to do and see what the explanation is like) – it might suit you better than it suited me.

Windows PowerShell 2.0 Best Practices By Ed Wilson is published by Microsoft press ISBN 978-0-7356-2646-1 List price £47.49