Skip to main content
1 of 4
Aesin
  • 226
  • 1
  • 3

In addition to @janos's points:

cat vs echo

You've used quite a few HEREDOCs in this script, some of them pretty short. I'd tend to use echo for these instead, which not only makes it more explicit that the output is a string, but keeps it all inline, e.g.:

    cat << EOF
   The xmllint executable is missing from your system.
   Please install the program first, before using this script.
EOF

could be converted to:

echo "The xmllint executable is missing from your system."
echo "Please install the program first, before using this script."

or

echo -e "The xmllint executable is missing from your system.\n" \
        "Please install the program first, before using this script."

You could also move these messages out into separate variables, which would also minimise their impact on the script.

This also has the questionable benefit that you're not relying on an external program to do the printing, since echo is a bash built-in.

Tests

Strictly speaking, when used with [, the use of == is a bashism which is not portable to other shells. Even in bash, it's the operator intended for use when comparing strings, rather than numbers.

Use = for strings, and -eq for bare numbers, and check help test for other test operators.

This brings me on to:

if [ -x $1 ]; then
    echo "A file containing a list of URL's is missing."
fi

I'm not quite sure why you're checking whether your file of URLs is executable. If you just want to check whether it exists, you want the -a operator, but it's a little more effective to check whether it exists and is readable, which is -r.

As shellcheck says, you should also double-quote these variables: this stops things like filenames with spaces breaking your script, and also forces empty variables to expand to empty strings, rather than expanding to nothing at all and causing syntax errors.

Errors

In a few places you've printed out error messages, but the script doesn't stop, it carries on regardless. Is this intentional?

Also, error messages should, in general, go to stderr.

I tend to define some exit codes and do something like:

# Define these all in one place
ERR_INVALID_ARGS=5

#Then later, if args are invalid
echo "Error: invalid arguments provided" >&2 # redirects to stderr
exit $ERR_INVALID_ARGS

More general stuff

You could, if you wanted, make the output a command-line option using getopt, allow any number of input files as an argument using a while loop and shift, and output to stdout by default.

You might also want to check whether the file exists before you write to it, and you could do something like read a Y/N check whether you want to overwrite, rather than always appending.

None of the above is really a big problem here. The script is reasonably structured, and your biggest worry (IMO) is that you could expand an empty variable and get a syntax error, or accidentally screw up an existing file.

Aesin
  • 226
  • 1
  • 3