current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 4 down vote favorite

I run a script to download a file and unzip it. An import process then runs and leaves the files in their source location. I wrote the fully functioning code below to clean up that directory to be ready to use the next night.

I streamlined as much as I believe possible. I realize ~15 lines of code isn't exactly verbose or complicated and I've included the full script for reference. I feel, however, that specifically the second block - line SET DATE= through line DIR - can be better written but I don't know in what way. Am I simply seeing messy code where none exists? It just doesn't seem to 'read' well.

A note - the index.txt file is the same despite having two paths and is located in one or the other depending on the outcome (errors or not) of the import.

SET PATH="\\server\path\path2"
IF NOT EXIST %PATH%\Upload.zip GOTO Done

SET DATE=%date:~4,2%-%date:~7,2%-%date:~10,4%
MKDIR %PATH%\Archives\%date%
SET ARCHIVE=%PATH%\Archives\%date%\
DIR %PATH% /B > %ARCHIVE%log-%date%.txt

MOVE %PATH%\Upload.zip %ARCHIVE%Upload-%date%.zip
MOVE %PATH%\index.txt %ARCHIVE%index-%date%.txt ||(
MOVE %PATH%\error_files\index.txt %ARCHIVE%index-%date%.txt
)
DEL %PATH%\*.tif
RMDIR %PATH%\error_files

PUSHD %PATH%\Archives &&(
FORFILES /S /D -15 /C "CMD /C IF @isdir == TRUE RD /S /Q @path"
) & POPD

:Done
share|improve this question
1  
I think & separates two commands. Regarding the last lines: Is there a difference between command1 && (command2) & command3 and command1 && (command2 & command3 ) and comman1 && command2 & command3 ? When will command3 be executed? –  miracle173 Aug 28 '14 at 23:57
    
I used an existing snippet to create that and more recently began to understand it. As such, I am still questioning the use of && as opposed to listed three separate commands. I realize it will run command2 and command3 only if command1 is successful, but I can't think of an instance where PUSHD would fail. –  UserUnknown Aug 29 '14 at 14:54

2 Answers 2

up vote 3 down vote accepted

I don't think there's a better way to format your %DATE% variable. And be careful, because I think this won't work in all locales. I had this problem a few years ago, and I remember that there was no good solution to format dates using native commands. I had to make a simple dateformat utility myself to generate date formats easily and in a locale independent way.

Just a small thing, I recommend NOT to put the trailing \ in the %ARCHIVE% variable. So instead of this:

SET ARCHIVE=%PATH%\Archives\%date%\
DIR %PATH% /B > %ARCHIVE%log-%date%.txt

MOVE %PATH%\Upload.zip %ARCHIVE%Upload-%date%.zip

Write like this:

SET ARCHIVE=%PATH%\Archives\%date%
DIR %PATH% /B > %ARCHIVE%\log-%date%.txt

MOVE %PATH%\Upload.zip %ARCHIVE%\Upload-%date%.zip

This may be a matter of taste, but I like to see explicitly that it's a directory there, not just a prefix.

Another formatting tip about this:

MOVE %PATH%\index.txt %ARCHIVE%\index-%date%.txt ||(
MOVE %PATH%\error_files\index.txt %ARCHIVE%\index-%date%.txt
)

I find it easier to read when the commands inside (...) are indented, like this:

MOVE %PATH%\index.txt %ARCHIVE%\index-%date%.txt || (
    MOVE %PATH%\error_files\index.txt %ARCHIVE%\index-%date%.txt
)
share|improve this answer
1  
I follow the logic regarding the trailing back-stroke - I agree. Looking at the code again, it's also inconsistent with my other variables so I should change for that reason, as well. And the indent! I always forget that - thanks for the reminder. –  UserUnknown Aug 29 '14 at 14:35
up vote 3 down vote 
PUSHD %PATH%\Archives &&(
FORFILES /S /D -15 /C "CMD /C IF @isdir == TRUE RD /S /Q @path"
) & POPD

is the same as 

PUSHD %PATH%\Archives &&(
FORFILES /S /D -15 /C "CMD /C IF @isdir == TRUE RD /S /Q @path"
) 
POPD

but the latter syntax is simpler .

share|improve this answer
    
I'll make the update. This piece came from previous code; I never thought to review it. My thinking was "fewer lines are better". But the more I look at it, the latter is easier for other users to understand. Thank you for the direction, the research in to why both options work helps me learn. –  UserUnknown Aug 29 '14 at 14:43

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.