4
\$\begingroup\$

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
         }
       }
    }
 }
\$\endgroup\$
2
  • \$\begingroup\$ I'd start by trading that stack of if/elseif/else statements for a Switch. \$\endgroup\$
    – mjolinor
    May 8, 2015 at 14:32
  • 1
    \$\begingroup\$ Welcome to Code Review! I hope you get some good answers! \$\endgroup\$
    – Phrancis
    May 8, 2015 at 19:14

1 Answer 1

6
\$\begingroup\$

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.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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