current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 3 down vote favorite
1

I'm working with a Rails app that requires a few layers of infrastructure to run: MySQL, Redis, Sidekiq, and (of course) the Rails server. I wrote some functions to help me manage the various servers (start/stop/check and one to kill the rails server when it gets hung up and decides to eat all my memory rather than shutting down).

I'm still new to Bash scripting, and looking for all the feedback/criticism I can get, so please, don't hold back. I'm particularly interested in how I can further DRY up this code.

Generic helpers to check if a named process is running and get its pid:

function running {
  pgrep -f "$1" &> /dev/null
}

function rpid {
  pgrep -f "$1" 2> /dev/null
}

MySQL server:

function msta {
  if ! running mysql; then
    mysql.server start
  else
    echo "MySQL already running"
  fi
}

function msto {
  if running mysql; then
    mysql.server stop
  else
    echo "MySQL not running"
  fi
}

function mstat {
  if running mysql; then
    echo "MySQL running"
  else
    echo "MySQL not running"
  fi
}

Redis server:

function resta {
  if ! running redis; then
    redis-server ~/.redis/redis.conf
    until running redis; do
      sleep 1
    done
    echo "Redis daemon started"
  else
    echo "Redis daemon already running"
  fi
}

function resto {
  if running redis; then
    kill $(rpid redis)
    while running redis; do
      sleep 1
    done
    echo "Redis daemon stopped"
  else
    echo "Redis daemon not running"
  fi
}

function restat {
  if running redis; then
    echo "Redis daemon running"
  else
    echo "Redis daemon not running"
  fi
}

Sidekiq:

function sksta {
  if ! running sidekiq; then
    sidekiq -d
    until running sidekiq; do
      sleep 1
    done
    echo "Sidekiq daemon started"
  else
    echo "Sidekiq daemon already running"
  fi
}

function sksto {
  if running sidekiq; then
    kill $(rpid sidekiq)
    while running sidekiq; do
      sleep 1
    done
    echo "Sidekiq daemon stopped"
  else
    echo "Sidekiq daemon not running"
  fi
}

function skstat {
  if running sidekiq; then
    echo "Sidekiq daemon running"
  else
    echo "Sidekiq daemon not running"
  fi
}

Rails server:

# kill the Rails server
function krs {
  if running 'rails s'; then
    kill $1 $(rpid 'rails s')
    for i in 1 2; do
      if ! running 'rails s'; then
        break
      else
        sleep 1
      fi
    done
    reportkill
  else
    echo "Rails server not running"
  fi
}

function reportkill {
  if ! running 'rails s'; then
    echo "Rails server killed"
  else
    echo "Rails server survived (pid $(rpid 'rails s'))"
  fi
}

# when a `kill -9` is required
function krs9 {
  krs '-9'
}

function rastat {
  if running 'rails s'; then
    echo "Rails server running"
  else
    echo "Rails server not running"
  fi
}

MySQL + Redis + Sidekiq

function asta {
  msta && resta && sksta
}

function asto {
  sksto && resto && msto
}

function astat {
  mstat && restat && skstat
}
share|improve this question

1 Answer 1

up vote 2 down vote accepted

Pretty nicely written! In fact, if you want to DRY this up, I'm pretty sure you could do it yourself if you seriously tried... But here you go anyway ;-)

Non-standard features

The function keyword is non-standard:

function rpid {
    pgrep -f "$1" 2> /dev/null
}

Write it this way instead:

rpid() {
    pgrep -f "$1" 2> /dev/null
}

DRY-ing up

You could write running in terms of rpid:

running() {
    rpid "$1" > /dev/null
}

rpid() {
    pgrep -f "$1" 2> /dev/null
}

This way, the pgrep logic is only in one place. If you need to change something, you could do it in one place. Just remember that the caller (running) expects the correct exit code of success / failure, and no output on stderr.

The start / stop / status commands have the common elements of checking if the process is running or not. You could create helper functions to reduce the duplication:

start() {
    procname=$1; shift
    name=$1; shift
    if ! running "$procname"; then
        "$@"
    else
        echo "$name already running"
    fi
}

stop() {
    procname=$1; shift
    name=$1; shift
    if running "$procname"; then
        "$@"
    else
        echo "$name not running"
    fi
}

status() {
    procname=$1
    name=$2
    if running "$procname"; then
        echo "$name running"
    else
        echo "$name not running"
    fi
}

Then you can simplify the MySQL functions using these helpers:

msta() {
    start mysql MySQL mysql.server start
}

msto() {
    stop mysql MySQL mysql.server stop
}

mstat() {
    status mysql MySQL
}

Similarly for redis:

redis_start() {
    redis-server ~/.redis/redis.conf
    until running redis; do
      sleep 1
    done
}

redis_stop() {
    kill $(rpid redis)
    while running redis; do
      sleep 1
    done
}

resta() {
    start redis "Redis daemon" redis_start
}

resto() {
    stop redis "Redis daemon" redis_stop
}

restat() {
    status redis "Redis daemon"
}

And I left the rest for you as exercise:

  • rastat can be simplified using status
  • krs can be simplified similar to the way I did redis above
  • You can leave reportkill as it is, as it's a different pattern from the others

There remains still some duplication in the start, stop, status helpers, but trying to reduce that might hurt readability. I think the current form is the "sweet spot", trying to optimize further will have diminishing returns.

shellcheck.net

This site is really awesome for checking your shell scripts for common mistakes:

http://www.shellcheck.net/#

share|improve this answer
1  
Wow, I learned a bunch from this. Thanks! I didn't know you could wait until the second link the chain to suppress stdout (rpid and running), but that makes sense. Learned more about handling positional parameters from your refactor as well. Kudos! –  ivan Aug 10 '14 at 16:21

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.