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
Param([switch]$verbose)
Function TestPath {...}
Function SetConnectionString {...}
Function ReadData {...}
Function CloseAdoConnection {...}
$FilePath = "C:\BestPractices\excel.xls"
TestPath($FilePath)
SetConnectionString
ReadData
CloseAdoConnection
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
ELSE
{
Write-Host -foregroundcolor red "$filePath not found."
Exit
} #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"
SetConnectionString
ReadData
CloseAdoConnection
}
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]"
NewAdoConnection
} #end SetConnectionString
Function NewAdoConnection()
{
$Script:objConn = New-Object System.Data.OleDb.OleDbConnection(`
"$strProvider;$strDataSource;$strExtend")
$sqlCommand = New-Object System.Data.OleDb.OleDbCommand($strQuery)
$sqlCommand.Connection = $objConn
$Script:objConn.open()
$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
$objConn.open()
$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
Comments
- Anonymous
January 01, 2003
How refreshing to read an informed critical review of a recent major PowerShell title. I have enjoyed Ed's book greatly as some points are made clear but shall now enjoy it even more knowing that there are different perspectives on many of the points made. I too found many of the examples overly obscure and was surprised to see [ref] used in GetVersion .. not having encountered it before. (A reflection of my inexperience ?) Thanks to Ed for the book and to James for the review.