HiveBrain v1.2.0
Get Started
← Back to all entries
patternshellMinor

Getting a list of all computer objects in each AD domain

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
eachobjectsallgettingcomputerlistdomain

Problem

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

Solution

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(?.)" | 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.

Code Snippets

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

Context

StackExchange Code Review Q#90199, answer score: 6

Revisions (0)

No revisions yet.