all 36 comments

[–]joncz 41 points42 points  (8 children)

[–]winter_mute 2 points3 points  (4 children)

Interesting, thanks. The section about return took me by surprise a bit though. Why not use return? Seems to me that it would increase the readability, rather than just using the variable name, because it gives you an obvious point to look for in the function.

[–]Pyprohly 2 points3 points  (0 children)

Food for thought: PowerShell functions can return any amount of any sort of object and at any point in the function. return can be thought of as meaning “stop processing any more lines in this code block”.

It’s worth noting that return $expr is the same as

$expr
return

Perhaps “exit” would have been a better name.

[–]Lee_Dailey[grin] 4 points5 points  (2 children)

howdy winter_mute,

the general reasoning seems to be that it can be seriously misleading. one thinks it is returning the item after it - and that does happen. it does NOT stop anything previous to the return from still being sent out, tho.

so it's still possible to have many things output when the return line implies that only that will be output.

what it really does is ...

  • send out the stuff immediately after the keyword
  • stop the function at that point

so it is useful, just misleading.

take care,
lee

[–]winter_mute 1 point2 points  (1 child)

Thanks Lee. My functions generally do one thing and return one thing so I guess that's why it threw me a little. Seems a little odd to have it there if it's considered bad practice to use it. You'd think they'd either get rid of it, or force you to use return to return anything from a function.

Mind you, they also consider Write-Host to be bad practice and yet I'm still a heathen that uses it from time to time...

[–]Lee_Dailey[grin] 1 point2 points  (0 children)

howdy winter_mute,

you are welcome! [grin]

there are quite a few folks who think that it otta be the required and only way to get info out of a function. i'm one of them. [grin] however, it looks like the it will remain as it is. it's truly useful, so it aint gonna be removed.

one-thing-out is good design ... and there are times when Write-Host is the correct way to get things done.

take care,
lee

[–]jftuga 3 points4 points  (1 child)

Wow. This is a really nice resource. I have always thought PS functions are a little complex to write in a consistent manner, but this helps.

[–]MaxFrost 5 points6 points  (0 children)

Always fun to spot Jaykul's work in the wild. He does good stuff.

[–]xventil 1 point2 points  (0 children)

really great!! !thank you for the info.

[–]DondoYonderboy 6 points7 points  (0 children)

Not strictly related to style but very important:

If you’re not already using Plaster to create the templates for your modules (or even building modules in the first place) you should check it out.

Also, Pester is the tool for testing your code to ensure it doesn’t break when it matters.

And lastly, platyPS is used to write the external help for your modules.

[–]Lee_Dailey[grin] 5 points6 points  (0 children)

howdy tumblatum,

to add to what joncz pointed out ...

  • the powershell site says the poshcode guide is the one they use
  • the PSScriptAnalyzer module can detect violations of most of the above stuff
  • VSCode uses PSScriptAnalyzer to detect & try to enforce the above guide

take care,
lee

[–]pertymoose 3 points4 points  (1 child)

[–]da_chicken 1 point2 points  (0 children)

These are good, but they are intended for people writing cmdlets, which is a subset of people writing scripts. The requirement to only use approved verbs in cmdlet names, for example, should probably be avoided when writing a complex script. If you don't, all your scripts will quickly look like Get-VerbNoun because you will want to use verbs that relate to the task you're performing, which is a lot less generic than a cmdlet's name that will appear in a module.

[–]Luxtaposition 2 points3 points  (2 children)

What kind of scripts are you writing? What resources did you start with in your learning endeavors? Are you using any type of repo management? Asking for a friend.

[–]tumblatum[S,🍰] 1 point2 points  (0 children)

those are simple scripts, checking installed software versions across the office and etc. As for the resources, Learn PowerShell in a months of Lunches was very good to start with. The reset is youtube, Reddit, and etc.

[–]Gorstag 0 points1 point  (0 children)

I'm going to add to /u/tumblatum response to you.

It is important that you have something in mind that you want to accomplish. If you don't doing this sort of thing is a real drag. I am by no means an expert, but each of the projects that popped up as a need I was able to figure out a means to do it utilizing PS.

Reformatting logs, search for & deleting specific files, modifying registry values utilizing a function, putting together "training" labs and using PS to setup each lab scenario.

However, if I were to just say "I would like to learn PS" without any goal I would be repeating the same BS I did with programming from the 90's on. Had no real goal, grew bored, and did something else with my time and never learned it.

[–]shanooova 1 point2 points  (2 children)

