Re: [tlug] banal bash script opinion

[Josh Glover <jmglov@example.com>]

Quoth Pietro Zuco (Sat 2003-05-17 05:37:05PM +0200):

> I wanted to write a script that I pass the name of a file, the new name I 
> want it had and a number indicating the number of copies of that file with 
> the new names that I want.
> 
> Do it for me thanks ..

Oh har har. You are lucky that I hit PgDn, or your flame-retardent overalls
would have been put to the test. ;)

> I wrote the script and it worked well for my needs. I just wanted to know 
> some suggestions about it, if it could be made in less lines.
> I know this is a banal script but I'm learning bash scripting.

OK, here are my suggestions. Note that I am a big fan of readability, even
(or maybe especially) in [shell] scipts.

> I pass the name of an existing file, the new name for the copies and the
> numer
> 
> ./scriptname filename.extension newfilename.extension number_of_copies

First of all, this invocation leads to (presumably) undesired behaviour:

: jmglov@example.com; ./old.sh foo.sh bar.sh 3
: jmglov@example.com; ls *.sh
bar.sh0.sh  bar.sh1.sh  bar.sh2.sh  cnt-comments.sh  foo.sh  new.sh  old.sh


This is probably what we want the user to do:

: jmglov@example.com; ./old.sh foo.sh bar 3
: jmglov@example.com; ls *.sh
bar.sh0.sh  bar.sh2.sh  bar1.sh  cnt-comments.sh  new.sh
bar.sh1.sh  bar0.sh     bar2.sh  foo.sh           old.sh


So, the first order of business is to provide the user with a good, clear
usage message.

The main way to improve your script is to merge the two cases (where there
is an extension and where there is none) by adding a '.' to the beginning
of the extension if there is one. Then, you can happily add the extension
to the new filename unconditionally, if there is none, it is just the empty
string.

So, after my re-write, we have a script that is easier to read and only
three nanoseconds slower on my trivial test case:

: jmglov@example.com; dd if=/dev/urandom of=test-one.case bs=512 count=1
: jmglov@example.com; dd if=/dev/urandom of=test-two.case bs=512 count=1
: jmglov@example.com; ls -l *.case
-rw-rw----    1 jmglov   jmglov        512 May 18 11:26 test-one.case
-rw-rw----    1 jmglov   jmglov        512 May 18 11:26 test-two.case
: jmglov@example.com; time ./old.sh test-one.case test-old 100

real    0m0.400s
user    0m0.140s
sys     0m0.250s
: jmglov@example.com; time ./old.sh test-two.case test-new 100

real    0m0.403s
user    0m0.180s
sys     0m0.220s


Here is my take on your script:

-------------------------------------------------------------------------------
#!/bin/bash


# Usage message
USAGE="Usage: $0 <oldfile> <newfile> <num>"

# Control variables
oldfile=""      # Original filename
newfile=""      # New filename
copies=0        # Number of copies
ext=""          # Optional extension


# If the first argument ($1) is null, spew the usage message and exit with
# an error code
function checkArg
{
  if [ -z "$1" ]; then
    echo $USAGE
    exit 255
  fi

} # checkArg()


# Handle arguments

# Make sure we have a valid arg
checkArg $@

# Maybe the user is sending a cry for help?
if [ "$1" == "-?" -o "$1" == "--help" ]; then
  echo $USAGE
  exit 0
fi

# OK, grab the args that we expect to see
oldfile="$1"
shift
checkArg $@
newfile="$1"
shift
checkArg $@
let "copies=$1"
shift


# Is there an extension?
ext=$(echo $oldfile | sed -n -e 's/[[:print:]]*\.//gp')
if [ -n "$ext" ]; then ext=".$ext"; fi

# Copy the file
i=0
while [ $i -lt $copies ]; do
  cp $oldfile $newfile$i$ext
  let "i++"
done
-------------------------------------------------------------------------------
 

-- 
Josh Glover <jmglov@example.com>

Associate Systems Administrator
INCOGEN, Inc.
http://www.incogen.com/

GPG keyID 0x62386967 (7479 1A7A 46E6 041D 67AE  2546 A867 DBB1 6238 6967)
gpg --keyserver pgp.mit.edu --recv-keys 62386967

Attachment: pgp00049.pgp
Description: PGP signature