How could I improve this script which sorts files according to extension?

Question

I have a small script which simply shall go through my Downloads folder and then sort the files according to the extension.

How can I make this cleaner/better? I would like to simply maintain a list of extensions and corresponding directories and have the command run with e.g. a for loop, so I don't have to add a new line every time I want to add an extension.

script as it is now:

#!/bin/sh

LOCKFILE=/tmp/.hiddensync.lock

if [ -e $LOCKFILE ]
        then
        echo "Lockfile exists, process currently running."
        echo "If no processes exist, remove $LOCKFILE to clear."
        echo "Exiting..."

        exit
fi

touch $LOCKFILE
timestamp=`date +%Y-%m-%d::%H:%M:%s`
echo "Process started at: $timestamp" >> $LOCKFILE

## Move files to various subfolders based on extensions
find ~/Downloads -maxdepth 1 -name "*.pdf" -print0 | xargs -0 -I % mv % ~/Downloads/PDF/
find ~/Downloads -maxdepth 1 -name "*.opm" -print0 | xargs -0 -I % mv % ~/Downloads/OPM/
find ~/Downloads -maxdepth 1 -name "*.yml" -print0 | xargs -0 -I % mv % ~/Downloads/YML/
find ~/Downloads -maxdepth 1 -name "*.css" -print0 | xargs -0 -I % mv % ~/Downloads/CSS/
find ~/Downloads -maxdepth 1 -name "*.tar.gz" -print0 | xargs -0 -I % mv % ~/Downloads/archives/
find ~/Downloads -maxdepth 1 -name "*.zip" -print0 | xargs -0 -I % mv % ~/Downloads/archives/
find ~/Downloads -maxdepth 1 -name "*.jpg" -print0 | xargs -0 -I % mv % ~/Downloads/Pictures/
find ~/Downloads -maxdepth 1 -name "*.png" -print0 | xargs -0 -I % mv % ~/Downloads/Pictures/
find ~/Downloads -maxdepth 1 -name "*.tiff" -print0 | xargs -0 -I % mv % ~/Downloads/Pictures/
find ~/Downloads -maxdepth 1 -name "*.pm" -print0 | xargs -0 -I % mv % ~/Downloads/Perl/
find ~/Downloads -maxdepth 1 -name "*.xls*" -print0 | xargs -0 -I % mv % ~/Downloads/Excel/
find ~/Downloads -maxdepth 1 -name "*.doc*" -print0 | xargs -0 -I % mv % ~/Downloads/Word/

echo "Task Finished, removing lock file now at `date +%Y-%m-%d::%H:%M:%s`"
rm $LOCKFILE

Answer 1

When there are multiple extensions for a destination, you could put more logic into the find directives:

find ~/Downloads -maxdepth 1 \( -name "*.tar.gz" -o -name "*.zip" \) -print0 | xargs -0 -I % mv % ~/Downloads/archives/

And you don't need to pipe to xargs:

find ~/Downloads -maxdepth 1 \( -name "*.tar.gz" -o -name "*.zip" \) -exec mv -t ~/Downloads/archives/ {} +

---

Since you have -maxdepth 1, do you really need find?

shopt -s nullglob
cd ~/Downloads
mv -t archives/ *.tar.gz *.zip
mv -t Pictures/ *.jpg *.png *.tiff
# etc

This approach will emit some errors if there are no files to move. You can get around that with something like:

