1
\$\begingroup\$

A few days ago I posted a previous version of my first bash script here to get few tips and reviews so I could improve it. So now it is improved a bit. I also shellchecked it and fixed the warnings. I would love to hear more tips from you guys.

Here is the code:

#!/usr/bin/env bash

declare -r CONF_DIR_PATH="$HOME/.config/rdoc"
declare -r TMP_FILE="/tmp/rdoc_tmp.$$" 

#Trap the following signals and do cleanup before exiting 
trap 'rm -f "$TMP_FILE" 2> /dev/null && exit 0' EXIT 
trap 'rm -f "$TMP_FILE" 2> /dev/null && echo && exit 1' SIGINT

fn_generate_configs() {
    local doc_dir_path
    local pdf_viewer_name

    mkdir -p "$CONF_DIR_PATH"
    
    printf "Please enter your documents directory full path: "
    read -r doc_dir_path
    echo "$doc_dir_path" > "$CONF_DIR_PATH/doc_dir"

    printf "\nPlease enter your pdf's viewer name: "
    read -r pdf_viewer_name
    echo "$pdf_viewer_name" > "$CONF_DIR_PATH/pdf_viewer"

    printf "\nYour configurations were generated succesfully.\n"
}

fn_read_configs() {
    if ! doc_dir=$(cat "$CONF_DIR_PATH/doc_dir" 2> /dev/null); then
        echo Error: one or all of your configuration files are missing.
        echo Try -h for help.
        exit 1
    fi

    if ! pdf_viewer=$(cat "$CONF_DIR_PATH/pdf_viewer" 2> /dev/null); then
        echo Error: one or all of your configuration files are missing.
        echo Try -h for help.
        exit 1
    fi
}

