Printing a directory's contents in a loop

Question

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

        if [ "$repeatTime" = "0" ]
        then
                for a in $choice
                do
                eval temp=\${${a}}
                        cat $SYSPATH${temp}
                done

                break;
        else
                for a in $choice
                do
                eval temp=\${${a}}
                        cat $SYSPATH${temp}
                done

                sleep $repeatTime
        fi

done

Answer 1

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}"

Answer 2

A few suggestions at your disposal: You can make use of getopts instead of getopt to avoid shift operation. You can make use of a simple flag to make sure that the mandatory -c option has been passed. Also from this post you can make use of any of the solutions for check for numeric value. Maybe something on these lines:

...
usage(){
...
}
...
#Process arguments passed
c_set=false  # Flag to check if mandatory -c is passed or not
repeatTime=0 # Set repeatTime value to 0 which will be default

while getopts ":c:l:h" opt; do
case "$opt" in
    c)
        # Check is borrowed from the mentioned link
        ! [[ "$OPTARG" =~ ^[0-9]+$ ]] && echo "Non-numeric passed to option:\"-c\"" && usage
        # if can be used instead of the above one-liner as posted in the link
        choice="$OPTARG"
        c_set=true

    ;;
    l)
        ! [[ "$OPTARG" =~ ^[0-9]+$ ]] && echo "Non-numeric passed to option:\"-l\"" && usage
        repeatTime="$OPTARG"
    ;;
    h)
        usage
    ;;
    *)
        echo "Invalid parameters passed"
        usage
    ;;
esac
done

# If the mandatory option not provided exit
# This will be false even if no arguments passed
[[ $c_set != true ]] && usage
#Actual operation
set -- $list
...

Edit: Added a version with getopt using the tips from tldp

...
c_set=false  # Flag to check if mandatory -c is passed or not
repeatTime=0 # Set repeatTime value to 0 which will be default

args=$(getopt :c:l:h "$@") || usage
eval set -- "$args"

while [ ! -z "$1" ]
do
  case "$1" in
    -c)
        # One liner in case of -l can be used instead of this "if"
        if ! [[ "$2" =~ ^[0-9]+$ ]];then
            echo "Non-numeric passed to option:\"-c\"" && usage
        fi
        choice="$2"
        c_set=true
    ;;
    -l)
        # "if" in case of -c can be used instead of this one-liner
        ! [[ "$2" =~ ^[0-9]+$ ]] && echo "Non-numeric passed to option:\"-l\"" && usage
        repeatTime="$2"
    ;;
    -h)
        usage
    ;;
  esac

  shift
done

[[ $c_set != true ]] && usage
...

Improvements/comments/remarks are welcome. Hope this helps!