by scottbass at 2012-09-18 05:57:18
Hi,by DonJ at 2012-09-18 13:11:20
I was hoping someone would be willing to do a code review on the attached script. The script works - you don’t have to run it. However, just by reviewing the code, you may see better architectural approaches, or just ways to tighten it up. I’m happy for comments to be terse - Google is my friend
This script is related to viewtopic.php?f=2&t=263. I moved away from using an ArrayList, since I decided I wanted to print the matches as diagnostic information, so I populate a new array rather than delete (throw away) items from the ArrayList approach in the above URL.
I’ve completed the inline help, so I won’t say much more here. I really appreciate any tips you may have to make this better.
Willing; it’ll just be over the weekend.by DonJ at 2012-09-22 07:18:50
Put [CmdletBinding()] before the Param() block. You should do this whenever adding extended parameter attributes, like Mandatory, and it’ll give you your -whatif switch for free - and do so in a way more consistent with the way the rest of the shell works.by scottbass at 2012-09-22 17:25:19
Also, consider not using a -silent switch. Assume silent operation as the default, and output non-silent information using Write-Verbose. [CmdletBinding()] will give you -Verbose for free, and running the command with it will enable the output. Again, just making your script work more consistently with the rest of the shell.
It pains me a little bit to see the output you’ve put together. Because you’re spitting out formatted tables, the potential for re-using or re-directing that output is severely limited, and you’re loading the pipeline with multiple tables (and mixed, nonformatted output). That can create inconsistencies and errors in PowerShell sometimes - it’s just generally a less-desired practice. Unless your script uses the verb “Show” in its name (e.g., Show-Something), it should either output nothing, or output a single kind of object containing all the information you need to display. That can be externally piped to Format-Table (or whatever) upon need. Like, right now, if I wanted that output in HTML, I couldn’t get it through ConvertTo-HTML, which is sad.
Thanks for the comments, much appreciated. I’ll implement all comments.by DonJ at 2012-09-23 07:00:32
However, for my script, how do I “…output a single kind of object containing the information you need to display”? What kind of object: array, hash, custom object (which I need to learn how to create and use)? Does that object contain other objects (i.e. process and file system objects), or just the strings I need to display?
Finally, is there any reason not to use [CmdletBinding()], or should it just be at the top of all my scripts (that use parameters)?
A custom object.
New-Object -Type PSObject -Prop $props
Those values can be anything, including other objects.
And there’s really no downside to [CmdletBinding()] that I know of.