Professional opinion of a bash script.

beanaroo · 08-28-2015, 10:15 PM

Greetings! Long time lurker, first time poster.

I recently discovered i3blocks, which provides a status line for i3bar. Each block is its own script. I attempted to write my own battery status indicator from scratch. It works!
But I'd like some seasoned advice on whether or not it's of acceptable standard and if I could be doing things 'better'. I'm still studying up on Bash Scripting and tools like awk, which I think might be better suited.

The output is three lines, which then get parsed. I'm making use of FontAwesome on my system.

PHP Code:



#!/bin/bash 
 
# Get battery status from acpi 
# $BLOCK_INSTANCE is an option that can be set in i3block.conf 
BATCALL=$(acpi -b | grep "Battery $BLOCK_INSTANCE") 
 
# Set Status and Percentage variables 
STATUS=$(echo $BATCALL | cut -d':' -f 2 | cut -d',' -f 1) 
PERCENT=$(echo $BATCALL | cut -d',' -f 2 | tr -d %) 
 
# Set Icon 
if [ $STATUS == "Discharging" ] ; then 
    if [ $PERCENT -ge 90 ] ; then 
        ICON="" 
    elif [ $PERCENT -ge 65 ] ; then 
        ICON="" 
    elif [ $PERCENT -ge 40 ] ; then 
        ICON="" 
    elif [ $PERCENT -ge 15 ] ; then 
        ICON="" 
    elif [ $PERCENT -lt 15 ] ; then 
        ICON="" 
    fi 
elif [ $STATUS == "Charging" ] || [ $STATUS == "Full" ] ; then 
    ICON="" 
fi 
 
echo $ICON # Full text label 
echo $ICON # Short text label (Used when status line gets too long) 
 
# Set color value for the above 
if [ $PERCENT -ge 85 ] ; then 
    echo "#9FA91F" 
elif [ $PERCENT  -ge 50 ] ; then 
    echo "#FFBF00" 
elif [ $PERCENT  -ge 20 ] ; then 
    echo "#FF7E00" 
elif [ $PERCENT  -lt 20 ] ; then 
    echo "#FF033E" 
fi

Thanks for reading!

goumba · 08-28-2015, 10:40 PM

Looks good, but I am no pro.

Very nice on using $() as opposed to backticks.

Code:

echo $ICON # Full text label
echo $ICON # Short text label (Used when status line gets too long)

The same thing, did you mean to do something else with that?

Comparing strings, typically the variable name will be in quotes as well, so you'll see things like this elsewhere:

Code:

if [ "$STATUS" == "Discharging" ]; then

One other thing is that usually uppercase names are used with environment variables, lower case variable names are preferred by many in shell scripts.

beanaroo · 08-29-2015, 12:18 AM

Thanks, goumba! I've updated the string comparisons and variable cases.

The short text version is used by the status bar when there isn't enough space for the full text version. In my case they are both only one character long.

catkin · 08-29-2015, 04:54 AM

Very nice

[[ ... ]] has advantages over [ ... ] as explained at http://mywiki.wooledge.org/BashFAQ/031

There is a convention you may like of using lower-case names for your own variables. The advantage is that any upper case names stand out as being either bash internal variables or environment variables.

Numeric tests -- ((...)) -- can be used instead of [[ ... ]] with numeric comparison operators. They have the advantages of being more concise, of being immediately recognisable as numeric tests and of supporting the $ dereference operator to yield their numeric value.

unSpawn · 08-29-2015, 05:08 AM

Quote:

Originally Posted by goumba

lower case variable names are preferred by many in shell scripts.

That indeed is personal preference because likewise one could say upper case var names make them stand out making things easier to read.

Quote:

Originally Posted by beanaroo

(..) and if I could be doing things 'better'.

Looks good. Here's my take on things using an array and including error handling:

Code:

#!/bin/bash --
# Uncomment next for debugging:
# set -vx
# Uncomment next for renicing:
# which renice >/dev/null 2>&1 && renice -n +20 -p $$
# which ionice >/dev/null 2>&1 && ionice -c 3 -p $$
# Uncomment next if shouldn't be run as root:
# [ $(id -u 2>/dev/null) -eq 0 ] && { echo "Unpriv only, exiting."; exit 1; }
# Uncomment next if shouldn't run if no Xorg in sight:
# [ -z ${DISPLAY} ] && { echo "Couldnt find Xorg display, exiting."; exit 1; }
# Missing environment variable:
[ -z ${BLOCK_INSTANCE} ] && { echo "Couldnt find envvar, exiting."; exit 1; }
# Missing binaries:
BINARIES="acpi"; for ITEM in $BINARIES; do which "${ITEM}" >/dev/null 2>&1 \
|| { echo "Please install acpi first, exiting."; exit 1; }; done

# Initialization done, now get battery status from acpi into array:
# $BLOCK_INSTANCE is an arbitrary string env var set in i3block.conf
BATCALL=($(acpi -b 2>/dev/null| grep "Battery ${BLOCK_INSTANCE}"))
# Mandatory amount of items in array:
[ ${#BATCALL[@]} -ne 3 ] || { echo "acpi returns wonky values, exiting."; exit 1; }
# Set Status and Percentage variables
STATUS=${BATCALL[2]}; STATUS=${STATUS//,/}
[ -z ${STATUS} ] && { echo "Empty var, exiting."; exit 1; }
PERCENT=${BATCALL[3]}; PERCENT=${PERCENT//%/}
[ -z ${PERCENT} ] && { echo "Empty var, exiting."; exit 1; }
[ ${PERCENT} -eq ${PERCENT} ] || { echo "Odd int value, exiting."; exit 1; }

# Set Icon
case "${STATUS}" in 
Dischar*)
     if [ $PERCENT -ge 90 ]; then ICON=""
      elif [ $PERCENT -ge 65 ]; then ICON=""
      elif [ $PERCENT -ge 40 ]; then ICON=""
      elif [ $PERCENT -ge 15 ]; then ICON=""
      elif [ $PERCENT -lt 15 ]; then ICON=""
     fi
    ;;
Charg*|Full) ICON=""
    ;;
*) ICON=""
    ;;
esac

echo $ICON # Full text label
echo $ICON # Short text label (Used when status line gets too long)

# Set color value for the above
if [ $PERCENT -ge 85 ] ; then echo "#9FA91F"
elif [ $PERCENT  -ge 50 ] ; then echo "#FFBF00"
elif [ $PERCENT  -ge 20 ] ; then echo "#FF7E00"
elif [ $PERCENT  -lt 20 ] ; then echo "#FF033E"
fi

exit 0

beanaroo · 08-30-2015, 01:40 AM

@catkin - I noticed the double brackets in a script for WiFi status but couldn't find a quick answer to the difference. Thanks for the link! The entire FAQ looks awesome!

@unSpawn - I just started learning case statements today. Thank you. I like the style much better. There's a lot of useful stuff in there too!

Keith Hedger · 08-30-2015, 07:47 AM

When using string comaparisons its a good idea to add an extra character to both string to prevent null string errors so

Code:

if [ "X$var1" = "X$var2" ];then
...
fi

The two 'X's cancel and it prevents an error if one variable is not set.