4
\$\begingroup\$

Trying to script a simple command line tool for ingesting media to a target location following a structured naming convention.

It's functional in its current state but I'd like to know if there are things to improve.

#!/usr/local/bin/bash

#make target directory for transfer

make_directory(){

    echo -e "\n\nFollow the prompt to create a project directory.\n\n"
    sleep .5

    while [[ -z "$target_directory" ]]
        do
                echo -e "Path of target directory?"
                read target_directory
        done

    while [[ -z "$brand" ]]
        do
            echo -e "\nBrand Prefix?"
            read brand
        done

    while [[ -z "$project" ]]
        do
            echo -e "\nProject Name?"
            read project
        done

    while [[ -z "$media_type" ]]
        do
            echo -e "\nMedia Type?"
            read media_type
        done

    while [[ -z "$location" ]]
        do
            echo -e "\nLocation?"
            read location
        done

    while [[ -z "$employee" ]]
        do
            echo -e "\nEmployee?"
            read employee
        done

    path=${target_directory}/$(date +'%Y%m%d')_${brand}_${project}_${media_type}_${location}_${employee}

    echo -e "\nCreating directory: ${path}\n\n"

    mkdir -p "${path}"

}

# construct rsync command

construct_rsync(){

    echo -e "\n\nFollow the prompt to construct the rsync command.\n\n"
    while [[ -z "$source_path" ]]
    do
        echo -e "Path to source media?"
        read source_path
    done

    if [[ "$option" == "2" ]]; then
        while [[ -z "$target_directory" ]]
        do
            echo -e "Target directory?"
            read target_directory
        done
    path=$target_directory
    fi

    while true;
    do
        read -p "Additional rsync options? [y/n] " rsync_add
        case $rsync_add in
            [Yy]* )
                echo -e "\nEnter additional rsync parameters:"
                read rsync_options
                break;;
            [Nn]* )
                break;;
            *) echo "Please enter y or no!"
        esac
 done

    echo -e "\nConstructing rsync command...\n"
    sleep .5
    echo -e "Running rsync command:\n
    rsync \n
    -r \n
    --info=progress2 \n
    --log-file=${path}/$(date +'%Y%m%d')_transfer_log.txt \n
    ${rsync_options} \n
    ${source_path}/ \n
    ${path} \n"

    rsync -r --info=progress2 --log-file="${path}/$(date +'%Y%m%d')_transfer_log.txt" ${rsync_options} "${source_path}/" "${path}"
}

# log exit code of rsync

log(){

    echo -e "\nCreating error log..."
    echo $? > "${path}/error_log.txt"

    sleep .5

    if [[ "$?" == "0" ]]; then
        echo -e "\nTransfer complete!"
    elif [[ "$?" != "0" ]]; then
        echo -e "\nError in transfer! Please refer to error_log.txt!"
    fi
}

# read user input and run appropriate functions

while true
do
read -p "Enter [1] to start an ingest or [2] to complete a partial ingest. " option

    case $option in

        1 )
            make_directory
            sleep .5
            construct_rsync
            sleep .5
            log
            break;;
      2 )
            construct_rsync
            sleep .5
            log
            break;;
      * )
            echo "Please enter a valid option!";;
    esac
done
\$\endgroup\$
  • \$\begingroup\$ Welcome to Code Review! I rolled back your last 2 edits. After getting an answer you are not allowed to change your code anymore. This is to ensure that answers do not get invalidated and have to hit a moving target. If you have changed your code you can either post it as an answer (if it would constitute a code review) or ask a new question with your changed code (linking back to this one as reference). Refer to this post for more information \$\endgroup\$ – Sᴀᴍ Onᴇᴌᴀ Mar 26 at 14:59
  • 1
    \$\begingroup\$ Thanks! I wasn't familiar with proper etiquette. \$\endgroup\$ – usulmuaddib Mar 26 at 15:09
4
\$\begingroup\$

Your code is pretty clear and readable. Congratulations!

