patternshellMinor
Query AD and build CSV based on certain user object properties
Viewed 0 times
propertiesuserquerycsvobjectbasedandcertainbuild
Problem
My question is about code efficiency. I recently had to write a Powershell 3.0 script which had the following criteria:
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 =
- It had to pull all enabled user objects from active directory
- It had to pull specific properties about each user object
- It had to display what it was doing on screen so it didn't look like it was hanging
- 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 =
Solution
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.
Calling Get-ADuser twice
You call
Instead lets do want you intended. Grab all enabled users in one line. Keep reading to see what
Also you would never get duplicate users.
Performance issue querying AD
You are using
Unnecessary object creation
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
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
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
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 $propertiesAlso 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 $propertiesUnnecessary 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
Code Snippets
$allEnabledUsers = Get-ADUser -filter {Enabled -eq $true} -Properties $properties$properties = "EmailAddress", "GivenName", "Department", "DisplayName", "Company"
Get-ADUser -filter {Enabled -eq $true} -Properties $propertiesContext
StackExchange Code Review Q#116301, answer score: 5
Revisions (0)
No revisions yet.