5
\$\begingroup\$

I have finally finished creating my first real project. It's just a simple music player that can provides the user with the latest music from any site (as long as it contains MP3 files) he provides in the settings file. It can play it, download it and has an simple settings file from which the user can: add/change sites to get music from and change the music player used when playing music. It was first created as a simple bash script to play music to keep me focused when learning programming, but I think that after its development it can be treated as a self-contained music program.

I would really appreciate it if anybody can check it for me. I would also like to hear suggestions or improvements about it since this would really help me along the way.

#!/bin/bash
cd "$HOME/Music" # So that any download will be in the music directory
######################################### Initial Values and Declarations ######################################
declare -r settings_file='/usr/local/settings/PlayMusic.settings';line_number=0;declare SITES=();declare -A Tracks;declare -r OlD="$IFS";declare all=false;
declare player='mplayer';declare FILE;declare TIMES=1;declare FILE2;declare no_choice=true;declare only_download=false;declare restricted=false;
############################################## Functions ##############################################
function Play { # Function for playing music according to user preferences
    if $2 && [[ ${#1} -lt 23 ]];then notify-send "Now Playing: \"$1\" .." "$track";fi # Notifing the user,only if he'll play freely ($2 ) & the track's name isn't too long
    for ((i=0; i<TIMES; i++));do $player "$1";done
}
function Find { # function to look for a previously downloaded file on disk
    unset FILE;echo "Looking for '$1' on disk .. "; # Informing the user, if that's going to take long ( which depends on the contents of his home directory )
    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
        IFS=$'\n';echo 'File was found at various places .. Which one to use ?';local file
        select file in $FILES; do
            [[ -n $file ]] || continue
            FILE="$file";break
        done
        read -p 'Do you want to Delete the other ones ? ' rrf
        if IsYes $rrf;then for file in $FILES;do if ! [[ "$file" == "$FILE" ]];then rm "$file";fi;done;IFS="$OlD";fi
    else FILE="$FILES";fi;clear
}
function IsYes { d=$(echo $1 | tr [[:upper:]] [[:lower:]]);if ([[ "${d:0:1}" == 'y' ]] && [[  "${#d}" -le 4 ]]) || [[ $d == 'ok' ]];then return 0; else return 1;fi; }
function create_settings_file {
    local tmp_file="$HOME/Desktop/File_$RANDOM"
    cat > $tmp_file <<- EOF
mplayer \# the first line is always the media player
\# 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
    sudo mv $tmp_file '/usr/local/settings/PlayMusic.settings'
    rm $tmp_file &>/dev/null # just in case ..
}
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
        [[ $(dirname "$FILE") == "$HOME/Music" ]] || { # 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 "$FILE" "$HOME/Music" &>/dev/null;FILE="$HOME/Music/$name" # move the file to the music home directory
        }
        name="$FILE"
    fi;notify-send "Downloading ($track) .." "in [ $PWD ] ..";wget -cO "$name" "${Tracks[$track]}" && Play "$name" true
}
function UNINSTALL {
    read -p 'Do you want to keep settings ? '
    if ! IsYes $REPLY;then sudo rm '/usr/local/settings/PlayMusic.settings';fi
    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;IFS="$OlD" # recreating Everything ..
    for SITE in "${SITES[@]}";do
        echo -e "\tSite #$((++counter)): '$SITE'"
        for site in $(lynx -source "$SITE" | egrep -o 'http://.*\.mp3');do # Grabbing all music links ( newline delimeted )
            local name="$(echo $site | sed -Ee 's/_/ /g' -e 's#.*/##g' -e 's#.mp3##g' -e 's#.*%2D ##g' -e 's#%2B# #g' |
                         sed -Ee "s#%2527#\'#g" -e 's#%2528#(#g' -e 's#%2529#)#g' -e 's#.* - ##' -e "s#(\w+) (s$|s )#\1'\2#")" # Filtering names out of sites ..
            if echo "$name" | grep -q '%';then continue;fi
            if $restricted;then
                local dupp=false;local Ty
                for Ty in "${!Tracks[@]}";do if (echo "$Ty" | grep -Eq ".* - $name") || (echo "$name" | grep -Eq ".* - $Ty");then dupp=true;break;fi;done
                if $dupp;then continue;fi
            fi
            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 # Just in Case ;)
}
function parse_settings  { # function to parse settings file
    IFS=$'\n'
    if [[ -f $settings_file ]];then
        for line in $(cat /usr/local/settings/PlayMusic.settings | sed -Ee 's/^ +//g' -e 's/(.*) *#.*/\1/g' -e 's/( *)$//g' -e 's/ *#.*//g');do
            [[ -n $line ]] || continue
            case $((++line_number)) in
                1 ) player="$line";;
                * ) SITES[${#SITES[@]}]="$line";;
            esac
        done
    else
        notify-send "Settings File not Found .. !";read -p "You haven't created your settings file .. Do you want to create it ? ";
        if IsYes $REPLY; then create_settings_file;sudo nano $settings_file;notify-send 'Done !'
        else echo 'Then, The Default settings are going to be used this time ..';fi
    fi;IFS="$IFS";if [[ ${#SITES[@]} == 0 ]];then SITES=('http://ccmixter.org/view/media/samples/mixed');fi # If no Site was specified in the settings file
}
############################################### Parsing Arguments #################################
while getopts r:svdupaR opt;do # Getting options
    case $opt in
        r ) if IsNum "$OPTARG" && [[ "$OPTARG" -gt 1 ]];then TIMES="$OPTARG";else echo 'Invalid Number of Times ..' >&2;fi;;
        s ) create_settings_file;notify-send "Settings File Recreated Successfully !";exit 0;;
        v ) read -p 'Editor ? ';sudo $REPLY '/usr/local/settings/PlayMusic.settings';exit 0;;
        u ) read -p 'Are you sure you want to UnInstall ? ';if IsYes $REPLY;then UNINSTALL;fi;;
        d ) only_download=true;no_choice=false;; # download without asking
        p ) only_download=false;no_choice=false;; # play online without asking
        a ) all=true;;
        R ) restricted=true;; # turn on restricted mode ( it is an experimantal feature for now .. )
    esac