Here are some suggestions:

  1. Stop using sleep! Unless this is for a Hollywood hacker movie, it just slows things down. Nobody thinks delays are cool after the first three times you run something.

  2. Take command-line arguments instead of prompting for everything. It's a lot easier to just put stuff into a command line than it is to respond to it at the keyboard. Something like:

    ingest -t $HOME/media -b SONY -p "My Project" 
    

    There are plenty of SO answers on how to do this.

  3. Add some more functions! Anything you find yourself doing twice should be a function. Also, anything that you have to "break the flow" in order to do should be a function. Here are some examples:

    while [[ -z "$target_directory" ]]
        do
            echo -e "Path of target directory?"
            read target_directory
        done
    
    while [[ -z "$brand" ]]
        do
            echo -e "\nBrand Prefix?"
            read brand
        done
    

    That's twice! Write a function to do this:

    target_dir=$(prompt_for_variable target_dir 'Path of target directory?')"
    brand="$(prompt_for_variable brand 'Brand Prefix?')"
    

    Now consider this:

    while true;
    do
        read -p "Additional rsync options? [y/n] " rsync_add
        case $rsync_add in
            [Yy]* )
    

    Right in the middle of "construct_rsync" you stop and loop forever prompting the user for a y/n answer. Write a function for that!

    if get_yn 'Additional rsync options?'
    then
         read -p "Enter the additional options: " rsync_options
    fi
    
  4. Don't lie to anybody, especially yourself. You have a function called construct_rsync but what does it do? It runs the command. Either change the name, or change the function.

  5. Beware of $?. In your log function you refer to it several times. But $? is updated each time a command is run. So if you're going to make decisions based on the value, you should either capture the value into a separate variable (status=$? ... if $status) or make a single decision right up front (if $? ... else ... fi)

\$\endgroup\$
  • \$\begingroup\$ Thanks for the detailed feedback! I don't have much experience with bash or coding in general so it's very helpful to see some better approaches. I do not understand how to implement a user prompt function. I believe i'm misunderstanding bash syntax, how arguments are passed, or missing a piece of the problem. If I write a generalized while loop inside a function it does not run properly when using $1 and $2 to populate. prompt_user(){ while [[ -z $input ]] do echo "$2" read "$1" } \$\endgroup\$ – usulmuaddib Mar 26 at 14:10
  • \$\begingroup\$ There's potential problems when mixing while loops with user input, because in some circumstances (depending on bash version, also) the body of the while loop will execute as a sub-shell (separate process, with its own file handles and stuff). Be sure and check Stack Overflow if you're having trouble. \$\endgroup\$ – Austin Hastings Mar 26 at 20:57
  • \$\begingroup\$ I managed to get my original functionality working and it was a combination of misunderstanding the way variables were passed and some syntactical errors causing it to not work on my end. Appreciate the input! \$\endgroup\$ – usulmuaddib Mar 26 at 21:18
2
\$\begingroup\$

Consider setting -e and -u to make the script abort on some common failures, rather than wildly continuing. The existing script has almost no error checking; as a simple example, closing standard input will lead to an infinite loop repeatedly executing read.

Instead of using non-standard echo -e to prompt, prefer to supply the prompt as argument to read:

        read -p "Path of target directory? " target_directory

Instead of merely checking that the directory path is a non-empty string with [[ -z ]], we should probably be checking that it's a real directory, with [ -d ].

Output lines should end with newline, and should generally not begin with newline. And error messages should go to the standard error stream (>&2) rather than to standard output.

There are some quotes needed when expanding pathname variables - at present, any filenames including whitespace will be seen as two or more arguments.

Testing $? is an antipattern. This block:

sleep .5

if [[ "$?" == "0" ]]; then
    echo -e "\nTransfer complete!"
elif [[ "$?" != "0" ]]; then
    echo -e "\nError in transfer! Please refer to error_log.txt!"
fi

can be written much more simply as

if sleep .5
then
    echo "Transfer complete!"
else
    echo "Error in transfer! Please refer to error_log.txt!"
fi

(though I suspect you actually meant to test the exit code of a different command to the sleep).

