Batch file to zip logs date wise

Question

This is the batch file I have created which zips the particular month logs and deletes those logs from the source folder after successful zipping. If it does not find the zip file, then it again starts to zip the logs.

Please suggest any changes to be done.

@echo off
set zip="C:\Program Files7円-Zip7円z.exe"
rem Findout Month, Year and Date
FOR /F "tokens=1,2,3,4 delims=/ " %%A in ('Date /t') do set year=%%D
FOR /F "tokens=1,2,3,4 delims=/ " %%A in ('Date /t') do set month=%%B
FOR /F "tokens=1,2,3,4 delims=/ " %%A in ('Date /t') do set day=%%C
rem Solve problem Year start and adding 0 in month
if %month% EQU 01 (
 set month=12
 set /a year=%year%-1
) else (
 set /a month=%month%-1
)
if %month% LSS 10 set month=0%month%
rem Set file names for last month file.
set lastmonthfiles=server.log.%year%-%month%-
rem To compress the file.
:compress
%zip% -tzip a -y "bkp-%lastmonthfiles%.zip" %lastmonthfiles%*
ping -n 5 192.168.100.44 > nul 
if Not exist bkp-%lastmonthfiles%.zip (
 echo zipping failed
 pause
) else ( 
 DEL %lastmonthfiles%*
 ) 
pause

Answer 1

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

1. 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.

2. 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.

3. 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.

4. 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

1. %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.

2. 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.

3. 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.

4. 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.

• Vaibhav Veralkar: Thanks for your suggestions. I'll do my changes. Thank you. @rojo.

Answer 2

SET /A is a powerful tool.

1. Spaces don't work as delimiters here. Therefore, they are completely ignored.
2. Carriage returns \r and newlines \n, like spaces, are STRIPPED. This can be useful, especially when dealing with the output returned from WMIC

3. You can use multiple statements inside one command, like this:

   set/a"month=12,year-=1"
   

   instead of

   set month=12
   set /a year=%year%-1
   

4. SET /A can expand variables during execution phrase (which means that it can access underlying dynamic variables, like __CD__), but this isn't useful here

The resulting script looks like this:

@echo off
setlocal EnableDelayedExpansion EnableExtensions
set "zip=%ProgramFiles%7円-Zip7円z.exe" WHERE 7z.exe
::GetDate via WMIC
set "getlocaltime="%__APPDIR__%wbem\wmic.exe" path win32_localtime get year, month, day /format:list|findstr "=""
FOR /F %%L in ('
"!getlocaltime!" %= DELAYED EXPANSION, don't need to escape anything =%
') do set/a%%L %= Remove trailing CR from WMIC's unicode output =%
::Special case when MONTH is January
if "%month%" == "01" (set/a"month=12,year-=1") ELSE set/a"month-=1"
::Pad zeroes to MONTH
set "month=0!month!"
set "month=!month:~-2!"
::Set file names for last month file
set "lastmonthfiles=server.log.%year%-%month%-"
:compress - compress the file
if EXIST "%zip%" (
 "%zip%" -tzip a -y "bkp-%lastmonthfiles%.zip" %lastmonthfiles%*
)
ping 127.1 -n 5 >nul
if EXIST "bkp-%lastmonthfiles%.zip" (
 del %lastmonthfiles%*
) ELSE echo zipping failed
pause