patternbashMinor
Bash Music Player
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
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
You rely upon the presence of ~/Music, but never test that it exists.
Variable names: stick with one style, and don't make that style UPPER_CASE (one day you'll accidentally use
If you're changing
Use
Use ~/.config for user-specific configuration files instead of /usr/local/settings (where you need root-access)
- 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.