Finally, run Shellcheck on the code. I get far too many warnings (some of which I've already identified above):

shellcheck -f gcc  216201.sh
216201.sh:7:14: note: Backslash is literal in "\n". Prefer explicit escaping: "\\n". [SC1117]
216201.sh:7:16: note: Backslash is literal in "\n". Prefer explicit escaping: "\\n". [SC1117]
216201.sh:7:66: note: Backslash is literal in "\n". Prefer explicit escaping: "\\n". [SC1117]
216201.sh:7:68: note: Backslash is literal in "\n". Prefer explicit escaping: "\\n". [SC1117]
216201.sh:13:17: note: read without -r will mangle backslashes. [SC2162]
216201.sh:18:22: note: Backslash is literal in "\n". Prefer explicit escaping: "\\n". [SC1117]
216201.sh:19:13: note: read without -r will mangle backslashes. [SC2162]
216201.sh:24:22: note: Backslash is literal in "\n". Prefer explicit escaping: "\\n". [SC1117]
216201.sh:25:13: note: read without -r will mangle backslashes. [SC2162]
216201.sh:30:22: note: Backslash is literal in "\n". Prefer explicit escaping: "\\n". [SC1117]
216201.sh:31:13: note: read without -r will mangle backslashes. [SC2162]
216201.sh:36:22: note: Backslash is literal in "\n". Prefer explicit escaping: "\\n". [SC1117]
216201.sh:37:13: note: read without -r will mangle backslashes. [SC2162]
216201.sh:42:22: note: Backslash is literal in "\n". Prefer explicit escaping: "\\n". [SC1117]
216201.sh:43:13: note: read without -r will mangle backslashes. [SC2162]
216201.sh:48:14: note: Backslash is literal in "\n". Prefer explicit escaping: "\\n". [SC1117]
216201.sh:48:43: note: Backslash is literal in "\n". Prefer explicit escaping: "\\n". [SC1117]
216201.sh:48:45: note: Backslash is literal in "\n". Prefer explicit escaping: "\\n". [SC1117]
216201.sh:58:14: note: Backslash is literal in "\n". Prefer explicit escaping: "\\n". [SC1117]
216201.sh:58:16: note: Backslash is literal in "\n". Prefer explicit escaping: "\\n". [SC1117]
216201.sh:58:67: note: Backslash is literal in "\n". Prefer explicit escaping: "\\n". [SC1117]
216201.sh:58:69: note: Backslash is literal in "\n". Prefer explicit escaping: "\\n". [SC1117]
216201.sh:62:9: note: read without -r will mangle backslashes. [SC2162]
216201.sh:69:13: note: read without -r will mangle backslashes. [SC2162]
216201.sh:76:9: note: read without -r will mangle backslashes. [SC2162]
216201.sh:79:26: note: Backslash is literal in "\n". Prefer explicit escaping: "\\n". [SC1117]
216201.sh:80:17: note: read without -r will mangle backslashes. [SC2162]
216201.sh:88:14: note: Backslash is literal in "\n". Prefer explicit escaping: "\\n". [SC1117]
216201.sh:88:45: note: Backslash is literal in "\n". Prefer explicit escaping: "\\n". [SC1117]
216201.sh:90:36: note: Backslash is literal in "\n". Prefer explicit escaping: "\\n". [SC1117]
216201.sh:91:11: note: Backslash is literal in "\n". Prefer explicit escaping: "\\n". [SC1117]
216201.sh:92:8: note: Backslash is literal in "\n". Prefer explicit escaping: "\\n". [SC1117]
216201.sh:93:22: note: Backslash is literal in "\n". Prefer explicit escaping: "\\n". [SC1117]
216201.sh:94:59: note: Backslash is literal in "\n". Prefer explicit escaping: "\\n". [SC1117]
216201.sh:95:22: note: Backslash is literal in "\n". Prefer explicit escaping: "\\n". [SC1117]
216201.sh:96:21: note: Backslash is literal in "\n". Prefer explicit escaping: "\\n". [SC1117]
216201.sh:97:13: note: Backslash is literal in "\n". Prefer explicit escaping: "\\n". [SC1117]
216201.sh:99:87: note: Double quote to prevent globbing and word splitting. [SC2086]
216201.sh:106:14: note: Backslash is literal in "\n". Prefer explicit escaping: "\\n". [SC1117]
216201.sh:111:11: note: Check exit code directly with e.g. 'if mycmd;', not indirectly with $?. [SC2181]
216201.sh:112:18: note: Backslash is literal in "\n". Prefer explicit escaping: "\\n". [SC1117]
216201.sh:113:13: note: Check exit code directly with e.g. 'if mycmd;', not indirectly with $?. [SC2181]
216201.sh:114:18: note: Backslash is literal in "\n". Prefer explicit escaping: "\\n". [SC1117]
216201.sh:122:1: note: read without -r will mangle backslashes. [SC2162]
\$\endgroup\$
  • \$\begingroup\$ Excellent feedback! I've restructured a bit of my code per the two answers i've gotten. As I understand it, I've set up the mkdir such that it will create the parent directories if they do not exist. As to the others, I need flexibility in the naming to accommodate any string I just can't have empty strings. How do you suggest I check for empty string and re-prompt the user if the input is empty? I tried to implement a while loop in a function and my inexperience with bash is making progress slow. \$\endgroup\$ – usulmuaddib Mar 26 at 14:56
  • \$\begingroup\$ I may have misunderstood from reading the code whether the target directory had to pre-exist; sorry if I got that wrong. The simple loop seems reasonable to me: until [ "${dir-}" ]; do read -p "Enter directory: " dir; done (the ${dir-} expansion allows $dir to be unset without error despite set -u). \$\endgroup\$ – Toby Speight Mar 26 at 15:25

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.