I wrote a script which changes names of last downloaded songs and copy them to music directory. Can it be written better?

    #!/bin/bash
IFS=$'\n'
for file in $(find "/home/stanek/Downloads/" -iname "*.mp3" -mtime -7 | sort)
do
    nowy=$(echo $file \
    | sed "s/_/ /g" \
    | sed "s/\s*(*[A-z0-9.]*.pl)*//g" \
#leave brackets and text if it is remix
    | sed "s/\(\s*(\s\?[A-z0-9 ]*remix)\)\|\s*(\s\?[a-z0-9. ]*)/\1/gi")

    if [ "$nowy" != "$file" ]
    then
        echo "ZMIANA NAZWY DLA:"
        echo $nowy
        mv $file $nowy
    fi
done

for file in $(find "/home/stanek/Downloads/" -iname "*.mp3" -mtime -7 | sort)
do
    echo "I will copy: $file" | sed "s/\/home\/stanek\/Downloads\///g"
done
read -p "Is it ok? (y)" -n 1 -r
echo
if [[ ! $REPLY =~ ^[Yy]$ ]]
then
    exit 1
fi

i=0

for file in $(find "/home/stanek/Downloads/" -iname "*.mp3" -mtime -7 | sort)
do
    paste=$(echo $file | sed "s/\/home\/stanek\/Downloads\///g")
    cp $file "/home/stanek/Music/$paste"
    i=$((i+1))
done
echo "Copied $i files"
up vote 2 down vote accepted

"/home/stanek/Downloads/" is used often, consider putting that as a script variable, if it's not already present as an environment variable provided by your shell user profile? Let's assume USER_DOWNLOADS="/home/stanek/Downloads", MUSIC_DIR="/home/stanek/Music" for the suggestions below.

You are invoking find three times, the problem being that new files created during each invocation will be missed by the subsequent one. This may be less of a problem between the first two, but will be for the second and third loops that are separated by a user prompt.

The other question is ask is whether your renaming of files should be done as part of the copying (i.e. later) process, or do you always want to rename the files regardless of whether the copying is done.

If it's the former, then you should only construct the new name within the final loop, for example:

# not *exactly* sanitized for files with spaces, just used as illustration only!
files_to_copy=$(find $USER_DOWNLOADS -iname "*.mp3" -mtime -7 | sort)
echo "Files will be copied:"
for i in $files_to_copy; do basename $i; done
# prompt user
for i in $files_to_copy; do
    new_name=$(basename $i | sed -e 's/.../.../;...');
    cp $i $MUSIC_DIR/$new_name
done

You can string sed commands together: sed -e 's/.../.../;...' as shown above. basename will do the equivalent of your sed "s/\/home\/stanek\/Downloads\///g".

If it's the latter, i.e. same as what you have presented here, then you can leave the first loop aside, but build up your list of files to be copied first:

files_to_copy= # ...
# ...
# prompt user
cp $files_to_copy $MUSIC_DIR
echo "$(awk -F' ' '{print NF}' <<< $files_to_copy) file(s) copied."

To get past filenames with spaces in them, files_to_copy should be constructed as a variable array.

  • @siema glad to help! – h.j.k. Jun 10 '15 at 0:36

In addition to the other excellent suggestions,

Instead of using sed:

$(echo $file | sed "s/\/home\/stanek\/Downloads\///g")

try bash parameter substitution:

"${file//\/home\/stanek\/Downloads\//}"

And remember to quote - there's a few places that have a bare $variable that shouldn't be unquoted.

  • 1
    Bash substitution is a good all-pupose suggestion, but in the context of this question, basename will be more appropriate as the intention is to get the filename. :) Have +1 anyways. – h.j.k. Jun 12 '15 at 10:29
  • 1
    @h.j.k. - quite right; I hadn't spotted how the substitution was being used! – Toby Speight Jun 15 '15 at 13:44

Your Answer

 
discard

By clicking "Post Your Answer", you acknowledge that you have read our updated terms of service, privacy policy and cookie policy, and that your continued use of the website is subject to these policies.

Not the answer you're looking for? Browse other questions tagged or ask your own question.