Bash Music Player 2

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 /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
[[ "$f

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.

StackExchange Code Review Q#49128, answer score: 17