Simple Bash Music Player - follow-up

Question

This question is a follow-up to this question.

This is a year later, but it has the same context: I wanted new reviews for the updated code from the original question, but this time I wrote the code from scratch. I did this, not because the project is of any value or interest to me any more, as much as I wanted to compare my programming knowledge/style now to mine a year before. So, you will probably notice different algorithms and a different style of programming.

I would appreciate it if you could review the new code, and possibly refer to previous bad habits I might still be having. Also, I would be extra thankful if you would state any progress notes between the two versions of the code. I've implemented new features, fixed bugs, and incorporated command line options.

The program uses a bash utility library I've created for common tasks, called bash_lib:

#!/bin/bash

declare -rA _EXIT_CODES=(
    ['EX_OK']=0             # successful termination
    ['EX__BASE']=64         # base value for error messages
    ['EX_USAGE']=64         # command line usage error
    ['EX_DATAERR']=65       # data format error
    ['EX_NOINPUT']=66       # cannot open input
    ['EX_NOUSER']=67        # addressee unknown
    ['EX_NOHOST']=68        # host name unknown
    ['EX_UNAVAILABLE']=69   # service unavailable
    ['EX_SOFTWARE']=70      # internal software error
    ['EX_OSERR']=71         # system error (e.g., can't fork)
    ['EX_OSFILE']=72        # critical OS file missing
    ['EX_CANTCREAT']=73     # can't create (user) output file
    ['EX_IOERR']=74         # input/output error
    ['EX_TEMPFAIL']=75      # temp failure; user is invited to retry
    ['EX_PROTOCOL']=76      # remote error in protocol
    ['EX_NOPERM']=77        # permission denied
    ['EX_CONFIG']=78        # configuration error
    ['EX__MAX']=78          # maximum listed value
)

# Displays a menu-based list of choices to screen
# and echoes the associated value of the choice

# @ENVIROMENT_VAR $_MENU_CHOICES the associative array for choice (key) / returned_value (value)

menu()
{
    select choice in "${!_MENU_CHOICES[@]}" ; do
        [ -n "$choice" ] || continue
        echo "${_MENU_CHOICES[$choice]}"
        return
    done
}

# Outputs error message and exits with an error code

# @param $1 the error message, echo -e
# @param $2 the error code. If $2 is empty, no exit happens.

error()
{
    echo -e "$1" >&2
    log "error: $1"
    [ -n "$2" ] && _exit $2
}

# Returns host name of given site. ex: http://google.com/whatever -> google.com

# @param $1 the url

url_get_host()
{
    basename "$( dirname "$1" )"
}

# Returns the doceded url of given site. ex: http%3A%2F%2Fwww -> http://www

# @param $1 the url
# @param $2 the number of times to decode it. default: 2

url_decode()
{
    local res="$1"
    local num="$2"

    if [ "$num" = auto ] ; then
        while egrep '%[0-9]+' -q <<< "$res" ; do
            res="$( sed 's/%\([0-9A-F][0-9A-F]\)/\\\x\1/g' <<< "$res" | xargs -0 echo -e)"
        done
    elif [ -z "$num" ] || ! is_num "$num" ; then
        num=2
    fi

    if ! [ "$num" = auto ] ; then
        for ((i=0; i < $num; ++i)) ; do
            res="$( sed 's/%\([0-9A-F][0-9A-F]\)/\\\x\1/g' <<< "$res" | xargs -0 echo -e)"
        done
    fi

    echo "$res"
}
# Returns wether a text is a number

# @param $1 the text

is_num()
{
    egrep '^[0-9]+$' -q <<< "$1"
}

# Returns wether a text is a confirmation

# @param $1 the text. Or -p text: read -p $2 and check $REPLY instead
# @param $2 used only if $1 is -p: prompt text

is_yes()
{
    REPLY="$1"
    [ "$1" = "-p" ] && read -p "$2" REPLY
    egrep '^([yY]|[Yy]es)$' -q <<< "$REPLY"
}

# Returns wether a program exists in $PATH or as a function

is_prog_there()
{
    [ -x "$( which "$1" )" ] || [ -n type "$1" ]
}

# Looks for program and if not found, exists with error an code

# @param $1 the program name

