CODEWITHSUNDEEP
bashshell
Cristian
Cristian

Reputation: 7145

How Can I Improve This Script?

I'm learning shell scripting, and am finding it hard finding a good way to learn. I have created a script below which lets the user search various different Internet engines through options. I would be really grateful if someone could look through this and point out what I'm doing wrong, how to improve it, etc.

#!/bin/bash

## Get user search-engine option
while getopts aegwy: OPTIONS ; do
  case "$OPTIONS" in 
    a) ENGINE="http://www.amazon.com/s/ref=nb_sb_noss/?field-keywords";;
    e) ENGINE="http://www.ebay.com/sch/i.html?_nkw";;
    g) ENGINE="http://www.google.com/search?q";;
    w) ENGINE="http://en.wikipedia.org/wiki/?search";;
    y) ENGINE="http://www.youtube.com/results?search_query";;
    ?) ERRORS=true;;
  esac
done &>/dev/null

## Ensure correct command usage
[ $# -ne 2 ] || [ $ERRORS ] && printf "USAGE: $(basename $0) [-a Amazon] [-e eBay] [-g Google] [-w Wikipedia] [-y YouTube] \"search query\"\n" && exit 1

## Ensure user is connected to the Internet
ping -c 1 209.85.147.103 &>/dev/null ; [ $? -eq 2 ] && printf "You are not connected to the Internet!\n" && exit 1

## Reformat the search query
QUERY=`printf "$2" | sed 's/ /+/g'`

## Execute the search and exit program
which open &>/dev/null ; [ $? -eq 0 ] && open "$ENGINE"="$QUERY" &>/dev/null && exit 0 || xdg-open "$ENGINE"="$QUERY" &>/dev/null && exit 0 || printf "Command failed!\n" && exit 1

Thanks in advance everyone, means a lot!

Upvotes: 2

Views: 352

Answers (2)

Mark Reed
Mark Reed

Reputation: 95242

Best posted in codereviews, as indicated above, but here are some mostly stylistic comments. I should stress that the script is pretty much fine as-is; these are just minor improvements that I think will help make the code easier to read/maintain, more robust in a couple cases, etc.

You don't need to use all-caps for variable names just because environment variables are all-caps; shell variables and environment variables aren't the same thing.

Since your $OPTIONS variable only holds one option at a time, a singular name would be better (e.g. $option). Or you could go with $opt, which is somewhat traditional here.

The : in your getopts string (aegwy:) indicates that the -y option expects an argument, as in -ysomething instead of just -y by itself. Since you aren't doing anything with $OPTARG, I'm guessing that's not intentional.

As others have said, an if/then/elif/else would probably be clearer than the chain of && and ||.

The test [ $ERRORS ] is somewhat unclear because it can mean a lot of different things depending on the content of the $ERRORS parameter. A more explicit indication that you only care about whether or not it's set would be [ -n "$ERRORS" ].

Comparisons like [ -ne ] and friends are mostly holdovers from before the shell had built-in integer arithmetic; the more modern idiom would be (( $# != 2 )).

Your usage message implies that the -a, -e, -g, -w, and -y options take arguments of the form Amazon, eBay, Google, etc. It would be clearer what the actual syntax of the command is without those additions; you can include an extra paragraph in the help text listing what each option stands for.

As a rule, error messages should go to stderr instead of stdout (>&2).

It's fine to use basename $0 for consistency of output, but there's something to be said for leaving $0 alone as it will reflect however the user actually invoked the command. Something to consider.

Not much point in using printf if you're not using a format string; just use echo, which automatically appends the newline. Usage messages traditionally don't include quotation marks, either; it's up to the user to quote the arg or not depending on whether it's needed.

Checking a command for success is exactly how if works, so there's no need to do explicit checks of $? unless you really care about the exact exit value. In the case of your connectivity ping, you probably don't care about why it failed, only that it did:

  if ! ping -c 1 209.85.147.103 >/dev/null; then 
     echo >&2 "$0: You are not connected to the Internet!"
     exit 1
  fi

Your search query reformat might need to do more than just turn spaces into plus signs; what if it has an ampersand? But if you're just doing the spaces-to-pluses thing, you could use bash parameter expansion do it without sed: QUERY="${QUERY// /+}"

If your program relies on open/xdg-open etc, you should probably check for its availability at the top; no sense doing anything else if you know you can't possibly perform the requested operation anyway. And you can use a variable so you don't wind up repeating yourself in multiple clauses:

open=
for cmd in open xdg-open; do
   if type -p "$cmd" >/dev/null; then
     open="$cmd"
     break
   fi
done
if [ -z "$open" ]; then
   echo >&2 "$0: open command not found."
   exit 1
fi

And then later you can finish up with just this one line:

"$open" "$ENGINE=$QUERY" &>/dev/null

Upvotes: 2

Bram
Bram

Reputation: 825

http://linuxcommand.org/ is an excellent resource to improve your bash scripting skills.

http://tldp.org/LDP/abs/html/ is another great document.

Hope this helps.

Upvotes: 0

Related Questions