1
\$\begingroup\$

This is the batch file I have created which copy the specific folders which I want. I use the specific server folder name of which I want to copy. This batch file will also delete the folder which you don't want to copy. My batch file will ask for the user input for the source path as well the destination path and to name the folder. If the user does not provides the source and destination path and folder name it will take the default value. I have also added a command which will start the specific service which is stopped. The concept of starting the service is totally related to server where we use to down the server. I also wanted my code to ask whether to delete those logs.

Please help me regarding this script and please suggest any improvements.

@echo off
:: variables
echo This script takes the backup of file SwiftALM Important folders
set /P SOURCEPATHPATH=Enter SOURCEPATH example:D:\backup\test\swift (default SOURCEPATH path=D:\backup\test\swift):
 if "%SOURCEPATH%"=="" (
set SOURCEPATH=D:\backup\test\swift
) else (
 set SOURCEPATH=%SOURCEPATH%
)
set /P DESTPATH=Enter DESTPATH (default DESTPATH path=D:\backup4):
if "%DESTPATH%"=="" (
set DESTPATH=D:\backup4
) else (
 set DESTPATH=%DESTPATH%
)
set /P folder=Enter folder name:
echo default folder name=%Date:~-7,2%-%Date:~-10,2%-%Date:~-4,4%
if "%folder%"=="" (
set folder=%Date:~-7,2%-%Date:~-10,2%-%Date:~-4,4%
) else (
 set folder=%folder%
)
echo folder=%folder%
mkdir %DESTPATH%\%folder%
set xcopy=xcopy /E/V/Q/F/H/I 
echo echo conf folder will be copied
%xcopy% %SOURCEPATH%\conf %DESTPATH%\%folder%\conf
echo conf folder is copied
echo lib folder will be copied
%xcopy% %SOURCEPATH%\lib %DESTPATH%\%folder%\lib
echo lib folder is copied
echo deploy folder will be copied
%xcopy% %SOURCEPATH%\deploy %DESTPATH%\%folder%\deploy
echo deploy folder is copied
echo deployers folder will be copied
%xcopy% %SOURCEPATH%\deployers %DESTPATH%\%folder%\deployers 
echo deplyers folder is copied
echo files will be copy press enter to proceed
pause
echo Deleting folders after copy
rmdir /s %SOURCEPATH%\tmp 
rmdir /s %SOURCEPATH%\data 
rmdir /s %SOURCEPATH%\work 
echo folders have been removed
pause
echo To Start the task scheduler
sc start "Schedule"
pause
\$\endgroup\$
1
\$\begingroup\$

It seems you're not taking the advices from a previous reviews, which gave very good points:

  • Always use SETLOCAL, unless you really really really want variables to affect the environment of the calling process
  • It's good to use @ECHO even if you set ECHO OFF at the top, in case you might want to temporarily disable the ECHO OFF for debugging purposes
  • It's good to add some blank lines to separate related blocks and improve readability

Now on to this new code.

The path D:\backup\test\swift appears twice in this statement:

set /P SOURCEPATHPATH=Enter SOURCEPATH example:D:\backup\test\swift (default SOURCEPATH path=D:\backup\test\swift):

It's good to avoid repeating yourself, because you might want to change the value later, and then you'll have to change every occurrence, and risk the mistake of forgetting to change one of them. Extract duplicated strings into a variable, for example:

set DEFAULT_SOURCEPATH=D:\backup\test\swift
set /P SOURCEPATH=Enter SOURCEPATH example:%DEFAULT_SOURCEPATH% (default SOURCEPATH path=%DEFAULT_SOURCEPATH%):

Btw in the original code SOURCEPATHPATH looks like a typo (the ...PATHPATH at the end).

All the else blocks here look utterly pointless:

if "%SOURCEPATH%"=="" (
    ...
) else (
    set SOURCEPATH=%SOURCEPATH%
)
if "%DESTPATH%"=="" (
    ...
) else (
    set DESTPATH=%DESTPATH%
)
if "%folder%"=="" (
    ...
) else (
    set folder=%folder%
)

In all these examples, you set a variable to itself. That does nothing at all. It would be better to completely omit these else statements, and the code will be shorter.

A blank line between the treatment of SOURCEPATH, DESTPATH, folder would be nice for visual separation and readability.

Finally, I don't have a Windows with me now, but I think this would work and be a better way of implementing those if conditions:

if not defined SOURCEPATH (
    ...
)

if not defined DESTPATH (
    ...
)

if not defined folder (
    ...
)
\$\endgroup\$
  • \$\begingroup\$ thank you @janos. The if statement i have use in my code is just for user to give the input and if user does not gives any input then it will take the default value. This is what i thought. \$\endgroup\$ – Vaibhav Veralkar Dec 3 '14 at 5:41
0
\$\begingroup\$

You repeat code which makes the script hard to read. Why not use a subroutine to do repeated tasks?
Instead of

echo conf folder will be copied
%xcopy% %SOURCEPATH%\conf %DESTPATH%\%folder%\conf
echo conf folder is copied
etc.

do

CALL :CopyFolder conf
CALL :CopyFolder lib
CALL :CopyFolder lib
CALL :CopyFolder deploy
CALL :CopyFolder deployers
...
echo To Start the task scheduler
sc start "Schedule"
pause
REM new code from here on
GOTO :EOF

:CopyFolder
echo %1 folder will be copied
%xcopy% %SOURCEPATH%\%1 %DESTPATH%\%folder%\%1
echo %1 folder is copied
GOTO :EOF
\$\endgroup\$

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.