[Peer Review] My Advanced Function

Welcome Forums General PowerShell Q&A [Peer Review] My Advanced Function

This topic contains 4 replies, has 4 voices, and was last updated by

2 years, 6 months ago.

  • Author
  • #53544

    Points: 22
    Rank: Member

    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,

  • #53550

    Points: 1
    Rank: Member

    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

    Points: 638
    Helping Hand
    Rank: Major Contributor

    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"
              if ( $x=1 ) {
                  $status = "Blue"
              else {
                  $status = "Red"
              $props = @{
                  Source = $source
                  Destination = $Destination
                  Status = $status
              $results = New-Object -TypeName PSObject -Property $props
          end {
    • 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

    Points: 22
    Rank: Member

    Thanks for the advise guys.

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

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

  • #53691

    Points: 0
    Rank: Member

    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,

The topic ‘[Peer Review] My Advanced Function’ is closed to new replies.

denizli escort samsun escort muğla escort ataşehir escort kuşadası escort