4
\$\begingroup\$

My question is about code efficiency. I recently had to write a Powershell 3.0 script which had the following criteria:

  1. It had to pull all enabled user objects from active directory
  2. It had to pull specific properties about each user object
  3. It had to display what it was doing on screen so it didn't look like it was hanging
  4. It had to export the results to a CSV file

I accomplished that goal after playing around with things for a while, and was happy with the result.

The thing is, whenever my more seasoned colleagues write a script, it takes a lot less time and is generally a much shorter script. While my script works, I would love some pointers and advice about how to accomplish the same thing in less code.

(Side note: I know I have cheated with the the variables for "phonetic" name stuff. I couldn't find the true properties in the time I had).

Import-Module ActiveDirectory

$userCSV = "C:\temp\users.csv" #Set output file path
$UserArray = @() #Set Array to hold information
$Domain = (Get-ADDomain).DistinguishedName #Define Domain details
$UserProperties = Get-ADUser -filter * -SearchBase $Domain #Grab a load of user object information from AD
$Users = $UserProperties.name #Filter information into a list of names

#Loop through each name in the generated list
foreach ($User in $Users) {
    #Lookup each enabled user object in the list and grab all properties
    $userCurrent = Get-ADUser -filter {name -like $user -and Enabled -eq $true} -Properties * | select * 
    #Of all the properties gathered, grab some specific ones
    $Name = $userCurrent.Name
    $Email = $userCurrent.EmailAddress
    $FirstName = $userCurrent.GivenName
    $LastName = $userCurrent.Surname
    $Department = $userCurrent.Department
    $DisplayName = $userCurrent.DisplayName
    $Office = $userCurrent.Office
    $PhoneticCompanyName = $userCurrent.Company
    $PhoneticDisplayName = $userCurrent.DisplayName
    $PhoneticFirstName = $userCurrent.GivenName
    $PhoneticLastName = $userCurrent.Surname
    $PhoneticDepartment = $userCurrent.Department

    #Output process on screen
    Write-Host $Name -foregroundcolor "green" -backgroundcolor "black"
    Write-Host $Email -foregroundcolor "red" -backgroundcolor "black"
    Write-Host $FirstName -foregroundcolor "red" -backgroundcolor "black"
    Write-Host $LastName -foregroundcolor "red" -backgroundcolor "black"
    Write-Host $Department -foregroundcolor "red" -backgroundcolor "black"
    Write-Host $DisplayName -foregroundcolor "red" -backgroundcolor "black"
    Write-Host $Office -foregroundcolor "red" -backgroundcolor "black"
    Write-Host $PhoneticCompanyName -foregroundcolor "red" -backgroundcolor "black"
    Write-Host $PhoneticDisplayName -foregroundcolor "red" -backgroundcolor "black"
    Write-Host $PhoneticFirstName -foregroundcolor "red" -backgroundcolor "black"
    Write-Host $PhoneticLastName -foregroundcolor "red" -backgroundcolor "black"
    Write-Host $PhoneticDepartment -foregroundcolor "red" -backgroundcolor "black"

    #Build CSV from Array of results
    $UserData = New-Object PSObject
    $UserData | Add-Member -type NoteProperty -name "Name" -value $Name
    $UserData | Add-Member -type NoteProperty -name "E-Mail Address" -value $Email
    $UserData | Add-Member -type NoteProperty -name "First Name" -value $FirstName
    $UserData | Add-Member -type NoteProperty -name "Last Name" -value $LastName
    $UserData | Add-Member -type NoteProperty -name "Department" -value $Department
    $UserData | Add-Member -type NoteProperty -name "Display Name" -value $DisplayName
    $UserData | Add-Member -type NoteProperty -name "Office" -value $Office
    $UserData | Add-Member -type NoteProperty -name "Phonetic Company Name" -value $PhoneticCompanyName
    $UserData | Add-Member -type NoteProperty -name "Phonetic Display Name" -value $PhoneticDisplayName
    $UserData | Add-Member -type NoteProperty -name "Phonetic First Name" -value $PhoneticFirstName
    $UserData | Add-Member -type NoteProperty -name "Phonetic Last Name" -value $PhoneticLastName
    $UserData | Add-Member -type NoteProperty -name "Phonetic Department" -value $PhoneticDepartment

    $UserArray += $UserData
    }

#Do some sorting - not sure if this step is required due to object names likely being unique anyway
$UserArray = $UserArray | sort-object -Property  {$_.name } -Unique

#Write CSV in UTF8 in case of foreign names
$UserArray  | Export-Csv -Encoding UTF8 -LiteralPath $userCSV -NoTypeInformation 
Write-Host "Total Number of Unique Users found:"$UserArray.Length #Count
\$\endgroup\$
  • \$\begingroup\$ I've edited my script to add more comments into it. I am wondering if I show how I was thinking when I built it, it may help. \$\endgroup\$ – Nullldata Jan 9 '16 at 15:50
  • \$\begingroup\$ So are the phonetic properties show not just aliases? Are those lines place holders for custom properties or something. Depending on what those really are the suggestions are different \$\endgroup\$ – Matt Jan 10 '16 at 19:49
  • \$\begingroup\$ Yeah that was the idea... Look up the real values later. I couldn't find them at the time... possibly cos I was looking in the wrong place, and possibly cos they were not populated. \$\endgroup\$ – Nullldata Jan 11 '16 at 5:50
  • 2
    \$\begingroup\$ Welcome to Code Review! Hosch has rolled back the last edit. Please see what you may and may not do after receiving answers. \$\endgroup\$ – Vogel612 Jan 13 '16 at 23:42
  • \$\begingroup\$ Fair enough, however I would like to document for the record how I pulled those attributes back that are missing in my script. I will do it in a comment. Thanks for the information \$\endgroup\$ – Nullldata Jan 14 '16 at 0:16
4
\$\begingroup\$

There are several changes I would propose here. I will try and tackle them one an a time. I will include some snippets as we go along but the end will contain the whole code.

Redundant call to Get-ADDomain

It looks like you only have the one domain. Get-ADUser will automatically call the domain your are attached to. You don't need this unless you have multiple domains.

Calling Get-ADuser twice

You call Get-Aduser to get all users. Then you cycle each one and call Get-Aduser again! No need for this. Lets just do it once and use the pipeline to process. I would like to point out that Select * that you use is not required, in this case, and can be harmful as it can changes the membertypes of object properties. I will come back to this later.

Instead lets do want you intended. Grab all enabled users in one line. Keep reading to see what -Properties $properties is all about.

$allEnabledUsers = Get-ADUser -filter {Enabled -eq $true} -Properties $properties

Also you would never get duplicate users. -Unique would not serve a purpose. They could have the same displayname, surname and firstname (as well as others) but the user would need to be unique in several other aspects.

Performance issue querying AD

You are using -Properties *. This will pull all properties of the filtered objects. You however are ignoring most of these in favour of about a dozen. Instead we should get the properties we are going to use. Also need to keep in my that by default some of these are already returned by default.

$properties = "EmailAddress", "GivenName", "Department", "DisplayName", "Company"
Get-ADUser -filter {Enabled -eq $true} -Properties $properties 

Unnecessary object creation

Get-AdUser is already outputting objects no need to recreate new ones when we have almost the ones you want. Also, depending on how many users you have, the $UserArray += $UserData code can be costly from a performance perspective since the array is destroyed and recreated with the extra user appended. With 1000's of users this can add up. We can use the pipeline to mitigate this as well. You are just using different names for the properties so lets use calculated properties to do just that. Depending on how you like the readability of this you can easily make a function to do this but this does make sense so it is a personal choice if you choose to make that function.

This part of what I do could be biased and other might be fine with object creation. If you still op to do this I would at least use the type case [pscustom] on a hashtable. Simple example of this can be found on this blog which show some different approaches to object creation.

Multiple consecutive calls to write-host

I would love to introduce you to splatting. It is useful when you are calling cmdlets and using the same or similar parameters sets. Basically feed a hashtable of arguments to the cmdlet. Also write-host does take pipeline input so lets send the properties, set for Red, as an array. It keeps the code to as minimum as it gets.

I will show this but I caution that this a lot of information you are putting on the screen. You are already outputing it properly so I don't see the need for the wall of colour you would be creating.

Wrap it up

At the end you sort the object and output to CSV. Everything done up to this point allows us to use the pipeline so we do not need separate statements for this functions. We can still put them on separate lines for readability.


# Output parameters
$consoleOutputParameters = @{
    foregroundcolor = "green" 
    backgroundcolor = "black"
}
# Set output file path
$userCSV = "C:\temp\users.csv"
# Non standard properties to query
$properties = "EmailAddress", "GivenName", "Department", "DisplayName", "Company"
 # Grab a load of user object information from AD 
$allEnabledUsers = Get-ADUser -filter {Enabled -eq $true} -Properties $properties

# Build the calculated properties list for custom header output
$outputProperySet = "Name",
        @{Name="E-Mail Address";Expression={$_.EmailAddress}},
        @{Name="First Name";Expression={$_.GivenName}},
        @{Name="Last Name";Expression={$_.Surname}},
        "Department",
        @{Name="Display Name";Expression={$_.DisplayName}},
        "Office",
        @{Name="Phonetic Company Name";Expression={$_.Company}},
        @{Name="Phonetic Display Name";Expression={$_.DisplayName}},
        @{Name="Phonetic First Name";Expression={$_.GivenName}},
        @{Name="Phonetic Last Name";Expression={$_.Surname}},
        @{Name="Phonetic Department";Expression={$_.Department}}

#Loop through each name in the generated list
$allEnabledUsers | Select-Object $outputProperySet | ForEach-Object{
    # If you choose not to use any console output this whole ForEach-Object can be removed. 
    # Output process on screen. Set the Name to green
    $consoleOutputParameters.foregroundcolor = "Green"
    Write-Host $_.Name @consoleOutputParameters
    # The rest of the properties are Red
    $consoleOutputParameters.foregroundcolor = "Red"
    $_."E-Mail Address",$_."First Name",$_."Last Name",$_.Department,$_.Office,
            $_."Phonetic Company Name",$_."Phonetic Display Name",$_."Phonetic First Name",
            $_."Phonetic Last Name",$_."Phonetic Department" | Write-Host @consoleOutputParameters
    # Throw a blank line to split the content
    Write-Host ""
} | Sort-Object Name | Export-Csv -Encoding UTF8 -LiteralPath $userCSV -NoTypeInformation

# Display a summary on screen. 
Write-Host ("Total Number of Unique Users found: {0}" -f $allEnabledUsers.Length)

I want to call attention to something I wrote above just one more time. If you choose not to use any console output the ForEach-Object could be removed. If you have a requirement to show that on the screen then leave it.

There are some more property loops you could use. For example the one you see for write host. However readability starts to be affected so I think what I have shown above is a good compromise.

Note I have put the code comments above the lines they describe that is both a personal preference and something that makes code easier to read on Stack Exchange. I see no reason outside those two points to change how you have your comments.

\$\endgroup\$
  • \$\begingroup\$ Matt, Thanks very much! Let me digest what you have written, see if I can wrap my head around some of these concepts. I may possibly have a couple of questions if you don't mind? I really appreciate your time and effort mate! :) \$\endgroup\$ – Nullldata Jan 11 '16 at 6:05
  • \$\begingroup\$ My initial thoughts are : (Redundant call to Get-ADDomain : I did notice that this call was not being used much in my script, other than defining the domain I was currently on as the scope. I toyed with the idea of removing it, however in the case of being in a forest, I left it in. What are your thoughts on this?) (Calling Get-ADuser twice : I had a feeling that I was doing that unnecessarily!) (Performance issue querying AD : I have seen this way of grabbing properties before, so I am glad you highlighted it here. My colleagues use this method, I'm sure!) Continuing in next comment.... \$\endgroup\$ – Nullldata Jan 11 '16 at 6:24
  • \$\begingroup\$ Continued... (Unnecessary object creation : I need to read through these links. I have a feeling this is one of my major problems currently. This is the third or fourth script I have ever written, and the first script I have written for using with AD - having previously only ever worked against SharePoint before) (Multiple consecutive calls to write-host : I Also need to sit down and digest what you have done here. I am confused by the array at the top of the script. Are the values held in this array more like place holders to be replaced later in the script when called?) \$\endgroup\$ – Nullldata Jan 11 '16 at 6:24
  • \$\begingroup\$ Yes, Looking at the complete picture, I can see my "further reading" will consist of looking into the section where you are using "Calculated Properties" stored into Arrays. I don't understand fully what is going on there. I appreciate the links you have given! \$\endgroup\$ – Nullldata Jan 11 '16 at 6:31
  • \$\begingroup\$ Going to go get a few more reputation points later, so I can up vote your answer. :) I'll assign it as the answer if no-one else chips in :) \$\endgroup\$ – Nullldata Jan 11 '16 at 6:42

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.