Improving efficiency and flow by changing subroutine orders

Question

A batch file executes WinSCP.exe's console mode to download files and log the actions. Upon exit, WinSCP returns:

• ERRORLEVEL 0 if the files are successfully retrieved or
• ERRORLEVEL 1 if there is a problem with the connection - time outs, etc. - or if no file is found
• Unexplainably, WinSCP will occasionally return 0 when no files are found

Due to this, to meet audit requirements, I must check for four possibilities:

• ERRORLEVEL 1 and a connection problem
• ERRORLEVEL 0 and the file(s) exist(s)
• ERRORLEVEL 1 or ERRORLEVEL 0 and no file found

And respond with one of three options (respectively):

• Send email indicating error
• Send email indicating success
• Send email indicating no file

Note, the same group who receives the success notice, should also receive the no file notice as that is not download failure. The code reflects this.

I wrote the fully functional code below but am not sure it's as streamlined as it could be. My first concern is that I can improve flow by using GOTO and perhaps CALL or :EOF commands in more appropriate ways as I can't seem to find a better 'structure' for the conditions/responses. My goal is to eliminate un-necessary spaghetti-ing. Second, I am troubled to find a better way to reset the ERRORLEVEL to zero so I can use an ERRORLEVEL check twice (currently the %comspec%... line). I'm not even sure I need two checks. I appreciate all suggestions.

The code I'm concerned with begins the line after START /WAIT... but I present it in full, for reference:

SET [email protected]
SET [email protected]
SET [email protected]
SET MailSrcName=Automated FTP Process
SET OKMailSub=Files Have Been Moved
SET NoFileMailSub=Files Not Present - Maybe Next Time
SET FailMailSub=An Error Occurred!

IF EXIST WSCPLog.txt DEL WSCPLog.txt
SET LOG=WSCPLog.txt

START /WAIT WinSCP.exe /console /script=WSCP-GetC.txt /log=%log%
IF ERRORLEVEL 1 GOTO erroryes
IF NOT EXIST "\\server\path\Test_File.ext" GOTO nofilemail
GOTO okmail

:erroryes
%comspec% /c exit %0%
FINDSTR /C:"Test_File.ext: No such file or directory" %LOG%
IF ERRORLEVEL 0 GOTO nofilemail
IF EXIST %temp%\rdcto.txt DEL %temp%\rdcto.txt
FOR %%a IN (%FailMailDst%) DO ECHO %%a>>%temp%\rdcto.txt
"%~dp0sendmail.exe" /TO=%temp%\rdcto.txt /FROM=%MailSrcName% ^<%MailSrc%^> /REF=%FailMailSub% /MESSAGE=%log% /HOST=mail.mydomian.com
GOTO end

:nofilemail
IF EXIST %temp%\rdcto.txt DEL %temp%\rdcto.txt
FOR %%a IN (%MailDst%) DO ECHO %%a>>%temp%\rdcto.txt
"%~dp0sendmail.exe" /TO=%temp%\rdcto.txt /FROM=%MailSrcName% ^<%MailSrc%^> /REF=%NoFileMailSub% /MESSAGE=%log% /HOST=mail.mydomain.com
GOTO end

:okmail
IF EXIST %temp%\rdcto.txt DEL %temp%\rdcto.txt
FOR %%a IN (%MailDst%) DO ECHO %%a>>%temp%\rdcto.txt
"%~dp0sendmail.exe" /TO=%temp%\rdcto.txt /FROM=%MailSrcName% ^<%MailSrc%^> /REF=%OKMailSub% /MESSAGE=%log% /HOST=mail.mydomain.com

:end

Answer 1

1) What is this line supposed to do?

%comspec% /c exit %0%

I can almost guarantee it is not doing what you think. The most it can do is set the ERRORLEVEL. However, The EXIT command expects a numeric argument, but you pass %0%. The EXIT command returns 0 if it does not receive a numeric argument.

I doubt you have a variable with a name of "0", so the EXIT command sees a string literal value of %0%, which is non-numeric. It consequently returns 0 every time.

Your very next command (FINDSTR), is an external command, so it will set the ERRORLEVEL anyway. There is no need to initialize it.

2) It is generally a good idea to enclose SET assignments in quotes. This guards against poison characters, and inadvertent trailing spaces. (There are some exceptions, but it is a good rule of thumb).

3) It is generally a good idea to always enclose file paths in quotes if a portion is a variable. You never know if the variable may contain spaces or poison characters. The %TEMP% value normally does not have spaces, but it certainly could.

4) There is no need to delete "%temp%\rdcto.txt" when you immediately turn around and recreate it. You can simply redirect using overwrite mode (>) instead of append mode (>>). This will guarantee you start out with a fresh file each time.

5) Why do you write the value of %MailDst% using a FOR statement? You can simply echo the value directly.

>"%temp%\rdcto.txt" echo %MailDst%

6) You do not need to create an :end label and GOTO that label. Instead you can do either of the following: goto :eof or exit /b. They are identical, and neither form requires an explicitly declared label. The only difference is that exit /b can take an optional argument specifying the returned errorlevel.

7) You should encapsulate the logic for writing an email into a subroutine that accepts the variable portion as arguments. It reduces the amount of code, and eases maintenance.

8) I've simplified your ERRORLEVEL logic by using an extra variable to store the return code. I also make use of the conditional && and || operators with FINDSTR - I find them to be more convenient then checking the ERRORLEVEL. But I didn't use them with WinSCP because the logic is a bit complex.

Note that if the last command within a &&( successCommands ) block raises an error, then it would cause the ||( errorCommands ) block to fire. That is why my :sendMail routine explicitly exits with a return code of 0 (success).

Using the above principles, I would write the code as follows:

SET "[email protected]"
SET "MailSrcName=Automated FTP Process"

SET "[email protected]"
SET "[email protected]"

SET "OKMailSub=Files Have Been Moved"
SET "NoFileMailSub=Files Not Present - Maybe Next Time"
SET "FailMailSub=An Error Occurred!"

SET "LOG=WSCPLog.txt"

IF EXIST "%LOG%" DEL "%LOG%"
START /WAIT WinSCP.exe /console /script=WSCP-GetC.txt /log="%log%"
SET "RTN=%ERRORLEVEL%"
FINDSTR /C:"Test_File.ext: No such file or directory" "%LOG%" && (
  CALL :sendMail "%MAILdST%" "%NoFileMailSub%"
) || (
  IF %RTN% EQU 0 (
    CALL :sendMail "%MAILdST%" "%OKMailSub%"
  ) ELSE (
    CALL :sendMail "%FailMailDst%" "%FailMailSub%"
  )
)
EXIT /B

:sendMail
>"%temp%\rdcto.txt" ECHO %~1
"%~dp0sendmail.exe" /TO="%temp%\rdcto.txt" /FROM=%MailSrcName% ^<%MailSrc%^> /REF=%~2 /MESSAGE=%log% /HOST=mail.mydomian.com
EXIT /B 0

---

Other possible changes:

I didn't make any of the changes below because you said your code was working, and I don't know those programs enough to guarantee the changes are an improvement.

A) I don't see why you run WinSCP via START. I believe you could simply use:

WinSCP.exe /console /script=WSCP-GetC.txt /log=%log%

B) Your sendmail arguments look very suspicious to me. Normally values that contain spaces should be enclosed in quotes. Note that the :sendMail routine above preserves the lack of quotes by using %~1 and %~2 instead of %1 and %2.

C) I don't know WinSCP, but I suspect it will overwrite any existing log file, in which case you could remove the prior DEL command.