Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Lots of minor coding issues. #1

Open
BlessJah opened this issue Nov 26, 2016 · 3 comments
Open

Lots of minor coding issues. #1

BlessJah opened this issue Nov 26, 2016 · 3 comments

Comments

@BlessJah
Copy link

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 added a commit that referenced this issue Nov 26, 2016
adtac added a commit that referenced this issue Nov 26, 2016
adtac added a commit that referenced this issue Nov 26, 2016
adtac added a commit that referenced this issue Nov 26, 2016
adtac added a commit that referenced this issue Nov 26, 2016
adtac added a commit that referenced this issue Nov 26, 2016
@adtac
Copy link
Owner

adtac commented Nov 26, 2016

@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
Copy link

@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
Copy link
Owner

adtac commented Nov 29, 2016

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants