[Peer Review] My Advanced Function

PowerShell Help
1

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

2

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"
3

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: 
            <pre>

    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

4

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 :slight_smile:

5

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,