Bash Music Player 2

Question

This post is a follow-up to this.

I wanted further reviews since the code I provided became too old when I got replies, which is why I'm providing it again here.

I'm hoping I could get reviews for the new code.

#!/bin/bash
#
#   Need to sort music, try tinkering with sort -u at the end of the load_sites function ..
#   Need to organize error codes ..
#
mkdir -p "$HOME/Music"
cd "$HOME/Music"
clear # So that any download will be in the music directory
######################################### Initial Values and Declarations #############################
##### needed global variables
declare -r settings_file="$HOME/.config/PlayMusic/PlayMusic.settings" log_file="$HOME/Desktop/PlayMusic_log.log" # readonly variables
declare -A Tracks # names are keys, sites are values
declare FILE times_to_play=1 SITES=()
##### options & features
declare chosen=false dropbox=false just_download=false double_check=false all=false player='mplayer'
############################################## Functions ###################################
function Play { # Function for playing music according to user preferences
    if [[ ${#1} -lt 23 ]];then
        notify-send "Now Playing: \"$1\" .." "$track" # Notifing the user, only if the track's name isn't too long
    fi
    for (( i=0 ;  i < times_to_play ; i++ ));do
        $player "$1"
    done
}

function Find { # function to look for a previously downloaded file on disk
    local IFS=$'\n'
    unset FILE
    echo "Looking for '$1' on disk .. "
    local files=$(find ~ -name "$1*.mp3" 2>/dev/null) ; clear # The main file ( the found file, or null if not found ) ..
    if [[ $(echo "$files" | wc -l) -gt 1 ]];then # what to do if more than one file was found ..
        local file
        if ! $all;then # if the user is likely to be sitting on his computer and not sleeping ..
            echo 'File was found at various places .. Which one to use ?'
            select file in $files "Cancel";do
                [[ -n "$file" ]] || continue
                [[ "$file" == 'Cancel' ]] && exit 0
                FILE="$file"
                break
            done
            if IsYes -p 'Do you want to Delete the other ones ? ';then
                for file in $files;do
                    [[ "$file" == "$FILE" ]] || rm "$file"  # removing the file if not the selected file
                done
            fi
        else
            file="${files[0]}" # getting the nearest file
            mv -f "$file" "$HOME/Music" # moving it to the music directory
            FILE="$HOME/Music/$(basename "${files[0]}")" # getting that file
            for file in $files;do
                [[ "$file" == "$FILE" ]] || rm "$file"
            done
        fi
    else FILE="$files"
    fi ; clear
}
function IsNum {
    return $([[ "$1" =~ ^[0-9]+$ ]] && echo 0 || echo 1)
}
function create_settings_file {
    mkdir -p "$(dirname "$settings_file")"
    cat > "$settings_file" <<- EOF # the default music settings file
$player # the first line is always the media player
$dropbox # the second line is always for integration with dropbox
# other than that is only sites ..
http://ccmixter.org/view/media/samples/mixed
http://ccmixter.org/view/media/remix
http://mp3.com/top-downloads/genre/jazz/
http://mp3.com/top-downloads/
http://mp3.com/top-downloads/genre/rock/
http://mp3.com/top-downloads/genre/hip hop/
# empty or fully-commented lines are ignored ..

http://mp3.com/top-downloads/genre/emo/
http://mp3.com/top-downloads/genre/pop/
# PS: '#' is marking a comment , and is ignored by the settings parser ..
EOF
    if $2;then
        echo "# The settings file was recreated because the old one was useless or corrupt .." >> "$settings_file"
        nano "$settings_file"
        notify-send -t 450 'Here it is !'
    fi
}
function DOWNLOAD {
    local name="$track.mp3"
    Find "$track" # Looking for the specified track on disk
    if [[ -n $FILE ]] ;then # if the track was found on disk
        if [[ $(dirname "$FILE") != "$HOME/Music" ]];then # if the found file is not in the music directory
            notify-send "$track was found at ($(dirname "$FILE")) and was moved to ($HOME/Music)" # inform the user of the changes
            mv -f "$FILE" "$HOME/Music" &>/dev/null # move the file to the music home directory
            FILE="$HOME/Music/$name" # updating info
        fi
        name="$FILE"
    fi
    notify-send "Downloading ($track) .." "in [ $PWD ] .."
    wget -cO "$name" "${Tracks[$track]}"
    $dropbox && cp "$name" "$HOME/Dropbox/Music"
    $1 && Play "$name"
}
function UNINSTALL {
    IsYes -p 'Do you want to keep settings ? ' || rm "$HOME/.config/PlayMusic/PlayMusic.settings"
    sudo rm '/usr/local/bin/PlayMusic' &&
    sudo rm '/usr/local/man/man1/PlayMusic.1.gz' &&
    echo 'Uninstallation Success !' ; exit 0
}
function load_sites {
    local counter=0 name site
    for SITE in "${SITES[@]}";do
        echo -e "\t\033[30;3mSite \033[37m#\033[1m$((++counter))\033[0m\033[30;3m: '\033[1;33;3m$SITE\033[1;30;3m'\033[0m"
        for site in $(lynx -source "$SITE" | egrep -o 'http://.*\.mp3');do # Grabbing all music links ( newline delimeted )
            name="$(de_encode "$(basename "$site")" | sed -Ee 's/\.mp3$//' -e 's/\+|_/ /g' -e 's/^.+ - //')"
            Tracks["$name"]="$site" # Adding the name as the key, and the site as the value
        done
    done
    clear
    if [[ ${#Tracks[@]} == 0 ]];then
        echo 'No Music was Found ! '
        echo 'Please either check your internet connection or recreate the settings file using "( '"$(basename "$0") -s )"'"' >&2
        exit 1
    fi
}
function parse_settings  { # function to parse settings file
    local line_number=0 line
    if [[ -f "$settings_file" ]];then
        if [[ ! -r "$settings_file" ]];then
            echo "The Settings file ($settings_file) exists, but doesn't have read permission granted .." >&2
            exit 1
        fi
        while read -r line;do
            [[ -n "$line" ]] || continue
            case $((++line_number)) in
                1 ) player="$line" ;;
                2 ) if echo "$line" | egrep -q 'https?:.+';then # if a site ..
                        SITES[${#SITES[@]}]="$line" # add it
                     else # if not ..
                        [[ "$line" == 'true' ]] && dropbox=true || dropbox=false # if anything else, ignored ..
                     fi
                     $dropbox && mkdir -p "$HOME/Dropbox/Music" ;;
                * ) SITES[${#SITES[@]}]="$line" ;;
            esac
        done < <(sed -Ee 's/^ +//g' -e 's/(.*) *#.*/\1/g' -e 's/( *)$//g' -e 's/ *#.*//g' -e 's/ /%20/g' "$settings_file")
    else
        notify-send "Settings File not Found .. !"
        if IsYes -p "You haven't created your settings file .. Do you want to create it ? ";then
            create_settings_file false
        else
            echo 'Then, The Default settings are going to be used this time ..'
        fi
    fi
    if [[ ${#SITES[@]} == 0 ]];then
        create_settings_file true
    fi
    $all && echo -e "$(date)\n###############" >> "$log_file"
}
function EVERYTHING {
        $just_download && notify-send 'Downloading Everything .. !' || notify-send 'Playing Everything .. !'
        for track in "${!Tracks[@]}";do
            $just_download && DOWNLOAD false || Play "${Tracks[$track]}"
        done
        exit 0
}
function usage {
    cat <<- EOF
PlayMusic [OPTION]
-s              ReCreate the Settings file ..
-r [number]     Set the number of times a track should be played ..
-v              View the Settings file ..
-u              Uninstall the Program ..
-d              Force download ..
-p              Force play ..
-a              -d / -p applies for everything in the list, so everything will be downloaded/played automatically ..
-D              Turn on double check mode ..
--version   Show Program Info ..
EOF
}
############################################### Parsing Arguments ########################
[[ "$1" == '--version' ]] && { echo 'PlayMusic v1.3, Licence:GPL3' ; exit 0 ;  } # giving the version
while getopts r:svdupaD opt;do # Getting options
    case $opt in
        r )
            if IsNum "$OPTARG" && [[ "$OPTARG" -gt 1 ]];then
                declare times_to_play="$OPTARG"
            else
                echo 'Invalid Number of Times ..' >&2
            fi ;;
        s ) create_settings_file true
             notify-send "Settings File Recreated Successfully !"
             exit 0 ;;
        v ) read -p 'Editor ? ' editor
             if [[ -z "$editor" ]] || ! which "$editor" &>/dev/null;then
                editor='nano'
             fi 
             $editor "$HOME/.config/PlayMusic/PlayMusic.settings"
             exit 0 ;;
        u ) IsYes -p 'Are you sure you want to UnInstall ? ' && UNINSTALL ;;
        d ) declare just_download=true chosen=true ;; # download without asking
        p ) declare just_download=false chosen=true ;; # play online without asking
        a ) declare all=true ;;
        D ) declare double_check=true ;; # turn on double check mode ( it is an experimantal feature for now .. )
        \? ) usage >&2; exit 2;;
    esac
done
if $all && ! $chosen;then echo "All what ? use -d or -p option to specify what to do with each of them .." >&2 ; exit 2 ; fi
############################################### Starting Work ##########################
parse_settings
clear
printf "\033[0;37mLoading (\033[1m${#SITES[@]}\033[0;37m) Sites ..\n"
load_sites
$all && EVERYTHING
select track in "${!Tracks[@]}";do # interacting with the USER ..
    [[ -n $track ]] || continue # Continues the loop if choice is empty ( meaning that user's choice wasn't appropriate ) ..
    printf '\033[0m'
    clear
    if $chosen;then # if the user already specified something to do as an option ( -d / -p )
        $just_download && DOWNLOAD true || Play "${Tracks[$track]}"
    else
        select opt in "Download then Play" "Just Download" "Play Online" "Play then ask to download";do
            [[ -n "$opt" ]] || continue
            case "$opt" in
                "Play Online") Play "${Tracks[$track]}" ;;
                "Download then Play") DOWNLOAD true ;;
                "Just Download") DOWNLOAD false ;;
                "Play then ask to download")
                    Play "${Tracks[$track]}"
                    IsYes -p 'Do you want to download it ? ' && DOWNLOAD false ;;
            esac
            break
        done
    fi