fn_search_for_book() {
    local path
    local grep_opt="-q"
    local string_to_exclude="$1/"
    
    if [ "$i_status" -eq 1 ]; then 
        grep_opt="-qi"
    fi

    if [ "$r_status" -eq 1 ]; then #Search recursively  
        for path in "$1"/*; do  
            if [ -d "$path" ]; then 
                fn_search_for_book "$path"
            elif [ -f "$path" ]; then 
                #Redirect grep help message if book_name has the program's options value.
                #This would happen if the user called the program with the defualt behaviour,
                #plus with one of the available options but he/she omitted to pass the doc_name
                #so book_name will get the value of the passed options wich will trhow the grep help message.
                if echo "$path" | grep $grep_opt "$book_name" 2> /dev/null; then 
                    echo "${path//"$string_to_exclude"/}" >> "$TMP_FILE"
                fi
            fi
        done
    else
        for path in "$1"/*; do
            if [ -f "$path" ]; then 
                #Redirect grep help message for the same reasons as above
                if echo "$path" | grep $grep_opt "$book_name" 2> /dev/null; then 
                    echo "${path//"$string_to_exclude"/}" >> "$TMP_FILE"
                fi
            fi
        done

    fi
}

fn_display_books() {
    local doc
    local founded_docs
    
    #Make sure a book was founded and TMP_FILE was generated
    if ! founded_docs=$(cat "$TMP_FILE" 2> /dev/null); then     
        printf "Error: no document was found with \'%s\' in it.\n" "$book_name"
        exit 1
    fi

    printf "These are the documents that were found:\n\n"
    #Set output's color to red
    tput setaf 1
    
    for doc in $founded_docs; do    
        echo "$doc"
    done

    #Reset output's color 
    tput sgr0 
}

fn_count_books() {
    local doc
    local founded_docs
    local cnt=0

    if ! founded_docs=$(cat "$TMP_FILE" 2> /dev/null); then
        printf "\nError: \'%s\' manipulation while the program is running are disallowed.\n" "$TMP_FILE"
        exit 1
    fi
    
    for doc in $founded_docs; do 
        (( cnt++ ))
    done

    return "$cnt"
}

fn_final_book_name() {
    printf "\nWhich one of them would you like to open: "
    read -r book_name
}

fn_generate_books_paths() {
    local path
    
    if [ "$r_status" -eq 1 ]; then
        for path in "$1"/*; do
            if [ -d "$path" ]; then
                fn_generate_books_paths "$path"
            elif [ -f "$path" ]; then
                 echo "$path" >> "$TMP_FILE"
            fi
        done
    else 
        for path in "$1"/*; do
            if [ -f "$path" ]; then
                echo "$path" >> "$TMP_FILE"
            fi
        done
    fi
}

fn_get_book_path() {
    local founded_paths
    local path
    local grep_opt="-q"

    if ! founded_paths=$(cat "$TMP_FILE" 2> /dev/null); then
        printf "\nError: \'%s\' manipulation while the program is running are disallowed.\n" "$TMP_FILE"
        exit 1
    fi

    if [ "$i_status" -eq 1 ]; then
        grep_opt="-qi"
    fi

    for path in $founded_paths; do 
        if ! echo "$path" | grep $grep_opt "$book_name"; then 
            continue
        fi

        book_path="$path"
        break
    done
}

fn_open_book() {

    if ! "$pdf_viewer" "$book_path" 2> /dev/null; then 
        printf "\nError: %s can\'t be opened.\n" "$book_path" 
        exit 1
    fi

    printf "\nOpening: %s\n" "$book_path"
}

fn_help_message() {
printf "Usage: rdoc <options> [argument]
 
Available options:                                                                             
 -h               Display this help message.                                                   
 -g               Generate new configuration files.                                            
 -r               Allow recursive searching for the document.                                  
 -i               Ignore case distinctions while searching for the document.                   
 -s               Search for the document and display results.                                   
                  This option takes a document name or a part of it as an argument.            
 -o               Search for the document, display results then open it using your pdf viewer. 
                  This option takes a document name or a part of it as an argument.            
                  (Default)                                                                    
NOTE:                                                                                          
     When using '-s' or '-o' option in a combination of other options like this:               
                                                                                               
     $ rdoc -ris document_name                                                                 
                                                                                               
     Please make sure that it's the last option; to avoid unexpected behaviour.
"            
}


doc_dir=""
pdf_viewer=""
book_path=""
book_name=${!#} #book_name equals to the last arg by defualt so the default option ('-o') will work.

#Options status
r_status=0
i_status=0
s_status=0
o_status=1 #Make -o the default option 

#Display help message if no options were passed 
if [ $# -eq 0 ]; then
    fn_help_message
    exit 0
fi

while getopts ":hgris:o:" opt; do
    case $opt in 
        h)
            fn_help_message
            exit 0
            ;;
        g)
            fn_generate_configs
            o_status=0
            ;;
        r)
            r_status=1
            ;;
        i)
            i_status=1
            ;;
        s)
            book_name="$OPTARG"
            s_status=1
            o_status=0 
            ;;
        o)
            book_name="$OPTARG"
            ;;
        :)
            printf "Error: an argument is required for \'-%s\' option.\n" "$OPTARG"
            echo Try -h for help.
            exit 1
            ;;
        *)
            printf "Error: unknown option \'-%s\'.\n" "$OPTARG"
            echo Try -h for help.
            exit 1
            ;;
    esac
done

if [ "$s_status" -eq 1 ]; then
    fn_read_configs
    fn_search_for_book "$doc_dir"
    fn_display_books
elif [ "$o_status" -eq 1 ]; then 
    fn_read_configs
    fn_search_for_book "$doc_dir"
    fn_display_books
    fn_count_books
    if [ $? -gt 1 ]; then #If more than 1 book were found with $book_name in it
        fn_final_book_name
        #Clean any leftovers of $TMP_FILE to search properly
        rm "$TMP_FILE" 2> /dev/null
        #Make sure that the user chose an available document
        fn_search_for_book "$doc_dir"
        if [ ! -f "$TMP_FILE" ]; then 
            printf "\nError: no document was found with \'%s\' in it.\n" "$book_name"
            exit 1
        fi

        #Make sure that the user is specific enough about the book name
        fn_count_books
        if [ $? -gt 1 ]; then
            printf "\nError: More than 1 book was found with the name \'%s\' in it.\n" "$book_name"
            exit 1
        fi
    fi
    
    : > "$TMP_FILE" #Make sure $TMP_FILE is empty so it'll be usable in fn_generate_books_paths
    fn_generate_books_paths "$doc_dir"
    fn_get_book_path
    fn_open_book
fi

exit 0

Here is a link to the repo on github for the ones who would prefer to review it there: https://github.com/yankh764/rdoc

Thanks guys.

\$\endgroup\$
1
\$\begingroup\$

Good

  • nice indentation
  • good docs for the options
  • good variable names
  • a reasonable amount of inline comments
  • yay for being on top of the shellcheck already

Suggestions

  • For conditionals it is safer to use double square brackets. Some folks on here discourage the double square brackets for portability, but bash/zsh/ksh all support the more modern syntax.
  • Put your config into the format of a script and then source it.
  • Put your usage function near the top and include a line that says what the gist of the command is.

Nits

These are minor issues that are more matters of taste.

  • I wouldn't proceed every function with the same fn_, but I can see that might make it easier to keep a script of this size straight in your head.
  • Putting a space after the hash in a comment will make it easier to read.
\$\endgroup\$
1
  • \$\begingroup\$ Thank you for such a great review. I was surprised that you also included the good things alongside to the tips and suggestions which is really nice and encouraging, so I appreciate it. Lastly I will of course learn from your tips and modify my script to improve it :) \$\endgroup\$ – yan_kh May 10 at 19:56

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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