Well, first off, I must commend you on the readability of your code. You've employed nice spacing and enough comments to be able to find different sections easily without a lot of reading. I think I could learn a thing or two from you here. Nice job!
There are a few things you can do to improve your code, though. Let's start with the general practices first.
General practice advice
Unless your script is intended to set environment variables for other scripts or applications, you should always use
setlocal. Even if the script you're writing is intended to append a new directory to
%PATH%, you should still
setlocal at the top until your internal flow is complete and you're ready to commit the change to
%PATH%. This way you don't pollute your environment with a bunch of variables that only have meaning within a particular script -- or worse, have meaning in a different script that expects the variable not to be defined yet. Whenever you
setlocal should automatically be the next thing you type.
When setting variables to string values, it's good practice to
set "varname=string" with the quotation marks surrounding both the variable and its value. That way, whenever you use your variable later, there's no ambiguity whether your
variable=value. Also, in a future script, you might capture special characters into a variable, like an ampersand or a percent. Get into the habit of
set "variable=value" now and you won't have to change your coding style for special cases like that, and you'll spend less time debugging.
Likewise, in your
if statements, you should enclose the items on both sides of your comparison operator.
if "%foo%" equ "%bar%", or
if "%%~xI"==".exe". I can't count all the times as a rookie scripter I would struggle with errors when
%foo% contained a space, causing "blah was unexpected at this time" because I didn't use quotes.
set /a has some shorthand syntax you might find helpful. Interestingly, when you're doing math with variables, you don't have to use
% around the variable names. For example, instead of
set /a year=%year%-1 you can
set /a year=year-1. You can also combine operator and assignment like
/=, etc. So instead of
set /a year=%year%-1 you can
set /a year-=1.
Now, there are a few issues specific to this script that can be improved.
date /t are ambiguous. Some locales list date as
MM/DD/YYYY, while others use
DD/MM/YYYY, and still others use
YYYY/MM/DD. (more information.) A more agnostic way of scraping the date would be to use
wmic. See Method 2 on this page for a way to put the date into variables that should work more universally.
del with a wildcard, consider adding the
/q switch to suppress confirmation, unless you intentionally want your script to ask the user to confirm deletion.
ping -n 5 192.168.100.44 > nul to
ping -n 5 0.0.0.0 >NUL. Having an actual IP there might (at at glance) prompt the reader to wonder whether the script will behave differently whether the host does or does not respond; whereas
0.0.0.0 makes it obvious that you're using the
ping command as nothing more than a period of sleep.
if Not exist bkp-%lastmonthfiles%.zip <-- If this is ever true, you are going to
pause twice, then exit. Examine your logic here. Did you leave out a
What it looks like you intended to do is attempt to zip; then if the zip file doesn't exist, echo a notice to the user, pause, then loop back to
:compress to try again. Otherwise, assume everything went fine and delete all the old stuff and exit. What happens if a file is in use and locked, and 7-zip skips archiving it but was otherwise successful with the other log files?
bkp-%lastmonthfiles%.zip still exists, and your script could potentially delete the file that was skipped.
If I may make yet another suggestion, you should rewrite the end of your script to take advantage of 7-zip's exit codes. Try this instead. (Note:
"%zip%" is in quotes on the assumption that you followed "General practice advice #2" above.)
"%zip%" -tzip a -y "bkp-%lastmonthfiles%.zip" %lastmonthfiles%* && (
DEL /q %lastmonthfiles%*
echo Zipping complete. Press any key to exit.
) || (
if ERRORLEVEL 2 (
echo Zipping failed ^(exit status %ERRORLEVEL%^). Trying again in 5 seconds...
) else (
echo Zip completed with warnings ^(most likely because a file was locked by another
echo process and had to be skipped^). Trying again in 5 seconds...
del "bkp-%lastmonthfiles%.zip" >NUL 2>&1
ping -n 6 0.0.0.0 >NUL
A note about the
|| notation there: That's shorthand code for testing the exit code of the command preceding it.
program.exe && success || fail. See conditional execution for more details on how this works.