done

Answer 1

You completely ignored the first points from the review your earlier question, and they still apply here:

Whitespace is not a precious resource: your code is far too dense for my taste

• fewer semicolons, more newlines.
• try to limit your line length to 90 chars for readability

I would go even further, and recommend to stay within 70 chars. Look at this for example:

declare -r settings_file="$HOME/.config/PlayMusic/PlayMusic.settings" log_file="$HOME/Desktop/PlayMusic_log.log" # readonly variables

This is more than just a matter of taste. The left part of any text is always more readable and less easy to overlook than the far right part. This line should have been written as 3 lines instead:

# readonly variables
declare -r settings_file="$HOME/.config/PlayMusic/PlayMusic.settings" 
declare -r log_file="$HOME/Desktop/PlayMusic_log.log"

You gain nothing by squeezing this into one hardly readable line.

The same goes for all the other declare statements too. Just because you can declare multiple variables on one line doesn't mean you should. I think it's a lot more readable if you declare one variable per line in general, with few exceptions.

Since you didn't take this advice from your previous posting, let me quote something from Steve McConnell's Code Complete:

Favor read-time convenience to write-time convenience. Code is read far more times than it’s written, even during initial development. Favoring a technique that speeds write-time convenience at the expense of read- time convenience is a false economy.

Some more specific notes about using more spaces:

