1
\$\begingroup\$

link to original question: Simple media ingest script

As per answers I have refactored my code and would like to get feedback on the current structure.

The purpose of this tool is to allow someone to copy media from cards (SD cards, CFast, etc) using rsync and to also allow someone to complete incomplete transfer that they have initiated.

I've chosen to use prompts because it's more user friendly for the environment the script will be used in. The intended users are not command line tool friendly.

I'm not very familiar with bash and am quite out of practice with code in general so I would like to check behind myself. I received excellent feedback in the previous post and have addressed several of the points raised.

#!/usr/local/bin/bash
set -e
set -u

# prompt user for input
prompt_user(){
        while [[ -z "${!2:-}" ]]
            do
            read -r -p "$1" "$2"
        done
}

# prompt user for additional rsync options
add_rsync_options(){
    while true;
    do
        read -r -p "Add rsync options? [y/n] " add_options
        case $add_options in
            [Yy]* )
                read -r -p $'Enter additional rsync options:\n' rsync_options
                break;;
            [Nn]* )
                break;;
            *) echo "Please enter y or no!"
        esac
 done
}

# make target directory for transfer
make_directory(){
    echo  $'Follow the prompt to create a project directory.\n'

    prompt_user $'Path of target directory?\n' target_directory
    prompt_user $'Brand Prefix?\n' brand_prefix
    prompt_user $'Project Name?\n' project_name
    prompt_user $'Media Type?\n' media_type
    prompt_user $'Location?\n' location
    prompt_user $'Employee?\n' employee

    destination_path=${target_directory}/$(date +'%Y%m%d')_${brand_prefix}_${project_name}_${media_type}_${location}_${employee}

    echo "Creating directory: ${destination_path}"

    mkdir -p "${destination_path}" "${destination_path}/logs"
}

# run rsync command
run_rsync(){
    echo $'Follow the prompt to complete the rsync command.\n'

    prompt_user $'Path to source media?\n' source_path

# if partial ingest indicate pre-existing target directory
    if [[ "$option" == "2" ]]; then
        prompt_user $'Target directory?\n' target_directory
        destination_path=$target_directory
    fi

    add_rsync_options

    echo $'Running rsync command:\n'

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

}

# clear terminal for readability
clear

# start
while true
do
# read user input and run appropriate functions
        read -r -p $'Enter [1] to start an ingest or [2] to complete a partial ingest.\n' option
        case $option in
            1 )
                make_directory
                run_rsync
                break;;
          2 )
                run_rsync
                break;;
          * )
                echo $'Please enter a valid option!\n';;
        esac
done

Error handling is at a minimum still I believe and I'm not too sure what would even be relevant to handle past what's implemented. I'm not very experienced in that regard so resources are appreciated!

Thanks!

\$\endgroup\$
2
\$\begingroup\$

What is $option in run_rsync?

option is read in the global scope to decide if user wants a full ingest or continue a partial ingest. In that loop that reads the prompt, the meaning of option is understandable.

Referring to $option in run_rsync is just too far away. It's not clear anymore where it comes from and what it means. It would be better if the prompt loop passed the decision to run_rsync as a parameter, to make it perfectly clear. For example:

while true
do
    read -r -p $'Enter [1] to start an ingest or [2] to complete a partial ingest.\n' option
    case $option in
        1 )
            make_directory
            run_rsync full
            break;;
        2 )
            run_rsync partial
            break;;
        * )
            echo $'Please enter a valid option!\n';;
    esac
done

And then in run_rsync:

run_rsync() {
    local ingestType=$1

    echo $'Follow the prompt to complete the rsync command.\n'

    prompt_user $'Path to source media?\n' source_path

    if [[ "$ingestType" = "partial" ]]; then
        prompt_user $'Target directory?\n' target_directory
        destination_path=$target_directory
    fi

    # ...

Inconsistent terminology

In make_directory, the term "target directory" refers to the base directory in which a timestamped sub-directory will be created, with brand name, project name, and so on, also in the name.

In run_rsync, in case of partial ingest the user is prompted for "target directory", but here it will mean the full destination path. If would be better to name it as such.

Inconsistent formatting

The code uses inconsistent indentation. For example the content of prompt_user is over-indented, compared to other functions. The body of the while loop in the global scope is over-indented, and also haphazardly indented. It would be easier to read if indenting was consistent throughout.

Unnecessary comments

Most comments in the program state the obvious. They are just noise, and it would be better to remove them.

\$\endgroup\$
  • \$\begingroup\$ Excellent points! Thanks for the detailed response. Passing the ingest type to the function makes a lot of sense and did not occur to me at all. \$\endgroup\$ – usulmuaddib Mar 28 at 13:20

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.