Bash script - automate an upload image + description process on a server

Question

I was tasked at work to improve a system where through a web interface a client, which eventually became us, uploads a bunch of images with descriptions (latter coming from csv files), the images are then automatically resized yada yada for the site.

Nowt wrong with the latter, but the upload and image description data entry is unbelievably tedious - the latter having to be filled out by hand and the uploads done through http.

The usual way to do this would be for me to create a more sensible interface. However, since it's us that end up doing it and me being inexperienced at *nix sysadmin type stuff and wanting to gain some experience, I thought it would be more interesting to write a shell script to do it accessed by SSH, on the basis we can upload the CSVs and image files easily enough. Have also used sed, perl, and a line of php to help me encode data as it comes from the CSV.

With the exception of removing my company name from the logo, here is the unaltered code:

How could this shell script be improved?

#! /bin/bash

# Yes, the below is ludicrous overkill. 

# B
# U
# T

# I need to become good enough at shell scripting that I don't have to look up 
# elementary syntax before I write anything that is not just a collection of 
# commands. 

# It was fun :)

function quitTheBestShellScriptInTheWorld {
    tput setaf 0
    tput sgr0
    if [[ $tempFile != "" ]]; then
        rm $tempFile
    fi
    exit
}

function randomColour {
    tput setaf $[ RANDOM % 8]
    if [[ $1 == "e" ]]; then
        echo
    fi
}

function everyLineADifferentColour {
    while read eachLine
    do
        randomColour
        echo $eachLine
    done

}

trap quitTheBestShellScriptInTheWorld 0 1 2 3 15 #let's be tidy

clear

tput bold
randomColour

if [[ $1 == "-l" ]]; then
    echo "Er, logging feature not actually programmed. Add it yourself? Will be reasonably straightforward [y/n]"
    read -n 1 dolog

    if [[ $dolog == "y" ]]; then
    (nano coolscript.sh)
    else
    echo 'Wise choice, if you change your mind then run again and answer yes, or just edit this script (which is all that pressing y would have done)'
    fi
    exit 0
fi


