As a follow-up to my previous blog post, I plan to pick out a submission or two or three which stood out as my personal favorite and least favorite and tell you why I think this by pointing pieces of code that was either put together nicely or could have been improved in one way or another. Depending on my time, I will do at least 1 Advanced and 1 Beginner submission for both ‘Favorite’ and ‘Not so Favorite. I’ll start out by listing the code and then discussing it bullet point style to highlight my thoughts. So with that, lets begin this journey through the Event 1 submissions.
One last thing: Please don’t take any of this personal if I negatively critique your code. I will be a little tougher on Advanced than on Beginner because if you are doing the Advanced category, I expect that you know what you are doing. As someone who has competed in the past in the games and also who publishes various scripts and articles online, getting reviewed (and corrected) by your peers is one of the best ways to better yourself as a scripter.
Advanced Category – Not So Favorite Submission
<# .SYNOPSIS script that can run against local folder structure to move program logs older than specified time to specified location. The script will keep the folder structure intact, creating subfolders as needed to match the folder structure of the source. .DESCRIPTION Advanced 1 4-29-13 Dr. Scripto is in a tizzy! It seems that someone has allowed a series of application log files to pile up for around two years, and they’re starting to put the pinch on free disk space on a server. Your job is to help get the old files off to a new location. Actually, this happened last week, too. You might as well create a tool to do the archiving. The current set of log files are located in C:\Application\Log. There are three applications that write logs here, and each uses its own subfolder. For example, C:\Application\Log\App1, C:\Application\Log\OtherApp, and C:\Application\Log\ThisAppAlso. Within those subfolders, the filenames are random GUIDs with a .LOG filename extension. Once created on disk, the files are never touched again by the applications. Your goal is to grab all of the files older than 90 days and move them to \\NASServer\Archives ‐ although that path will change from time to time. You need to maintain the subfolder structure, so that files from C:\Application\Log\App1 get moved to \\NASServer\Archives\App1, and so forth. Make those paths parameters, so that Dr. Scripto can just run this tool with whatever paths are appropriate at the time. The 90 ‐day period should be a parameter too. You want to ensure that any errors that happen during the move are clearly displayed to whoever is running your command. If no errors occur, your command doesn’t need to display any output – “no news is good news.” .EXAMPLE Advanced_1.ps1 This will move the logs from the source folder to the default destination folder "\\NASServer\Archives\" with the default time period of 90 days or older. Advanced_1.ps1 -d RemoteLocation This will move the logs from the source folder to the specified destination folder with the default time period of 90 days. The path should have a trailing backslash. Advanced_1.ps1 -t FilesOlderThanThisNumber This will move the logs from the source folder to the default destination folder "\\NASServer\Archives\" with the specified time period. This number should be represented by a negative number. Advanced_1.ps1 -d RemoteLocation -t FilesOlderThanThisNumber This will move the logs from the source folder to the specified destination folder with the specified time period. #> Param( [alias("d")][string]$RootDestination="\\scanbox\workstation\archive\", [alias("t")][string]$TimePeriod="-90" ) GCI C:\Application\Log -Recurse -Filter "*.log" | ` ?{$_.LastWriteTime -lt (get-date).AddDays($TimePeriod)} | ` %{$Dest=$_.Directory.Name;` If(-not(Test-Path -Path $RootDestination$Dest))` {NI -itemtype directory $RootDestination$Dest | Out-Null};` MV -Path $_.FullName -Destination $RootDestination$Dest}
- Has commented help BUT it is missing the parameters that are in this script. As an advanced function, this has to be complete if it is going to be a re-usable tool.
- Aliases are everywhere! From GCI to % to ?, this makes it very hard to read
- Using backticks “`” as a line break is completely unnecessary, especially when you have the pipes “|” that server as a natural break.
- A lack of any error handling always hurts a script/function
- Giving the script a better name other than advanced_1.ps1 that better represents what you are using it for. Move-LogFile.ps1 is much better or something similar.
- Kudos to the default parameter values; just too bad there weren’t a few more so hard coded values were used later in the script. “.Log” and the Source location should have been their own parameters.
- Would be better as a function for re-usability rather than calling script each time
Advanced Category – Favorite Submission
function Move-LogFile { <# .SYNOPSIS Move files with a .log extension from the SourcePath to the DestinationPath that are older than Days. .DESCRIPTION Move-LogFile is a function that moves files with a .log extension from the first level subfolders that are specified via the SourcePath parameter to the same subfolder name in the DestinationPath parameter that are older than the number of days specified in the Days parameter. If a subfolder does not exist in the destination with the same name as the source, it will be created. This function requires PowerShell version 3. .PARAMETER SourcePath The parent path of the subfolders where the log files reside. Log files in the actual SourcePath folder will not be archived, only first level subfolders of the specified SourcePath location. .PARAMETER DestinationPath Parent Path of the Destination folder to archive the log files. The name of the original subfolder where the log files reside will be created if it doesn't already exist in the Destination folder. Destination subfolders are only created if one or more files need to be archived based on the days parameter. Empty subfolders are not created until needed. .PARAMETER Days Log files not written to in more than the number of days specified in this parameter are moved to the destination folder location. .PARAMETER Force Switch parameter that when specified overwrites destination files if they already exist. .EXAMPLE Move-LogFile -SourcePath 'C:\Application\Log' -DestinationPath '\\NASServer\Archives' -Days 90 .EXAMPLE Move-LogFile -SourcePath 'C:\Application\Log' -DestinationPath '\\NASServer\Archives' -Days 90 -Force #> [CmdletBinding()] param ( [string]$SourcePath = 'C:\Application\Log', [string]$DestinationPath = '\\NASServer\Archives', [int]$Days = 90, [switch]$Force ) BEGIN { Write-Verbose "Retrieving a list of files to be archived that are older than $($Days) days" try { $files = Get-ChildItem -Path (Join-Path -Path $SourcePath -ChildPath '*\*.log') -ErrorAction Stop | Where-Object LastWriteTime -lt (Get-Date).AddDays(-$days) } catch { Write-Warning $_.Exception.Message } $folders = $files.directory.name | Select-Object -Unique Write-Verbose "A total of $($files.Count) files have been found in $($folders.Count) folders that require archival" } PROCESS { foreach ($folder in $folders) { $problem = $false $ArchiveDestination = Join-Path -Path $DestinationPath -ChildPath $folder $ArchiveSource = Join-Path -Path $SourcePath -ChildPath $folder $ArchiveFiles = $files | Where-Object directoryname -eq $ArchiveSource if (-not (Test-Path $ArchiveDestination)) { Write-Verbose "Creating a directory named $($folder) in $($DestinationPath)" try { New-Item -ItemType directory -Path $ArchiveDestination -ErrorAction Stop | Out-Null } catch { $problem = $true Write-Warning $_.Exception.Message } } if (-not $problem) { Write-Verbose "Archiving $($ArchiveFiles.Count) files from $($ArchiveSource) to $($ArchiveDestination)" try { If ($Force) { $ArchiveFiles | Move-Item -Destination $ArchiveDestination -Force -ErrorAction Stop } Else { $ArchiveFiles | Move-Item -Destination $ArchiveDestination -ErrorAction Stop } } catch { Write-Warning $_.Exception.Message } } } } END { Remove-Variable -Name SourcePath, DestinationPath, Days, Force, files, folders, folder, problem, ArchiveDestination, ArchiveSource, ArchiveFiles -ErrorAction SilentlyContinue } }
- Great use of commented help to include all parameters and nice examples
- Use of a function (with proper naming convention) which is great!
- Using Write-Verbose and making sure to use [cmdletbinding()] allows the user running the function to specify the –Verbose parameter to add more information to the console when being run.
- Great use of error handling to deal with anything that comes up
- I wish that there would have been a parameter for Filter so it could handle other file extensions.
- Makes good use of the Begin,Process and End blocks; these were not technically needed as none of the parameters allowed for the use of the pipeline.
- I like the use of Join-Path to combine the folder paths; makes the code look cleaner
- Also liked the use of ‘*\*.log’ in the Get-ChildItem command to get all of the files; clever
Beginner Category – Not So Favorite Submission
Get-ChildItem -Path 'C:\Application\Log' -Recurse | Where-Object { $_.LastWriteTime -lt (Get-Date).AddDays(-90) } | Move-Item -Force -Destination '\\NASServer\Archives'
Note: This is a one-liner that I intentionally broke up so I can all fit without much scrolling.
- This assumes that there will only be .Log files; should have used –Filter to grab only the .log files. Never assume with a file search.
- No appropriate destination folders are created to archive each of the log files from their specified source directory.
- Because the proper destination directory doesn’t exist, the logs are all bundled together in one folder.
- Commented help would always be great; even in a beginner event. The sooner you start adding the help, the better off you will be.
- I do like that full cmdlet names were used instead of aliases
Beginner Category – Favorite Submission
<# .SYNOPSIS Moves application log files that are older than 90 days to \\NASServer\Archives. .DESCRIPTION This script was originally written for Event 1 of the 2013 Scripting Games (Beginners Track). .EXAMPLE C:\Scripts\Move-LogFiles.ps1 #> $files = Get-ChildItem –Path “C:\Application\Log” -Filter *.log –Recurse | Where-Object -FilterScript {$_.CreationTime –lt (Get-Date).AddDays(-90)} foreach($file in $files) { if(!(Test-Path -LiteralPath "\\NASServer\Archives\$($file.Directory.Name)")) { New-Item -Path "\\NASServer\Archives\$($file.Directory.Name)" -ItemType Directory -ErrorAction Stop | Out-Null } Move-Item -Path $file.FullName -Destination "\\NASServer\Archives\$($file.Directory.Name)\$($file.Name)" }
- Kudos for the commented help in this script. This makes it easier for someone to pick and run Get-Help against to better understand how this works.
- Good use of –Filter against the .log extension
- I like that there were no vbscript style concatenation happening with the folder paths
- Per the submission requirement, using Out-Null with New-Item to create each directory suppresses the object output that occurs when creating anything with the New-Item cmdlet.
- Clean code that makes it easy to read and understand what is happening. Could have done a line break at the Where-Object, but still readable.
That’s it for Event 1. Thanks again to everyone who has competed this year and submitted their entries! Everyone here has done incredible work and I am truly looking forward to seeing what is submitted for the future events! Keep it up!
Could you elaborate on the use of ‘*\*.log’ ? I’m not seeing the problem it solves…
That’s my advanced script and my snippet of beginner. I know someone who won’t use backticks again!
I wrote the ‘not so favorite’ beginner one-liner. Thanks for your criticism.
I disagree on your first two comments. Those points were considered during creation. The problem clearly states that all files are named .log. The filter was included to prevent extraneous files from being involved. The problem also states there are three applications which create logs in the folder and explicitly gives the literal path names of the folders where the log files are stored.
The folders issue: I cleaned my submission too thoroughly. Originally I had noted that the first time the one-liner is run we needed to manually ensure the folders were there.
Personally, I would not use Powershell for this. I would use robocopy. It will create any folder structure needed and can filter by file age.
Finally, I will include the help bits for future submissions.
Thank you for your comment.
You say that you disagree with my first two comments which is absolutely fine. I want to make sure to address both of these items.
1: Based on the requirements, while it says that the .log files do reside on the specified folders, it doesn’t explicitly say that those are the only files. With that, we really cannot assume that those are the only files (or even the only sub folders) underneath the C:\Application\Log folder. Even if the event said that there were no sub folders and the only files that would exists would be .log files, it is a good practice regardless to use the -Filter parameter to more efficiently find files that match a certain string (in this case, the .log). Remember that the games are more about learning than anything else.
2: I am assuming that you are talking about my comment about not having created the destination folders that mirror what was at the source. I do like the idea of using robocopy to handle this and wouldn’t have had an issue with that (though I hear most of the community was downvoting folks who did this) as it would be innovative and accomplish the requirements of this event. Another option if you wanted to keep the one-liner approach would have been to use Move-Item with something like this:
Move-Item -Force -Destination {mkdir (Join-Path “\\NASServer\Archives\” (Split-Path -Path $_.Directory -Leaf)) -Force}
Using -Force on New-Item with a directory will still output an object but doesn’t overwrite any of the sub items underneath it.
Hope this helps…
Pingback: Scripting Games 2013: Event 1 ‘Favorite’ and ‘Not So Favorite’ Submissions | PowerShell.org