require()
{
    for arg in "$@" ; do
        is_prog_there "$arg" || error "Required program not found: $arg" EX_UNAVAILABLE
    done
}

# Logs a string to a log file

# @param $1 the string
# @param $2 the log file path. default is ${_LOGFILE}

# @ENVIRONMENT_VAR ${_LOGFILE} default location of logfile if $2 is empty

log()
{
    echo "[$( date )]: $1" >> "${_LOGFILE:-$2}"
}

# Exits after logging

# @param $1 the exit code. default is 0

# @ENVIRONMENT_VAR ${_FORCED_EXIT} if set to true, does not check error code.

_exit()
{
    local code="$1"

    if [ -z "$code" ] ; then
        code=0
    elif ! is_num "$code" ; then
        [ -n "${_EXIT_CODES["$1"]}" ] && code=${_EXIT_CODES["$1"]} || code=0
    fi

    if ( ! [ "${_FORCED_EXIT}" = true ] ) &&
        ([[ $code -eq 1 ]] || [[ $code -eq 2 ]] ||
            ([[ $code -ge 127 ]] && [[ $code -le 165 ]]) || [ $code = 255 ]); then
        error "Wrong exit code reported: $code. Exit codes must NOT be \
1,2,127-165,255, these are system reserved.\
Use the _EXIT_CODES associative array instead." ${_EXIT_CODES['EX_SOFTWARE']}
    fi

    log "Exiting with error code $code"
    exit "$code"
}

Here is the program source code (sorry if it's longer than you'd expect):

#!/bin/bash

declare -r SettingsFile="$HOME/.config/PlayMusic/settings.conf"
declare -r _LOGFILE="$HOME/.config/PlayMusic/logfile.log"
declare -r Version=0.3.2

source bash_lib

log "Program started: $1"

declare -A Settings=(
    ['Dropbox']=false               # Wether to copy downloaded tracks to the dropbox folder
    ['TimesToPlay']=1               # Number of times to play a track
    ['Download']=false              # Wether to download the track
    ['Play']=true                   # Wether to play the track
    ['All']=false                   # Wether to act on all tracks found, or just ask to specify track(s)
    ['Player']='mplayer'            # Command to invoke a CLI sound player
    ['Downloader']='wget -c -O'     # Command to invoke a CLI downloader
    ['Editor']="${EDITOR:-nano}"    # Command to invoke a CLI editor
    ['MusicPlace']="$HOME/Music"    # Directory to download music
)

declare 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/'
    'http://mp3.com/top-downloads/genre/emo/'
    'http://mp3.com/top-downloads/genre/pop/'
)   # Array of sites to search for tracks at

declare Extensions=(
    'mp3'
    'ogg'
)   # Array of extensions to look for

# Key: Track title. Value: Track url.
declare -A Tracks

# Displays settings in a user-friendly manner on the screen

info()
{
    cat << __EOF_

    # General Settings #
    Settings File:          $SettingsFile
    Log File:           ${_LOGFILE}
    Version:            $Version

    # Behaviour Settings #
    Dropbox Support:        ${Settings['Dropbox']}
    Music Place:            ${Settings['MusicPlace']}
    Player:             ${Settings['Player']}
    Editor:             ${Settings['Editor']}
    Player:             ${Settings['Player']}

    # Music Action Settings #
    Times to Play:          ${Settings['TimesToPlay']}
    Act on all tracks:      ${Settings['All']}
    Action:             $( get_music_action )

__EOF_
}

# Saves $Settings and $Sites to $SettingsFile in a valid format

save_settings()
{
    log "Creating settings file"

    mkdir -p "$( dirname "$SettingsFile" )"
    echo "Created on $( date )" > "$SettingsFile" || _FORCED_EXIT=true error "Fatal Error!" $?

    for option in "${!Settings[@]}" ; do
        echo "$option = ${Settings["$option"]}" >> "$SettingsFile"
    done

    echo "Sites" >> "$SettingsFile"
    for site in "${Sites[@]}" ; do
        echo "$site" >> "$SettingsFile"
    done

    log "Created settings file successfully"
}

# Extracts program name from a command

# @param $1 the cmd.

get_prog_name()
{
    cut -d ' ' -f 1 <<< "$1"
}