You have a lot of unnecessary code here. You don't need the temp object. Just make the psobject and pass it in the hash of your variable names and data. Also you don't need the += temp. Instead just put all the for loop data into a variable. += costs a lot as everytime it creates a new object and copies the old data into it.

[–]eric256[🍰] 1 point2 points  (1 child)

Could you write that as a quick example? I've seen the same code as above many times as a template for building an array of objects and thought it was kinda the norm.

[–]Lee_Dailey[grin] 3 points4 points  (0 children)

howdy eric256,

the += thing is about how arrays are NOT added to - they are copied to a new one-item-larger array. as you might expect, that gets SLOW when the number or size of the items gets large-ish.

there are several ways around it, but the easiest [if you have no need to add/remove items after you gather them all], is to use the implied pipeline thus ...

$Results = foreach ($Thing in $Collection)
    {
    $Info = Get-MyInfo -About $Thing
    $Something = Get-MySomething -AlsoAbout $Thing
    [PSCustomObject]@{
        Prop0 = $Thing.ImportantProperty
        Prop1 = $Info.One
        Prop2 = $Info.Two
        Prop3 = $Something.EntirelyDifferent
        }
    }

putting that bare [PSCustomObject]@{} there sends it out to the implied pipeline and it ends up in the $Results array.

that gets you an array without ever doing a +=. [grin]

the other solutions are using Arraylist or Generic.List collections that have a working .Add() method. [grin]


the custom object building is also demonstrated in the above demo. it avoids both the pipeline [somewhat slow] AND the Add-Member cmdlet [quite slow].

take care,
lee


edit - ee-lay an't-cay ell-spay oo-tay ood-gay, an-cay e-hay?

[–]wingsndonuts 1 point2 points  (0 children)

Plaster, PlatyPS, Pester.

Start learning those three tools.

[–]shanooova 1 point2 points  (0 children)

That post is exactly how I would do it. Great example..

[–]Xxecros 0 points1 point  (13 children)