echo '
      .___                                      
      |   | _____ _____     ____   ____         
      |   |/     \\__  \   / ___\_/ __ \        
      |   |  Y Y  \/ __ \_/ /_/  >  ___/        
      |___|__|_|  (____  /\___  / \___  >       
                \/     \//_____/      \/        
__________              .__                     
\______   \ ____   _____|__|_______ ___________ 
 |       _// __ \ /  ___/  \___   // __ \_  __ \ 
 |    |   \  ___/ \___ \|  |/    /\  ___/|  | \/
 |____|_  /\___  >____  >__/_____ \\___  >__|   
        \/     \/     \/         \/    \/       

'
randomColour
echo Enter settings, or press enter for defaults:
randomColour e
echo Loading directory paths from Perl
success=failure

cd ../www/properties/lib/

uploadDataStore=$(perl -e 'use WIMSConfig; $z = WIMSConfig->new(); print $z->{uploadDataStore}')

if [[ $? == 0 ]] && [[ $uploadDataStore != "" ]]; then
currentDataStore=$(perl -e 'use WIMSConfig; $z = WIMSConfig->new(); print $z->{currentDataStore}')
    if [[ $? == 0 ]] && [[ $currentDataStore != "" ]]; then
        success=success
    fi
fi

if [[ $success != "success" ]]; then
    randomColour
    echo "Failure: Problem setting directory paths from perl code"
    exit 100
fi


tput setaf 4
randomColour e
echo fyi, I have uploadDataStore of $uploadDataStore and currentDataStore of $currentDataStore
echo
success=no
while [[ $success == "no" ]]; do
randomColour
echo 'Which directory from within uploadDataStore to get files from? (default = subfolder)'
randomColour
read theDirectory
if [[ $theDirectory == "" ]]; then
    theDirectory=subfolder
fi

if [[ -e $uploadDataStore$theDirectory ]]; then
    success=yes
else
    randomColour
    echo "Can't find a folder called $uploadDataStore$theDirectory"
    echo
fi
done

declare -i NameCounter
declare -A fileNameToNumberMap
declare -A numberToFileNameMap

NameBuilder='Submit=Save+and+Process'

success=no
while [[ $success == "no" ]]; do
randomColour e
echo 'What is name of CSV file? (default = csvfile.csv)'
randomColour
read CSVFilename
if [[ $CSVFilename == "" ]]; then
    CSVFilename=csvfile.csv
fi
CSVFilename=$uploadDataStore$theDirectory/$CSVFilename
if [[ -e $CSVFilename ]]; then
    success=yes
else
    randomColour
    echo Counld not find file by name of $CSVFilename, try again
fi
done

tempfile=$(mktemp)

echo -n $NameBuilder > $tempfile
cat $CSVFilename | php -r 'while(($a = fgets(STDIN))!==false) echo str_replace("---___---","\n",str_replace("-_.-_.",",",urlencode(str_replace("\n","---___---",str_replace(",","-_.-_.",$a)))));' | awk -F "," '{prefix=sprintf("%d_",NR);printf("&%s_Name=%s&%sdataStore=%s&%sbranch=%s&%sdevelopmentName=%s&%saltDevelopmentName=%s&%slocation=%s&%scompletionYear=%s&%sdevelopmentType=%s&%sdevelopmentComprises=%s&%sstageOfCompletion=%s&%sshotType=%s&%sdetail%s&%simageType=%s&%scopyright=%s&%sphotoExists=%s&%soriginalFilename=%s",prefix,$1,prefix,$3,prefix,$4,prefix,$5,prefix,$6,prefix,$7,prefix,$8,prefix,$9,prefix,$10,prefix,$11,prefix,$12,prefix,$13,prefix,$14,prefix,$15,prefix,$16,prefix,$17)}' >> $tempfile
tempfileSize=$(stat -c%s $tempfile)
echo Starting Perl Script
randomColour e

cd ../

cat $tempfile | QUERY_STRING="Folder=$theDirectory&action=doProcessUploads" CONTENT_LENGTH=$tempfileSize perl -T index.cgi 2>&1 | sed  -u -n '/NEW IMAGE/ {s/\(.*\)IMAGE\(.*\)/Perl looks like it is processing image\2/p}' | everyLineADifferentColour

cd ../../upload/

randomColour e
echo 'Hope you enjoyed the ****** Image Resizer Experience!'

tput setaf 0
tput sgr0

Answer 1

Your code is so full of irrelevant content that distracts from the purpose of the code.

The most Beautiful shell scripts are the ones that do a job, and do it well. They do it in a standard way with the least surprise. They accept input from pipes, redirects, and files, and they output in a way that is redirectable, loggable, and readable.

The following is my long-winded-way of reviewing the superfluous fluff that makes what could otherwise be a Beautiful script (there is more serious content after that...)

---

Meant to be in Jest stuff

O *V* E *R* K *I* L *L*

_________                                       __                              
\_   ___ \  ____   _____   _____   ____   _____/  |_  ______                    
/    \  \/ /  _ \ /     \ /     \_/ __ \ /    \   __\/  ___/                    
\     \___(  <_> )  Y Y  \  Y Y  \  ___/|   |  \  |  \___ \                     
 \______  /\____/|__|_|  /__|_|  /\___  >___|  /__| /____  >                    
        \/             \/      \/     \/     \/          \/                     
       .__                 .__       .___ ___.                                  
  _____|  |__   ____  __ __|  |    __| _/ \_ |__   ____                         
 /  ___/  |  \ /  _ \|  |  \  |   / __ |   | __ \_/ __ \                        
 \___ \|   Y  (  <_> )  |  /  |__/ /_/ |   | \_\ \  ___/                        
/____  >___|  /\____/|____/|____/\____ |   |___  /\___  >                       
     \/     \/                        \/       \/     \/                        
                           .___     ___.   .__                              .___
_______   ____ _____     __| _/____ \_ |__ |  |   ____   _____    ____    __| _/
\_  __ \_/ __ \\__  \   / __ |\__  \ | __ \|  | _/ __ \  \__  \  /    \  / __ | 
 |  | \/\  ___/ / __ \_/ /_/ | / __ \| \_\ \  |_\  ___/   / __ \|   |  \/ /_/ | 
 |__|    \___  >____  /\____ |(____  /___  /____/\___  > (____  /___|  /\____ | 
             \/     \/      \/     \/    \/          \/       \/     \/      \/ 
                     _____     .__ ._._._.                                      
 __ __  ______ _____/ ____\_ __|  || | | |                                      
|  |  \/  ___// __ \   __\  |  \  || | | |                                      
|  |  /\___ \\  ___/|  | |  |  /  |_\|\|\|                                      
|____//____  >\___  >__| |____/|____/_____                                      
           \/     \/                \/\/\/                                      

Gimmicks are fun, but, they get tiring fast.

And, they break the 'spirit' of UNIX scripting, which is that they can do things for you in as many situations as possible.

so, what if you wanted to use this in a pipe?

what if you are running this from a screen terminal?

What if you are using a small mobile device to access things?

What if the clutter simply overwhelms the rest of it, and the point of your program is lost.

That is what has happened here

• B
• U
• T

Somewhere 1n th3r3 1s A re4l pr0gr4m, although... I was convinced at:

quitTheBestShellScriptInTheWorld.

Who am I to be critical?

At least you spelled colour correctly ;-)

---

Serious stuff

If you cut through the crap, you have an incomplete script... Lets go through some things..

Traps

This is your trap:

function quitTheBestShellScriptInTheWorld {
    tput setaf 0
    tput sgr0
    if [[ $tempFile != "" ]]; then
        rm $tempFile
    fi
    exit
}

And you enable 20 lines later with:

trap quitTheBestShellScriptInTheWorld 0 1 2 3 15 #let's be tidy

Apart from the humble function name, the system suffers because:

• you should set the trap in the same code-location as the method so that it is obvious they belong together
• there is no need for the exit inside the trap
• It is more readable to use the signal numbers instead of the names
• you only need to trap EXIT (which will be invoked whenever the shell exits (successful or not)
• you should set a the tempFile variable so it has a clear value before the trap is set.

It should look like:

# Setting up the exit Clean-Up trap.

tempfile=""

function exiting {
    if [[ -f $tempFile ]]; then
        rm $tempFile
    fi
}

trap exiting EXIT

Error Handling

You have none.... and, it's really easy... do:

set -e

near the top of your script.

If any command fails then your program will terminate (with a non-zero exit code).

If you have commands that you expect to fail, you can test them with if statements, or put conditionals after them so that the status is handled....

     set -e      Exit immediately if a pipeline (which may consist  of  a
                  single  simple command),  a subshell command enclosed in
                  parentheses, or one of the commands executed as part  of
                  a  command  list  enclosed  by braces (see SHELL GRAMMAR
                  above) exits with a non-zero status.  The shell does not
                  exit  if  the  command that fails is part of the command
                  list immediately following a  while  or  until  keyword,
                  part  of  the  test  following  the  if or elif reserved
                  words, part of any command executed in a && or  ||  list
                  except  the  command  following  the final && or ||, any
                  command in a pipeline but the last, or if the  command's
                  return  value  is being inverted with !.  A trap on ERR,
                  if set, is executed before the shell exits.  This option
                  applies to the shell environment and each subshell enviâ
                  ronment separately (see  COMMAND  EXECUTION  ENVIRONMENT
                  above), and may cause subshells to exit before executing
                  all the commands in the subshell.

Terminal Funkiness

You clear the terminal.... and you play with terminal colour and boldness, etc.

What if you are on a simple terminal that does not support these features? Your program will either fail, or will look really ugly.

A user operating in a terminal is looking for efficiency and usability. The trates you have added are not efficient, and ruin the usability experience the user wants... (otherwise they would be in a GUI...)

Printing out a huge banner is also not useful... what if the user has a small screen... it's ugly,

randomColour
echo Enter settings, or press enter for defaults:
randomColour e

The above implies that you are expecting user input, but then you don't... odd.

Directory Requirements

You expect your user to be inside a specific directory, and this is not 'friendly'. It means a user cannot add the program to their $PATH and expect it to work:

cd ../www/properties/lib/

You can solve this problem by putting the script itself in a know directory, and then getting the actual directory name from there, and then using relative directories:

SCRIPT_PATH=$(cd `dirname "${BASH_SOURCE[0]}"` && pwd)

Then you can use that variable like:

cd $SCRIPT_PATH/../www/properties/lib/

although, that is not ideal, you should not need to be changing directories in the main part of the script.

User Input

You get a directory from the user.... inside a loop:

while [[ $success == "no" ]]; do

But, your indentation seems to die inside that loop. Makes it hard to read.

Also, you should have a mechanism for the stript to take the argument from the commandline, and only prompt for input if there was no commandline argument...

# check first argument.     
theDirectory=$1 
if [[ -d $theDirectory ]] ; then
    success=yes
fi

Same thing for the CSVFile.