current community

your communities

more stack exchange communities

Stack Exchange Inbox Reputation and Badges
tour help
Sign up ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 2 down vote favorite
1

I'm writing a bash plugin to help people create books in a specific way. It's an ordinary shell script to setup the project and create its subresources. I'm using a loop to create barebones assets and then seek to checks-in the project into a repository as well.

The plugin will eventually provide simple commands such as $ bookiza insert [insert_at] [number of pages], $ bookiza remove [page_no] to do a few repetive tasks for an author and give a little control over the project.

Here's what the first cut of my bash script looks like:

# -------------------------------------------------#
#                SUPERBOOK WRITERS                 #
# -------------------------------------------------#


# Add `~/bin` to the `$PATH`
export PATH="$HOME/bin:$PATH";

# TODO: Split methods below into logical files and include alongside `$PATH`.
for file in ~/bookiza/.{path}; do
  [ -r "$file" ] && [ -f "$file" ] && source "$file";
done;
unset file;

# ---------- BOOKIZA INITIALIZER --------- #


bookiza() {
  case "$1" in
    new)
      new "$@"
      ;;         
    insert)
      insert "$@"
      ;;         
    add)
      add "$@"
      ;;             
    remove)
      remove "$@"
      ;;
    length)
      cd "manuscript"
      getLength
      cd ".."
      ;;
    server)
      stop
      serve
      ;;         
    check)
      check
      ;;         
    help)
      help      
      ;;         
    *)
      echo $"Usage: $0 { new | insert | length | remove | server | check | help}"
      echo $"Try: $ bookiza help"
  esac
}

#--------- NEW PROJECT ---------#


new() {
  args=("$@")

  echo Number of arguments passed =: $#
  echo "Type: ${args[0]}, Project: ${args[1]} Booklength: ${args[2]}"


  PROJECTNAME=${args[1]}

  if [ ${PROJECTNAME:+x} ] ; then
    echo "Proceeding ........"
  else 
    echo "Halting ..........."
    validateProjectName $PROJECTNAME
  fi

  setupProject $PROJECTNAME


  BOOKLENGTH=${args[2]}

  if [ ${BOOKLENGTH:+x} ] ; then
    echo "Proceeding ........"
    validateNumeric $BOOKLENGTH
  else 
    echo "Halting ..........."
    validateBookLength $BOOKLENGTH
  fi

  createPages $BOOKLENGTH

  setupGitRepository 
}

###### Validations ######

