Author Posts

September 6, 2016 at 2:28 pm

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

September 6, 2016 at 2:44 pm

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"

September 6, 2016 at 5:18 pm

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

September 7, 2016 at 9:13 am

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 🙂

September 7, 2016 at 10:21 am

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,