done
############################################### Starting Work #####################################
parse_settings;clear;echo "Loading (${#SITES[@]}) Sites ..";load_sites
function EVERYTHING {
    if ! $no_choice;then
        $only_download && notify-send 'Downloading Everything .. !' || notify-send 'Playing Everything .. !'
        for track in "${!Tracks[@]}";do
            $only_download && DOWNLOAD || Play "${Tracks[$track]}"
        done;exit 0
    fi
        echo 'What "all" ? '
        select opt in 'Download All' 'Play All' 'Cancel'
            do [[ -n $opt ]] || continue;no_choice=false;
            [[ "$opt" == 'Download All' ]] && only_download=true
            [[ "$opt" == 'Cancel' ]] && { all=false;return 1;no_choice=true; }
        break;done
        EVERYTHING
}
$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 ) ..
    clear;if $only_download && ! $no_choice;then DOWNLOAD;exit 0;elif ! $only_download && ! $no_choice;then Play "${Tracks[$track]}" true;fi
    echo 'What do you want to do ?';name="$track.mp3" # Setting the name and asking the user ..
    select choice in "Download then Play" "Just Play Online";do [[ -n $choice ]] || continue # ( already demonstrated )
        if [[ $choice == "Download then Play" ]];then DOWNLOAD
        else Play "${Tracks[$track]}" true # Play Online
        fi;break
    done;break
done;exit 0
\$\endgroup\$
  • \$\begingroup\$ I updated the code here .. \$\endgroup\$ – Amr Ayman May 7 '14 at 18:45
6
\$\begingroup\$

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

You rely upon the presence of ~/Music, but never test that it exists.

  • add mkdir -p "$HOME/Music" at the top

Variable names: stick with one style, and don't make that style UPPER_CASE (one day you'll accidentally use PATH=something and then your script cannot find any programs anymore.

If you're changing $IFS in a function, use local IFS=... to localize the change to that function.

Use mktemp for creating temp files.

Use ~/.config for user-specific configuration files instead of /usr/local/settings (where you need root-access)

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.