One problem is that read $a etc is wrong: you should write read a if you need the read at all. As it stands, the value read is stored in the variable with the name stored in $a.
Another problem is that it is far from clear to the innocent user that they're supposed to enter 3 lines of information before the script will continue, but the three read lines force that.
Another problem is that you don't read into $choice (via read choice) so the for loop has nothing to do.
Another problem is that your script will inherit the values of any environment variables that happen to be the same as the names of the variables you're using.
Another problem is that you don't quote the file names. It mostly won't matter unless you have a name that contains spaces or other similarly awkward characters.
A cosmetic issue is that the printf statement is ridiculously long. Use one printf per line. Or use echo. Stuff that scrolls off the RHS of the page is bad (though I don't regard 80 characters as a fixed length for lines, there's a quadratic penalty for lines that are longer than 80 — as (length-80)2 increases, the pain of the longer line goes up.
At another level altogether, the interface is modestly grotesque. As an exercise in shell scripting, it makes sense. As an exercise in how to design good shell scripts, it is a very bad design.
A design that might make sense is:
- Set variables to empty:
a=""; b=""; c=""; etc.
- Offer a range of choices similar to those given now, but add an option to execute the command, and another to abandon ship.
- Have a loop that reads choices, and sets flags.
- When the user chooses execute, exit the loop and prompt for the file names.
- If all's well, execute the command.
Note that you should check that the read commands work; if they don't, fail safe (don't damage anything).
Putting all those together (with some slight differences, but the same overall effect — witness the use of local for the variables):
fileCpy()
{
local a b c file dest
echo "Choices:"
echo "0. Return to main menu"
echo "1. Interactive copy, answer yes/no before doing the copy"
echo "2. Make backups of existing destination files"
echo "3. Preserve file attributes"
echo "4. Do a recursive copy"
echo "5. Execute the copy"
while printf "Your choice: " && read choice
do
[ -z "$choice" ] && return 1 # Empty input - failure
case "$choice" in
(0) return 0;;
(1) a="-i";;
(2) b="--backup";;
(3) c="-p";;
(4) d="-R";;
(5) break;;
(*) echo "Unrecognized response ($choice); please enter 0..5";;
esac
done
[ "$choice" != 5 ] && return 1 # EOF - failure
printf "Type the name of the file you wish to copy/backup: "
read file
[ -z "$file" ] && return 1 # Empty file name - failure
printf "Type the name of the destination file/directory: "
read dest
[ -z "$dest" ] && return 1 # Empty file name - failure
cp $a $b $c "$file" "$dest"
}
Test code:
echo "a=$a b=$b c=$c file=$file dest=$dest"
if fileCpy
then : OK
else echo "Failed"
fi
echo "a=$a b=$b c=$c file=$file dest=$dest"
The last block is a simple test harness. It reports on the values of the variables used inside the function, runs the function, reports if the function failed, and re-echoes the variables to demonstrate that they've not been set.
I would not use that interface unless you paid me to do so, but it more or less meets the goal of a training exercise.