2
\$\begingroup\$

I have created an start, stop, restart script for a unix-mongodb-server.

It would be nice if you could look over my code and give me some helpful hints on what I could change. I tested the script under my Mac and it works.

This is also available under my GitHub account and available under GitHub -> Mongo_Start_Stop.

VERSION=1.1.2
SCRIPTNAME=$(basename "$0")
MONGOHOME=
MONGOBIN=$MONGOHOME/bin
MONGOD=$MONGOBIN/mongod
MONGODBPATH=
MONGODBCONFIG=


if [ $# != 1 ]
then
    echo "Usage: $SCRIPTNAME [start|stop|restart]"
    exit
fi

pid() {
    ps -ef | awk '/[m]ongodb/ {print $2}'
}

stopServer() {
    PID=$(pid)
    if [ ! -z "$PID" ]; 
    then
        echo "... stopping mongodb-server with pid: $PID"
    sudo kill $PID
    else
        echo "... mongodb-server is not running!"
    fi
}

startServer() {
    PID=$(pid)
    if [ ! -z "$PID" ];
    then
        echo "... mongodb-server already running with pid: $PID"
    else
        echo "... starting mongodb-server"
        sudo "$MONGOD" --dbpath "$MONGODBPATH" --config "$MONGODBCONFIG"
    fi
}

restartServer() {
    stopServer
    sleep 1s
    startServer    
}

case "$1" in

    start) startServer
           ;;

    stop) stopServer
          ;;

    restart) restartServer
         ;;

    *) echo "unknown command"
       exit
       ;;
esac
\$\endgroup\$

1 Answer 1

1
\$\begingroup\$

Bug

Can you spot the bug on this line?

    PID=$(ps -ef | grep "[m]ongodb" | awk {'print $2'})

The awk script here should be {print $2}, with the curly braces included, so that needs to be within the single-quotes:

    PID=$(ps -ef | grep "[m]ongodb" | awk '{print $2}')

Quoting path variables

It's good to make it a habit to always double-quote path variables in command statements, to protect from globbing and word splitting. Instead of:

SCRIPTNAME=$(basename $0)

Write like this:

SCRIPTNAME=$(basename "$0")

The same goes for this line:

    sudo $MONGOD --dbpath $MONGODBPATH --config $MONGODBCONFIG

Useless variable and echo

PID is not used anywhere else inside the script but here:

function isRunning {
    PID=$(ps -ef | grep "[m]ongodb" | awk '{print $2}')
    echo $PID    
}

You could simplify as:

pid() {
    ps -ef | awk '/[m]ongodb/ {print $2}'
}

I also renamed the function, because pid() reflects better its purpose.

User experience

Something's odd in this condition:

rc=$(isRunning)
echo "... stopping mongodb-server with pid: $rc"    
if [ ! -z "$rc" ]; then
sudo kill $rc
fi

If the process is not running $rc will be empty, and users may think the output is weird and the script is buggy. You might want to rearrange this part, for example if the server is not running, then say so.

Make the most of awk

Very often a grep ... | awk ... combo can be simplified, because awk can filter all by itself. Instead of:

PID=$(ps -ef | grep "[m]ongodb" | awk '{print $2}')

You can write:

PID=$(ps -ef | awk '/[m]ongodb/ {print $2}')

Style

This is fine:

function isRunning {
    # ...
}

But the recommended style for function declarations is like this:

isRunning() {
    # ...
}

Variable initialization

This is fine:

MONGOHOME=""
MONGOBIN="$MONGOHOME/bin"
MONGOD="$MONGOBIN/mongod"
MONGODBPATH=""
MONGODBCONFIG=""

But you could write simpler as:

MONGOHOME=
MONGOBIN=$MONGOHOME/bin
MONGOD=$MONGOBIN/mongod
MONGODBPATH=
MONGODBCONFIG=

Redundant semicolon

The semicolon at the end of exit; is redundant, I suggest to remove it:

if [ $# != 1 ]
then
    echo "Usage: $SCRIPTNAME [start|stop|restart]"
    exit;
fi
\$\endgroup\$
2
  • \$\begingroup\$ Thank you very much. Your comments are very helpfull and i will regard them in future. I also update your comments im my github repository. \$\endgroup\$
    – Patrick85
    Commented May 15, 2017 at 13:43
  • \$\begingroup\$ i edited my post with the new code from your review. Did you have a nicer solution with the if-statement? Did you know why i need to use an "slepp 1s" after "stopServer" and then "startServer" ? \$\endgroup\$
    – Patrick85
    Commented May 15, 2017 at 18:18

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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