How can this script be improved?

[Cannonfodder]

This is probably the longest script I've ever written. It's purpose is to generate a thumbnail directory for each directory in a tree. So if I call it with a parameter

 

/dev/photos

 

It will check each directory and

 

1. If a prior thumbnail directory is present, it is deleted. directory name is /thumbs.

 

2. Make sure all jpg files in the directory have the same extension JPG, JPEG and jpeg all get renamed to jpg.

 

3. Check to make sure there is a jpg file to thumbnail and then generates thumbnails using imagemagick.

 

As it stands, it works and does everything I need. My purpose for posting this is to run it by the experts. I'm fairly new to scripting in linux so I am wondering how it can be improved.

 

What are the obvious things I'm missing? Using the tree command for example? Was that a good idea? Having to check for the thumbs directory so it won't be processed was a surprise. Apparently the tree command is re-run each loop.

 

Checking for a jpg file was a deal for me too. Doing a

[ -f $i/*.jpg ] would generate an error if a *.jpg file was preseent. Apparently the command only works for specific instances. Or do I have that wrong? :)

 

Here's the code..

 

#!/bin/bash

# Loop through directories and

# 1. Check for thumbs folder, delete if found

# 2. run thumb command if jpg files found

echo Start directory is `pwd`

myTree=$(tree -dfin --noreport $1)



for i in $myTree; do

isThumb=$(basename $i)



if [ ! "${isThumb}" = "thumbs" ]

then

   renameJPG=0

   renameJPEG=0

   renamejpeg=0



   # check for thumbs directory. Remove if found



   if [ -d $i/thumbs ]

   then

     echo removing $i/thumbs

     rm -fR $i/thumbs

   fi



   # check all files in directory for possible renames

   for x in $i/*

   do



     case "${x##*.}" in

     JPG)

         renameJPG=1

        ;;

     JPEG)

         renameJPEG=1

        ;;

     jpeg)

         renamejpeg=1

        ;;

     esac



   done



   # do renames

   [ $renameJPG -eq 1 ] && rename JPG jpg $i/*.JPG

   [ $renameJPEG -eq 1 ] && rename JPEG jpg $i/*.JPEG

   [ $renamejpeg -eq 1 ] && rename jpeg jpg $i/*.jpeg



   # check to see if we now have any .jpg files

   hasjpg=0



   for x in $i/*

   do



     case "${x##*.}" in

     jpg)

         hasjpg=1

        ;;

     esac



   done



   # make thumbnails of all jpg files. Results will be .jpeg files

   thisDir=`pwd`



   if [ $hasjpg -eq 1 ]

   then

     echo "processing $i"

     cd $i

     mogrify -format jpeg -geometry '100x100' -quality '50%' *.jpg



     # copy all thumbnails to thumbs directory

     mkdir thumbs

     cd thumbs

     mv ../*.jpeg .

     rename jpeg jpg *.jpeg

     cd $thisDir

   fi



 fi



done

[aru]

Sorry, I've noticed your post right now while hunting for unanswered posts ;)

 

The first thing one notices when checking your script is that you used a lot of unuseful and redundant loops. You only need two, one main loop for directories and another one for files within those dirs.

 

Instead of commenting your code, I'm going to re-write the script how I would have done it. I'm not doing this from scratch, but following your logic, so I'll do the steps you did in a little more efficient way.

 

I'm writing the script in this window, so I'm not sure if it is bug free, or even if it will work at all. But I guess that if you read it you'll understand where were your flaws ;)

 

#! /bin/bash

# Loop through directories and 

# 1. Check for thumbs folder, delete if found 

# 2. run thumb command if jpg files found 



# let's check a bit the commandline:

if [ $# != 1 ] || [ ! -d "$1" ]; then

   echo "Usage: "${0##*/} <path>"" >&2 && exit 1

else

   cwd=$1

   echo Start directory is $cwd

fi



# vars needed at some point later (notice that "find" itself will remove the thumbs folders):

thumbDirName="thumbs"

ext="jpg"

myTree='find "$cwd" -type d -name "$thumbDirName" -exec rm -fr {}; -or -type d -print 2>/dev/null'

makeThumb="mogrify -format jpeg -geometry '100x100' -quality '50%'"



eval $myTree | while read dir; do   # Use "while" instead of "for"; it's safer.

   thumbDir="${dir}/${thumbDirName}"

   # check all files in directory for possible renames and make the thumbnails moving 

   # them their directory (all names default to ".jpg")

   for file in ${dir}/*; do

        case "${file##*.}" in

           JPG|JPEG|jpeg|jpg)

               [ "${file##*.}" != "jpg" ] &&  mv "${file}" "${file%.*}.${ext}"

               echo "processing $file"

               mkdir -p "${thumbDir}" && ${makeThumb} "${file%.*}.${ext}" &&

               mv "${file%.*}.jpeg" "${thumbDir}/$(basename ${file} ${file##*.})${ext}"

          ;;

        esac

   done

done

 

I'm sure that this can be even much more efficient, but I've writen it 'on the fly' so I haven't got the chance of checking/testing it

 

Checking for a jpg file was a deal for me too. Doing a  

[ -f $i/*.jpg ] would generate an error if a *.jpg file was preseent. Apparently the command only works for specific instances. Or do I have that wrong?

That conditional means: check if *A FILE* exists and it is a regular file. Before the shell can be able to evaluate that expression, it produces the filename, parameter, .... expansion, thus your conditional will look to the shell like: [ -f /path/to/foo.jpg /path/to/bar.jpg ... ] which is not what the shell expected. Hence the error

 

 

 

HTH, and please comment any doubts/complains you have about my code :D

 

 

EDITED: Improved the find command to remove thumbs dirs while outputing the directory tree

EDITED: Few fixes :mrgreen:

[aru]

A few speed improvements. Ofcourse them are *almost* useless since the limitant step is the command morgify; but anyhow, optimizating is always funny :D

 

#! /bin/bash 

# Loop through directories and 

# 1. Check for thumbs folder, delete if found  

# 2. run thumb command if jpg files found 



# let's check a bit the commandline: 

if [ $# != 1 ] || [ ! -d "$1" ]; then

 echo "Usage: "${0##*/} <path>"" >&2 && exit 1

