Feedback on a working script

This topic contains 9 replies, has 3 voices, and was last updated by Profile photo of Matt Bloomfield Matt Bloomfield 2 years, 2 months ago.

  • Author
    Posts
  • #25335
    Profile photo of Jacob Sampson
    Jacob Sampson
    Participant

    I would love some feedback on this script of mine. Specifically, I would like to know if there is a more efficient way of accomplishing what I am doing or a more sophisticated way of writing it.

    The company I work for has about 50 different locations and each have there own Active Directory OU. My goal is the populate each user in each OU with the address and phone info for the facility that they work at. My goal was to write the script so that it didn't have to be manually update each time we purchased a new building and created a new OU. It should scale automatically.

    Any and all feedback is appreciated.

    #region Function Definitions
    function write-toADObject
    {
    param($currentOU)

    $currentOUName = $currentOU.name

    $allUsersInCurrentOU = Get-ADUser -Filter * -Properties * -SearchBase "OU=$currentOUName,OU=facilities,DC=midwest-health,DC=com"
    foreach ($user in $allUsersInCurrentOU)
    {
    $user.company = ($currentOU.Description)
    $user.postalCode = ($currentOU.PostalCode)
    $user.physicalDeliveryOfficeName = ($currentOU.Description)
    $user.streetAddress = ($currentOU.Street)
    $user.st = ($currentOU.State)
    $user.telephoneNumber = ($currentOU.TelephoneNumber)
    $user.city = ($currentOU.City)
    $user.country = ($currentOU.Country)
    Set-ADUser -instance $user
    }
    }
    #endregion

    #region Script Body

    $facilityOUs = Get-ADOrganizationalUnit -filter * -Properties * -SearchBase 'OU=facilities,DC=midwest-health,DC=com' -SearchScope OneLevel

    foreach ($ou in $facilityOUs)
    {

    write-toADObject ($ou)
    }

    #endregion

  • #25336
    Profile photo of Don Jones
    Don Jones
    Keymaster

    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.

  • #25337
    Profile photo of Don Jones
    Don Jones
    Keymaster

    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.

  • #25339
    Profile photo of Jacob Sampson
    Jacob Sampson
    Participant

    Don Jones,

    I appreciate your feedback. It is exactly what I was looking for. There is a paradigm shift that I need to make in how I think through the scripts I am writing. Anyway, I really appreciate the detailed feedback.
    Is there a book or other source you would recommend for learning PowerShell standards and best practices?

    Jacob Sampson

  • #25341
    Profile photo of Don Jones
    Don Jones
    Keymaster

    Both of my Month of Lunches books on Powershell :).

  • #25357
    Profile photo of Jacob Sampson
    Jacob Sampson
    Participant

    When I pipe Get-ADOrganizationalUnit to my function it only passes the last OU found. It doesn't seem to pass all OUs one at a time.

    function Set-MHMADObject
    {
        param([Parameter(ValueFromPipeline=$True)]$currentOU)
    
        $currentOU
    
        $currentOUName = $currentOU.name
    
        "OU : $currentOUName"
    
        $allUsersInCurrentOU = Get-ADUser -Filter * -Properties * -SearchBase "OU=$currentOUName,OU=facilities,DC=midwest-health,DC=com"
        foreach ($user in $allUsersInCurrentOU)
        {
            "User: $user"
            $user.company = $currentOU.Description
            $user.postalCode = $currentOU.PostalCode
            $user.physicalDeliveryOfficeName = $currentOU.Description
            $user.streetAddress = $currentOU.Street
            $user.st = $currentOU.State
            $user.telephoneNumber = $currentOU.TelephoneNumber
            $user.city = $currentOU.City
            $user.country = $currentOU.Country
            Set-ADUser -instance $user  
        }
    }
    #endregion
    
    #region Script Body
    
    Get-ADOrganizationalUnit -filter * -Properties * -SearchBase 'OU=facilities,DC=midwest-health,DC=com' -SearchScope OneLevel | Set-MHMADObject
    
    #endregion
    
    
  • #25358
    Profile photo of Jacob Sampson
    Jacob Sampson
    Participant

    Never mind, I see now that the get-adorganizationalunit must pass the entire group of items that it finds to the function. I have to build into my function the ability to handle the items one at a time. Correct?

    Thanks,
    Jacob Sampson

  • #25361
    Profile photo of Jacob Sampson
    Jacob Sampson
    Participant

    Never mind my never mind. Apparently Get-ADOrganizationalUnit is only passing the last item it finds. So I am still working through this.

  • #25363
    Profile photo of Jacob Sampson
    Jacob Sampson
    Participant

    OK. I have the script working. I figured someone had already had a similar problem with piping, so I searched the forum and found the discussion you had with a user where in you explain the need for the PROCESS block. I added that and away we go. In case it is helpful to anyone else I will post my final script below. I have not added all the changes you recommended because want to do some studying to make sure I grasp the concepts before I implement them.

    
    #region Function Definitions
    function Set-MHMADObject
    {
        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: $user"
                    $user.company = $currentOU.Description
                    $user.postalCode = $currentOU.PostalCode
                    $user.physicalDeliveryOfficeName = $currentOU.Description
                    $user.streetAddress = $currentOU.Street
                    $user.st = $currentOU.State
                    $user.telephoneNumber = $currentOU.TelephoneNumber
                    $user.city = $currentOU.City
                    $user.country = $currentOU.Country
                    Set-ADUser -instance $user  
                }
            }
        }
    }
    #endregion
    
    #region Script Body
    
    Get-ADOrganizationalUnit -filter * -Properties * -SearchBase 'OU=facilities,DC=midwest-health,DC=com' -SearchScope OneLevel |
        Set-MHMADObject
    
    #endregion
    

    Thanks,

    Jacob.

  • #25369
    Profile photo of Matt Bloomfield
    Matt Bloomfield
    Participant

    @Jacob,

    As well as the Lunches books, there is [url=https://www.penflip.com/powershellorg/the-community-book-of-powershell-practices]The Community Book of PowerShell Practices[/url].

    There's also video training on PluralSight: [url=http://www.pluralsight.com/courses/windows-powershell-best-practices-patterns]Windows PowerShell Best Practices and Patterns[/url]

You must be logged in to reply to this topic.