current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 4 down vote favorite

So I wrote this script and although it does work I can't help but think that it can be more efficient with all the repetitive code. Can anyone offer any suggestions on making it fewer lines and eliminating the repeat lines?

$Domains = (Get-ADForest).domains | %{Get-ADDomain $_ | Select DistinguishedName,ReplicaDirectoryServers}

#Gets list of all Computer objects in each domain
foreach ($Domain in $Domains){
Write-Output "Working on $($Domain.DistinguishedName)"
  #Iterates through Computer array
   foreach ($Comp in (Get-ADComputer -Server $($Domain.ReplicaDirectoryServers[0]) -SearchBase $($Domain.DistinguishedName) -filter {OperatingSystem -notlike "*Server*"} -Properties     Name,LastLogonDate,DistinguishedName| ?{ $_.DistinguishedName -notlike "*OU=Disabled*" -and $_.DistinguishedName -notlike "*OU=Servers*"}|Select     Name,LastLogonDate,DistinguishedName)){
      #Performs test on date, if date is greater then $Days moves to $OU.
      if(((New-TimeSpan -Start ($Comp.LastLogonDate) -End (Get-Date)).days) -gt $Days -and $Exclusion -notcontains $Comp.name){
         $Computers += $Comp
         #Finds what OU each object should go to.  Checks if object is already in dumpster OU
         if($($domain.DistinguishedName) -like "*DomainA*"){
         Write-Output ((Get-Date).ToString('MM-dd-yyyy:hh:mm:ss') + ": Moved $($comp.DistinguishedName) to $DomainAOUDump") | Out-File $log -Append
         Move-ADObject -Identity $comp.DistinguishedName -Server $($Domain.ReplicaDirectoryServers[0]) -TargetPath $DomainAOUDump 
         }
         elseif($($domain.DistinguishedName) -like "*DomainB*"){
         Write-Output ((Get-Date).ToString('MM-dd-yyyy:hh:mm:ss') + ": Moved $($comp.DistinguishedName) to $DomainBOUDump") | Out-File $log -Append 
         Move-ADObject -Identity $comp.DistinguishedName -Server $($Domain.ReplicaDirectoryServers[0]) -TargetPath $DomainBOUDump
         }
         elseif($($domain.DistinguishedName) -like "*DomainC*"){ 
         Write-Output ((Get-Date).ToString('MM-dd-yyyy:hh:mm:ss') + ": Moved $($comp.DistinguishedName) to $DomainCOUDump") | Out-File $log -Append
         Move-ADObject -Identity $comp.DistinguishedName -Server $($Domain.ReplicaDirectoryServers[0]) -TargetPath $DomainCOUDump
         }
         elseif($($domain.DistinguishedName) -like "*DomainD*"){
         Write-Output ((Get-Date).ToString('MM-dd-yyyy:hh:mm:ss') + ": Moved $($comp.DistinguishedName) to $DomainDOUDump") | Out-File $log -Append
         Move-ADObject -Identity $comp.DistinguishedName -Server $($Domain.ReplicaDirectoryServers[0]) -TargetPath $DomainDOUDump
         }
         else{
         Write-Output ((Get-Date).ToString('MM-dd-yyyy:hh:mm:ss') + ": I don't know where $($comp.name) should be moved") | Out-File $log -Append
         }
       }
    }
 }
share|improve this question

migrated from stackoverflow.com May 8 at 19:08

This question came from our site for professional and enthusiast programmers.
    
I'd start by trading that stack of if/elseif/else statements for a Switch. –  mjolinor May 8 at 14:32
1  
Welcome to Code Review! I hope you get some good answers! –  Phrancis May 8 at 19:14

1 Answer 1

up vote 5 down vote

One thing that I notice, you can change this repetitive fragment

if($($domain.DistinguishedName) -like "*DomainA*"){
    Write-Output ((Get-Date).ToString('MM-dd-yyyy:hh:mm:ss') + ": Moved $($comp.DistinguishedName) to $DomainAOUDump") | Out-File $log -Append
    Move-ADObject -Identity $comp.DistinguishedName -Server $($Domain.ReplicaDirectoryServers[0]) -TargetPath $DomainAOUDump 
}

Into the following (minus some variable definitions, e.g. $TimeStamp):

$DomainPrefix = Get-DomainPrefix $Domain
$TargetPath = Get-Variable -Name "Domain$($DomainPrefix)OUDump"

Write-Output "$TimeStamp: Moved $DistinguishedName to $TargetPath" | Out-File $log -Append
Move-ADObject -Identity $DistinguishedName -Server $Server -TargetPath $TargetPath

with Get-DomainPrefix defined to capture the first character after "Domain", e.g.

Function Get-DomainPrefix([String] $DomainName){
    $DomainName -match "Domain(?<Prefix>.)" | Out-Null
    $Matches.Prefix
}

So, the script will become (minus definition for necessary functions to hide the complexity)

foreach ($Domain in Get-Domains) {
    foreach ($Comp in (Get-ComputerArray -Domain $Domain)) {
        if ( ShouldBeMoved -Computer $Comp ) {
            $DomainPrefix = Get-DomainPrefix $Domain
            $TargetPath = Get-Variable -Name "Domain$($DomainPrefix)OUDump"
            $TimeStamp = Get-TimeStamp
            $CompName = $Comp.DistinguishedName
            $Server = Get-Server

            Write-Output "$TimeStamp: Moved $CompName to $TargetPath" | Out-File $log -Append
            Move-ADObject -Identity $CompName -Server $Server -TargetPath $TargetPath
        }
    }
}

Note that the else clause is not handled in the simplified script above.

For further cleaning, you can redefine how you store the $TargetPath into a more friendly structure so that we don't need to use Get-Variable. Perhaps a map from DomainPrefix to the TargetPath.

Note: I'm not familiar with ADObject and stuff, so the variable naming might be a little off.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.