fi



# vars needed at some point later (notice that "find" itself will remove the thumbs folders): 

cwd="$1"

thumbDirName="thumbs"

def_ext="jpg"

myTree='find "$cwd" -type d -name "$thumbDirName" -exec rm -fr {}; -or -type d -print 2>/dev/null'

makeThumb="mogrify -format jpeg -geometry '100x100' -quality '50%'"



echo Start directory is $cwd 



eval $myTree | while read dir; do  # Use "while" instead of "for"; it's safer. 

 thumbDir="${dir}/${thumbDirName}"

 # check all files in directory for possible renames and make the thumbnails moving 

 # them their directory (all names default to ".jpg") 

 for file in ${dir}/*; do

    basename="${file##*/}"

    extname="${file##*.}"

    case ${extname} in

     JPG|JPEG|jpeg|jpg)

       [ "${extname}" != "jpg" ] && mv "${file}" "${file%.*}.${def_ext}"

       [ -d "${thumbDir}" ] || mkdir -p "${thumbDir}"

       echo "processing $file" 

       ${makeThumb} "${file%.*}.${def_ext}" && 

       mv "${file%.*}.jpeg" "${thumbDir}/${basename%.*}.${def_ext}"

    ;;

    esac

 done

done

 

EDITED: Silly of me! I forget to put the ${makeThumb} "${file%.*}.${ext}" && stuff (instead there was some debug code I used to test performance improvements)

[Cannonfodder]

Aru,

 

In this line..

 

myTree='find "$cwd" -type d -name "$thumbDirName" -exec rm -fr {} ; -or -type d -print 2>/dev/null'

 

What does the -or -type d -print 2>/dev/null' do?

 

I am guessing if it can't find a thumbs directory it just redirects the not found message to null?

 

If I use this instead..

 

find "$cwd" -type d -name "$thumbDirName" -exec rm -fr {} ;

 

It will remove the directory but then complains it can't find it? Why is that?

[aru]

myTree='find "$cwd" -type d -name "$thumbDirName" -exec rm -fr {} ; -or -type d -print 2>/dev/null'

 

What does the -or -type d -print 2>/dev/null' do?

Remember what did your myTree='tree ...' command. You wanted the list of directories for processing them later.

 

My find command does the same, it outputs to stdout the list of directories. But as find is a very powerfull tool, while retrieving the list of directories it can also be instructed to remove those we didn't want arround (thumbs)

 

What it exactly does is:

 

from $cwd dir descend to every directory and WHEN finds a directory (type -d) named thumbs (-name "$thumbDirName") proceed to delete it ( -exec rm -fr {} ; ) . OR (-or) if it finds a directory (-type d) , print it to stdout (-print) generating our directory list (w/o thumbs folders)

 

I am guessing if it can't find a thumbs directory it just redirects the not found message to null?  

 

If I use this instead..

 

find "$cwd" -type d -name "$thumbDirName" -exec rm -fr {} ;

 

It will remove the directory but then complains it can't find it? Why is that?

 

That's right. It seems to me that after removing the the directory, find tries to enter in it to continue with its recursion but w/o knowing that the command it has just executed, deleted its target (find is not that smart to know what are the consecuences of the command executed through -exec), hence the error. Since It is a minor error I just send that anoying stderr output to /dev/null

 

Another way, with the same performance (I guess, but more clear to understand could be:

 

myTree='find "$cwd" -type d -name "$thumbDirName" -exec rm -fr {} 2>/dev/null; -or -print'

[Cannonfodder]

That makes sense, it has to print the output to stout so it can be used later.. Still looking at your script.. I'll post an update when I'm done :)

 

Thanks for the help!

[static]

Where could I get a comprehensive reference on bash programming aru? You've got it down and I'd like to...

 

I want to make newbie's lives easier. Therefore linux easier. Therefore linux more widely accepted, and therefore more widely used.

[aru]

Where could I get a comprehensive reference on bash programming aru? You've got it down and I'd like to...

 

If you already know how to move in bash, this is the text:

Advanced Bash-Scripting Guide

 

My favorite, though it is rough:

Bash Reference Manual

 

Some useful sites:

http://www.shelldorado.com/ <-- Heiner Steven's site. Many links, tips and scripts

usenet's comp.unix.shell <-- The shell guru's habitat

[ramfree17]

If you already know how to move in bash, this is the text:

Advanced Bash-Scripting Guide

 

My favorite, though it is rough:

Bash Reference Manual

 

Some useful sites:

http://www.shelldorado.com/ <-- Heiner Steven's site. Many links, tips and scripts

usenet's comp.unix.shell <-- The shell guru's habitat

 

i think we have there some pretty incriminating evidence.

 

ciao!

[aru]

:lol: