9
\$\begingroup\$

For my current project I need to validate responses. The requests will be send from multiple different shell scripts and should be controlled from another script. I'd never targeted a .sh with a .sh before, so let's try some FizzBuzz first.

Fizz.sh

#!/bin/bash

if [ $# -eq 0 ]
then
    echo "This FizzBuzz is interactive. Please provide the upper limit."
    echo "Usage : $0 limit"
    exit 1
fi

for i in `seq $1`
do
   echo `source ./buzz.sh $i`
done

Buzz.sh

#!/bin/bash

([ $(($1%15)) -eq 0 ] && echo 'FizzBuzz') ||
([ $(($1%5)) -eq 0 ] && echo 'Buzz') ||
([ $(($1%3)) -eq 0 ] && echo 'Fizz') ||
echo $1;
  • Is it idiomatic?
  • Is this the best way to hand all output from Buzz to Fizz?

The goal is to learn decent BASH. The FizzBuzz has no need for optimization, it's mostly about style, how data should be passed from script to script and whether I followed BASH-practices or not.

\$\endgroup\$
  • 2
    \$\begingroup\$ Do you want a good fizzbuzz (in which case breaking it up into two scripts is a bad idea), or do you want validation for your two-script technique (in which case the fizzbuzz use case is kind of hypothetical)? \$\endgroup\$ – 200_success Jul 14 '15 at 17:12
  • \$\begingroup\$ @200_success It's mostly about the two-script technique, the fizzbuzz is there as a learning aid. However, since the goal is to learn BASH, everything is open for critique. \$\endgroup\$ – Mast Jul 14 '15 at 17:15
7
\$\begingroup\$

If you're using source, you shouldn't have the shebang in the second script. If you're using the shebang, you shouldn't use source.

But rather than use source directly, I would source the script once at startup, and in it define a function that was called at the later times.

You should use {} instead of () to avoid forcing a separate subshell to spawn.

I would provide a default instead of exiting if no argument is passed. Something like limit=${1:-100}. Also, I always use named variables as early as possible.

Note that if you were using a function (with or without source), you should use local to avoid leakage.

Instead of `seq $1`, use for ((i=1; i<=limit; ++i)) to avoid precalculating and storing the entire sequence in memory (and also spawning an unnecessary subprocess).

Instead of [ $(($1%15)) -eq 0 ], use n=$1 to name the argument then use ((n % 15 == 0)). Note that let is an alias for (()), but it's idiomatic to use let for assignments and (()) for expressions.

Beware of the possibility of echo failing! Using an if-then-else chain as @glennjackman suggested will avoid this pitfall. It would also be possible to replace echo 'FizzBuzz' with {echo 'FizzBuzz'; true} but that's ugly IMO.

\$\endgroup\$
8
\$\begingroup\$

Two points:

  1. echo $(something that already prints) is just redundant. This is simpler

    source ./buzz.sh $i
    
  2. There does not appear to be any need for two separate files. Put buzz in a function. Also, you are using a lot of subshells that are unnecessary (a). I'd stick with the more verbose but clearer if

    buzz() {
        if (( $1 % 15 == 0 )); then
            echo 'FizzBuzz'
        elif (( $1 % 5 == 0 )); then
            echo 'Buzz'
        elif (( $1 % 3 == 0 )); then
            echo 'Fizz'
        fi
        echo $1
    }
    

    then

    for i in $(seq $1); do buzz $i; done
    

a) if you want a grouping construct without the overhead of spawning a new shell, use braces:

{ echo foo; echo bar; echo baz; } | tac
\$\endgroup\$
  • \$\begingroup\$ FizzBuzz is often used to learn a specific part of a language. In this case: passing data between scripts. Merging the scripts would defeat the purpose of the exercise. Thank you for the rest of your points. \$\endgroup\$ – Mast Jul 14 '15 at 17:26
  • \$\begingroup\$ I haven't seen fizzbuzz used to illustrate separate programs. Anyway, you could just write ./buzz.sh $i without echo or source \$\endgroup\$ – glenn jackman Jul 14 '15 at 17:42

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.