shopt -s nullglob
movefiles() {
    local dest=$1
    shift
    if (( $# > 0 )); then
        mkdir -p "$dest"
        mv -t "$dest" "$@"
    fi
}
cd ~/Downloads
movefiles PDF/      *.pdf
movefiles OPM/      *.opm
movefiles YML/      *.yml
movefiles CSS/      *.css
movefiles archives/ *.zip *.tar.gz
movefiles Pictures/ *.jpg *.png *.tiff
movefiles Perl/     *.pm
movefiles Excel/    *.xls*
movefiles Word/     *.doc*

Notes:

• without nullglob, if no files match a pattern, then the function will receive the pattern as a string.
  • for example, if there are no pdf files, the shell will execute movefiles PDF/ "*.pdf"
• with nullglob, if there are no matches, then the shell removes the pattern from the command: movefiles PDF/
• this is why I check the number of arguments: if no files match, then after shifting, $# is zero and hence there's nothing to move.

Answer 2

As a matter of taste, I hate duplicating code and I tend to separate data from code, so I would have some way to define an extension<->target relationship (here in a key-indexed array, but that could be a config file or an sqlite DB):

#! /bin/bash

declare -A destinations

destinations["jpg"]="images"
destinations["mp4"]="videos"
destinations["mov"]="videos"

shopt -s nullglob
for ext in "${!destinations[@]}"
do
    files=(*.$ext)
    [[ 0 -eq ${#files[*]} ]] && continue
    mv -t ${destinations[$ext]} "${files[@]}"
done

This is for bash. The shebang of your script uses sh which is sometimes bash and sometimes something else (dash on Ubuntu...) so unless the code is completely trivial I would avoid using sh in shebangs.

Answer 3

1. find is able to do the mv command by itself, syntax to use is:

   find <filter> -exec mv {} <destination>

   Note that: {} will be replaced by filename and you sometimes need to escape it that way \{\}

2. If you tolerate bash'ism, you can take advantages of bash arrays and rewrite your script that way:

    BASE_PATH="~/Downloads/"
    # 1st element: find filter, 2d element: destination
    EXT_TO_PATH=(
        'a=( "-iname \"*.pdf\" " "PDF" )'
        'a=( "-iname \"*.jpg\" -o -iname \"*.png\" " "Pictures" )'
    )

    for elt in "${EXT_TO_PATH[@]}" ; do
        eval $elt
        echo "Extensions: ${a[0]}"
        echo "Destination: ${a[1]}"
        find $BASE_PATH -maxdepth=1 ${a[0]} -exec mv {} ${BASE_PATH}/${a[1]}
    done

Answer 4

It doesn't matter that much in this case, because your task is idempotent, but your locking is incorrect and subject to a race condition.

You check for the presence of the lockfile, and then create it, using two distinct (non-atomic) operations. Because of this, If two instances of the script are launched at the same time, it is very possible that both will check the existence of the lockfile at the same time, and both proceed to run in parallel.

What you should do instead, is use a single atomic operation to grab the lock and decide if you should proceed. There are multiple ways to do this; for instance, if you are under linux, you can use the flock command to perform advisory locking.

If you don't want to use the linux-specific flock, underbash, you can do something like

set -o noclobber
if ! exec 9>/$LOCKFILE
then
    echo "lockfile already present" >&2
    exit 1
fi 
...
echo "Process started at ..." >&9

This performs a single atomic operation (open the lockfile). The noclobber option makes sure that the exec will fail if the file has already been created. You can later append to the file by writing to the open file descriptor.

Also, when writing diagnostic messages for human consumption, prefer writing them on stderr, not stdout. For instance by doing

echo "This is an error message" >&2

Answer 5

Wander along the directories and move all matching items into each in turn

for item in *
do
    [[ -d "$item" ]] && mv -f *."$item" "$item" 2>/dev/null
done

The error output from mv is discarded so that illfounded attempts to move nonexistent files are not reported.

Alternatively, you could try each file and move it into any corresponding directory

for item in *
do
    if [[ -f "$item" ]]
    then
        ext="${item##*.}"
        [[ -d "$ext" ]] && mv -f "$item" "$ext"
    fi
done

Answer 6

In essence, you need two (connected) values: The pattern <--> Dest dir.

For example "*.jpg" <--> ~/Downloads/JPG

That's the same structure of an associative array.

So, we can list all pattern-dest in one array (in ksh, bash, zsh)

unset filetype; declare -A filetype
filetype["*.pdf"]="$dir/PDF"
filetype["*.opm"]="$dir/OPM"
filetype["*.yml"]="$dir/YML"
filetype["*.css"]="$dir/CSS"
filetype["*.tar.gz"]="$dir/archives"
filetype["*.zip"]="$dir/archives"
filetype["*.jpg"]="$dir/Pictures"
filetype["*.png"]="$dir/Pictures"
filetype["*.tiff"]="$dir/Pictures"
filetype["*.pm"]="$dir/Perl"
filetype["*.xls*"]="$dir/Excel"
filetype["*.doc*"]="$dir/Word"

And the loop gets reduced to:

## Move files to various subfolders based on extensions
for ftype in "${!filetype[@]}"     # list of array **keys**
do  
    find "$dir" -maxdepth 1 -name "$ftype" -exec mv {} "${filetype[$ftype]}" \;
done

The whole script could then be:

#!/bin/bash

LOCKFILE=/tmp/.hiddensync.lock

if [ -e "$LOCKFILE" ]
then
        echo "Lockfile exists, process currently running."
        echo "If no processes exist, remove $LOCKFILE to clear."
        echo "Exiting..."
        exit
fi

timestamp=$(date +%Y-%m-%d::%H:%M:%s)
echo "Process started at: $timestamp" > "$LOCKFILE"

dir=~/Downloads

unset filetype; typeset -A filetype
filetype["*.pdf"]="$dir/PDF"
filetype["*.opm"]="$dir/OPM"
filetype["*.yml"]="$dir/YML"
filetype["*.css"]="$dir/CSS"
filetype["*.tar.gz"]="$dir/archives"
filetype["*.zip"]="$dir/archives"
filetype["*.jpg"]="$dir/Pictures"
filetype["*.png"]="$dir/Pictures"
filetype["*.tiff"]="$dir/Pictures"
filetype["*.pm"]="$dir/Perl"
filetype["*.xls*"]="$dir/Excel"
filetype["*.doc*"]="$dir/Word"


## Move files to various subfolders based on extensions
for ftype in "${!filetype[@]}"
do  
    find "$dir" -maxdepth 1 -name "$ftype" -exec mv {} "${filetype[$ftype]}" \;
done

echo "Task Finished, removing lock file now at $(date +%Y-%m-%d::%H:%M:%s)"
rm "$LOCKFILE"