ronreiter / interactive-tutorials

Interactive Tutorials
Apache License 2.0
4.09k stars 2.6k forks source link

"Shell" should set some standards #524

Open Gorian opened 3 years ago

Gorian commented 3 years ago

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

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

ronreiter commented 3 years ago

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.

Gorian commented 3 years ago

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