HiveBrain v1.2.0
Get Started
← Back to all entries
patternMinor

Copying files into Dropbox folder from command line

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
dropboxlineintofilescopyingfoldercommandfrom

Problem

I made the following code.
Since it is a Batch, any kind of problems can appear.
Please review it and point any problems when copying files into Dropbox!
Its intent is that you will copy a file into an Dropbox folder.
Its syntax is dropbox .

@echo off
set dropbox=%userprofile%\dropbox
if exist %dropbox% (if not exist %dropbox%\%2 mkdir %dropbox%\%2) else (echo Get Dropbox for PC before using this!)
if exist %dropbox%\%2 copy %1 %dropbox%\%2
@echo on


It would have been cool to use the dropbox tag as well, right?

Solution

Your script will be a lot easier to read if you expand the long statements to multiple lines:

@echo off
set dropbox=%userprofile%\dropbox
if exist %dropbox% (
    if not exist %dropbox%\%2 mkdir %dropbox%\%2
) else (
    echo Get Dropbox for PC before using this!
)
if exist %dropbox%\%2 copy %1 %dropbox%\%2
@echo on


Is that @echo on at the end necessary? Probably not, but I don't have a Windows with me to play around to verify.

The last if statement is pointless to execute if the first if statement was false. It would be better to move this inside the first if statement:

@echo off
set dropbox=%userprofile%\dropbox
if exist %dropbox% (
    if not exist %dropbox%\%2 mkdir %dropbox%\%2
    if exist %dropbox%\%2 copy %1 %dropbox%\%2
) else (
    echo Get Dropbox for PC before using this!
)


Possibly an even better way is to use the equivalent of an early return,
by leaving the main execution path when something is wrong.
You can do this using goto :eof.
After this the else statement can be flattened:

@echo off
set dropbox=%userprofile%\dropbox
if not exist %dropbox% (
    echo Get Dropbox for PC before using this!
    goto :eof
)
if not exist %dropbox%\%2 mkdir %dropbox%\%2
if exist %dropbox%\%2 copy %1 %dropbox%\%2


Rather than using the meaningless symbols %1 and %2 throughout the script,
it would be better to put these values to variables with descriptive names, for example:

@echo off
set source=%1
set target=%2
set dropbox=%userprofile%\dropbox
if exist %dropbox% (
    echo Get Dropbox for PC before using this!
    goto :eof
)
if not exist %dropbox%\%target% mkdir %dropbox%\%target%
if exist %dropbox%\%target% copy %source% %dropbox%\%target%

Code Snippets

@echo off
set dropbox=%userprofile%\dropbox
if exist %dropbox% (
    if not exist %dropbox%\%2 mkdir %dropbox%\%2
) else (
    echo Get Dropbox for PC before using this!
)
if exist %dropbox%\%2 copy %1 %dropbox%\%2
@echo on
@echo off
set dropbox=%userprofile%\dropbox
if exist %dropbox% (
    if not exist %dropbox%\%2 mkdir %dropbox%\%2
    if exist %dropbox%\%2 copy %1 %dropbox%\%2
) else (
    echo Get Dropbox for PC before using this!
)
@echo off
set dropbox=%userprofile%\dropbox
if not exist %dropbox% (
    echo Get Dropbox for PC before using this!
    goto :eof
)
if not exist %dropbox%\%2 mkdir %dropbox%\%2
if exist %dropbox%\%2 copy %1 %dropbox%\%2
@echo off
set source=%1
set target=%2
set dropbox=%userprofile%\dropbox
if exist %dropbox% (
    echo Get Dropbox for PC before using this!
    goto :eof
)
if not exist %dropbox%\%target% mkdir %dropbox%\%target%
if exist %dropbox%\%target% copy %source% %dropbox%\%target%

Context

StackExchange Code Review Q#111483, answer score: 3

Revisions (0)

No revisions yet.