# Load $Settings and $Sites from the $SettingsFile

load_settings()
{
    log "Parsing settings file"

    local line_number=0 line
    local key value
    local correct
    local is_site=false site

    # Check Settings file
    if ! ([ -r "$SettingsFile" ] || [ -f "$SettingsFile" ]) ; then
        save_settings
        return
    fi

    while read -r line; do
        ((line_number++))

        if [ -z "$line" ] || [ $line_number = 1 ] ; then
            continue
        fi

        correct=false

        # Check line format
        if $is_site ; then
            Sites["${#Sites[@]}"]="$line"
            continue
        else
            if [ "$line" = "Sites" ] ; then
                is_site=true
                Sites=()
                continue
            elif ! egrep "^[a-zA-Z]+\s*=.+$" -q <<< "$line" ; then
                error "$SettingsFile:$line_number: Incorrect format of line" EX_CONFIG
            fi
        fi

        # Extract Key,Value pair
        key="$( egrep -o '^[a-zA-Z]+' <<< "$line" )"
        value="$( sed -E "s/^$key\s*=\s*(.+)$/\1/" <<< "$line" )"

        # Check if value is valid
        case "$key" in
            'Dropbox'|'Download'|'All'|'Play' )
                if egrep 'false|true' -q <<< "$value" ; then
                    correct=true
                else
                    error "Expected true or false"
                fi;;
            'TimesToPlay' )
                if is_num "$value" ; then
                    correct=true
                else
                    error "Expected a number"
                fi;;
            'Player'|'Editor'|'Downloader' )
                if is_prog_there "$( get_prog_name "$value" )" ; then
                    correct=true
                else
                    error "Expected an executable program"
                fi;;
            'MusicPlace' )
                if [ -d "$value" ] && [ -w "$value" ] ; then
                    correct=true;
                else
                    error "'$value' is not a writable directory"
                fi;;
            * ) error "$SettingsFile:$line_number: Invalid option: $key\nValid Options are:\n" \
            "\tDropbox, Download, All, Play, TimesToPlay, Player, Editor" EX_CONFIG;;
        esac

        if ! $correct ; then
            error "$SettingsFile:$line_number: Invalid value: '$value' to option: '$key'" EX_CONFIG
        fi

        Settings["$key"]="$value"
    done < "$SettingsFile"

    log "Parsed settings file successfully"
}

# Displays program usage in a user-friendly manner to screen

usage()
{
cat << _USAGE_
    Usage: ./PlayMusic
        -v|--version            Output version then exit.
        -h|--help               View this help then exit.
        -x|--dropbox            Allow copying downloaded files to $HOME/Dropbox.
        -t|--playtimes [num]    Times to play a track.
        -d|--download           To download a track without asking.
        -D|--no-download        To not download a track without asking.
        -p|--play               To play a track without asking.
        -P|--no-play            To not play a track without asking.
        -k|--ask                To force ask what to do with a track.
        -a|--all                To act on all tracks found.
        -y|--player [cmd]       The command to run the music player.
        -e|--edtor [cmd]        The command to run the editor.
        -l|--downloader         The command to run the downloader.
        -m|--music-place [dir]  To specify a music directory other than the one found at the settings file.
        -r|--recreate-settings  To recreate the settings file to default then exit.
        -E|--edit-settings      To edit the settings file then exit.
        -s|--save               To save the given settings (runs after analyzing all options).
        -i|--info               To display given settings so far then exit.
_USAGE_
}

# Outputs the function name of what to do with a track

get_music_action()
{
    if ${Settings['Download']} && ! ${Settings['Play']} ; then
        echo "download"
    elif ! ${Settings['Download']} && ${Settings['Play']} ; then
        echo "play"
    elif ${Settings['Download']} && ${Settings['Play']} ; then
        echo "download_then_play"
#   else
#       echo "ask"
    fi
}

# Parses program arguments

# @param $1 program arguments

