This is actually my first shell script and just by looking at it I know this can look better. I use Elixir and Phoenix for most of my projects and I started using Zurb Foundation 6, changing from Bootstrap to Foundation is kinda tiresome sometimes so I decided to make a script to automate this process.
This is the script
#!/bin/sh
JAVI_PATH="$HOME/.javi"
name=${1}
path=${2}
rest=${@:3}
### Funcs
get_files() {
cd ${name}/assets
echo "Installing extra dependencies"
npm install --save jquery sass-brunch foundation-sites normalize-scss
echo "Getting files to replace"
get_brunch
get_scss
cd ../..
echo "Got and replaced files on 'assets' folder"
# Replaces content from 'app.html.eex' file
get_apphtml
}
get_brunch() {
rm -rf brunch-config.js && \
wget https://raw.githubusercontent.com/aguxez/javi/master/brunch-config.js
}
get_scss() {
cd css && \
rm -rf app.css phoenix.css && \
wget https://raw.githubusercontent.com/aguxez/javi/master/app.scss
}
get_apphtml() {
cd lib/${name}_web/templates/layout && \
rm -rf app.html.eex && \
wget https://raw.githubusercontent.com/aguxez/javi/master/app.html.eex
# Here we replace the title with the 'name' variable
uppercased_name=`sed -e "s/\b\(.\)/\u1円/g" <<< "${name}"`
eval sed -i -e 's/CHANGE_TITLE/${uppercased_name}/g' app.html.eex
}
## If installing
INSTALL=0
if [ "1ドル" = "install" ] ; then
INSTALL=1
fi
if [ "$INSTALL" = 1 ] ; then
if [ ! -d "$JAVI_PATH" ] ; then
mkdir $JAVI_PATH && \
curl -sSL https://raw.githubusercontent.com/aguxez/javi/master/javi > $JAVI_PATH/javi && \
chmod +x $JAVI_PATH/javi
echo "Javi has been configured"
fi
elif [ -z "${name}" ]; then
echo "Please give a name to your project"
elif [[ -d ${path} ]]; then
echo "Creating project in ${path}"
cd ${path}
mix phx.new ${name} ${rest}
# Runs function
get_files
else
mix phx.new ${name} ${rest}
# Function
get_files
fi
Maybe I could think of a better way of handling arguments here
if [ "$INSTALL" = 1 ] ; then
if [ ! -d "$JAVI_PATH" ] ; then
mkdir $JAVI_PATH && \
curl -sSL https://raw.githubusercontent.com/aguxez/javi/master/javi > $JAVI_PATH/javi && \
chmod +x $JAVI_PATH/javi
echo "Javi has been configured"
fi
elif [ -z "${name}" ]; then
echo "Please give a name to your project"
elif [[ -d ${path} ]]; then
echo "Creating project in ${path}"
cd ${path}
mix phx.new ${name} ${rest}
# Runs function
get_files
else
mix phx.new ${name} ${rest}
# Function
get_files
fi
But I'm not sure what I can change or HOW can I change it.
1 Answer 1
I'm not a shell expert, but I think I could help a bit. The most relevant tips I could give are:
1. Quote your expansions
Unquoted expansions are the most common source of bugs and security issues in shell scripts. There are cases where it's safe to leave expansions unquoted (as in foo=$bar
) but, unless extremely necessary, it's preferable to double-quote every expansion you have.
2. Prefer $(command)
over `command`
The $(command)
syntax is POSIX-compliant and it can be easily nested.
3. Be consistent
You mix different styles in your code.
Quotes: There are both quoted and unquoted expansions. As I said before, prefer quoted expansions unless you are explicitly looking for side effects like word splitting.
Braces: When doing parameter expansions, sometimes you use braces, sometimes not. Braces are optional in most of the simple substitutions but they are useful when you have two-digit positional parameters or to avoid ambiguity. I personally prefer to always put my parameters within braces.
[
and[[
: If portability is a concern, use[
. If not, use[[
.
4. Be careful with eval
Avoid it if you are not completely sure of what you are doing.
5. Arrays
I suppose you use the rest
variable to save your remainder arguments. The problem is that you are saving them as a single string and you are relying on word splitting to handle it.
If your shell support arrays, this would be a safer and cleaner solution:
rest=( "${@:3}" )
...
mix phx.new "${name}" "${rest[@]}"
6. Shell builtins
If available, prefer them over external commands.
For example, if you use a recent version of bash and portability is not a big deal, you could rewrite uppercased_name=`sed -e "s/\b\(.\)/\u1円/g" <<< "${name}"`
as uppercased_name="${name^}"
7. Arguments parsing
First, I recommend you to read the Utility Conventions chapter of the POSIX specification. It's useful to understand how argument syntax works in standard utilities and serves as a guide to the rules you should follow when you create some program.
The goal is to make sure your script follows the program [options] [operands]
syntax. I mean, instead of rely on the position of arguments to assign variables (as in name=${1}
), your script should parse --name foo
(or something similar) to change the value of the name
variable.
There are basically two approaches:
- Using utilities like
getopts
- Pros:
getopts
is a POSIX builtin command.- It can handle things like
-abc
without effort. - Easy and intuitive.
- Cons:
- Can't handle long options.
- Pros:
- Writing your own parser
- Pros:
- Full control of what and how it is parsed.
- Handling of long options.
- Could implement non-standard syntax.
- Cons:
- It's harder to handle things like
-abc
and edge cases.
- It's harder to handle things like
- Pros:
Greg's and Bash Hackers' (1, 2) wikis have tutorials for both approaches, so check them out.
I've also written an example script to show how to do manual parsing and colored output:
Script with manual parsing and colored output
-
1\$\begingroup\$ I didn't know one can write an opening parenthesis before each of the patterns in a case statement. There is a hack to make
getopts
handle long options. It's mentioned in a blog of an autotools maintainer IIRC, but now I can only find this longer version: gist.github.com/rtfpessoa/867ac97c7795dcc647063245d27dd88c. There's alsogetopt
, but I think it introduces more problems than it solves. A good review you have here. \$\endgroup\$Gao– Gao2017年12月10日 17:05:33 +00:00Commented Dec 10, 2017 at 17:05 -
\$\begingroup\$ Thank you for the great review. Here I wanted to just "get things done" but I didn't like this approach, I reviewed the sources you linked and understand a lot more of the problems I have in the script, I'll try to rewrite it following your advices. Some things: I could think of another solution instead of using
eval
but from what I read, it can be harmful if I'm accepting input from the user and doing something on the system but since here I'm just usingsed
to change a word in a file I can't think of anything that could go wrong (Even when something can still happen) 1/? \$\endgroup\$Aguxez– Aguxez2017年12月10日 23:47:59 +00:00Commented Dec 10, 2017 at 23:47 -
\$\begingroup\$ I'll definitely be using arrays on the
rest
variable. With the variable I'm usingsed
to uppercase, I was thinking on using the^
builtin but I tried the script on Zsh too and sadly it doesn't work there (I use Zsh more often than Bash) and that's why I'm using that approach. And for the Arguments passing I can look at thegetopts
utility but I was questioning this since the only arguments you can pass to the scripts are handled already by Phoenix, those are options to Phoenix and no the script itself (Not sure if I explained clearly) and that's why I'm not sure if it should 2/? \$\endgroup\$Aguxez– Aguxez2017年12月10日 23:49:17 +00:00Commented Dec 10, 2017 at 23:49 -
\$\begingroup\$ be used in this case, but nonetheless I still appreciate the help! 3/3 \$\endgroup\$Aguxez– Aguxez2017年12月10日 23:53:18 +00:00Commented Dec 10, 2017 at 23:53