2
\$\begingroup\$

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]\)/\\\x1円/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]\)/\\\x1円/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.

asked Jul 23, 2015 at 17:25
\$\endgroup\$

2 Answers 2

1
\$\begingroup\$

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

answered Jul 24, 2015 at 7:13
\$\endgroup\$
4
  • 1
    \$\begingroup\$ About the numbered attempts to decode a url: Consider this string 'Lil%2BBoy', it will decode from the first attempt to this: 'Lil+Boy' which would clearly mean it's decoded successfully. But some words are decoded several times, like this: 'You%2BWon%2527t%2BBe%2BMissed', try to decode it, it will be: 'You+Won%27t+Be+Missed', decode it again and it will be: 'You+Won't+Be+Missed', implying success. And yes, I do have access to perl but I'm trying to use commands that are more likely to be found on other systems. \$\endgroup\$ Commented Jul 24, 2015 at 7:48
  • \$\begingroup\$ Hmms... I get what you mean, but in your example the quote should have been encoded as %27 ideally... I suppose these are just out of your control... \$\endgroup\$ Commented Jul 24, 2015 at 7:58
  • \$\begingroup\$ About checking yes inputs, I get your point, and I think this command would be more efficient: egrep '^([Yy]|[Yy][Ee][Ss])$' -q <<< "$REPLY". About the log note, yes I think your implementation is more efficient. About your last simplification, I don't see how that is simpler despite being less efficient as it uses a loop. \$\endgroup\$ Commented Jul 24, 2015 at 7:58
  • 1
    \$\begingroup\$ @AmrAyman that loop is just used as an example to cycle through the valid/invalid $code values to let you see and verify both have the same results. ;) What I'm trying to show is to "put them all within one [[ ... ]]". \$\endgroup\$ Commented Jul 24, 2015 at 8:04
2
\$\begingroup\$

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]\)/\\\x1円/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]\)/\\\x1円/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
answered Jul 24, 2015 at 18:23
\$\endgroup\$

Your Answer

Draft saved
Draft discarded

Sign up or log in

Sign up using Google
Sign up using Email and Password

Post as a guest

Required, but never shown

Post as a guest

Required, but never shown

By clicking "Post Your Answer", you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.