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 have a shell script to mount different remote systems. While it does work, the area I struggle with (not being a programmer) is the logic of the script: it feels quite stilted and there seems to be some unnecessary duplication.

I would appreciate any comments as to how I could improve it—both in terms of the logic, but also any other tips about style or approach.

#!/bin/sh
# mount boxes by SSHFS

usage () {
    cat <<EOF
    sshmnt -[c,s,u,h]
      -c Box1
      -s Box2
      -u unmount
      -h print this message

EOF
}

mnt(){
    sshfs jason@"$host":/home/jason /media/"$dir" \
    -C -p "$port" \
    -o reconnect,IdentityFile=/home/jason/.ssh/id_rsa \
    && ls /media/"$dir"
}

umnt(){
    fusermount -u /media/"$dir"
}

# unmounting 
if [ "$1" = "-u" ]; then
    box=$(mount | grep Box1)
    if [ -n "$box" ]; then
        dir=Box1
    else
        dir=Box2
    fi
    umnt && exit
fi

# check if on LAN
lan="$(ip addr | grep .102/)"

if [ -n "$lan" ]; then 
    case "$1" in
      -c) int=100
          ;;
      -s) int=200 
          ;;
    esac
    host="192.168.1.$int"
else
    host="XXX.XXX.XXX.XXX"
fi

# box specifics
case "$1" in
  -c) dir=Box1 port="XXXX"
      mnt
      ;;
  -s) dir=Box2  port="XXXX"
      mnt
      ;;
   *) usage && exit
       ;;
esac

I have obscured the relevant ports and hostnames.

share|improve this question
add comment

1 Answer

up vote 6 down vote accepted

Your functions shouldn't depend on variables but use specified parameters. You always specified variables before calling your function instead of just specifying them as parameters, e.g:

DIR=Box1
Host=192.168.1.100
mnt

but you can easily write it as

mnt Box1 192.168.1.100

I would also only parse your command line arguments in one place and create a dedicated function for determine the appropriate ip address for your hosts.

I would also remove the ls call from your mnt function as mnt doesn't do what the name suggests. But this is a rather minor issue and I didn't change it in my code.

#!/bin/sh
# mount boxes by SSHFS

usage () {
    cat <<EOF
sshmnt -[c,s,u,h]
      -c Box1
      -s Box2
      -u unmount
      -h print this message

EOF
}

mnt(){

    if [ $# -ne 3 ] ; then
       echo "Wrong parameters for mnt - host dir port" >&2
       return 1
    fi

    host="$1"
    dir="$2"
    port="$3"
    sshfs jason@"$host":/home/jason /media/"$dir" \
    -C -p "$port" \
    -o reconnect,IdentityFile=/home/jason/.ssh/id_rsa \
    && ls /media/"$dir"
}

umnt(){
    dir="$1"
    fusermount -u /media/"$dir"
}

get_host(){
    # check if on LAN
    lan="$(ip addr | grep .102/)"

    if [ -z "$lan" ] ; then
        echo XXX.XXX.XXX.XXX
        return 0
    fi

    case "$1" in
        Box1) echo 192.168.1.100 ;;
        Box2) echo 192.168.1.200 ;;
        *) echo "Unknown Parameter" >&2 && return 1 ;;
    esac
}

case "$1" in
    -c) mnt $(get_host Box1) Box1 port ;;
    -s) mnt $(get_host Box2) Box2 port ;;
    -u)
        mount | grep Box1 && umnt Box1
        mount | grep Box2 && umnt Box2
        ;;
     *) usage && exit 1;;
esac

Another suggestion is to get rid of all your hard coded paths and texts like, /media/, /home/jason/ and jason and replace them with variables, e.g:

MOUNT_PREFIX=/media/
SSH_KEY=/home/jason/.ssh/id_rsa
MOUNT_SOURCE=/home/jason

and just specify the user for your hosts in ~/.ssh/config

share|improve this answer
    
Thanks Ulrich: that is very helpful. Would you mind expanding on your comment that "functions shouldn't depend on environment variables but use specified parameters"? I don't quite follow. –  jasonwryan Aug 8 '12 at 0:12
    
@jasonwryan ok updated the answer - also have a look at the last paragraph about your hard coded values –  Ulrich Dangel Aug 8 '12 at 0:20
    
thank you for the added detail; it was a lightbulb moment. The extra variables are also a good suggestion. –  jasonwryan Aug 8 '12 at 0:37
add comment

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.