3
\$\begingroup\$

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.

\$\endgroup\$
1

1 Answer 1

1
\$\begingroup\$

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 doing curl $API_URL somewhere and wondering why it is giving him errors. So I would prefer to name the variable API_URL_FORMAT or API_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 install bash 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 be curl $ADDING_URL. This leaves a pretty boring curl line I admit, but by using the variable it is easier to see what is happening with the printf before thinking about sending it out to the web. And you may want more curl options later so the curl 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 exported/in the env variables and lower case for local/not in the env variables. In the case all of your variables would be lower case since none of them are exported into the env.
  • 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 the for 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 shell for loop and loop over the values instead of the indexes. I see that you use the index inside of the curl, but I'm not sure what that is doing for you.
\$\endgroup\$
3
  • \$\begingroup\$ Thanks for those points. The array declaration of UPS_PROPS is intentional, and is mentioned in the question itself. I did run the script through shellcheck (emacs is configured for flycheck-mode); and I have updated the script in next few iterations a lot. I'll post a follow up soon. \$\endgroup\$
    – hjpotter92
    Apr 16, 2018 at 17:05
  • \$\begingroup\$ I am using the index value as thingspeak API requires the fields to be indexed from 1, and like field1=val1 and field2=val2 etc. \$\endgroup\$
    – hjpotter92
    Apr 16, 2018 at 17:06
  • \$\begingroup\$ As for inconsistent capitalisations, coming from python/lua primarily, I use CAP_CASE for globals and constants, and snake_case for local or changing variables. If there is a PEP or similar casing guidelines, I'd be happy to follow that practice. \$\endgroup\$
    – hjpotter92
    Apr 16, 2018 at 17:08

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.