parse_args()
{
    log "Parsing Arguments"

    local save=false
    local args=`getopt -o vhxt:dpDPy:e:amX::islk --long version,help,dropbox,playtimes:,download,play,no-download,no-play,player:,editor:,all,music-place:,extensions::,info,save,downloader,ask -n 'PlayMusic' -- "$@"` || error "Internal Error!" EX_SOFTWARE

    eval set -- "$args"

    while true ; do
        case "$1" in
            -v|--version ) echo "$Version"; _exit;;
            -h|--help ) usage; _exit;;
            -i|--info ) info; _exit;;
            -x|--dropbox ) Settings['Dropbox']=true;;
            -t|--playtimes )
                if is_num "$2" ; then
                    Settings['TimesToPlay']="$2"
                else
                    if [ -n "$2" ] ; then
                        error "'$2' is not a number" EX_CONFIG
                    else
                        error "Please provide a number for the '$1' option" EX_CONFIG
                    fi
                fi; shift;;
            -d|--download ) Settings['Download']=true;;
            -p|--play ) Settings['Play']=true;;
            -D|--no-download ) Settings['Download']=false;;
            -P|--no-play ) Settings['Play']=false;;
            -y|--player ) require "$( get_prog_name "$2" )" 2; Settings['Player']="$2"; shift;;
            -e|--editor ) require "$( get_prog_name "$2" )" 2; Settings['Editor']="$2"; shift;;
            -a|--all ) Settings['All']=true;;
            -A|--selective ) Settings['All']=false;;
            -m|--music-place )
                if ! ([ -d "$2" ] && [ -w "$2" ]) ; then
                    error "'$2' is not a writable directory to store music in" EX_CONFIG
                fi;;
            -X|--extensions )
                if [ -n "$2" ] ; then
                    Extensions=(${2//,/ })
                else
                    echo "Extensions: ${Extensions[@]// /,}"
                fi;;
            -s|--save ) save=true;;
            -l|--downloader ) require "$( get_prog_name "$2" )" 2; Settings['Downloader']="$2"; shift;;
            -k|--ask ) Settings['Play']=false; Settings['Download']=false;;
            -- ) break;;
            * ) error "Unknown argument: $1" EX_CONFIG;;
        esac
        shift
    done

    if ${Settings['All']} ; then
        local act=$( get_music_action )
        [ -z act ] && act="ask what to do with"

        is_yes -p "Are you sure you want to $act all tracks [y/n] ? " || Settings['All']=false
    fi

    if $save ; then
        log "Saving settings"
        save_settings
        exit
    fi

    log "Parsed Arguments successfully"
}

# Fills $Tracks by looking through $Sites for tracks ending in $Extensions. This is the core backend functionality.

