2
\$\begingroup\$

I wrote a script to copy a file, but this file already exists in the target directory. So, first I need to rename the original file.

The script works, but a colleague did not want to use it because it is too complex. I would like to know how can I improve the code of this script. In some sense it seems to be overkill for simple task. I would like to find an objective reason to that opinion.

###GLOBAL VARIABLES

$goodDir = "..."
$targetDir = "..."
$goodFile = "..."
$targetFile = "..."

$app = "..."
$logDir= "..."
$timestamp = Get-Date -Format "dd-MMM-yyyy HH:mm:ss.fff" | foreach {$_ -replace ":", "."}

$log_name = $app + "_pscp-" + $timestamp + ".log"
$log_file = $logDir + $log_name



########################
#FUNCTIONS
########################



Function WriteLog
{
  param(

    [Int]$ecode=0,
    [string]$msg="Error Message"
  )
  $now = Get-Date -format "dd-MMM-yyyy HH:mm:ss.fff"
  $messageToLog = $now + ":[exit code " + $ecode.ToString() + "] :`t" + $msg
  Add-Content $log_file -value $messageToLog
}

function rename-File {
    param(
        # file to copy
        [Parameter(Mandatory)]
        [string]$fname,
        [Parameter(Mandatory)]
        [string]$from
        )
        $now = Get-Date -format "dd-MMM-yyyy"
        $toNewName = $fname + "-Copy-" + $now
        try {
            Rename-Item -Path $from$fname $toNewName -ErrorAction Stop
        }
        catch {
            WriteLog -ecode 4 -msg "The item $_.Exception.ItemName failed with the following message $_.Exception.Message" 
            Break 
        }
        Finally {
            WriteLog -msg "Rename FILE $fName `r`n`t`t`t`t`t`tFROM $from `r`n`t`t`t`t`t`tTO $toNewName"
        }

    }



function copy-File {
    param(
        # file to copy
        [Parameter(Mandatory)]
        [string]$fname,
        [Parameter(Mandatory)]
        [string]$from,
        [Parameter(Mandatory)] 
        [string]$to    )

        try {
            Copy-Item -Path $from$fname -Destination $to -ErrorAction Stop
        }
        catch {
        WriteLog -ecode 4 -msg "The item $_.Exception.ItemName failed with the following message $_.Exception.Message" 
        Break}
        Finally {
        WriteLog -msg "Copy FILE $fName `r`n`t`t`t`t`t`tFROM $from `r`n`t`t`t`t`t`tTO $to"
    }

    }

### MAIN ###


rename-File -fname $targetFile -from $targetDir
copy-File -fname $goodFile -from $goodDir -to $targetDir$targetFile
\$\endgroup\$

migrated from stackoverflow.com Aug 13 '16 at 9:58

This question came from our site for professional and enthusiast programmers.

  • \$\begingroup\$ WriteLog -ecode 4 -msg "The item $_.Exception.ItemName..." - complex enough that you didn't even run it enough to notice that it doesn't work properly. \$\endgroup\$ – TessellatingHeckler Aug 8 '16 at 15:17
3
\$\begingroup\$

Here are some tipps:

  • Try to avoid / minimize the amout of global variables.
  • Use approved verbs (see Get-Verb) for your functions (the first letter should be uppercase).
  • Use the Join-Path cmdlet to combine a path.
  • Use string interpolation

    "${app}_pscp-$timestamp.log"
    

    or the format operator

    '{0}_pscp-{1:dd-MMM-yyyy HH.mm.ss.fff}.log' -f $app, (Get-Date)
    

    instead of string concatenation.

Also, I would not create a Copy-File nor a Rename-File function. Just invoke Copy-Item and Rename-Item in the main…

\$\endgroup\$
  • \$\begingroup\$ I am new on this forum. I am not sure if say thank you is common. But I am very grateful with your comments Martin Brandl. Just one thing, if I want to rename and copy and do something else in each of those tasks, is it Ok to put them in a function? Or what could be a best practice for a powershel script? \$\endgroup\$ – Christianhv Aug 9 '16 at 9:35
  • 1
    \$\begingroup\$ I would clarify one of your points. He is not using global variables his functions should have access to the parent scope which is where those variable are located. Avoided globals is a good idea but these are not technically globals \$\endgroup\$ – Matt Aug 16 '16 at 14:12
  • \$\begingroup\$ @Matt I see your point. The only real global variable there is $log_file, it is used in the WriteLog function. I try to avoid using variables inside of a function when these variables are declared in the parent scope. For me is clearer in that way. \$\endgroup\$ – Christianhv Aug 16 '16 at 20:58

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.