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.

Below is a script to create and manage text snippets. It is also posted on GitHub. Most of the code is error checking and displaying help because I wanted to make a solid command line interface. The blanks function is used when copying a snippet with a blank (@) character in it. The user fills in the blanks.

I am looking for a review of the script: Is it easy to use? Is the code confusing? Are there any horrible bugs that I left unnoticed?

It is tested in OS X, so I am not sure if the syntax will work elsewhere. Specifically, the pbcopy and pbpaste aliases are untested.

#!/bin/sh

list() {
    echo "snp snippets:\n"
    echo "$(ls -R $snippets_dir)"
}

move() {
    if [[ "$2" == "" && $3 == "" ]]; then
        echo "snp usage:\n"
        echo "snp move <name> <group>"
    elif [[ -e $snippets_dir/$2 ]]; then
        if -d $snippets_dir/$3 ]]; then
            mv $snippets_dir/$2 $snippets_dir/$3/$2
        else
            echo "ERROR: Group $snippets_dir/$3 does not exist."; exit 1
        fi
    else
        echo "ERROR: Snippet $2 does not exist."; exit 1
    fi
}

new() {
    if [[ "$2" == "" && "$3" == "" ]]; then
        echo "snp usage:\n"
        echo "snp new <name> \"<text>\""
        echo "snp new group <name>"
    elif [[ "$2" == "group" || "$2" == "g" ]]; then # New group
        if [[ "$3" != "" ]]; then
            mkdir $snippets_dir/$3
            echo "Created new group \"$3\"."
        else
            echo "snp usage:\n"
            echo "snp new group <name>"
        fi
    else # New snippet
        if [[ "$2" == "" ]]; then
            echo "snp usage:\n"
            echo "snp new <name> \"<text>\""
        else
            if [[ -e $snippets_dir/$2 ]]; then
                echo "Snippet $snippet_dir/$2 already exists."
                echo "Overwrite? [y/n]"
                read yn
                if [[ $yn == "y" || $yn == "Y" ]]; then
                    rm $snippets_dir/$2
                else
                    return
                fi
            fi
            printf "$3" >> $snippets_dir/$2
            echo "Created new snippet \"$2\"."
        fi
    fi
}

remove() {
    if [[ "$2" == "" ]]; then
        echo "snp usage:\n"
        echo "snp remove <name>"
        echo "snp remove group <name>"
    elif [[ "$2" == "group" || "$2" == "g" ]]; then # Remove group
        if [[ "$3" != "" ]]; then
            if [[ -e "$snippets_dir/$3" ]]; then
                rm -rf $snippets_dir/$3
                echo "Removed group \"$3\"."
            else
                echo "ERROR: Group $3 does not exist."; exit 1
            fi
        else
            echo "snp usage:\n"
            echo "snp remove group <name>"
        fi
    elif [[ "$2" != "" ]]; then # Remove snippet
        if [[ -e $snippets_dir/$2 ]]; then
            rm $snippets_dir/$2
            echo "Removed snippet \"$2\"."
        else
            echo "ERROR: Snippet $2 does not exist."; exit 1
        fi
    fi
}

blanks() {
    data=$(cat $1)
    count=0
    OIFS=$IFS
    IFS="@"
    blanx=$(echo "$data" | tr -d -c '@' | wc -c | awk '{print $1}')

    result=""

    for x in $data; do
        if [ $count -eq $blanx ]; then
            echo "$x"
            result="$result$x"
            break
        fi
        echo "$x"
        result="$result$x"
        read -p"snp> " v
        result="$result$v"
        count=`expr $count + 1`
    done
    IFS=$OIFS

    printf "$result" >> $1-tmp
}

copy() {
    if [[ "$1" == "" ]]; then
        usage
    elif [[ -e $snippets_dir/$1 ]]; then
        if [ ! $(uname -s) = "Darwin" ]; then
            alias pbcopy='xsel --clipboard --input'
        fi

        blanks $snippets_dir/$1
        cat $snippets_dir/$1-tmp | pbcopy
        echo "Snippet text copied to clipboard."

        rm $snippets_dir/$1-tmp
    else
        echo "ERROR: Snippet $1 does not exist."; exit 1
    fi
}

paste() {
    if [[ "$2" == "" ]]; then
        usage
    elif [[ -e $snippets_dir/$2 ]]; then
        if [ ! $(uname -s) = "Darwin" ]; then
            alias pbcopy='xsel --clipboard --input'
            alias pbpaste='xsel --clipboard --output'
        fi

        blanks $snippets_dir/$2
        cat $snippets_dir/$2-tmp | pbcopy
        pbpaste

        rm $snippets_dir/$2-tmp
    else
        echo "ERROR: Snippet $2 does not exist."; exit 1
    fi
}

