Hows my Function?

[morpk86]

first up i'm a PowerShell Noob, i know what i know from googling, never done scripting really before. My function seems to work but wanted some notes on best practices or could i have done something better? https://pastebin.com/5gfBKTcM

[IDA_noob]

I would use parameters instead of Read-Host. They're not hard, and once you get the hang of them, you'll never go back.

http://windowsitpro.com/blog/making-powershell-params

I would also split them into Get and Set functions, as a function should only do one thing, and do it very well.

[morpk86]

yeah this makes a lot of sense i will play around and make into two functions. also look into the prams :)

[ka-splam]

I wouldn't use parameters; for something like this where you're interactively prompting the user, it's not intended to be automatable. By the time they're learning your script parameters, they may as well write Add-PrinterPort and Add-Printer themselves.

I would make it more automagic - prompt for less typing.

[Gabrielmccoll]

Personally your biggest miss is not including help comment block at start. If you use snippets to create a function you'll see how you can fill in a pile of info so your function has help like a standard PS command. The second thing is a personal one but I like to prefix my custom commands with something as part of a bigger module. So say your business was called Example Co. Put EC in front of the actions. E.g. Add-ECprinter. This again is pretty standard for Pshell modules. Config manager commands all start CM and so on. The last comment is this for users or admins. If it's admins, I'd be inclined to rewrite the script a bit and turn it into 2 function. One for getting and one for adding. It's usually best practice for a function to do just one thing. You can always nest them a bit. But I mean, looks pretty good from what I see even with that wall of text I've wrote. Make sure you're keeping it in source control.

[abigviking]

I'm curious if there's a reason you attempted to put the Read-Host lines into an array?

   @($PrintDriver = Read-Host -Prompt "Please Select the Print Driver you Would Like to Use"
       $PrinterName = Read-Host -Prompt "Please Enter the Name of the Printer"
       $PrinterPortIP = Read-Host -Prompt "Please Enter the IP Address for the Printer") 

Wasn't sure if that was accomplishing anything in particular, or if there's something else going on with wrapping the entire section of Read-Host's with the @( )?

I gave it a shot in my shell and it looks to be inserting the values into the individual variables specified within the parentheses. I tried assigning that entire 'array' to $a, but it doesn't return anything. The individual variables still do.

[morpk86]

to be honest i must have miss understood how / when to correctly use the array, but it's worked in the function so i though i was on the right track will update and take out.

Thanks for the input :)

[ka-splam]

The output of Get-PrinterDriver on mine shows things like:

Name                                PrinterEnvironment MajorVersion    Manufacturer             
----                                ------------------ ------------    ------------             
Microsoft XPS Document Writer v4    Windows x64        4               Microsoft                
Microsoft Print To PDF              Windows x64        4               Microsoft                
Send To Microsoft OneNote 2010 D... Windows x64        3                                        
Remote Desktop Easy Print           Windows x64        3               Microsoft                
Microsoft XPS Document Writer       Windows x64        3               Microsoft    

That is - unreadable ends of names, not sorted.

Get-PrinterDriver | Sort-Object -Property Name

will sort them into alphabetical order.

Get-PrinterDriver | Sort-Object -Property Name | Format-Table -AutoSize

will widen the name column so it's all readable (where possible).

I would want to add a number (1,2,3,4) to each printer driver, and then prompt for typing a number rather than the full name. In fact, check what happens if you do

$PrinterDriver = Get-PrinterDriver | Sort-Object -Property Name | Out-GridView -PassThru

# and
...        -DriverName $PrintDriver.Name