A shell script to get the current battery status

Question

Is this code well written? I'm not doing much, just rendering my battery status for my status bar but the code seems pretty long. I am new to Bash and Linux, so I thought this code could be improved somewhere.

#!/bin/bash
# Send a notification if battery is low, change status in info file accordingly
notify-low () {
 # Battery isn't much but is charging (don't notify)
 [ "$status" = "Charging" ] && return
 # Battery critically low
 if [ "$capacity" -le 10 ] && [ -z "$(grep critically-low $lowinfo)" ]; then
 notify-send -i $HOME/.config/icons/critical-battery.png -u critical "Battery critically low!" 
 echo "critically-low" > $lowinfo
 # Battery is low 
 elif [ "$capacity" -le 20 ] && [ -z "$(grep low $lowinfo)" ]; then
 notify-send -i $HOME/.config/icons/low-battery.png -t 5500 "Battery low!"
 echo "low" > $lowinfo
 fi
}
battery="/sys/class/power_supply/BAT0"
status="$(cat "$battery/status")"
capacity="$(cat "$battery/capacity")"
# Get the low battery status from file ~/.cache/battery-low-status
lowinfo="$HOME/.cache/battery-low-status"
[ ! -e "$lowinfo" ] && touch $lowinfo
# Set charging icon and capacity icon
[ "$status" = "Charging" ] && charging_icon=" " || charging_icon=""
[ "$capacity" -gt 100 ] && capacity=100
if [ "$capacity" -lt 15 ]; then capacity_icon=' ' 
elif [ "$capacity" -lt 40 ]; then capacity_icon=' '
elif [ "$capacity" -lt 60 ]; then capacity_icon=' ' 
elif [ "$capacity" -lt 90 ]; then capacity_icon=' '
else capacity_icon=' ' 
fi
# Report low battery 
[ "$capacity" -le 20 ] && notify-low
# Reset low battery information in these cases
([ "$capacity" -gt 20 ] || [ "$status" = "Charging" ]) && [ -n $(cat "$lowinfo") ] && echo "" > $lowinfo
printf "%s%s%d%%\n" "$charging_icon" "$capacity_icon" "$capacity"

Answer 1

Consider which shell to use

You've written this as a Bash script, but the only thing preventing this being a portable shell (/bin/sh) script is the use of - in the function name. Replace that with _, and we can change the shebang.

The benefits of being able to use a different shell include faster start-up, lower memory overhead and portability to systems that don't have Bash installed.

---

"Lint" your script

Shellcheck complains about:

280513.sh:10:34: note: Use ! grep -q instead of comparing output with [ -z .. ]. [SC2143]
280513.sh:10:60: note: Double quote to prevent globbing and word splitting. [SC2086]
280513.sh:11:20: note: Double quote to prevent globbing and word splitting. [SC2086]
280513.sh:12:29: note: Double quote to prevent globbing and word splitting. [SC2086]
280513.sh:15:36: note: Use ! grep -q instead of comparing output with [ -z .. ]. [SC2143]
280513.sh:15:51: note: Double quote to prevent globbing and word splitting. [SC2086]
280513.sh:16:20: note: Double quote to prevent globbing and word splitting. [SC2086]
280513.sh:17:18: note: Double quote to prevent globbing and word splitting. [SC2086]
280513.sh:27:30: note: Double quote to prevent globbing and word splitting. [SC2086]
280513.sh:43:1: note: Use { ..; } instead of (..) to avoid subshell overhead. [SC2235]
280513.sh:43:64: error: -n doesn't work with unquoted arguments. Quote or use [[ ]]. [SC2070]
280513.sh:43:64: warning: Quote this to prevent word splitting. [SC2046]
280513.sh:43:97: note: Double quote to prevent globbing and word splitting. [SC2086]

Minimal changes to fix these:

