[Peer Review] My Advanced Function

This topic contains 4 replies, has 4 voices, and was last updated by Profile photo of Tim Ker Tim Ker 3 weeks ago.

Viewing 5 posts - 1 through 5 (of 5 total)
  • Author
    Posts
  • #53544
    Profile photo of Nigel Tatschner
    Nigel Tatschner
    Participant

    Hi all,

    Just wanted to get some peer reviews on my function, trying to better myself and streamline my coding.

    I created this function Move-NTOrphanedHomeFolder

    Technically, it's fully working and does what it's supposed too but it feels like I could have reduced the lines of code I used.

    Please be as open and honest as possible, I want to improve!

    Many Thanks,
    Nigel

    #53550
    Profile photo of Chris Bakker
    Chris Bakker
    Participant

    I had only a short look, but a see a ton of times:

    $obj = New-Object System.Management.Automation.PSObject -Property $Props
    $obj.sAMAccountName = $($Folder.Name)
    $obj.OU = $($User | select @{ n = "OU"; e = { ($_.CanonicalName).Replace($($_.CanonicalName).split('/')[-1], "") } })
    $obj.Source = $($Source + '\' + $($Folder.Name))
    $obj.Destination = $($Destination + '\' + $($Folder.Name))
    $obj.FileCount = $($FileCount.Count)$obj.Status = "Error Unknown – In Loop"

    To shorten in adn make it more easy to reed you could make a function of it with a parameter:

    creat_object "Error Unknown – In Loop"

    #53563
    Profile photo of Rob Simmers
    Rob Simmers
    Participant

    Here are some suggestions:

    • Use consistent data types as you have some long-type and some short. Typically, short data types are used (e.g. int, string, etc.)
    • Validating if a path is valid can be done as part of parameter validation. Take a look at this Simplify Your PowerShell Script with Parameter Validation. The example is testing a registry path, but you can do the same for a passed unc or standard path.
    • Wrap your Import-Module with a try\catch or use some logic with Get-Module to verify that the module is available because your function won't work without it.
    • You don't need Write-Output and it's typically bad practice to use it. The return keyword is also unnecessary.
    • A lot of your code is recreating the properties or objects over and over when you are only changing status. Try setting the status in a variable and then create the object once like in the example below:
      function Test-It {
          param (
              $Source = "C:\Test",
              $Destination = "C:\Temp"
          )
          begin{}
          process{
              $x=0
      
              if ( $x=1 ) {
                  $status = "Blue"
              }
              else {
                  $status = "Red"
              }
      
              $props = @{
                  Source = $source
                  Destination = $Destination
                  Status = $status
              }
      
              $results = New-Object -TypeName PSObject -Property $props
          }
          end {
             $results 
          }
      }
      
      Test-It
                  
    • Another note is your first try should be wrapped around your other try\catches for the folders because if your Get-ChildItem fails, the variable would be null and your loop would fail
    #53673
    Profile photo of Nigel Tatschner
    Nigel Tatschner
    Participant

    Thanks for the advise guys.

    Rob,
    Instead of using Write-Output I should just use $obj?

    I'll add the validating paths in and cleanup my data types 🙂

    #53691
    Profile photo of Tim Ker
    Tim Ker
    Participant

    I would move a lot of the content inside the foreach folders into its own function (Move-SingleFolder, but not the batch stuff).
    Also I would use a for loop in this case, instead of a foreach
    I would consider using write-error, or adding [bool]Success to the output object,

Viewing 5 posts - 1 through 5 (of 5 total)

You must be logged in to reply to this topic.