Script is re-reading arguments

Question

When I supply the script with the argument: hi[123].txt it will do exactly what I want. But if I specify the wildcard character ( hi*.txt ) it will be re-reading some files.

I was wondering how to modify this script to fix that silly problem:

#!/bin/sh

count="0"
total="0"
FILE="$1"  #FILE specification is now $1 Specification..

for FILE in $@
do
  #if the file is not readable then say so
     if [ ! -r $FILE ];
         then
         echo "File: $FILE not readable"
     exit 0
     fi


# Start processing readable files
  while read line
          do

             if [[ "$line" =~ ^Total ]];
                 then
                   tmp=$(echo $line | cut -d':' -f2)

                   total=$(expr $total + $tmp)

                   echo "$FILE (s) have a total of:$tmp "
                   count=$(expr $count + 1)

             fi

        done < $FILE
done
echo " Total is: $total"
echo " Number of files read is:$count"

Answer 1

I don't know what is wrong with it, but one little point i noticed:

Change for FILE in $@ into for FILE in "$@" . Because if files have embedded spaces, you are now on the safe way. It will expand into "$1" "$2" ... then, instead of $1 $2 ... (and note everywhere you use $FILE too remember to "" it).

And what others say, you don't need to initialize FILE before you enter the loop. It will be set to each of the filenames of the expanded positional parameters in the for loop automatically.

However, i would go with an awk script like this:

awk -F: '
/^Total/ { 
    total += $2
    # count++ not needed. see below
    print FILENAME "(s) have a total of: " $2
} 

END { 
    print "Total is: " total
    print "Number of files read is: " (ARGC-1) 
}' foo*.txt

Note that when a file contains multiple "^Count" lines, you would indeed say you read more files than you actually read if you rely on count to tell you the number of files read.

Answer 2

How about this solution:

for FILE in `/bin/ls $@`
do
. . .

This will effectively eliminate duplicates because /bin/ls hi1.txt hi1.txt hi1.txt should only show hi1.txt once.

Though I'm not sure why it's re-reading files. The wildcard expansion should only include each file once. Do you have some files matched by hi*.txt that are links to files matched by hi[123].txt?

Answer 3

This seems redundant:

FILE="$1"  #FILE specification is now $1 Specification..

for FILE in $@
  ...

The initial assignment is promptly overwritten.

On the whole this seems to be a task better suited to a line processing language like awk or perl.

Consider something along the lines of this awk script:

BEGIN{
   TOTAL=0;
   COUNT=0;
   FS=':';
}
/^Total/{
   TOTAL += $2;
   COUNT++;
   printf("File '%s' has a total of %i",FILENAME,TOTAL);
}
END{
   printf("Total is %i",TOTAL);
   printf("Number of files read is%i",COUNT);
}

Answer 4

On error, exit with a non-zero status. Also on error, report errors to standard error, not standard output - though that may be a bit advanced for you as yet.

echo "$0: file $FILE not readable" 1>&2

The 1 is theoretically unnecessary (though I remember problems with a shell implementation on Windows if it was omitted). Echoing the script name '$0' at the start of the error message is a good idea too - it makes error tracking easier later when your script is used in other contexts.

I believe this Perl one-liner does the job you are after.

perl -na -F: -e '$sum += $F[1] if m/^Total:/; END { print $sum; }' "$@"

I understand that you are learning shell programming, but one of the important things with shell programming is knowing which programs to use.