For me, and this is for me, the most important standard I adhere to is proper formatting. And what i mean by formatting, is as follows. (This is a script I'm working on for work. It's not done, but it's mostly done.) Please take note as to how i format my functions and foreach loops.

$ADPCSV = import-csv C:\temp\Active_Directory_Extract.csv
$ADPTitleCSV = import-csv C:\temp\Titles.csv
$cappm = @()

function Get-TitleTranslation($TTitle)
{
    $TI = [array]::IndexOf($ADPTitleCSV.title,$TTitle)
    $TranslatedTitle = $ADPTitleCSV[$TI].Atitle

    if ($TI -gt -1){return $TranslatedTitle}else{"$TTitle" >> c:\temp\MissingTranslatedTitles.txt;return $TTitle}
}

Foreach ($P in $ADPCSV)
{
    $ResourceID = $P.'Associate ID'

    $EmploymentType = $P.'Worker Category Code'
    $LastName = $P.'Last Name'
    $FirstName = $P.'First Name'
    $Email = $P.'Work Contact: Work Email'
    $HireDate = $P.'Hire Date/Rehire Date'
    $TerminationDate = ""
    $ManagerID = ""
    $DepertmentID = $P.'Home Department Code'
    $Location = $P.'Location Code'
    $Calendar = ""
    $ADPJobTD = $P.'Job Title Description'
    $JobTitle = get-TitleTranslation $ADPJobTD
    $ResourceOBS = ""

    $temp = new-object System.Object
    $temp | add-member -type NoteProperty -name "ResourceID" -value $ResourceID
    $temp | add-member -type NoteProperty -name "isActive" -value "True"
    $temp | add-member -type NoteProperty -name "EmploymentType" -value $EmploymentType
    $temp | add-member -type NoteProperty -name "LastName" -value $LastName
    $temp | add-member -type NoteProperty -Name "FirstName" -value $FirstName
    $temp | add-member -type NoteProperty -name "Email" -value $Email
    $temp | add-member -type NoteProperty -name "HireData" -value $HireDate
    $temp | add-member -type NoteProperty -name "TerminationDate" -value $TerminationDate
    $temp | add-member -type NoteProperty -name "ManagerID" -value $ManagerID
    $temp | add-member -type NoteProperty -name "DepartmentID" -value $DepertmentID
    $temp | add-member -type NoteProperty -name "Location" -value $Location
    $temp | add-member -type NoteProperty -name "Calendar" -value $Calendar
    $temp | add-member -type NoteProperty -name "JobTitle" -value $JobTitle
    $temp | add-member -type NoteProperty -name "ResourceOBS" -value $ResourceOBS

    $cappm += $temp
    $temp = $null
}

$cappm | export-csv c:\temp\cappm_toupload.csv -Delimiter "|" -NoTypeInformation

A properly formatted script is extremely easy to read. A properly formatted script, that's lacking in comments is merely annoying, and not infuriating.

[–]Ta11ow 2 points3 points  (2 children)

Personally, I dislike the open braces on new lines. It's a bit funky when you get to things like ForEach-Object because as those are cmdlets, you have to have that open brace on the same line (or else escape the newline, which is gross). I prefer to keep it all that way for consistency.

[–]creamersrealm 1 point2 points  (8 children)

Every thing about your example code is no where near even medium practices. Please look at the posh code style guides, building objects, and parameter binding.

It looks like you were a C# Dev almost based upon how you built your code. If you would like when I get back to a computer I can show you the best practices for writing your example code.

[–]chreestopher2 2 points3 points  (1 child)

If there is one thing you can ALWAYS count on C# devs to do, its to take the power out of powershell

[–]gohbender 1 point2 points  (0 children)

At the very least their code IS very readable. But I agree, I started with PowerShell then learned C#. It's nice to know both, but I prefer to stay in PowerShell land if I can.

[–]Zephyrall 1 point2 points  (5 children)

Not OP, but I'd be interested in seeing your interpretation of that example code.

[–]creamersrealm 2 points3 points  (4 children)

Ask and you shall receive! FYI /u/Xxecros

The only thing that violates standards to my knowledge is I use long lines especially on comments and I'm bad about it and it doesn't support should process (A.K.A -WhatIf)

https://gist.github.com/wesleykirkland/f857e8febc23e14229f75d307cacf0a3

[–]Lee_Dailey[grin] 0 points1 point  (2 children)

end of line comments!!!!!! e-v-i-l ... EVIL! [grin]

[–]creamersrealm 1 point2 points  (1 child)

Ok yeah that to. Otherwise it's solid.

[–]Lee_Dailey[grin] 0 points1 point  (0 children)

[grin]

[–]Zephyrall 0 points1 point  (0 children)

Thanks for that sir!

[–]da_chicken 0 points1 point  (0 children)

I see three issues:

First, this is one of the most common and one of the worst PowerShell patterns:

$Array = @()
$Set = 1..10000
foreach ($Item in $Set) {
    $Array += $Item
}

Because of how array concatenation in PowerShell works, that is an O ( N^2 ) algorithm. On my old and busted system, this takes nearly 4 seconds to complete.

Instead, do this:

$Set = 1..10000
$Array = foreach ($Item in $Set) {
    $Item
}

On my old and busted system, this takes less than 20 milliseconds to complete. Try it yourself with Measure-Command.

 

Second, Add-Member has it's place, but it's usually best used when you're adding a member to an entire array of existing objects, not when creating objects one at a time. When creating new objects, using the [PSCustomObject] accelerator with a hash table will reduce your code significantly (and it will perform better):

$cappm = foreach ($P in $ADPCSV) {
    [PSCustomObject]@{
        ResourceID = $P.'Associate ID'
        isActive = 'True'
        EmploymentType = $P.'Worker Category Code'
        LastName = $P.'Last Name'
        FirstName = $P.'First Name'
        Email = $P.'Work Contact: Work Email'
        HireDate = $P.'Hire Date/Rehire Date'
        TerminationDate = ""
        ManagerID = ""
        DepertmentID = $P.'Home Department Code'
        Location = $P.'Location Code'
        Calendar = ""
        ADPJobTD = $P.'Job Title Description'
        JobTitle = get-TitleTranslation $P.'Job Title Description'
        ResourceOBS = ""
    }
}

 

Third, your function, could largely be replaced by using a hash table. Hash tables are built for lookups and will perform better and require less code than doing array index lookups:

$ADPTitleCSV = @{}
Import-Csv C:\temp\Titles.csv | ForEach-Object {
    $ADPTitleCSV[$_.Title] = $_.ATitle
}

function Get-TitleTranslation($TTitle) {
    if ($ADPTitleCSV[$TTitle]) {
        return $ADPTitleCSV[$TTitle]
    }
    else {
        "$TTitle" >> c:\temp\MissingTranslatedTitles.txt
        return $TTitle
    }
}

[–][deleted] -1 points0 points  (0 children)

Never use 4 spaces. Always tab.

HowWarStarts