File handling script This is my first bash script so I ask to you to say what you think.

The script must check every directory and if it finds a pdf file in this directory:  move all the pdf files to parent directory, change the name of the files and remove directory.

So if I had this folder structure:

folder1
  file.pdf
  file.txt
folder2
  some.pdf
  another.pdf
folder3
  some.txt


the script must change it to:

 folder1.pdf
 folder2.pdf
 folder2_2.pdf
 folder3
   some.txt


Here is a script code:

#!/bin/bash

working_directory="$HOME/MEGAsync/downloads/books"

cd $working_directory

# for each directory in working directory
for D in `find . -type d`
do
    # skip '.'
    if [[ $D != "." ]]; then
        # get folder name 
        folder_name=${D##*/}
        # and cd to folder
        cd $D

        # number of pdf files in the current directory
        declare -i pdf_count=0
        # for each file in the current directory
        for f in *; do

            # get file without path
            file=$(basename "$f")
            extension="${file##*.}"
            filename="${file%.*}"

            if [[ $extension == 'pdf' ]]; then
                pdf_count=pdf_count+1
                if [[ pdf_count &lt; 2 ]]; then
                    new_filename="${working_directory}/${folder_name}.pdf";
                else
                    new_filename="${working_directory}/${folder_name}_${pdf_count}.pdf";
                fi

                echo cp $working_directory/$folder_name/$f $new_filename
                cp $working_directory/$folder_name/$f $new_filename
            fi
        done

        # return to parent folder
        cd ..

        # delete directory if pdf-file was found
        if [[ $pdf_count != "0" ]]; then
            echo rm -R $working_directory/$folder_name
            rm -R $working_directory/$folder_name
        fi
    fi
done
For a first script, this is great, and I am impressed.


your structure is neat
you are using loops properly
you are using variable substitution which is 'advanced'


The script appears that it will work for all the standard cases, and, I have seen much worse in 'production' code.

So, as a beginner, well done.

What's next?


the edge cases are going to kill this script - spaces in file names.
the logging and error handling could be better
I recommend basename and dirname instead of the variable regexes you use:

dirname: http://linux.die.net/man/1/dirname
basename: http://linux.die.net/man/1/basename (note that basename path/to/file.pdf .pdf returns file)

consistency on for->do loop - you have do on the next line once, and then for ... ; do the next time. Either is fine, but be consistent.


All of the error-handling, logging, and the spaces-in-files can be solved easily by using a function for running commands. Also, the "$@" construct in bash is 'special'

Consider the bash function:

function runcmd {
    echo "$@"
    # Fail if the command fails...
    "$@" || exit 1
}


So, this function will print the command, and then will run it, and exit the script if the command fails.

Now, instead of structures like:


            echo cp $working_directory/$folder_name/$f $new_filename
            cp $working_directory/$folder_name/$f $new_filename



you can do:

            runcmd cp "$working_directory/$folder_name/$f" $new_filename


and you will still get the echo, you will get the new error handling, and you will handle spaces in names just fine.