locked
1st Script Constructive Criticism Please RRS feed

  • Question

  • Hi All, I have just had to write my 1st powershell script to be used to move files from server to server for file processing in another country on a scheduled basis.

    This will be ran using windows task scheduler, the paths and other data have been changed to protect the innocent ;).

    I have tried to design it to be easy to use for many file types and folder paths with the option for additional processed processing if needed. 

    Can any of you guru's please give me constructive criticism about my design and any recommended improvement i should or can make.

    if its ok, anyone else feel free to use at you own peril lol

    code below

    # To run PS Script from task scheduler run powershell as the program and a this parameter -ExecutionPolicy Bypass c:\scripts\scriptname.ps1
    # Setup date & time variables used for log file names and time stamps for message entries
    $datenow = Get-Date # put date and time into $datenow
    $d = $dateNow.day # put day number into $d
    $m = $dateNow.month  # put month number into $m
    $y = $dateNow.year # put year number into $y
    $h = $datenow.hour # put hour number into $h
    $mm = $datenow.minute # put minutes number into $mm
    $s = $datenow.second # put seconds number into $s

    #S***********Start setting up unique variables for this script***********
    $scriptName = "File Mover Script" # Setup Script Name variable
    $moveToProcessedflag = $True # Setup post processing file move is needed variable
    $logFilePath = "C:\Logs\" # Setup log file path variable
    $logFileName = "$y$m$d-Log.txt" # Setup logfile name variable
    $sourceDir = "C:\Source\" # Setup source file pickup folder path variable
    $destDir = "C:\Destination\" # Setup destination file drop off path variable
    $processedDir = "C:\Processed\" # Setup post processing processed folder path variable
    $currFileXtn = "txt" # Setup pickup file extention type variable
    $tempFileXtn = "tmp" # Setup temp file extention variable
    #E**********End for unique variables setup***********

    #********** Functions for repeated tasks***********
    Function testPathExists($path)
        {
            if (!(Test-Path $path)) # Test that the path exists if not error & stop
            {
                sendEmail "ERROR - $scriptName" "Error: Path test failed $path does not exist or is not accessable at the moment."; # Send email message to alert about the error
                Add-Content "$logFilePath$logFileName" "Error: Path test failed $path does not exist or is not accessable at the moment."
                Write-Host -ForegroundColor RED "Error: Breaking out at testPathExists Path test failed $path does not exist or is not accessable at the moment."; # Write a messgae to the screen if runing in console mode during manual execution
                Break # Quit the script
            }
        }
    Function renameFiles($cur, $new) 
        {
            try
            {
                Rename-Item $cur $new -ErrorAction 'Stop' -Errorvariable errorMessage;
                Add-Content "$logFilePath$logFileName" "INFO: File renamed from $cur to $new"
            }
            catch
            {
                sendEmail "ERROR - $scriptName" "Error: There was a problem renaming the files to xtn $new Please investigate and try again. - $errorMessage"; # Send email message to alert about the error
                Add-Content "$logFilePath$logFileName" " "; # Add blank line to the log file
                Add-Content "$logFilePath$logFileName" "Error: There is a problem renaming the files to xtn $new Please investigate and try again. - $errorMessage"; # Wtite a message to the log file
                Write-Host -ForegroundColor RED "Error: Breaking out at renameFiles There is a problem renaming the files to xtn $new Please investigate and try again - $errorMessage"; # Write a messgae to the screen if runing in console mode during manual execution
                Break
            }
        }

    Function copyFiles($src, $dest)
        {
            try
            {
                Copy-Item $src $dest -ErrorAction 'Stop' -Errorvariable errorMessage;
                Add-Content "$logFilePath$logFileName" "INFO: File copied from $src to $dest"
            }
                catch
            {
                sendEmail "ERROR - $scriptName" "Error: There is a problem copying the files from $src to $dest Please investigate and try again. - $errorMessage"; # Send email message to alert about the error
                Add-Content "$logFilePath$logFileName" " "; # Add blank line to the log file
                Add-Content "$logFilePath$logFileName" "Error: There is a problem renaming the files from $src to $dest Please investigate and try again. - $errorMessage"; # Wtite a message to the log file
                Write-Host -ForegroundColor RED "Error: Breaking out at copyFiles There is a problem renaming the files from $src to $dest Please investigate and try again - $errorMessage"; # Write a messgae to the screen if runing in console mode during manual execution
                Break
            }
        }

    Function deletefiles($filename)
        {
            try
            {
                remove-item -path $filename -force -ErrorAction 'Stop' -Errorvariable errorMessage; # Remove the supplied file name
                Add-Content "$logFilePath$logFileName" "INFO: File deleted $filename" # Wtite a message to the log file
            }
            catch
            {
                sendEmail "ERROR - $scriptName" "Error: There was a problem deleting $filename Please investigate and try again. - $errorMessage"; # Send email message to alert about the error
                Add-Content "$logFilePath$logFileName" " "; # Add blank line to the log file
                Add-Content "$logFilePath$logFileName" "Error: There is a problem deleting $filename Please investigate and try again. - $errorMessage"; # Wtite a message to the log file
                Write-Host -ForegroundColor RED "Error: Breaking out at deleteFiles There is a problem deleting $filename Please investigate and try again - $errorMessage"; # Write a messgae to the screen if runing in console mode during manual execution
                Break
            }
        }

    Function moveFiles($curloc, $newloc)
        {
            Try
            {
                Move-Item $curloc $newloc -ErrorAction 'Stop' -Errorvariable errorMessage;
                Add-Content "$logFilePath$logFileName" "INFO: File moved from $curloc to $newloc" # Wtite a message to the log file
            }
            catch
            {
                sendEmail "ERROR - $scriptName" "Error: There was a problem moving $filename Please investigate and try again. - $errorMessage"; # Send email message to alert about the error
                Add-Content "$logFilePath$logFileName" " "; # Add blank line to the log file
                Add-Content "$logFilePath$logFileName" "Error: There is a problem moving $filename Please investigate and try again. - $errorMessage"; # Wtite a message to the log file
                Write-Host -ForegroundColor RED "Error: Breaking out at deleteFiles There is a problem moving $filename Please investigate and try again - $errorMessage"; # Write a messgae to the screen if runing in console mode during manual execution
                Break
            }
        }
        
    Function tempFilePresentTest($srcdir, $filextn)
        {
            if (Test-Path $srcdir* -include *.$filextn) # Test to see if TEMP files are present in the source dir indecating that a failure has occured during last processing
            {   
                sendEmail "ERROR - $scriptName" "Error: $srcdir contains temporary files, please clean up and try again."; # Send email message to alert about the error
                Add-Content "$logFilePath$logFileName" " "; # Add blank line to the log file
                Add-Content "$logFilePath$logFileName" "Error: $srcdir contains temporary files, please clean up and try again."; # Write error message to log file
                Write-Host -ForegroundColor RED "Error: Breaking out at tempFilePresentTest $srcdir contains temporary files, please clean up and try again"; # Write a messgae to the screen if runing in console mode during manual execution
                Break # Quit the script
            }
        }

    Function filesToBeProcessedPresentTest($srcdir, $xtn)
        {
            if (Test-Path $srcdir* -include *.$xtn) # Test to see if there are any files to be processed in the sourceDir
            {
                Add-Content "$logFilePath$logFileName" "INFO: .$xtn files are present in $srcdir folder" # Write a status message to the log file
            }
            else
            {
                Add-Content "$logFilePath$logFileName" "INFO: There are no .$xtn files for precessing in $srcdir"; # Write a status message to the log file
                Add-Content "$logFilePath$logFileName" "INFO: Script Complete $datenow"; # Write a status message to the log file
                Write-Host -ForegroundColor GREEN "INFO: There are no .$xtn files for precessing in $srcdir"; # Write a messgae to the screen if runing in console mode during manual execution
                Write-Host -ForegroundColor GREEN "INFO: Script Complete $datenow" # Write a messgae to the screen if runing in console mode during manual execution
                Break
            }
        }

    Function arrOfFileInDir($path, $xtn)
        {
            Try
            {
                $arrFiles = Get-ChildItem -path $path -filter *.$xtn -name -ErrorAction 'Stop' -Errorvariable errorMessage; # Setup and array of files in the pickup folder
                return, $arrFiles # Return a list of files to the caller
            }
            catch
            {
                sendEmail "ERROR - $scriptName" "Error: There was a problem getting a list of files to process frpm $path Please investigate and try again. - $errorMessage"; # Send email message to alert about the error
                Add-Content "$logFilePath$logFileName" " "; # Add blank line to the log file
                Add-Content "$logFilePath$logFileName" "Error: There was a problem getting a list of files to process from $path Please investigate and try again. - $errorMessage"; # Wtite a message to the log file
                Write-Host -ForegroundColor RED "Error: Breaking out at deleteFiles There was a problem getting a list of files to process from $path Please investigate and try again - $errorMessage"; # Write a messgae to the screen if runing in console mode during manual execution
                Break
            }
        }

    Function sendEmail ($subj, $bod) # Send email message function
        {
            Send-MailMessage -To "My Name <myemail@domain.co.uk>" -Subject "$subj" -Body "$bod" -SmtpServer "EmailServer" -From "File Mover Script <myemail@domain.co.uk>" # Send email message
        }
    #***********End of Defind Functions***********

    #***********Start Execution**********
        testPathExists $logFilePath # Test that log file path is present
        Add-Content "$logFilePath$logFileName" " " # Add blank line to the log file
        Add-Content "$logFilePath$logFileName" "INFO: Script Start  $datenow" # Write status messgae to the log file
        Write-Host -ForegroundColor GREEN "INFO: Script Start  $datenow" # Write a messgae to the screen if runing in console mode during manual execution
        testPathExists $sourceDir # Test to see if source path exists
        testPathExists $destDir # Test to see if destination path exists
        testPathExists $processedDir # Test to see if processed path exists
        tempFilePresentTest $sourceDir $tempFileXtn # Test to see if temp files are still present in source path
        tempFilePresentTest $destDir $tempFileXtn # Test to see if temp files are still present in the destination path
        filesToBeProcessedPresentTest $sourceDir $currFileXtn # Test that there are files present to be processed
        
        Foreach ($item in (arrOfFileInDir $sourceDir $currFileXtn)) # Carry out processing for each file in the folder
        {
            $string = $item; # Add current file in variable $item to variable $string
            $shortString = $string.trimend($currFileXtn);
            renameFiles $sourceDir$string $sourceDir$shortString$tempFileXtn;
        }
        
        Foreach ($item in (arrOfFileInDir $sourceDir $tempFileXtn)) # Carry out processing for each file in the folder
        {
            $string = $item; # Add current file in variable $item to variable $string
            $shortString = $string.trimend($tempFileXtn);
            copyFiles $sourceDir$shortString$tempFileXtn $destDir$shortString$tempFileXtn
        }

        Foreach ($item in (arrOfFileInDir $destDir $tempFileXtn)) # Carry out processing for each file in the folder
        {
            $string = $item # Add current file in variable $item to variable $string
            $shortString = $string.trimend($tempFileXtn)
            renameFiles $destDir$string $destDir$shortString$currFileXtn
        }

        if ($moveToProcessedflag) # test the value of $moveToProcessedflag if 0 delete source files if 1 move source files to 
        {
            Foreach ($item in (arrOfFileInDir $sourceDir $tempFileXtn)) # Carry out processing for each file in the folder
            {
                $string = $item; # Add current file in variable $item to variable $string
                $shortString = $string.trimend($tempFileXtn);
                moveFiles $sourceDir$shortString$tempFileXtn $processedDir$shortString$currFileXtn;
            }
        }
        else
        {
            Foreach ($item in (arrOfFileInDir $sourceDir $tempFileXtn)) # Carry out processing for each file in the folder
            {
                $string = $item; # Add current file in variable $item to variable $string
                $shortString = $string.trimend($tempFileXtn);
                deleteFiles $sourceDir$string;
            }
        }
        Add-Content "$logFilePath$logFileName" "INFO: Script Complete $datenow"
        Write-Host -ForegroundColor GREEN "INFO: Script Complete $datenow"

     
    Sunday, May 14, 2017 10:33 PM

