HiveBrain v1.2.0
Get Started
← Back to all entries
patternbashMinor

Bash Music Player

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
musicplayerbash

Problem

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/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

Solution

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)

Context

StackExchange Code Review Q#48187, answer score: 6

Revisions (0)

No revisions yet.