Printing a directory's contents in a loop

I wrote a bash script to print contents of files in a given directory in a loop.

How can I improve command line parameters handling to the script? I feel the parameter processing as of now uses too much code and it is quite clumsy. I think I had to write too much code to validate the the option arguments are numeric.

The script takes either -c numeric_choice_id and/or -l time_in_seconds and keep printing the contents of the files.

• only -c number: it cats the file and ends

• only -l number: repeatedly cats all the files

• -c num1 and -l num2: repeatedly cats that particular file every num2 seconds

``
#!/bin/bash

SYSPATH="$PWD/"

FILES="$PWD/file1
$PWD/file2
$PWD/file3"

count=0
for i in $FILES
do
(( count++ ))
menu="${menu}\n${count}. basename $i"
list="${list} basename $i"
done

testNumeric()
{
if ! echo "$1" | grep -q '^[0-9]\+$'
then
echo "$0 Non numeric argument supplied to choice or time"
exit
fi

}

usage()
{
echo -e "basename $0 -c numeric_choice [ -l time in seconds ]\n
-c numeric id for the choice
-l print o/p every time seconds
-h print this menu

choice could be: \n$menu"

exit
}

# Test if any arguments have been provided
if [ "x$1" = "x" ] || [ "$1" = "-h" ]
then

usage
fi

# Allow getopt to parse the options and report error if needed
args=getopt "c:l:" "$@"

if [ "$?" -ne "0" ]
then
usage
fi

# Set positional parameters
set -- $args

for i;
do
case "$i" in
-c )
choice=$2
testNumeric $choice
shift
shift ;;

-l ) repeatTime=$2
testNumeric "$repeatTime"
shift
shift ;;

-- )
break ;;
esac
done

[ "x$choice" = "x" ] && choice=seq $count`
[ "x$repeatTime" = "x" ] && repeatTime=0

set -- $list

while [ true ]
do

Storing a list of file names and iterating over it

FILES="$PWD/file1
$PWD/file2
$PWD/file3"

So you're setting FILES to a newline-separated list of files. That's a bad idea because your script will choke if a file name contains a newline. In some circumstances, where you control the file names, that can be ok, but don't do it unless you really have no better way. Since you're writing a bash script, you never need to do this. Store the file names in an array instead:

FILES=("$PWD/file1" "$PWD/file2" "$PWD/file3")

Then, to iterate over the array:

for i in "${FILES[@]}"; do …

If you had a shell without arrays and decided to use a newline-separated list of file names, then you would need some extra effort to split the lines at newlines only. The IFS variable specifies where to split words in unquoted variable substitutions, so you need to set it to contain only a newline (the default value also contains the ordinary space character and the tab character). Also, the shell performs globbing (filename generation from wildcards) on the results of unquoted variable substitutions, so you need to turn that feature off. Here's a POSIX-compliant way of iterating over the pieces of a newline-delimited string:

IFS='
'
set -g
for i in $FILES; do …

Since you never change directories, you could store just the files' base names instead of full paths. If you control the file names, you then don't need to be so careful with special characters in the names.

Always use double quotes around variable substitutions

I see a lot of places where you write unquoted variable substitutions, e.g. basename $i. Do you really want to split $i into words, then perform globbing on the result? If you don't, then use double quotes: "$i". Only leave off the double quotes if you intend to perform word splitting and filename generation (as in the for loop example above), and then be mindful of the assumptions you're making on the value of the variable.

On the same note, if there was a risk that $i began with a -, you would need to call basename -- "$i" to avoid $i being interpreted by basename as an option. This is not going to happen with your script since $i is an absolute path, beginning with /.

I also recommend not using the backquote command susbsitution, which behaves strangely with nested quotes sometimes. Use $(…), which has an intuitive syntax.

menu="${menu}\n${count}. $(basename -- "$i")"

Incidentally, note that this puts the characters \n into menu, not a newline. The \n is expanded by echo -e below.

Testing for a number

if ! echo "$1" | grep -q '^[0-9]\+

No need to call grep for that! Here's a simple bash construct:

if [[ $1 = *[^0-9]* ]]; then … # non-numeric


Here's a POSIX construct:

case $1 in
          *[!0-9]*) …;; # non-numeric
        esac


Here documents

To insert a multiline string in a script, use a here document. It's more readable.

usage()
{
     cat <<EOF
$(basename -- "$0") -c ID [-l TIME]
-c ID     numeric id for the choice   
-l TIME   print output every TIME seconds
-h        print this menu 

ID could be:
$menu
EOF
}


It's conventional to use upper case for argument descriptions in the symbolic syntax. Clearly indicate the options that take an argument in the left-hand column. Don't use obscure abbreviations that barely save a few characters like o/p (I think you meant “output”, but I'm not sure).

Parsing the command line

Use the shell builtin getopts rather than the external command getopt. If you have bash, you have getopts. If you don't have getopts, then you're running an embedded shell which may lack other features, so check that the other features you want to use are supported.

To test if there are any arguments, check the number of arguments, in $#.

if [ $# -eq 0 ] || [ "$1" = "-h"]; then …


Actually, you don't need any special handling for -h. Treat it like any other option.

I'm not going to go over getopts usage, there are plenty of examples around.

Before the main loop

set -- $args
…
set -- $list


Remember what I wrote about unquoted substitutions above? Each of these commands performs word splitting and filename generation.

[ "x$choice" = "x" ] && choice=`seq $count`
[ "x$repeatTime" = "x" ] && repeatTime=0


You need to initialize choice and repeatTime before the argument parsing, unless you intended to allow them to be pulled from the environment.

The main loop

while [ true ]


This is a strange way of writing while true. You could also write while [ false ] or while ! [ ! true ]. Keep it simple.

eval temp=\${${a}}
                cat $SYSPATH${temp}


In bash, you can avoid the use of eval. Also, remember to double quote variable substitutions unless you have a good reason not to.

cat "$SYSPATH${!a}"

No need to call grep for that! Here's a simple bash construct:

%%CODEBLOCK_6%%

Here's a POSIX construct:

%%CODEBLOCK_7%%

Here documents

To insert a multiline string in a script, use a here document. It's more readable.

%%CODEBLOCK_8%%

It's conventional to use upper case for argument descriptions in the symbolic syntax. Clearly indicate the options that take an argument in the left-hand column. Don't use obscure abbreviations that barely save a few characters like o/p (I think you meant “output”, but I'm not sure).

Parsing the command line

Use the shell builtin getopts rather than the external command getopt. If you have bash, you have getopts. If you don't have getopts, then you're running an embedded shell which may lack other features, so check that the other features you want to use are supported.

To test if there are any arguments, check the number of arguments, in $#.

%%CODEBLOCK_9%%

Actually, you don't need any special handling for -h. Treat it like any other option.

I'm not going to go over getopts usage, there are plenty of examples around.

Before the main loop

%%CODEBLOCK_10%%

Remember what I wrote about unquoted substitutions above? Each of these commands performs word splitting and filename generation.

%%CODEBLOCK_11%%

You need to initialize choice and repeatTime before the argument parsing, unless you intended to allow them to be pulled from the environment.

The main loop

%%CODEBLOCK_12%%

This is a strange way of writing while true. You could also write while [ false ] or while ! [ ! true ]. Keep it simple.

%%CODEBLOCK_13%%

In bash, you can avoid the use of eval. Also, remember to double quote variable substitutions unless you have a good reason not to.

%%CODEBLOCK_14%%

StackExchange Code Review Q#8706, answer score: 4