7
\$\begingroup\$

I have this PowerShell code that loops through Excel files in a specified directory; references a list of known passwords to find the correct one; and then opens, decrypts, and saves that file to a new directory. This currently processes ~40 workbooks in roughly 5 minutes, which is still longer than I'd like (but better than the initial 40 minute runtime).

Note that I previously posted under the same title here, but now I am looking for a review after solving a rather obvious error.

# Get Current EXCEL Process ID's so they are not affected but the scripts cleanup
# SilentlyContinue in case there are no active Excels
$currentExcelProcessIDs = (Get-Process excel -ErrorAction SilentlyContinue).Id

$a = Get-Date

$ErrorActionPreference = "SilentlyContinue"

CLS

# Paths
$encrypted_path = "C:\PoShTest\Encrypted"
$decrypted_Path = "C:\PoShTest\Decrypted\"
$processed_Path = "C:\PoShTest\Processed\"
$password_Path  = "C:\PoShTest\Passwords\Passwords.txt"

# Load Password Cache
$arrPasswords = Get-Content -Path $password_Path

# Load File List
$arrFiles = Get-ChildItem $encrypted_path

# Create counter to display progress
[int] $count = ($arrfiles.count -1)

# New Excel Object
$ExcelObj = $null
$ExcelObj = New-Object -ComObject Excel.Application
$ExcelObj.Visible = $false

# Loop through each file
$arrFiles| % {
    $file  = get-item -path $_.fullname
    # Display current file
    write-host "`n Processing" $file.name -f "DarkYellow"
    write-host "`n Items remaining: " $count `n

    # Excel xlsx
    if ($file.Extension -like "*.xls*") {

    # Loop through password cache
        $arrPasswords | % {
            $passwd = $_

            # Attempt to open file
            $Workbook = $ExcelObj.Workbooks.Open($file.fullname,1,$false,5,$passwd)
            $Workbook.Activate()


            # if password is correct save decrypted file to $decrypted_Path
                if ($Workbook.Worksheets.count -ne 0 ) 

                {   
                    $Workbook.Password=$null
                    $savePath = $decrypted_Path+$file.Name
                    write-host "Decrypted: " $file.Name -f "DarkGreen"
                    $Workbook.SaveAs($savePath)

             # Added to keep Excel process memory utilization in check
                    $ExcelObj.Workbooks.close()

             # Move original file to $processed_Path
                    move-item $file.fullname -Destination $processed_Path -Force

                }
                else {
            # Close Document
                    $ExcelObj.Workbooks.Close()
                }
        }

    }



$count--
# Next File
}
# Close Document and Application
    $ExcelObj.Workbooks.close()
    $ExcelObj.Application.Quit()

Write-host "`nProcessing Complete!" -f "Green"
Write-host "`nTime Started   : " $a.ToShortTimeString()
Write-host "Time Completed : " $(Get-Date).ToShortTimeString()
Write-host "`nTotal Duration : " 
NEW-TIMESPAN –Start $a –End $(Get-Date)

# Remove any stale Excel processes created by this scripts execution
Get-Process excel -ErrorAction SilentlyContinue | Where-Object{$currentExcelProcessIDs -notcontains $_.id} | Stop-Process
\$\endgroup\$
8
\$\begingroup\$

Once you've found the right password and moved the file, then there is no need to try the rest of the passwords on it. You can break out of the loop with a break command.

(You have to change the pipeline % into a foreach ( in ) because otherwise the break would break out of the outer pipeline too. Thanks to Lieven in the comments for that.)

foreach ($passwd in $arrPasswords)
{
    # ...

    # if password is correct save decrypted file to $decrypted_Path
    if ($Workbook.Worksheets.count -ne 0 ) 
    {   
        # ...
        break; 
    }

    # ...
}

That may or may not speed things up.

Otherwise, I haven't tried them but I understand that there are .NET Excel libraries that you can use to manipulate Excel files without having to use Excel itself. COM automation is pretty slow, so using one of these libraries might be faster.

\$\endgroup\$
  • \$\begingroup\$ I'm not sure if it's me or the code but adding a break there exited the entire script and all further processing stopped. \$\endgroup\$ – SQL_Underworld Mar 20 '17 at 5:08
  • 1
    \$\begingroup\$ @SQL_Underworld - You are right, the break ends all pipelines. Replacing the $arrPasswords | % {with a foreach ($pwd in $arrPasswords) { should work or replace your break with return \$\endgroup\$ – Lieven Keersmaekers Mar 20 '17 at 5:47
  • 2
    \$\begingroup\$ Thanks @Lieven. I've updated the answer. I tested return just now, BTW. It doesn't break, it just returns for that one value. \$\endgroup\$ – Dangph Mar 20 '17 at 6:18
3
\$\begingroup\$

This will not make your processing faster but it might help make the code more readable.

  1. You can make the globa variables of the script params with default values. Thanks to this you can change them when using your script and it's clear they're global variables used by the script.

    param(
        $encrypted_path = "C:\PoShTest\Encrypted",
        $decrypted_Path = "C:\PoShTest\Decrypted\",
        $processed_Path = "C:\PoShTest\Processed\",
        $password_Path  = "C:\PoShTest\Passwords\Passwords.txt"
    )
    
  2. If you set the $ErrorActionPreference = "SilentlyContinue" at the beggining of the script it will be inherited by all cmdlets. But I'm not sure you want this $ErrorActionPreference for all cmdlets.

  3. Renaming variables:

    • $a to $startTime
    • $arrPassword to $password, I don't think there is any value in prefixing variables names with types
    • $arrFiles to $encryptedFiles
  4. Code formatting

    • No need for paranthesis: [int] $count = $encryptedFiles.count - 1
    • No need for this line $ExcelObj = $null
    • No need to Get-Item here since $_ is of type FileInfo already: $file = get-item -path $_.fullname. You can just rename it $encryptedFile = $_
    • Write-Host adds new lines by default so you don't need to prefix messages with `n
    • be consistent with brackets - open them always in the same line or always in the next line
    • You can use Join-Path it's clear then that you concatenating paths $savePath = Join-Path $decrypted_Path $encryptedFile.Name
    • I think it's a good idea to keep consistent casing - replace write-host with Write-Host
    • to get a TimeSpan between two dates you can just substract them: $startTime - $endTime
    • putting multiple pipelines in new lines make the processing readable

      Get-Process excel `
      | Where { $currentExcelProcessIDs -notcontains $_.id } `
      | Stop-Process
      
    • or as per @Matt suggestion:

      Get-Process excel |
          Where { $currentExcelProcessIDs -notcontains $_.id } |
          Stop-Process
      

The code with my changes is available here: http://pastebin.com/UCqXUYHU

\$\endgroup\$
  • 1
    \$\begingroup\$ putting multiple pipelines in new lines make the processing readable I have a bone to pick with that formatting choice. Lose the backticks and move the pipe up a line each, indent each following line. No need for backticks this way. \$\endgroup\$ – Matt Mar 28 '17 at 11:36
  • 1
    \$\begingroup\$ thanks @Matt, the formatting you've suggested is nice. I like both of them, don't know what annoys you in the backticks? \$\endgroup\$ – inwenis Mar 29 '17 at 13:21

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.