4
\$\begingroup\$

I have a script and I was looking to have it reviewed to see where I should improve it.

Our need: We need to backup a folder every day to a specific USB drive. There may be multiple USB drives plugged into the computer. We need to ensure that it is only backing up to a specific drive.

Code:

function ex{exit}

#####################################
### Check PC for backup USB Drive ###
#####################################

$diskdrive = Get-WmiObject win32_diskdrive

foreach($drive in $diskdrive){
  $partitions = Get-WmiObject -Query "ASSOCIATORS OF {Win32_DiskDrive.DeviceID=`"$($drive.DeviceID.replace('\','\\'))`"} WHERE AssocClass = Win32_DiskDriveToDiskPartition"

    foreach($part in $partitions){
        $name=$drive.model
        $vols = Get-WmiObject -Query "ASSOCIATORS OF {Win32_DiskPartition.DeviceID=`"$($part.DeviceID)`"} WHERE AssocClass = Win32_LogicalDiskToPartition"

    foreach($vol in $vols){
       If($name -eq "General UDisk USB Device" ){
       $USBDisk=$vol.name
       }
     }
  }
}

if ( $USBDisk -eq $null ) {
[void][System.Windows.Forms.MessageBox]::Show("Custom Backup, Please Plug-In the USB Drive","USB Drive not found")
ex
}


###########################################
### Purge USB for data over 90 days old ###
###########################################

$limit = (Get-Date).AddDays(-90)

Get-ChildItem -Path $USBDisk -Recurse -Force | Where-Object { !$_.PSIsContainer -and $_.CreationTime -lt $limit } | Remove-Item -Force

# Delete any empty directories left behind after deleting the old files.
Get-ChildItem -Path $USBDisk -Recurse -Force | Where-Object { $_.PSIsContainer -and (Get-ChildItem -Path $_.FullName -Recurse -Force | Where-Object { !$_.PSIsContainer }) -eq $null } | Remove-Item -Force -Recurse

#########################
### Backup Custom DBs ###
#########################

# Import Assemblies
[void][reflection.assembly]::Load("System.Windows.Forms, Version=2.0.0.0, Culture=neutral, PublicKeyToken=b77a5c561934e089")

$date = Get-Date -Format MMMM.d.yyyy
$backupFolder = "Backup_" + "$date" 

if ( $USBDisk -eq $null ) {
#[void][System.Windows.Forms.MessageBox]::Show("No USB Disk Found, Please Plug-In the USB Drive","USB Drive not found")
} 
else {
$Usbfreedisk =  ((Get-WmiObject win32_logicalDisk  | where { $_.DeviceID -eq $USBDisk }).FreeSpace /1gb -as [INT])
$FPBackup= Get-ChildItem -Recurse "C:\Folder" | Measure-Object -property length -sum
[int]$DataFilesize = ($FPBackup.sum / 1GB )

if ( $Usbfreedisk -lt $DataFilesize ) {
    [void][System.Windows.Forms.MessageBox]::Show("Your $USBDisk Drive don't have enough space, for backup we need $DatafileSize GB of free space.","Not Enough space")
    }
else {
    $testfolder = Test-Path "$USBDisk\$backupFolder" 
		if ( $testfolder -eq $true ) {
		[void][System.Windows.Forms.MessageBox]::Show("You already have backup folder $USBDisk\$backupFolder Please rename it or delete it before running the backup ","Backup Folder Exists")	
		}
	else{	
		mkdir "$USBDisk\$backupFolder"
		Robocopy "C:\Folder"  "$USBDisk\$backupFolder\CYA" /mir /r:2 /w:3
    if ($lastexitcode -eq 0){
            write-host "Robocopy succeeded"
            }
            else{
            write-host "Robocopy failed with exit code:" $lastexitcode
            }
        }
    }
} 

Please let me know if anyone needs any clarification on anything.

Thanks

\$\endgroup\$
3
  • 1
    \$\begingroup\$ Welcome to Code Review! What happens when Robocopy fails for reasons you haven't caught? Is this acceptable? You spell usbdisk with different capitals at times, is this on purpose? What do you think of this practice yourself? \$\endgroup\$ Commented Aug 13, 2019 at 14:04
  • \$\begingroup\$ @Mast Thank you very much for your response! The Robocopy is a good point. I can give that some consideration and make modifications. The USBDisk typos were not on purpose and I have updated them in my script. \$\endgroup\$ Commented Aug 13, 2019 at 14:35
  • \$\begingroup\$ @Mast I just added in a catch for a failure. I'll have to consider what we would like to do if it does fail, but I figured that could be a start. \$\endgroup\$ Commented Aug 13, 2019 at 14:46

1 Answer 1

3
\$\begingroup\$

Get-WmiObject has been superceded by Get-CimInstance since PowerShell 3 and removed since PowerShell 6, so you should avoid using it to ensure forward compatibility.

Use Get-ChildItem with the -File or -Directory parameters so you don't have to do all these PSIsContainer checks.

Consider using comment-based help to document your script. And please explain why you feel the need to wrap exit inside the function ex?

Be consistent in naming your variables: I recommend adopting the camelCase style for naming variables in PowerShell. Use more descriptive names for your variables: what does $FPBackup mean?

Avoid using Write-Host:

The use of Write-Host is greatly discouraged unless in the use of commands with the Show verb. The Show verb explicitly means 'show on the screen, with no other possibilities'.

[...]

Replace Write-Host with Write-Output or Write-Verbose depending on whether the intention is logging or returning one or more objects.

I find it easier to do the querying in PowerShell cmdlets than WQL. This has the added benefit of not needing to escape backslashes and quotation marks, and to quote the WQL documentation linked earlier:

When working with WMI, and especially with WQL, don't forget that you are also using Windows PowerShell. Often, if a WQL query doesn't work as expected, it's easier to use a standard Windows PowerShell command than to debug the WQL query.

Unless you are returning massive amounts of data from across bandwidth-constrained remote systems, it's rarely productive to spend hours trying to perfect a complicated WQL query when there is an acceptable PowerShell cmdlet that does the same thing.

So this block of code:

$diskdrive = Get-WmiObject win32_diskdrive

foreach($drive in $diskdrive){
  $partitions = Get-WmiObject -Query "ASSOCIATORS OF {Win32_DiskDrive.DeviceID=`"$($drive.DeviceID.replace('\','\\'))`"} WHERE AssocClass = Win32_DiskDriveToDiskPartition"

    foreach($part in $partitions){
        $name=$drive.model
        $vols = Get-WmiObject -Query "ASSOCIATORS OF {Win32_DiskPartition.DeviceID=`"$($part.DeviceID)`"} WHERE AssocClass = Win32_LogicalDiskToPartition"

    foreach($vol in $vols){
       If($name -eq "General UDisk USB Device" ){
       $USBDisk=$vol.name
       }
     }
  }
}

becomes easier to write and more readable when rewritten without WQL:

$USBDisk = Get-CimInstance Win32_DiskDrive `
    | Where-Object { $_.Model -eq "General UDisk USB Device" } `  
    | Get-CimAssociatedInstance -Association Win32_DiskDriveToDiskPartition `
    | Get-CimAssociatedInstance -Association Win32_LogicalDiskToPartition `
    | Select-Object -ExpandProperty Name

and similarly when using Windows Storage Management-specific cmdlets:

$USBDisk = Get-Disk `
    | Where-Object { $_.Model -eq "General UDisk USB Device" } `
    | Get-Partition `
    | Get-Volume `
    | Select-Object @{ Name="Name"; Expression={ $_.DriveLetter + ":" } }

though with the caveats that the Windows Storage Management APIs require Windows 8 and above, and Get-Disk does not return dynamic disks, which is a deprecated technology slated to be fully replaced by Storage Spaces. This blog post (translated into English from the original Japanese at https://8thway.blogspot.com/2014/04/wmi-ssd.html?m=0) compares the properties of the MSFT_Disk, MSFT_PhysicalDisk and Win32_DiskDrive WMI classes.

The way you check if there is enough free space on the backup drive to hold the files is flawed. Different filesystems and different allocation sizes, for example, would produce different outcomes, so this method of checking is bound to be imprecise.

About Robocopy: this tool is overrated, but I am still testing it and searching for alternatives. So far, I can list 9 negatives of using this tool:

  1. It has a different, almost bizarre, syntax for syncing files than syncing folders.
  2. It lacks unicode support. The /UNILOG: and /UNICODE options are a lie.
  3. It seems to mistaken extended-length paths (those that begin with \\?\), and by extension the volume GUID paths, as UNC paths, so we have to access the volumes by their assigned drive letters.
  4. In my tests so far with a full backup on an SMR HDD with UASP support and a flash drive, Robocopy only performed on par or slightly better than copying files through File Explorer or the PowerShell command Copy-Item most of the time, and that required a lot of testing and tweaking different flags for different file compositions. If you get the parameters wrong, it could be almost an order of magnitude slower than plain old copy.
  5. It can do folder synchronization by using the /MIR option that only checks for a difference in file size or modified time, but can not do delta backups like rsync.
  6. The progress feedback is very primitive.
  7. Statistics are misleading and require getting used to.
  8. The multithreading flag /MT messes up both normal output and the final statistics.
  9. Unpredictable behaviors and bugs and vague documentation that is the result of many years of neglect by Microsoft.

Due to the many drawbacks of using Robocopy as listed above, I am really pondering whether I should just use Copy-Item or rsync in WSL, which share almost none of the problems, and call it a day.

\$\endgroup\$

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.