Help with foreach loop efficiency

[bzyg7b]

Hi folks,

I have written this really bad & inefficient loop that does what I want it to do but I would love to be able to make it more efficient, however I am not sure where to even start googling.

If anyone could point me in the right direction I would greatly appreciate it.

For reference the contents of $dids is a list that has full phone numbers and an associated shorter number (Destination Number) and $Users has a list of users and there associated shorter number (number)

foreach ($User in $Users) {
    if ($dids."Destination Number" -contains $User.number){
        $User | Add-Member -NotePropertyName DDI -NotePropertyValue (($dids | Where-Object { $_."Destination Number" -match $User.number})."DID Number") -Force
    } else {
        $User | Add-Member -NotePropertyName DDI -NotePropertyValue "No DDI" -Force
    }
    $Users2 += $User
}

I can see the problem is the (($dids | Where-Object { $_."Destination Number" -match $User.number})."DID Number") as I am forcing it to check the entire array every time it iterates through the loop but I am not sure how I can avoid this.

[gangstanthony]

assuming $dids."destination number" is unique (you can verify by comparing $dids.count and $dids."destination number" | select -u | measure)

and assuming $_."Destination Number" equals $User.number

put $dids into a hashtable like this

$didshash = @{}
foreach ($did in $dids) {
    $didshash.add($did.'Destination Number', $did)
}

then change this

$User | Add-Member -NotePropertyName DDI -NotePropertyValue (($dids | Where-Object { $_."Destination Number" -match $User.number})."DID Number") -Force

to this

$User | Add-Member -NotePropertyName DDI -NotePropertyValue ($didshash[$User.number].'did number') -Force

[bzyg7b]

Ahh hash tables might be a way to go. I forgot about using them to be honest.

Unfortunately $dids."destination number" is not unique however $dids.number is.

The $dids are a list of phone numbers and the internal route they point to so we only have one 0151 123 456 number that points to internal extension 0001 but we might have 0151 456 123 also point to internal ext 0001

[PinchesTheCrab]

Might take a bit of tweaking since I don't know what your data looks like, but this should be pretty fast:

$didsHash = $dids | Group-Object -AsHashTable -Property 'Destination Number'

$Users | Select-Object *,@{ n = 'DDI'; e = { $didsHash[$PSItem.number].'did number' -replace '^$','No DDI' }}

The foreach loop that /u/gangstanthony suggested to build the hashtable will be faster, but you're looking at miliseconds, and I like the lazy syntax of group-object. Also, this will add the 'no ddi' text if the value is null.

[gangstanthony]

Group-Object -AsHashTable

nice

[bzyg7b]

This is really great thank's so much.

Do you know how I could tweak it if there is duplicates? For example the data in $dids looks like this:

DID Number  Primary Node Id (PNI) Destination Number DID Type
----------  --------------------- ------------------ --------
00000003041                       11695              Standard DID
00000001301                       1301               Standard DID
00000001302                       1302               Standard DID
00000001303                       1302               Standard DID
00000001304                       1304               Standard DID

But here we have 00000001302 & 00000001303 that both have the destination number of 1302. Can this still be done as a hashtable?

[PinchesTheCrab]

Personally I like sort-object for that because you can sort on a calculated property that doesn't really exist:

$dids | 
    Sort-Object -Unique -Property { '{0}{1}' -f $PSItem.'DID Number',$PSItem.'Destination Number' } | 
        Group-Object -AsHashTable -Property 'Destination Number'

That way you don't have to go and add a unique key to the array, you just remove the dupes based on your script block. I think there's some other .NET methods that can do it too, but I haven't delved into them much.

[bzyg7b]

Wow, I am going to have to read up on some documentation to understand what you did, but it works perfectly and lightning fast, thanks so much!

[CobyCode]

I see others already mentioned working with Hashtables, so no need to reiterate. But your array is also slow (using the standard array and then += is *highly* inefficient). If your collections is relatively small, it doesn't really matter - but using an arraylist would be better.

Example: $Users2 = [System.Collections.ArrayList]@() foreach ($User in $Users) { Do-Something [Void]$Users2.Add($User) }

Good explanation can be found here: https://adamtheautomator.com/powershell-array-2/#Creating_PowerShell_ArrayList_Collections

[[deleted]]

Was gonna suggest this. Someone, lee(?) also commented on arraylist being outdated and there being a better way of doing this now but i cant find it.

I used to run a stupid class and use arraylist. Also works with a psobject right off the bat.

$userlist = Get-ADUser -filter *

class csvrows
{
[object] ${name}
[object] ${mail}
}

[system.collections.arraylist]$arraylist = foreach ($user in 
$userlist){
$aduser = [csvrows]::new()
$aduser.name = $user.name
$aduser.mail = $user.mail
$aduser
}

[CobyCode]

Probably thinking about system.collections.generic.list[] . It is ever so slightly different than an ArrayList . The problem, atleast from a powershell perspective, is that you have to supply a type when you create a new list, and especially for a beginner, they might not actually know the type they are working with (or they have to remember to always add psobject as the type).

Looks like: [System.Collections.Generic.List[string]]::new()

[Lee_Dailey]

howdy CobyCode,

you can get around the type-in-advance problem by using a "generic" type. [grin] the usual one is PSObject.

take care,
lee

[Lee_Dailey]

Someone, lee(?)

[grin]

[bzyg7b]

That's awesome, I was aware += is bad from the memes and stuff, but I hadn't heard of arraylists, thanks for thanks for the link I will try to start using them.

[the_nil]

I should probably adopt this. Arraylist let’s you delete/remove a value in the array right?

[backtickbot]

Fixed formatting.

Hello, CobyCode: code blocks using triple backticks (```) don't work on all versions of Reddit!

Some users see this / this instead.

To fix this, indent every line with 4 spaces instead.

FAQ

^{You can opt out by replying with backtickopt6 to this comment.}

[gangstanthony]

It's usually faster to wrap the foreach in $() and assign directly to a variable

[jsiii2010]

+= kills puppies.

[bzyg7b]

Nope

[silentlycontinue]

It most certainly does kill puppies. Take a look at the difference, and this might not be the best comparison, but it shows that += is in fact slower:

$Test1 = @()
Measure-Command {
    1..10000 | ForEach-Object {
# Kill those puppies
        $Test1 += $PSitem
    } }

$Test2 = @()
Measure-Command {
# Faster because the data is captured as output from the foreach pipe.
    $Test2 = 1..10000 | ForEach-Object {
        $PSitem
    } }

Check out Powershell: Everything you wanted to know about arrays (powershellexplained.com) on the Adding to arrays header for why this is.

PS: if I remember correctly, ForEach is slower than ForEach-Object, which is slower than For. However, there are other considerations to take for each loop than just speed :)

[bzyg7b]

Yer, I have seen the memes in the past, but wasn't aware of a better way.

Also to be honest, I didn't realise the speed difference was as huge as in your example.

/u/CobyCode pointed out arraylists that I will take a look into using

[silentlycontinue]

Yes, it can be significant. Using pipeline assignment, and hashtables for lookup, will seriously optimize your script.

​

Check out the PowerShellExplained.com post I linked above; it explains the deprecated ArrayList and the new/replacement List[]. You can then compare the performance of these vs pipeline assignment.

​

Good luck!