adtac / climate

The swiss-army knife of utility tools for Linux.
GNU General Public License v3.0
1.42k stars 76 forks source link

Lots of minor coding issues. #1

Open BlessJah opened 7 years ago

BlessJah commented 7 years ago

Nice script, I find some of the ideas are pretty clever. Consider running it through shellcheck, lot of low hanging fruits in there.

Issues:

For portability reasons (I don't mean BSD and others, there are linux distros in the wild not having bash under /bin/bash):

-#!/bin/bash
+#!/usr/bin/env bash

Every day is a good day to learn something new. The variable is not used anyway.

-ABSOLUTE_PATH="$(cd "$(dirname "${0}")" && pwd)/$(basename "${0}")"
+ABSOLUTE_PATH="$(readlink -nf "${0}")"

Avoid variable leaking (example below).

 spinner() {
-    spin="\\|/-"
-    i=0
+    local spin="\\|/-"
+    local i="0"
bar() {
    foo='Hello world!'
}
bar
echo "${foo}"

Always quote variables to avoid splitting.

-while kill -0 $1 2>/dev/null; do
+while kill -0 "$1" &>/dev/null; do

Variables shouldn't be used as printf formatting string

-        printf "\b${spin:$i:1}"
+        printf "\b%s" "${spin:$i:1}"

There is no reason to use single square brackets test. In bash always use double. Also quotes.

 extract () {
-    if [ -f $1 ] ; then
-        if [[ $2 == "" ]]; then
-            case $1 in
-                *.rar)                             rar x   $1       ${1%.rar}/        ;;
+    if [[ -f "$1" ]] ; then
+        if [[ "$2" == "" ]]; then
+            case "$1" in
+                *.rar)                                  rar x   "$1"    "${1%.rar}/"     ;;

Reading from $1 and $2 and shifting is cleaner than double-shift.

 replace() {
-    find_this="$1"
-    shift
-    replace_with="$1"
-    shift
+    local find_this="$1"
+    local replace_with="$2"
+    shift 2

Awk is magic; quotes.

-    PERCENTAGE=$(upower -i $(upower -e | grep battery) |
-                 grep "percentage" |
-                 grep -o "[0-9]*")
+    PERCENTAGE=$(upower -i "$(upower -e | grep battery)" |
+        awk -F: '/percentage/{gsub(/^\s+|\s+$/, "", $2); print $2}')

Backticks are evil, use $(). Also putting --utc near every date in script may be a good idea (yeah, yeah, I know it's %s).

-    date1=$((`date +%s` + $commandargs));
+    local start_date="$(date --utc '+%s')"
+    local stop_date="$(( $now + $commandargs ))"
adtac commented 7 years ago

@BlessJah thanks! I've implemented most of the things you've suggested (probably not every case of all issues you brought up, but I'll do that soon).

Glad you like it :)

mckennajones commented 7 years ago

@adtac In the install script, on line 144, you are checking for npm instead of pip, in the pip_verify function. Figured I'd let you know here as it's a very small issue. Cheers!

adtac commented 7 years ago

@mckennajones argh, thanks! I just copy pasted the npm one with some changes :P I'll fix it right away :+1: