1
\$\begingroup\$

Processing a set of conditions with 'Continue'

I found the following more readable, opposed to trying to parse out if/elseif. What are your thoughts about this? Is there a better way?

What The Code Does

This code is a section of a for loop that does some testing on a file on a remote system (no need to check creds). I'm using Get-Process to see if we have access to the remote system which returns all of the running processes. If that fails continue to the next host. It does more tests against a particular file and after the test is complete it performs a rename and copy.

Question

Most of the time I see If then elseIf and so forth, seemingly my "continue" statements are easy to follow IMO and I wanted so feedback on this style of moving to the next item (ComputerName) in the list of systems in my array.

UPDATE: I have included the entire "main" section of this script which I didn't want to do because of adding potential confusion but it was asked for. In more detail:

*Even more detail Copy the IRR file to a remote system based on the right conditions and keep track of the results. Reruns of script should focus on the ones not done yet.

Script determines if it has been installed before and if not, installs a IRR file if IRR_Application is not running etc.. It then keeps track of which systems it installed it on (in _success and _failed files). Next time the script runs it just checks the _failed list. It will continue this until they are all done.

If you are to delete the temporary _success and _failed files that keep track of the progress, it simply starts over. The good thing is that it is smart enough to not install again on other systems because there is a text file it checks for on that system and sees that it has already been done. Hence you can run it as many times as you want with no impact. Later we can run a cleanup script if we care about a single text file etc. The text file contains the date the file was updated and the file name.

CODE