usage() {
    echo "snp usage:\n"
    echo "snp <name>                # Copy snippet to clipboard"
    echo "snp paste <name>          # Paste name (from clipboard)"
    echo "snp new <name> \"<text>\"   # New snippet"
    echo "snp new group <name>      # New group"
    echo "snp list                  # List snippets"
    echo "snp move <name> <group>   # Move snippet to group"
    echo "snp remove <name>         # Remove snippet"
    echo "snp remove group <name>   # Remove group"
    echo "snp help                  # Display usage"
}

snippets_dir=~/".snp"

# Check for/create snippet dir
if [[ ! -d $snippets_dir ]]; then
    echo "ERROR: $snippets_dir doesn't exist."
    echo "Create it now? [y/n]"
    read yn
    if [[ $yn == "y" || $yn == "Y" ]]; then
        mkdir $snippets_dir
        echo "$snippets_dir created"
    else
        exit 0
    fi
fi

case $1 in
    p|paste)
        paste "$@" ;;
    n|new)
        new "$@" ;;
    l|list)
        list ;;
    m|move)
        move "$@" ;;
    r|remove)
        remove "$@" ;;
    h|help)
        usage ;;
    *)
        copy "$@" ;;
esac

exit 0
share|improve this question

2 Answers 2

up vote 6 down vote accepted

Overall

  • You're using some bash-specific things ([[ ... && ... ]]), so your shebang line should be #!/bin/bash
  • very good indentation, readable code
  • more quotes: mv "$snippets_dir/$2" "$snippets_dir/$3/$2"

list function

  • ls knows how to print to the screen, don't need echo $(ls ...)

move function

  • you need echo -e if you want \n to be interpreted as a newline.
  • syntax error on 2nd if, missing [[

new function

blanks function

  • declare local variables with local
  • to read content from a file in bash: data=$(< "$1")
  • bash can do arithmetic: (( count++ )) -- see http://www.gnu.org/software/bash/manual/bashref.html#Conditional-Constructs
  • you only ever use this function to copy stuff to the clipboard, so you don't need a temp file:

    blanks "$snippets_dir/$1" | pbcopy
    
  • don't use printf "$astring" -- if $astring contains %-directives you'll get "not enough arguments" errors. Stick to echo, or if you're specifically avoiding a trailing newline, printf "%s" "$astring"

paste function

  • I would use case here, if you decide you need OS-specific aliases:

    case $(uname -a) in
        Darwin) : ;;
        *) alias pbcopy='...'
           alias pbpaste='...'
           ;;
    esac
    
share|improve this answer
    
Great answer. Thank you very much! –  adamlazz Feb 19 at 16:54
1  
One more thing I just noticed: the code where you set the pbcopy alias should not be in the copy function, it should be in the "main" scope, or in a setup or config function. –  glenn jackman Feb 19 at 16:58

I am not a great fan of fall-through logic ... if there is an argument issue (missing argument, whatever), you are falling through the work functions, and letting the program end. I would make this more explicit with an exit 3. Programs should always set an exit code unless they successfully complete. I don't consider argument errors to be a successful completion.

aside - Bash has some reserved exit codes, which you should typically not use. 1 is one of the reserved exit codes. As a result, try to use values other than 1 or 2 for exit codes from shell scripts....

I would recommend creating an error-handling function... something along the lines of:

generalerror() {
    echo "snp usage:"
    echo $1
    echo
    usage
    exit 3
}

checkargument() {
    if [[ "$1" == "" ]]; then
        generalerror $2
    fi
}

This will allow you to centralize a lot of your error handling with some simpler messages/handling... the reason I am suggesting this is because of the following:

remove() {
    if [[ "$2" == "" ]]; then
        echo "snp usage:\n"
        echo "snp remove <name>"
        echo "snp remove group <name>"
    elif [[ "$2" == "group" || "$2" == "g" ]]; then # Remove group
        if [[ "$3" != "" ]]; then
            if [[ -e "$snippets_dir/$3" ]]; then
                rm -rf $snippets_dir/$3
                echo "Removed group \"$3\"."
            else
                echo "ERROR: Group $3 does not exist."; exit 1
            fi
        else
            echo "snp usage:\n"
            echo "snp remove group <name>"
        fi

That code does not exit 1.... probably because it was just an oversight... and this is why functions are good ;-)

The above code could be:

remove() {
    checkargument $2 "snp remove <name>\nsnp remove group <name>"
    if [[ "$2" == "group" || "$2" == "g" ]]; then # Remove group
        checkargument $3 "snp remove group <name>"
        ....

Right, then your error handling:

cat $snippets_dir/$1-tmp | pbcopy

The above line is critical to your program.... but, it does not check for errors.... Did the pbcopy succeed?

So:

  • functions to extract common logic are useful....
  • exit code set for bad values.
  • different exit code for functional problems ... (failed sub-commands)
  • error handling!
share|improve this answer
    
Thanks for your answer, this is a very interesting recommendation. I will definitely consider implementing it in the future. –  adamlazz Feb 19 at 16:56

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.