find_tracks()
{
    log "Looking for tracks"

    local exts="${Extensions[@]// /|}"
    local num

    for site in "${Sites[@]}" ; do
        log "Checking site: '$site'"

        num=0
        [ "$1" = '-v' ] && echo "Parsing $site"

        for track in $( curl -Ls "$site" | egrep -o "\bhttp://.*\.("$exts")\b" ) ; do
            name="$( url_decode "$( basename "$track" )" auto )"
            Tracks["${name//+/ }"]="$track"
            ((num++))
        done

        log "Found $num track(s) at the site"
    done

    [ ${#Tracks[@]} = 0 ] && error "Couldn't find any tracks!" 0
}

# Edits $SettingsFile by ${Settings['Editor']}

edit_settings()
{
    log 'Edit settings requested'
    ${Settings['Editor']} "$SettingsFile"
}

# Handles music action

# @param $1 action
# @param $2 track url
# @param $3 track name (with extension)

handle_action()
{
    ([ -n "$1" ] && [ -n "$2" ] && [ -n "$3" ]) || return

    [ "$1" = 'download' ] || local name="$( sed -E 's/\.(\w+)$//' <<< "$3" )"
    $1 "$2" "$([ "$1" = 'play' ] && echo "$name" || echo "$3")" "$([ "$1" = 'download_then_play' ] && echo "$name")"
}

# Downloads a track to ${Settings['MusicPlace']} by ${Settings['Downloader']}
# If $3 is -v, Outputs the track location on disk.

# @param $1 the track url
# @param $2 the track name (with extension)
# @param $3 -v. optional.

download()
{
    local download_to="${Settings['MusicPlace']}/$2"

    log "Action: Download [$1] to [$download_to]"
    wget -c -O "$download_to" "$1"
    log "wget returned $?"

    if $Settings['Dropbox'] ; then
        cp "$download_to" "$HOME/Dropbox/Music"
        log "Copying file to dropbox"
    fi

    [ "$3" = '-v' ] && echo "$download_to"
}

# Plays a track by ${Settings['Player']}

# @param $1 the track location
# @param $2 the track name (without extension). optional.

play()
{
    log "Action: Play [$1]"
    [ -n "$2" ] && notify-send "PlayMusic: Playing $2"
    ${Settings['Player']} "$1"
}

# Downloads then plays a track

# @param $1 the track url
# @param $2 the track name (with extension).
# @param $3 the track name (without extension). optional.

download_then_play()
{
    log "Action: Download then Play [$1]"
    play $( download "$1" "$2" -v ) "$3"
}

# Asks the user what to do with a track

# @param $1 the other option, outputs nothing when chosen.

ask()
{
    local -A _MENU_CHOICES=(['Download']='download' ['Play']='play' ['Download then Play']='download_then_play' ["$1"]='')
    menu
}

# Main entry point

main()
{
    local site_number=0
    local com="$( get_music_action )"

    echo "Looking for Tracks in ${#Sites[@]} Site$([[ ${#Sites[@]} -gt 1 ]] && echo "s") .."
    find_tracks -v

    if ${Settings['All']} ; then
        [ -n "$com" ] || com="$( ask 'Exit' )"
        [ -n "$com" ] || _exit

        for track in "${Tracks[@]}" ; do
            handle_action "$com" "${Tracks["$track"]}" "$track"
        done
    else
        echo "Choose Track .."
        select track in "${!Tracks[@]}" 'Quit' ; do
            [ -n "$track" ] || continue
            [ "$track" = 'Quit' ] && _exit
            [ -n "$com" ] || com="$( ask 'Return back' )"
            [ -n "$com" ] || continue

            handle_action "$com" "${Tracks["$track"]}" "$track"
        done
    fi
}

# Handles signales. Should not be used directly.

forced_exit()
{
    _FORCED_EXIT=true error "Signal [$1] forced an exit." "$((128+$1))"
}

# Initializes the environment for processing

init_env()
{
    trap 'forced_exit 2' SIGINT
    trap 'forced_exit 3' SIGQUIT
    trap 'forced_exit 4' SIGABRT
    trap 'forced_exit 15' SIGTERM

    for i in "$@" ; do
        if [ "$i" = "-r" ] || [ "$i" = '--recreate-settings' ] ; then
            save_settings
            _exit
        fi

        if [ "$i" = "-E" ] || [ "$i" = '--edit-settings' ] ; then
            edit_settings
            _exit
        fi
    done

    mkdir -p "${Settings['MusicPlace']}"

    load_settings
    parse_args "$@"

    if [ ${#Sites[@]} = 0 ] ; then
        error "No Sites were found!" 0
    fi
}

ARGS="$@"
init_env "$ARGS"

require "$( get_prog_name "${Settings['Player']}" )" "$( get_prog_name "${Settings['Editor']}" )" "$( get_prog_name "${Settings['Downloader']}" )"

main "$ARGS"

I would like to get reviews for anything and everything: The documentation style, the coding style, the choice of language, variable naming, algorithms, etc.

Answer 1

This is just a review on bash_lib...

# Returns the doceded url of given site. ex: http%3A%2F%2Fwww -> http://www

# @param $1 the url
# @param $2 the number of times to decode it. default: 2

I'm curious as to why you need to specify the number of times (attempts?) to decode such values... Also, do you have access to perl? Because if you do then it's easier and shorter to do via a simple Perl script. ;)

To check for yes (or equivalent) inputs, I think a better approach is to normalize the casing first, then apply a case-insensitive egrep:

$ for i in y Y Yes YES no; do egrep -i '^y(es|)$' -q <<< $i \
    && echo $i - OK; done
y - OK
Y - OK
Yes - OK
YES - OK

Compare that with your current method:

$ for i in y Y Yes YES no; do egrep '^([yY]|[Yy]es)$' -q <<< $i && echo $i - OK; done
y - OK
Y - OK
Yes - OK

For your log() function, you can use date to format the output directly:

date +"[%c]: $1"        # this
echo "[$( date )]: $1"  # instead of this

([[ $code -eq 1 ]] || [[ $code -eq 2 ]] ||
            ([[ $code -ge 127 ]] && [[ $code -le 165 ]]) || [ $code = 255 ])

That can be simplified too, by putting them all within one [[ ... ]]:

$ for code in 1 2 120 127 160 165 240 255; do 
    if ([[ $code -eq 1 ]] || [[ $code -eq 2 ]] || 
            ([[ $code -ge 127 ]] && [[ $code -le 165 ]]) || [ $code = 255 ]); then 
        echo $code - Y1; fi; 
    [[ $code -eq 1 || $code -eq 2 || ($code -ge 127 && $code -le 165) || $code -eq 255 ]] \
        && echo $code - Y2; done
1 - Y1
1 - Y2
2 - Y1
2 - Y2
127 - Y1
127 - Y2
160 - Y1
160 - Y2
165 - Y1
165 - Y2
255 - Y1
255 - Y2

A step further is to combine it with ! [ "${_FORCED_EXIT}" = true ], I'll leave that as an exercise for the reader.

Finally, a small nitpick: wether is spelled wrongly... it should be whether.

edit Further review on url_decode() and the actual 'program'...

The other weird things about url_decode() are that you are repeating your decoding across two different loops, and carefully interpreting and handling for $num when $2 = auto (which isn't 'documented'). Also, since you mentioned that you prefer a more bash-like solution, perhaps you can consider the following too so that you can even not depend on grep and sed:

url_decode() { 
    local s="$1"; local n=${2:-0}; 
    while [[ $((n--)) -gt 0 || (-z $2 && $s =~ %[[:xdigit:]]{2}) ]]; do 
        s=$(echo -e ${s//%/\\\x}); 
    done; echo $s
}

Rather than defaulting to 2, you might as well incrementally apply the decoding until you don't see %XX, where X is a hexadecimal character. This is (better) represented using the regex character class [[:xdigit:]]. =~ replaces the egrep and ${s//%/\\\x} replaces the sed. The loop condition simply says:

• countdown $n until it reaches 0, i.e. [$n - 1, 0], or
• $2 is not specified and we still have 'leftover' values to decode.

If you still prefer to stick to a safe default such as 2, replace n=${2:-0} with n=${2:-2}, and then you can drop the || condition.

A quick test:

$ url_decode 'You%2BWon%2527t%2BBe%252BMissed'
You+Won't+Be+Missed

$ url_decode 'You%2BWon%2527t%2BBe%252BMissed' 1
You+Won%27t+Be%2BMissed

$ url_decode 'You%2BWon%2527t%2BBe%252BMissed' 2
You+Won't+Be+Missed

As for your actual 'program', I don't see many major problems with it...

Answer 2

Copy-paste your code on http://www.shellcheck.net/, it spots quite a few interesting points to improve.

---

Whenever possible, instead of running sub-shells with egrep or another command to validate patterns, it's more efficient to use [[. For example you could rewrite is_num as:

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

---

Avoid code duplication. In url_decode, the complex expression with sed is repeated. It would be better to extract that to a helper function.

---

In this code, checking for [ "$num" = auto ] and then again ! [ "$num" = auto ] is not pretty:

if [ "$num" = auto ] ; then
    while egrep '%[0-9]+' -q <<< "$res" ; do
        res="$( sed 's/%\([0-9A-F][0-9A-F]\)/\\\x\1/g' <<< "$res" | xargs -0 echo -e)"
    done
elif [ -z "$num" ] || ! is_num "$num" ; then
    num=2
fi

if ! [ "$num" = auto ] ; then
    for ((i=0; i < $num; ++i)) ; do
        res="$( sed 's/%\([0-9A-F][0-9A-F]\)/\\\x\1/g' <<< "$res" | xargs -0 echo -e)"
    done
fi

It would be better to introduce helper functions and refactor like this:

if [ "$num" = auto ] ; then
    while [[ $res =~ %[0-9]+ ]] ; do
        res=$(url_decode_once "$res")
    done
elif [ -z "$num" ] || ! is_num "$num" ; then
    res=$(url_decode_n_times "$res" 2)
else
    res=$(url_decode_n_times "$res" $num)
fi

---

Instead of this:

for arg in "$@" ; do

You can simplify as:

for arg; do