• Put a space after semicolons, for example:
  • Instead of for ...;do write as for ...; do
  • Instead of if [[ ... ]];then write as if [[ ... ]]; then
• Put one or two empty lines before function definitions
• Put one empty line between large logical steps, for example when you have a large while loop followed by a large if followed by another large while, put some empty lines between those blocks of code for visual separation

Simplifications

You could simplify the IsNum function:

IsNum() {
    return $([[ "$1" =~ ^[0-9]+$ ]] && echo 0 || echo 1)
}

You don't need the subshell to return 0 or 1, you could use the exit code of [[ ... ]] itself, and you don't need to quote $1 inside it:

IsNum() {
    [[ $1 =~ ^[0-9]+$ ]]
}

At other places, instead of exit 0, you could simply exit, as 0 is the default exit code (success).

Functions

According to the bash hackers wiki, this writing style of function declarations is not recommended:

function Play {
    # ...
}

This is the preferred way:

Play() {
    # ...
}

Suspicious code

As shellcheck.net points out, be careful with these kind of expressions:

$just_download && DOWNLOAD false || Play "${Tracks[$track]}"

A && B || C is NOT an if-then-else. C will be executed when:

• A fails
• A succeeds but B fails

It seems that if $just_download is true, you won't want to play. In that case you should rewrite the above with an if:

if $just_download; then DOWNLOAD false; else Play "${Tracks[$track]}"; fi

Copy-paste your code on shellcheck.net and review the warnings of all the places where you use this pattern. Even if it's an innocent use case (for example when you know for sure that B always succeeds), it's a good practice to rewrite these suspicious cases to make all tests pass.