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.
2 Answers 2
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 reaches0
, 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...
-
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\$Amr Ayman– Amr Ayman2015年07月24日 07:48:46 +00:00Commented 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\$h.j.k.– h.j.k.2015年07月24日 07:58:50 +00:00Commented 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\$Amr Ayman– Amr Ayman2015年07月24日 07:58:55 +00:00Commented 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\$h.j.k.– h.j.k.2015年07月24日 08:04:45 +00:00Commented Jul 24, 2015 at 8:04
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