6
\$\begingroup\$

I've written a little script to download sequentially named files, rename them, and delete files smaller than an certain number of kilobytes. I came up with this but I'm not too happy. Any advice for me?

#!/bin/sh
echo
echo -n "Start file #:"
read start
echo
echo -n "End file #:"
read end
echo

wget --progress=bar:force http://www.mybox.com/pictures/show_pic.asp?src_id={$start..$end} 2>&1 | progressfilt

sleep 1

for file in *.asp
do
    mv "$file" "${file%.asp}.jpg"
done

find * -maxdepth 1 -name "*.jpg" -size -300k -delete

echo "tasks completed...exiting"
exit
\$\endgroup\$
2
\$\begingroup\$
  • sh is not bash. This shouldn't be a problem for your script though, because I don't think there are any bashisms in there.
  • Use More Quotes™. If there had been another parameter in the URL you would have needed an ampersand (&) in there, and that's the special character to run a command in the background. That's one of many ways to trip up if you don't comment basically any non-trivial string.
  • You don't need to sleep, because the wget commands are synchronous.
  • That said, you might get a result much faster if you run them in parallel, by doing several wget […] & commands followed by a wait.
  • You don't need to exit at the end of a shell script. That's only useful if you want to exit prematurely or with a different exit code from the previous command.
  • If you use Bash you can use read -p "Start file #: " start etc. to avoid most of the echoing.
  • However, it is much more common to make shell scripts take parameters instead of being interactive, making the script even simpler:

    start="$1"
    end="$2"
    
  • Use set -o errexit -o nounset (and -o pipefail if you're really in Bash) to make sure you catch any common bugs early.
  • I'm not sure what progressfilt does, but you can suppress the wget progress bar by using --quiet.
  • To be done slightly faster you should swap around the deleting and the renaming.
  • Rather than find * you should use find . to avoid exceeding the maximum argument length. This is a good practice in general, but will only really break with lots of files.
  • One very common feature of *nix scripts is that they are silent unless there's something important to report. Therefore, I'd remove the last echo.

Your script should now be 9 SLOC, and clean as a whistle!

\$\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.