#!/bin/sh
# Send a notification if battery is low, change status in info file accordingly
notify_low () {
 # Battery isn't much but is charging (don't notify)
 [ "$status" = "Charging" ] && return
 # Battery critically low
 if [ "$capacity" -le 10 ] && ! grep -q critically-low "$lowinfo"; then
 notify-send -i "$HOME/.config/icons/critical-battery.png" -u critical "Battery critically low!" 
 echo critically-low >"$lowinfo"
 # Battery is low 
 elif [ "$capacity" -le 20 ] && ! grep -q low "$lowinfo"; then
 notify-send -i "$HOME/.config/icons/low-battery.png" -t 5500 "Battery low!"
 echo low >"$lowinfo"
 fi
}
battery="/sys/class/power_supply/BAT0"
status="$(cat "$battery/status")"
capacity="$(cat "$battery/capacity")"
# Get the low battery status from file ~/.cache/battery-low-status
lowinfo="$HOME/.cache/battery-low-status"
[ ! -e "$lowinfo" ] && touch "$lowinfo"
# Set charging icon and capacity icon
[ "$status" = "Charging" ] && charging_icon=" " || charging_icon=""
[ "$capacity" -gt 100 ] && capacity=100
if [ "$capacity" -lt 15 ]; then capacity_icon=' ' 
elif [ "$capacity" -lt 40 ]; then capacity_icon=' '
elif [ "$capacity" -lt 60 ]; then capacity_icon=' ' 
elif [ "$capacity" -lt 90 ]; then capacity_icon=' '
else capacity_icon=' ' 
fi
# Report low battery 
[ "$capacity" -le 20 ] && notify_low
# Reset low battery information in these cases
{ [ "$capacity" -gt 20 ] || [ "$status" = "Charging" ]; } && [ -n "$(cat "$lowinfo")" ] && echo "" >"$lowinfo"
printf "%s%s%d%%\n" "$charging_icon" "$capacity_icon" "$capacity"

---

Minor improvements

Some things I didn't spot in the other answers:

• Consider set -u to catch misspellings of variable names, and set -e to avoid blundering on when a command fails.
• No need to read a file to check it's non-empty; replace [ -n "$(cat "$lowinfo")" ] with [ -s "$lowinfo" ].
• Prefer to pass data to functions as arguments, rather than in shared variables. In particular, I think status and capacity should be arguments to notify_low; I'm happy for lowinfo to be shared as it acts as a global constant.
• Use functions to help isolate independent sections of code. I'd have a function to select the capacity icon, even though it's used only once.
• An empty string as the only argument to echo can be removed - both echo "" and echo produce a single newline.
• Avoid testing a negative when the alternative is simpler - e.g. [ -e "$lowinfo" ] || touch "$lowinfo" instead of [ ! -e "$lowinfo" ] && touch "$lowinfo" (that said, in this case I don't see any reason not to just touch unconditionally, as we're not using the timestamp for anything).
• Prefer single-quotes when there are no variables to expand. This makes life easier for the reader, and saves the shell a tiny bit of checking, too.
• Think about preparing your user messages for translation.

Answer 2

Overall

This is a very readable and maintainable script. I would not mutter nasty words under my breath if I had to make changes to this code in five years. Your variable names are good and the comments are helpful without being too verbose. Quoting all of your variable substitutions is a best practice and it is great that you're doing so everywhere I see. You are not repeating yourself. For the length, it seems pretty brief and to the point to me.

Suggestions

• Since you've specified bash in your sh-bang line (which is also a good best practice to follow), you can use bash features like double square brackets ([[ ]]) for conditionals. These will give you fewer surprises and are a good habit to get into.
• Your $lowinfo refers to a filename, but it isn't immediately obvious that this is so since it is used in code above where it is defined. Naming it something like $lowinfo_file would make this more self-evident.
• I tend to put the variable definitions at the top, above any function definitions. This is a hold-over from my C days, but I feel it makes the variables easier to find, which the reader often needs to refer back to.
• The if after the [ "$capacity" -gt 100 ] && capacity=100 line blends into the conditionals right above it. An extra blank line would be nice before the if.
• Having more compact code, as in the previously mentioned if totally works and is readable if the conditional and command are small enough that it fits in ~80 characters. But your ([ "$capacity" -gt 20 ] || [ "$status" = "Charging" ]) && [ -n $(cat "$lowinfo") ] && echo "" > $lowinfo line does not meet this criterion because the conditional part is so large. It wouldn't hurt to go ahead and turn this into a multi-line if so the conditional and the action are easier to find.
• You don't have to check the length of grep's output. You can use grep outside of any square brackets, but after an if to check the return value of the command. You would also probably want to redirect the output to > /dev/null. There's nothing wrong with how you did it. It is just a cool feature of if in the shell to keep in mind.

