LinuxQuestions.org

LinuxQuestions.org (/questions/)
-   Programming (http://www.linuxquestions.org/questions/programming-9/)
-   -   Looking for suggestions on bash Script (http://www.linuxquestions.org/questions/programming-9/looking-for-suggestions-on-bash-script-4175430023/)

jamesvjj 10-02-2012 06:12 AM

Looking for suggestions on bash Script
 
I wrote the following bash script. The goal of the script is to read in a file with a list of file names, then copy those files from one directory to another directory. The list of file names has 197 file names, the source directory has over 3000 plus files.

This worked for me, but I am looking for suggestions on how I can improve this script.

#!/bin/bash
sourcedir=~/Desktop/images
targetdir=~/Desktop/images_g1
file="./imagefilenames_g1.csv"
value=0
exec<$file
while read line
do
value=`expr $value + 1`;
echo $value;
image=`sed -n "$value"\p $file`;
echo "$image";
cp $sourcedir/"$image" $targetdir;
done

I am new to bash scripting and I'm not a programmer. I was thrilled I got this to work, and I'll be using it on and off to copy files for various different groups. Doing this got me interested in learning more about bash.

Thanks!

pan64 10-02-2012 06:21 AM

this looks like a one-liner in awk or perl or something like this in bash:
Code:

for image in `cat ./imagefilenames_g1.csv`
do
cp "$sourcedir/$image" $targetdir;
done

how do you use the loop variable line in your original script?

jamesvjj 10-02-2012 07:45 AM

I was looping through each line of the file and using the sed command. Too complicated, your solution is much better. Thanks!

pan64 10-02-2012 07:51 AM

while read line will read config file line by line. You could use it in cp, but you did not do that:
Code:


# ...
# same as before

while read image
do
    echo "$image";
    cp $sourcedir/"$image" $targetdir;
done

is ok too

David the H. 10-04-2012 05:11 AM

I'd like to add a few comments about the OP script:

1) Please use ***[code][/code] tags*** around your code and data, to preserve the original formatting and to improve readability. Do not use quote tags, bolding, colors, "start/end" lines, or other creative techniques.

2)
Code:

sourcedir=~/Desktop/images
"~" is an interactive alias. In a script it's usually better to use the built-in "$HOME" variable.

3)
Code:

exec<$file
a. QUOTE ALL OF YOUR VARIABLE SUBSTITUTIONS. You should never leave the quotes off a parameter expansion unless you explicitly want the resulting string to be word-split by the shell (globbing patterns are also expanded). This is a vitally important concept in scripting, so train yourself to do it correctly now. You can learn about the exceptions later.

http://mywiki.wooledge.org/Arguments
http://mywiki.wooledge.org/WordSplitting
http://mywiki.wooledge.org/Quotes

(Actually, this script is ok, since $file has been previously set to a safe value. But do it anyway to get into the habit! )

b. The line as written is completely unneeded. There's no need set up a fancy file descriptor in most scripts, and even then you'd probably use a different descriptor number.

4)
Code:

while read line...done
Again, instead of using an exec redirect to feed the while loop, just use 'done <"$file"' to redirect the file contents into it directly. It's cleaner and makes the code easier to comprehend.

5)
Code:

value=`expr $value + 1`;
a. $(..) is highly recommended over `..`

b. expr is wholly unnecessary in any modern shell. integer-based arithmetic is built-in.
Try a simple "(( value++ ))" instead.

6)
Code:

cp $sourcedir/"$image" $targetdir;
The quoting is all wrong here; you're leaving $sourcedir and $targetdir completely unprotected. The general rule of thumb is to simply quote the longest string possible. e.g. ...

Code:

cp "$sourcedir/$image" "$targetdir"


All times are GMT -5. The time now is 01:26 PM.