2
\$\begingroup\$

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\(.\)/\u\1/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.

\$\endgroup\$
4
\$\begingroup\$

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\(.\)/\u\1/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.
  • 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.

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

| improve this answer | |
\$\endgroup\$
  • 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 also getopt, but I think it introduces more problems than it solves. A good review you have here. \$\endgroup\$ – Gao Dec 10 '17 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 using sed 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 Dec 10 '17 at 23:47
  • \$\begingroup\$ I'll definitely be using arrays on the rest variable. With the variable I'm using sed 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 the getopts 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 Dec 10 '17 at 23:49
  • \$\begingroup\$ be used in this case, but nonetheless I still appreciate the help! 3/3 \$\endgroup\$ – Aguxez Dec 10 '17 at 23:53

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.