• Toby Speight: I think we disagree on whether using [[ is a good habit. It might be so, for someone who writes only in Bash, but for those of us who also maintain portable shell scripts, it can encourage failing to quote expansions, and it can make code reuse harder. I'm not going to try to persuade you to agree with me, and Diwas10 can surely make an independent judgement of whose advice to take.
• chicks: He's running this thing on a Linux laptop so there's no reason not to take advantage of modern shell conveniences provided by bash. The google shell style guide also recommends only scripting in bash.

Answer 3

Use the exit code of commands instead of their output when possible

This command checks if grep's output is empty:

if [ -z "$(grep low $lowinfo)" ]; then

In addition, note that if ever "$(grep ...)" has large output, it would be a waste of memory to store it in a string.

It's better to take advantage that grep exits with success (0) when it finds anything, and with error (anything but 0) when it doesn't find a match:

if ! grep -q low "$lowinfo"; then

An additional benefit of -q is that grep can stop processing the input immediately when it finds a match, since the output would be discarded anyway.

Double-quote variables in commands

In most places the variables are correctly double-quoted, with few exceptions, such as:

if [ -z "$(grep low $lowinfo)" ]; then
...
notify-send -i $HOME/.config/icons/critical-battery.png -u critical "Battery critically low!" 
echo "critically-low" > $lowinfo

To make these safe for special characters such as whitespace, add the missing double-quotes:

if [ -z "$(grep low "$lowinfo")" ]; then
...
notify-send -i "$HOME/.config/icons/critical-battery.png" -u critical "Battery critically low!" 
echo "critically-low" > "$lowinfo"

Prefer one statement per line

This if-else chain is a bit difficult to read because the lines contain two statements, a conditional and an action:

if [ "$capacity" -lt 15 ]; then capacity_icon=' ' 
elif [ "$capacity" -lt 40 ]; then capacity_icon=' '
elif [ "$capacity" -lt 60 ]; then capacity_icon=' ' 
elif [ "$capacity" -lt 90 ]; then capacity_icon=' '
else capacity_icon=' ' 
fi

It's easier to read with one statement per line:

if [ "$capacity" -lt 15 ]; then
 capacity_icon=' ' 
elif [ "$capacity" -lt 40 ]; then
 capacity_icon=' '
elif [ "$capacity" -lt 60 ]; then
 capacity_icon=' ' 
elif [ "$capacity" -lt 90 ]; then
 capacity_icon=' '
else
 capacity_icon=' ' 
fi

This example is the hardest to read, because readers must scroll horizontally:

([ "$capacity" -gt 20 ] || [ "$status" = "Charging" ]) && [ -n $(cat "$lowinfo") ] && echo "" > $lowinfo

With a classic if statement, and some other improvements:

if [ "$capacity" -gt 20 ] || [ "$status" = "Charging" ]; then
 [ -s "$lowinfo" ] && > "$lowinfo"
fi

The other improvements:

• Instead of writing a blank line to $lowinfo with echo "" > $lowinfo, we make it empty with > "$lowinfo".
  • I also added the missed double-quotes
• Instead of checking that the file is empty by reading its content with -n $(cat "$lowinfo"), we can use -s "..." to the same effect.
  • Notice that in case of large files, this can make a significant difference in memory use.

Use echo instead of printf when it's good enough

Instead of this:

printf "%s%s%d%%\n" "$charging_icon" "$capacity_icon" "$capacity"

I think this is easier to read with the variables embedded in the line, and also shorter:

echo "${charging_icon}${capacity_icon}${capacity}%"