Problem
I have an APC UPS, which comes with the apcaccess
utility to get various statistical values from the apcupsd
daemon service.
I am using the following script to stream my line voltage data to thingspeak.com website which allows us to see the results streaming in real time.
The script is set as a cron job, executing every minute.
#!/bin/bash
API_URL="https://api.thingspeak.com/update?api_key=XXXXXXXXXXXXX&field%d=%s"
UPS_PROPS=(LINEV)
for index in ${!UPS_PROPS[@]}
do
value=$(/sbin/apcaccess -up ${UPS_PROPS[$index]})
curl $(printf $API_URL $(expr $index + 1) $value)
done
I have the UPS_PROPS
as an array because I want to later extend the code to also stream some other data points.
The chart can be seen over at this link.
Solution
Good things
- Using
$()
over backticks (``
) for command substitution is a current best practice and does make code like this more readable. - It is also good use variables for long unchanging items like your
API_URL
. - Thanks for including the link to the graph. This doesn’t make your code any better, but it does lead to happier reviewers. I’m such a fan of graphs I’d have thrown it inline.
Suggestions
- Since you’re using the
$()
form of command substitution, it is pretty easy to add some extra spaces inside of the parenthesis. I think this makes it even more readable.$( /sbin/apcaccess -up ${UPS_PROPS[$index]} )
- Using the variable as a
printf
format is a good way to do it, but that means the value of the variable isn’t a valid URL. If this were a bit longer than 10 lines it wouldn’t hard to imagine a naive maintenance programmer coming along and doingcurl $API_URL
somewhere and wondering why it is giving him errors. So I would prefer to name the variableAPI_URL_FORMAT
orAPI_URL_PROTO
to make it clear it isn’t a usable URL yet. #!/usr/bin/env bash
is considered more portable because it works with folks who can’t installbash
in the OS-proper place.- Run your code through https://www.shellcheck.net/ for several hints about quoting your substitutions.
- It would be a bit clearer what’s happening with the curl if you split it into two lines. The first would set a variable like
ADDING_URL="$(printf $API_URL_FORMAT $(expr $index + 1) $value)"
then the second line would just becurl $ADDING_URL
. This leaves a pretty boringcurl
line I admit, but by using the variable it is easier to see what is happening with theprintf
before thinking about sending it out to the web. And you may want morecurl
options later so thecurl
line could get more complicated. - Please use consistent case for your variable names! Some are upper case, which is pretty traditional for shell scripts, but other half are lower case. Some people like upper case because it makes the variables stand out more. I don’t think that makes much difference with modern syntax highlighting editors. But it should definitely be consistent, either way, in such a short script. One convention is to use upper case for
export
ed/in theenv
variables and lower case for local/not in theenv
variables. In the case all of your variables would be lower case since none of them areexport
ed into theenv
. - Why are you looping over an array with one item in it? Will it ever contain anything other than
LINEV
? If not the code can be significantly simplified by getting rid of thefor
and just putting the values into the two inner lines. - If the
for
loop has some value I would try to use it like a shellfor
loop and loop over the values instead of the indexes. I see that you use theindex
inside of thecurl
, but I’m not sure what that is doing for you.