validateProjectName() {
  if [[ $# -eq 0 ]] ; then
    echo "Project name not supplied. (HINT: My-New-Book-Name i.e. use hypens!)"
    read PROJECTNAME
    if [ ${PROJECTNAME:+x} ] ; then
      return 
    else 
      echo "Halting ..."
      validateProjectName $PROJECTNAME
    fi
    exit
  fi  
}

validateBookLength() {
  if [[ $# -eq 0 ]] ; then
    echo "Book length not supplied. (HINT: Must be even number i.e. 6, 12, 24!)"
    read BOOKLENGTH
    if [ ${BOOKLENGTH:+x} ] ; then
      validateNumeric $BOOKLENGTH
      return $BOOKLENGTH
    else 
      echo "Halting ..."
      validateBookLength $BOOKLENGTH
    fi
    exit
  fi  
}

validateNumeric() {
  BOOKLENGTH=$1
  reg='^[0-9]+$'
  if ! [[ $BOOKLENGTH =~ $reg ]] ; then
    echo "Error: Argument not a number, try again:" >&2;
    read BOOKLENGTH
    validateNumeric $BOOKLENGTH
  else
    validateEven $BOOKLENGTH
  fi
}

validateEven() {
  BOOKLENGTH=$1
  echo "Testing if ${BOOKLENGTH} is even now"
  if [ $((BOOKLENGTH%2)) -eq 0 ] ; then
    echo "Ok ....... Proceeding"
    echo "Setting book length = $BOOKLENGTH"
    return $BOOKLENGTH
  else
    echo "Error: Not an even number, try again:" >&2;
    read BOOKLENGTH
    validateEven $BOOKLENGTH
  fi  
}

setupProject() {
  echo "Setting up $PROJECTNAME now ..."
  mkdir -p "$1" && cd "$1" && touch README.md license.txt .gitignore && mkdir "trash" "cover" "templates" "images" "manuscript" || return $?
  echo "# $1" >> README.md
  cd "templates" && touch template.html head.html template.css template.js && cd ".."
}

createPages() {
  PAGES=$1
  cd "manuscript"
  p=0
  while [ "$p" -lt "$PAGES" ]; do
    p=$((p+1))
    mkdir -p "page-$p"
    cd "page-$p"
    touch "body.html"
    touch "style.css"
    echo "body{background:rgba(200, 235, 255, 0.99); margin:0 0; overflow:hidden;}" >> style.css
    cd ".."
  done  
  echo "Done!" && cd ".." #Head back to root
}

setupGitRepository() {
  git init
  git add . -A
  git commit -am "First commit: Setup new book project" --quiet
  echo "Provide GITHUB URL:"
  read REPO_URL
  if [ ${REPO_URL:+x} ] ; then
    git remote add origin "$REPO_URL"
    git push -u origin master
    echo "Project ready! Stacked ${PAGES} blank pages inside /manuscript correctly."   
  else
    echo "Error: Argument not supplied, try again!"
    read REPO_URL
  fi
}

# ---------- INSERT PAGES --------- #

insert() {
  args=("$@")

  echo Number of arguments passed = $#
  echo Type: ${args[0]}, INSERT_AT: "${args[1]}", [ No. of pages: "${args[2]}"]

  INSERT_AT=${args[1]}

  if [ ${INSERT_AT:+x} ] ; then
    echo "Ok ........ Proceeding"
    validateNumericalInsertAt $INSERT_AT
  else 
    echo "Halting ..........."
    validateInsertAt $INSERT_AT
  fi

  NUMBER_OF_PAGES=${args[2]}

  if [ ${NUMBER_OF_PAGES:+x} ] ; then
    validateNumberOfPages $NUMBER_OF_PAGES
  else
    NUMBER_OF_PAGES=2
  fi

  generatePages $INSERT_AT $NUMBER_OF_PAGES

  echo "DONE:)"
}

validateInsertAt() {
  if [[ $# -eq 0 ]] ; then
    echo "INSERT_AT: not supplied. (HINT: Must be integer!)"
    read INSERT_AT
    if [ ${INSERT_AT:+x} ] ; then
      validateNumericalInsertAt $INSERT_AT
      return $INSERT_AT
    else 
      echo "Halting ..."
      validateInsertAt $INSERT_AT
    fi
    exit
  fi  
}

validateNumericalInsertAt() {
  INSERT_AT=$1
  reg='^[0-9]+$'
  if ! [[ $INSERT_AT =~ $reg ]] ; then
    echo "Error: Argument not a valid number, try again:" >&2;
    read INSERT_AT
    validateNumericalInsertAt $INSERT_AT
  else
    return $INSERT_AT
  fi
}

validateNumberOfPages() {
  NUMBER_OF_PAGES=$1
  reg='^[0-9]+$'
  if ! [[ $NUMBER_OF_PAGES =~ $reg ]] ; then
    echo "Error: Argument not a valid number, try again:" >&2;
    read NUMBER_OF_PAGES
    validateNumberOfPages $NUMBER_OF_PAGES
  else
    return $NUMBER_OF_PAGES
  fi
}

generatePages() {
  args=("$@")
  echo INSERT_AT: "${args[0]}", [ Number of pages: "${args[1]}" ]

  p="$INSERT_AT"

  cd "manuscript"

  if [ -d "page-$p"  ]; then
    getLength
    q="$BOOKLENGTH"
    while [ "$q" -ge "$p" ]; do
      mv "page-$((q))" "page-$((q+NUMBER_OF_PAGES))"
      q=$((q-1))
    done

    q=0
    while [ "$q" -lt "$NUMBER_OF_PAGES" ]; do
      mkdir -p "page-$((p+q))"
      cd "page-$((p+q))"

      touch "body.html"
      touch "style.css"
      q=$((q+1))
      cd ".."
    done
  fi
  cd ".."
  getLength
}

# ---------- ADD PAGES --------- #

add() {
  args=("$@")

  echo Number of arguments passed =: $#
  echo "Type: ${args[0]}, Number of pages: ${args[1]}"
  cd "manuscript"

  NUMBER_OF_PAGES=${args[1]}

  if [ ${NUMBER_OF_PAGES:+x} ] ; then
    validateNumberOfPages $NUMBER_OF_PAGES
  else
    NUMBER_OF_PAGES=2
  fi

  getLength

  echo "SHOUT AT ME = ${NUMBER_OF_PAGES}"

  q=1
  while [ "$q" -le "$NUMBER_OF_PAGES" ]; do
    mkdir -p "page-$((BOOKLENGTH+q))"
    cd "page-$((BOOKLENGTH+q))"
    touch "body.html"
    touch "style.css"
    q=$((q+1))
    cd ".."
  done
  getLength
  cd ".."
}

remove() {
  args=("$@")

  echo Number of arguments passed =: $#
  echo "Type: ${args[0]}, Page number: ${args[1]}"

  cd "manuscript"
  PAGE_NO="${args[1]}"

  if [ ${PAGE_NO:+x} ] ; then
    trashPage $PAGE_NO
  else
    validatePageNo $PAGE_NO
  fi

  getLength
  cd ".."
}

trashPage() {
  args=("$@")
  echo PAGE_NO: "${args[0]}", [ Blah: "${args[1]}" ]

  PAGE_NO="${args[0]}"

  if [ -d "page-$PAGE_NO"  ]; then
    mv "page-$PAGE_NO" "../trash/page-$PAGE_NO"

    m="$PAGE_NO"
    echo "Starting directory reduction"
    m=$((m+1))

    until [ ! -d "page-$m"  ]; do

      mv "page-$((m))" "page-$((m-1))"
      m=$((m+1))
    done
  fi

}

# ---------- BOOKLENGTH --------- #

getLength() {
  BOOKLENGTH=1
  echo "Measuring book length ..."
  until [ ! -d "page-$((BOOKLENGTH+1))"  ]; do
    BOOKLENGTH=$((BOOKLENGTH+1))
  done
  echo Book length is: $BOOKLENGTH pages
  return
}

# ---------- LOCAL SERVER --------- #

serve() {
  echo "Server node server here?"
}

# ---------- LOCAL SERVER --------- #

check() { 
  echo "Check if project has even number of pages" 
}

# ---------- LOCAL SERVER --------- #

help() {
  echo "Help guide here"  
}

Here's the source on Github.

Before proceeding further into this work I want to have my code evaluated. DRY it up. Any tips or advice on better patterns and/or glaring mistakes (if any)? Given that return $variable doesn't take the value back to the function calling it, is it appropriate to write return $variable (readability) at all?

share|improve this question

1 Answer 1

up vote 3 down vote accepted

Single responsibility principle

The input validating functions do two things:

  1. Read input (when invoked without parameters)
  2. Validate input

It would be better to make a function do just one thing

Faulty input validation

The input validating functions don't validate at all when you pass parameters to them. As such these function names are misleading.

It seems you rely on that for example when calling validateProjectName $PROJECTNAME where the value of $PROJECTNAME is empty, then inside the function $# will have 0 as the value. This usage is misleading. If I rewrite the call as validateProjectName "$PROJECTNAME" then it will happily return from it (= valid project name), which is not right.

Furthermore, notice the duplicated validation logic: the half-baked [[ $# -eq 0 ]] is intended as the [ ${PROJECTNAME:+x} ]. The two tests look different but are designed for the same thing. It's a duplication that can be eliminated.

Broken input validation

If a parameter is not supplied, this function will behave very strange:

validateProjectName() {
  if [[ $# -eq 0 ]] ; then
    echo "Project name not supplied. (HINT: My-New-Book-Name i.e. use hypens!)"
    read PROJECTNAME
    if [ ${PROJECTNAME:+x} ] ; then
      return 
    else 
      echo "Halting ..."
      validateProjectName $PROJECTNAME
    fi
    exit
  fi  
}

It will keep asking for input until you give something to it, in each step printing "Halting...". When finally you enter something non-empty, the script will halt. Why doesn't it halt immediately then? This looks like a bug.

Pattern: input reading with retries

It's a common pattern to retry reading input until user enters something valid. But it's not common and not recommended to use recursion to repeat, because it can be confusing, and with enough failures a stack overflow can occur.

Consider this alternative:

isValidProjectName() {
    test "$1"
}

readValidProjectName() {
    PROJECTNAME=$1
    while ! isValidProjectName "$PROJECTNAME"; do
        echo "Please enter project name (HINT: My-New-Book-Name i.e. use hyphens!)"
        read PROJECTNAME
    done
}

readValidProjectName "$PROJECTNAME"

Points of interest:

  • Each function has precisely one purpose
  • Repeatedly asks for input in a loop until valid
  • The loop condition in readValidProjectName uses the exit code of isValidProjectName
  • The emptiness check is simplified by simply enclosing the variable within "..."

I suggest to follow this pattern in your other functions too.

Encapsulation

In the bookiza function, most actions call a dedicated function, which is nice, except length, in which the main task is wrapped within a cd "manuscript"; ...; cd ... The fact that it needs to go inside "manuscript" is an implementation detail that would be nicer to hide. I suggest to move these details inside a function, to handle this action uniformly like the others.

Moving into sub-directories

In general it's not recommended to change directories in scripts. It's easy to make a mistake, and if something goes wrong, the script might end up in the wrong directory.

Another issue with wrapping commands within a cd $somewhere; ...; cd .. is that if ever $somewhere will be more than one level deep, you'll have to remember to update cd .. accordingly.

A better and simpler way is to wrap within (...), for example:

(cd manuscript; getLength)

Notice that there's no cd .. at the end. There's no need: when the (...) exits, the script will be back in its original working directory.

Another alternative is using pushd and popd:

pushd manuscript; getLength; popd

Unnecessary double-quotes

It's good practice to double-quote path variables. On the other hand, when a directory name is a literal string with no special characters, then quoting is unnecessary, for example:

cd "manuscript"
cd ".."

These can be simply:

cd manuscript
cd ..

Minor things

Although your writing style is pretty clean, ShellCheck does pick up a few issues. It's a great site, I suggest copy-pasting your shell scripts in there to catch common mistakes and bad practices.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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