Function Main {
    # Main: Check that host is reachable and such, then perform a rename (because if locked don't do anything)
    #  of the file, then copy in the new one.

    $Success = @() 
    $Failed = @()

    # -- Read list of computers
    # If there is a 'failed' list file, the assumption is to focus on those, if you delete
    # that 'failed' file (or it's empty), the script will  to default to the all hosts
    
    $PCList = @()
    If (Test-Path -Path $global:FailedHostsFile -PathType Leaf) {
        $PCList = Get-SystemsFromFile -Filename $global:FailedHostsFile
    }

    if ($PCList.Count -lt 1) {
        Write-Host("No Failed list found, starting a new scan from all files.")
        $PCList = Get-SystemsFromFile -Filename $global:AllHostsFile
    }

    #$pclist = @("pcrpdvad415-001")
    Write-Host("IRR UPDATE`n----------------`n[" + [string]$PCList.Count + "] Systems found from file, starting install process. `n-------------------------------")
    Start-Sleep -Seconds 1

    ForEach ($PCName in $PCList) {
        
        # Remote host file, remote folder, renamed file name
        
        $RemoteFilePath = [string]("\\" + $PCName + "\" + $global:RemoteDropFolder + "\" + $global:ReplaceFileName)
        $RemoteFolderPath = [string]("\\" + $PCName + "\" + $global:RemoteDropFolder)
        $BackupFileName = [string]($global:ReplaceFileName + "-Backup-" + (Get-Date).tostring("yyyyMMddhhmmss"))
        $IRRUpdateFilePath = $RemoteFolderPath + "\"+ $global:ReplaceFileName + "_updatelog.txt"

        if ($global:verbose) {
            "-------------------"
            "Remote File      : " + $RemoteFilePath
            "Remote Folder    : " + $RemoteFolderPath
            "Backup File Name : " + $BackupFileName
            "IRR Status File  : " + $IRRUpdateFilePath
            "-------------------"
        }

        # Testing Section - Reachable? Already done? Vesta Process? Folder RW? etc.
        If ($true) {
            $ProcessList = Get-Process -ComputerName $PCname -ErrorAction SilentlyContinue 
            if ($null -eq $ProcessList) {
                Log -Msg "Error - Get process list from host failed, Host down? Skipping." -PCname $PCName
                $Failed += $PCName
                Continue
            }
            If (Test-Path -Path $IRRUpdateFilePath -PathType Leaf) {
                $text = Get-Content $IRRUpdateFilePath -Raw 
                $text = "*Success* - IRR Already Updated - "+$text
                Log -Msg $text -PCname $PCName
                $Success += $PCName
                Continue
            }
            $Process = $ProcessList | Where-Object { $_.ProcessName -match "IRRApplication$" }
            if ($SearchPss -eq [string]($Process.ProcessName)) {
                Log -Msg "Error - Process is still running. Skipping." -PCname $PCName
                $Failed += $PCName
                Continue
            }
            If (-Not (Test-RemoteFolderReadWrite -Path $RemoteFolderPath)) {
                Log -Msg "Error - Unable to Read/Write to remote filesystem. Skipping" -PCname $PCName
                $Failed += $PCName
                Continue
            }
            if (Test-FileLock -Path $RemoteFilePath) {
                Log -Msg "Error - Lock on remote file. Skipping." -PCname $PCName
                $Failed += $PCName
                Continue
            }
        }
 
        # Perform - File Replacement Steps --
        if ($true) {
            Rename-Item -Path $RemoteFilePath -NewName $BackupFileName -Force 
            if (Test-Path -Path $RemoteFolderPath+"\"+$BackupFileName -PathType Leaf) {
                Log -Msg "Error - Unable to move file to backup." -PCname $PCName
                $Failed += $PCName
                Continue
            }
            Copy-Item -Path $global:NewReplacementFile -Destination $RemoteFilePath
            if (Test-Path -Path $RemoteFilePath -PathType Leaf) {
                [string]("FILE: "+$global:ReplaceFileName+" UPDATED (VIA SCRIPT) : " + (Get-Date).tostring("MM-dd-yyyy HH:mm:ss")) | Out-File $IRRUpdateFilePath 
                if (Test-Path -Path $IRRUpdateFilePath -PathType Leaf) {
                    Log -Msg "*Success* - Copy success, File has been installed on host." -PCname $PCName
                    $Success += $PCName
                    Continue
                } Else {
                    Log -Msg "Error - Copy Failed."  -PCname $PCName
                    $Failed += $PCName
                    Continue
                }
            }
        }
    }

    # - Tally Winners and Losers 
    $Success | Out-File $global:SuccessHostsFile -Append
    Get-Content $global:SuccessHostsFile | Sort-Object -Unique | Set-Content $global:SuccessHostsFile
    $Failed  | Out-File $global:FailedHostsFile

    "Failed    : ["+$Failed.Count+"]"
    "Completed : ["+$Success.Count+"]"
    "-------------------"
    "Total Completed : [" + [string]((Get-Content $global:SuccessHostsFile).Count) + "]"

}
\$\endgroup\$
2
  • 2
    \$\begingroup\$ Note that you should only ever use continue (and break) inside a loop statement, a switch statement, or a trap statement - see this answer. \$\endgroup\$
    – mklement0
    Dec 10, 2021 at 0:07
  • 2
    \$\begingroup\$ @pacmaninbw I've included the entire function. ps the if ($true) are just for testing and grouping the tasks, they could easily be removed. \$\endgroup\$
    – Mike Q
    Dec 10, 2021 at 15:17

1 Answer 1

3
\$\begingroup\$

In a high level language one could write it even more terse and save a bit on repetitive code. Gramatical ... and-then ... and-then .... Collecting $PCName into $Failed as result. Parametrising with task step and log message.

But we have PowerShell, and as such the code above is nice enough.

The style reminds of if (...) { ...; return; } and the continue ensures that you need not read all remaining code to see whether an else was forgotten.

So yes, an acceptable style with a readability advantage over else if chaining. But not that much better than else if chaining to forbid the alternative.

But in PowerShell you can define functions for every step:

  • Its name can describe the step in human readable wording.
  • You can test the function isolated. (Unit tests!) With so many cases testing every branch of the control flow is tiresome.
  • The loop becomes terser, a couple of lines.
  • DRY: don't repeat yourself.

Put some effort in high level functions & constructs. Maybe it is at least as good.


Requested pseudo-code

I have to admit not having programmed in PowerShell, and it has many features like result tuples (which can be used here). My attempt at coding shows one thing: it is ugly!

Maybe someone with actual experience can brew a better solution.

The first schematic attempt would be an expression with alternatives:

        GetProcessOnComputer
        -or TestPath($IRRUpdateFilePath)
        -or SearchIRRApplication
        -or CannotRemoteFolderReadWrite($RemoteFolderPath)
        -or TestFileLock($RemoteFilePath)

The couple of in-/output parameters ($Failed, $Success, $PCname and of course true/false continuing) make it awkward. In OO one might use fields. Here you could use an array of functions typed with the correct parameters, to be called, which you then walk through, doing a call and then continue or stop when the boolean result is true.

In the following () -> {...} stands for an anonymous function.

[StepFunction] $StepFunctions = {
        () -> GetProcessOnComputer,
        () -> TestPath($IRRUpdateFilePath),
        () -> SearchIRRApplication,
        () -> CannotRemoteFolderReadWrite($RemoteFolderPath),
        () -> TestFileLock($RemoteFilePath),
};

A high level utility function would be nice:

       function Step($ActionFunc, $TestFunc, $LogMsgFunc, $FailSuccFunc) {
            $r = $ActionFunc();
            if ($TestFunc())) {
                Log -Msg $LogMsgFunc;
                $FailSuccFunc() += $Context.PCname;
            }
        }
        

Then an actual step would look like:

        function GetProcessOnComputer($Context) {
            $ProcessList = Step(
                () -> {Get-Process -ComputerName $Context.PCname -ErrorAction SilentlyContinue; $ProcessList}, 
                () -> {$null -eq $ProcessList},
                () -> {"Error - Get process list from host failed, Host down? Skipping." -PCname $Context.$PCName},
                () -> {$Context.$Failed});

In functional programming this would be okay, but here it might be a disappointment.

\$\endgroup\$
2
  • \$\begingroup\$ Thanks! Can you include a little pseudo code of what 'main' could look like if done in the way which you have described? \$\endgroup\$
    – Mike Q
    Dec 10, 2021 at 15:22
  • \$\begingroup\$ The peseudo gets across the point nicely, I actually originally had a function for everything but remove it to simplify things a bit. Anyway this is some great food for thought! \$\endgroup\$
    – Mike Q
    Dec 10, 2021 at 20:08

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.