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 "\t033円[30;3mSite 033円[37m#033円[1m$((++counter))033円[0m033円[30;3m: '033円[1;33;3m$SITE033円[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

• that other guy: You could consider fixing issues pointed out by automated tooling first, and making the code more syntactically consistent (not putting some loops/ifs or multiple statements on single lines, using the same amount of whitespace around keywords, using same capitalization everywhere). This will let human reviewers focus on the important things.

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.