-1

I'm trying to create a cleaner code for my script. I'm first defining the array, which will be used to check if the user account has been already queried. Currently the code has two nested if statements, which I want to combine into one if possible. Here is my code:

function addUser {
    [cmdletbinding()]
    Param(
        [Parameter(Mandatory=$true)]
        [String]$userName
        ,
        [Parameter(Mandatory=$true)]
        [String]$userDomain
        ,
        [Parameter(Mandatory=$true)]
        [String]$userAccount
    )
    Begin {}
    Process {
        $adAttribute = (Get-ADUser -Identity $userName -Server ($userDomain + "<FQDN HERE>") -Properties "<AD ATTRIBUTE NAME HERE>")."<AD ATTRIBUTE NAME HERE>"
        
        Write-Output (
            [pscustomobject]@{
                userName = $userName
                userDomain = $userDomain
                userAccount = $userAccount
                <AD ATTRIBUTE NAME HERE> = $adAttribute
            }
        )
    }
    End {}
            
}
    
$userList = @()
Get-Content $sourceFile | % {
    <SOME CODE HERE>
    if ($userList.Length -eq 0) {
        $userList += addUser -userName $userName -userDomain $userDomain -userAccount $userAccount
    }
    else {
        if (-not $userList.userAccount.Contains($userAccount)){
            $userList += addUser -userName $userName -userDomain $userDomain -userAccount $userAccount
        }
    }
    $userAdAttribute = $userList.Where({$_.userAccount -eq $userAccount})."<AD ATTRIBUTE NAME HERE>"
}
<SOME CODE HERE>

As you can see the following code is repetative:

$userList += <SOME FUNCTION CODE HERE>

but I cannot figure out how to make it cleaner, as in the beginning the array is empty and I cannot validate it before adding, so I'm wondering if anyone can share a tip?

TylerH
  • 20,799
  • 66
  • 75
  • 101
kal3js
  • 94
  • 1
  • 7
  • 1
    Does this answer your question? [Why should I avoid using the increase assignment operator (+=) to create a collection](https://stackoverflow.com/questions/60708578/why-should-i-avoid-using-the-increase-assignment-operator-to-create-a-colle) – Santiago Squarzon Mar 09 '22 at 16:06

1 Answers1

3

I think you could do better using a Hashtable or HashSet for that instead of adding to an array with += (which needs to rebuild the entire array in memory on every addition)

$userList = @{}
$userAccount = <SOME CODE HERE>

# add the $userAccount you get from the code block above to the hash
if (-not $userList.Contains($userAccount)) {
    # this user has not been processed, so do that here
    <CODE TO PROCESS USER>
    # next add to the Hashtable
    $userList[$userAccount] = $true  # the value is not important
}

Using a HashSet:

# on PowerShell below 5.0 use
# $userList = New-Object -TypeName 'System.Collections.Generic.HashSet[String]' -ArgumentList ([StringComparer]::InvariantCultureIgnoreCase)
$userList = [System.Collections.Generic.HashSet[string]]::new([StringComparer]::InvariantCultureIgnoreCase)
$userAccount = <SOME CODE HERE>

# add the $userAccount you get from the code block above to the hash
if (-not $userList.Contains($userAccount)) {
    # this user has not been processed, so do that here
    <CODE TO PROCESS USER>
    # next add to the Hashtable
    [void]$userList.Add($userAccount)
}

So, if it is not just about a string holding the userAccount (SamAccountName I gather), but about objects with multiple properties, you then need to use the Hashtable approach:

$userList   = @{}
$userObject = <THE CODE THAT CALLS FUNCTION 'addUser' AND RETURNS THE USER OBJECT>

# add the $userObject you get from the code block above to the hash
if (-not $userList.Contains($userObject.userAccount)) {
    # this user has not been processed, so do that here
    <CODE TO PROCESS USER>
    # next add to the Hashtable
    $userList[$userObject.userAccount] = $userObject  # the value is the userAccount Object
}
Theo
  • 57,719
  • 8
  • 24
  • 41
  • Thanks, for the advise. I just tried it with hash table and it did not work. Here is my code: `$userList = @{} $userAccount = "US\username" if (-not $userList.userAccount.Contains($userAccount)) {Write-Host "Adding user account"}` and the system threw and error: `You cannot call a method on a null-valued expression. At line:1 char:4 + if (-not $userList.userAccount.Contains($userAccount)) {Write-Host "Add ... + CategoryInfo : InvalidOperation: (:) [], RuntimeException + FullyQualifiedErrorId : InvokeMethodOnNull` – kal3js Mar 10 '22 at 07:58
  • Same thing with HashSet – kal3js Mar 10 '22 at 08:07
  • @kal3js Yes, because you are switching the `$userList` and `$UserAccount` variables. The `$userList` is the Hashtable or HashSet where you collect user account names. There **is no** `$userList.userAccount`. The Hash only takes the user account name, so to test if it is already in there, you do `$userList.Contains($userAccount)` – Theo Mar 10 '22 at 10:22
  • Yes, this will work. I made a mistake, by not explaining the process of my code correctly. The array holds objects, which have multiple properties, i.e. userName, userDomain, userAccount and additional AD attribute. I have not found any convenient way to use hastable for this purpose, hence using array with list of objects. – kal3js Mar 10 '22 at 13:47
  • I have updated the main question so that you can see the process (more or less) – kal3js Mar 10 '22 at 14:02
  • @kal3js Sorry for the late reply.. I have edited my answer to show how you can use a Hashtable as fast lookup table while it still contains the full user object. – Theo Mar 16 '22 at 15:53