Let’s nitpick first.
The function name should follow cmdlet naming, which usually means capitalizing the verb and noun - e.g., Write-ToADObject. Now, if “to” is some prefix that references your company or organization, it’s cool. If it’s a preposition - as in, “write this to an AD object” usage of “to” - then it’s incorrect in terms of usual PowerShell patterns. If your company is Midwest Health, something like Write-MWHADObject would be appropriate.
That said, Write is the wrong verb. You’re modifying an existing object; the correct verb is Set. So, Set-MWHADObject. The MWH distinguishes your command from Microsoft’s Set-ADObject.
Last, your call to
write-toADObject ($ou)
is potentially confusing. PowerShell functions are a kind of command; they’re not the kind of function you find in other languages. So the parens are unnecessary, and in some cases your style of usage could have unexpected results. Correct “best practice” usage would be
Set-MWHADObject -currentOu $ou
using the parameter by name, rather than positionally.
OK, nits done.
You use parentheses a lot within the function, and I’m unclear why. They’re unnecessary. That was a nit, too :).
I think you’re probably doing a lot of unnecessary work, honestly. You’ve got a very procedural-style setup, a la C# more than shell script. You’re also unnecessarily using ForEach. In one place, it doesn’t matter; in the other, it’s slowing down the code.
The first instance is where you enumerate the OUs:
foreach ($ou in $facilityOUs)
{
write-toADObject ($ou)
}
You could have written your function to handle pipeline input:
function write-toADObject
{
[CmdletBinding()]
param(
[Parameter(ValueFromPipeline=$True)]$currentOU
)
PROCESS {
foreach ($ou in $currentOU) {
$currentOUName = $OU.name
$allUsersInCurrentOU = Get-ADUser -Filter * -Properties * -SearchBase "OU=$currentOUName,OU=facilities,DC=midwest-health,DC=com"
foreach ($user in $allUsersInCurrentOU)
{
$user.company = ($OU.Description)
$user.postalCode = ($OU.PostalCode)
$user.physicalDeliveryOfficeName = ($OU.Description)
$user.streetAddress = ($OU.Street)
$user.st = ($OU.State)
$user.telephoneNumber = ($OU.TelephoneNumber)
$user.city = ($OU.City)
$user.country = ($OU.Country)
Set-ADUser -instance $user
}
}
}
You’d then call it like this
Get-ADOrganizationalUnit -filter * -Properties * -SearchBase 'OU=facilities,DC=midwest-health,DC=com' -SearchScope OneLevel | Write-toADObject
Now, that’s completely arbitrary, because all I’ve done is move the ForEach. So no performance gain whatsoever. But it does make your function better adhere to common shell practices, and it makes the composition of the controlling code (where you call the function) more shell-like, so that someone else could use it without needing to write out the ForEach construct.
The second instance is more interesting. I’d have done this:
@properties = @{
Company = $currentOU.company;
postalCode = $currentOU.postalcode;
etc = etc
}
Get-ADUser -Filter * -Properties * -SearchBase "OU=$currentOUName,OU=facilities,DC=midwest-health,DC=com" |
Set-ADUser @properties
You get a bunch of advantages that way. One, Set-ADUser is in managed code, not script, so it’ll enumerate the incoming pipeline objects from Get-ADUser a lot faster than your ForEach loop. Second, users can flow through the pipeline one at a time to Set-ADUser, which will run more or less in parallel with Get-ADUser. Faster. I’ll note that telephoneNumber and physicalDeliveryOfficeName can’t be accommodated that way; for telephoneNumber, you can use either -Add or -Replace on Set-ADUser. Ditto the office name one - you’d likely use -Replace.
Second, with my suggestion, you’re passing those parameters to Set-ADUser just one time, which means you have to read $currentOU only once. Faster.
Third, with my suggestion, you avoid the memory hit of filling up $allUsersInCurrentOU, and you avoid repeatedly creating $user in the ForEach construct. Faster, less memory usage.
Fourth, with my suggestion, you’re doing less work that has to be done on the DC anyway. E.g., you’re not grabbing an object, modifying it, and using it to send changes to the DC. You’re just telling the DC what to do. Faster.
It seems like you’ve got some good scripting or programming language in your background; the downside of moving to PowerShell is forgetting all that. If you’re really working in a good situation, then the shell becomes a series of chained pipeline commands. Not always, mind you - I get that. But here, you have an opportunity to better use the shell’s intended use pattern, and extend that by creating a command that fits that pattern. Your command can then be used in the same kind of way.
Anyway, what you’ve done isn’t wrong, which you knew already. It just is less “PowerShell-y” than it could be, and more C#-ish.