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 @echo off
, 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"
or 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.
Script-specific suggestions
%date%
and 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.
When using 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.
Consider changing 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 goto compress
?
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.)
:compress
"%zip%" -tzip a -y "bkp-%lastmonthfiles%.zip" %lastmonthfiles%* && (
DEL /q %lastmonthfiles%*
echo Zipping complete. Press any key to exit.
pause >NUL
goto :EOF
) || (
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
goto compress
)
A note about the &&
and ||
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.