interactive-tutorials icon indicating copy to clipboard operation
interactive-tutorials copied to clipboard

"Shell" should set some standards

Open Gorian opened this issue 5 years ago • 2 comments

I looked at a few of the "shell scripting" tutorials, and a few things immediately jumped out at me.

  • You should either commit to teach POSIX compliant shell scripting, or to teach bash specifically. You shouldn't mix the two, like using bash to write POSIX code in one tutorial, and then have the next section be about arrays, which are a bashism and not POSIX compliant
  • All code should be https://shellcheck compliant - note that shellcheck changes it's linting based on the shebang - so if you want posix compliance, use a shebang of #!/bin/sh vs. #!/bin/bash for bash-specific linting

Some specific examples: https://www.learnshell.org/en/Variables

#!/bin/bash
# Change this code
BIRTHDATE=
Presents=
BIRTHDAY=


# Testing code - do not change it

if [ "$BIRTHDATE" == "Jan 1, 2000" ] ; then
    echo "BIRTHDATE is correct, it is $BIRTHDATE"
else
    echo "BIRTHDATE is incorrect - please retry"
fi
if [ $Presents == 10 ] ; then
    echo "I have received $Presents presents"
else
    echo "Presents is incorrect - please retry"
fi
if [ "$BIRTHDAY" == "Saturday" ] ; then
    echo "I was born on a $BIRTHDAY"
else
    echo "BIRTHDAY is incorrect - please retry"
fi
  1. encouraging use of global variables - don't do this unless they are meant to be GLOBALS
  2. using [ instead of [[ in bash
  3. using [ instead of (( for integer comparison
  4. random casing in variables - one is capitalized, the other two are all caps? Why?
  5. randomly quoting some, but not all variables
  6. not using a main function
  7. this code doesn't even pass https://shellcheck.net/

https://www.learnshell.org/en/Loops

  1. Very odd that they did use correct bash code by managing arrays and using "${array[@]}" to loop over the array (although simple output could have been done easier in this case with something like printf "My name is %s\n" "${NAMES[@]}"), but they do something you shouldn't be encouraging anyone to do, like looping over the output of ls - this is what shell globbing is for. https://mywiki.wooledge.org/ParsingLs
  2. [ $COUNT -gt 0 ] again - this is legacy POSIX stuff - if you are using bash, this can instead be ((count > 0)) - it's odd that they use that format when 2 lines later that use arithmetic expansion in COUNT=$(($COUNT - 1)), although they use $count, when you don't use $ for variables in arithmetic expansion in bash - it is incorrect. See https://tldp.org/LDP/abs/html/arithexp.html

If you want to teach good shell scripting, getting users use to best practices, even if they don't understand them all yet, should be an important thing.

I'd recommend picking a shell standard and following it, for example https://google.github.io/styleguide/shellguide.html

Gorian avatar Nov 28 '20 12:11 Gorian

Hi @Gorian. Since I'm not an expert in bash or shell in general, I'd appreciate it if you can send over some pull requests to fix these issues.

Thanks.

ronreiter avatar Dec 16 '20 08:12 ronreiter

@ronreiter Sure, I'd be happy to see what I can do to help :)

Gorian avatar Dec 18 '20 00:12 Gorian