Answers

  • Here is a PowerShell method for validating path arguments.

    Param(
    	[Parameter(Mandatory)]
    	[ValidateScript({Test-Path $_})]
            [string]$Path
    )
    
    $Path

    In a scheduled task the failures will be logged.  Use an event script to email the results of the scheduled task.


    \_(ツ)_/

    • Marked as answer by addison.mj Monday, May 15, 2017 2:49 PM
    Sunday, May 14, 2017 11:23 PM

All replies

  • The first criticism is that yo learn to post code in forums using the code posting tool supplied for that purpose.


    \_(ツ)_/

    Sunday, May 14, 2017 10:58 PM
  • Second major criticism is that you should not wrap every command in a function.  This just creates chaos and it is what we call spaghetti code.  Don't feel bad as it is one the most popular errors that non-trained people bring to both programming and scripting.  I have seen this in new employees for decades.

    Write the minimum amount of code needed and ass basic error detection.  Add a global log function only after you are sure your script works correctly.


    \_(ツ)_/

    Sunday, May 14, 2017 11:03 PM
  • Clearly you need to spend more time learning PowerShell basics.  It will save you a lot of wasted time.  This is a god staring script but you should go back and look at mode scripts written by ex-persts and try to understand what they are doing.

    Example - here is how to date a file name.

    $logFileName = "$([datetime]::Now>ToString('yyyyMMdd'))-Log.txt"


    \_(ツ)_/


    • Edited by jrv Sunday, May 14, 2017 11:08 PM
    Sunday, May 14, 2017 11:07 PM
  • Here is a PowerShell method for validating path arguments.

    Param(
    	[Parameter(Mandatory)]
    	[ValidateScript({Test-Path $_})]
            [string]$Path
    )
    
    $Path

    In a scheduled task the failures will be logged.  Use an event script to email the results of the scheduled task.


    \_(ツ)_/

    • Marked as answer by addison.mj Monday, May 15, 2017 2:49 PM
    Sunday, May 14, 2017 11:23 PM
  • Thanks for your feed back, i will try my best to do better lol.

    Cheers

    Monday, May 15, 2017 2:50 PM
  • Thanks for your feed back, i will try my best to do better lol.

    Cheers


    You are on the right track.  Just keep reviewing articles on formatting and code design with PowerShell.  You will find that your script can be reduced to about 20 or so lines.

    \_(ツ)_/

    Monday, May 15, 2017 3:03 PM