current community

your communities

more stack exchange communities

company blog
Stack Exchange Inbox Reputation and Badges
tour help
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. Join them; it only takes a minute:

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top
up vote 3 down vote favorite

I recently started running but hate running both in the quiet and with music, so i've been listening to podcasts. Unfortunately, I've caught up with all my favorites and so I've started listening to some iTunesU courses, but unfortunately (again) very few of them have audio-only versions.

I wrote a shell script to copy any recently added iTunesU video files to another folder as m4a files (via ffmpeg) but since it's the first such script that i've ever written I am slightly worried to set it to run every morning ...

Anyway, I would appreciate some feedback from you expert types of what potential problems there might be with how I have it working now or suggestions of how I might improve part of it. Also, as you'll see, I tried to set up a function to do the extension check but couldn't get it to work that way, is there a way to do that?

#!/bin/bash

oldIFS="$IFS" # allow for files/directories with spaces in the names
IFS=$'\n'

is_video_file () 
{
    ext=$1

    if ([ $ext = "m4v" ] || [ $ext = "mp4" ]); 
        then
            return 1
        else
            return 0
    fi  
} # couldn't get this to work right...

iTunesU_Source=~/Music/iTunes_U/
iTunesU_Destination=~/Music/iTunesU_audio/

cd $iTunesU_Source

for sub in */; do
    echo;
    echo $sub

    dest_sub=${iTunesU_Destination}/${sub}

    # create the sub folder if it doesn't exist
    does_not_exist=false
    [ -d $dest_sub ] || does_not_exist=true

    if $does_not_exist;
        then
            mkdir $dest_sub
    fi

    # get the modified times for the two subfolders
    source_sub_time=$(stat -f "%m" -t "%s" ${sub})
    dest_sub_time=$(stat -f "%m" -t "%s" ${dest_sub})

    if $does_not_exist || (( $source_sub_time > $dest_sub_time )); 
        then
            cd $sub

            for file in *; do
                filename=$(basename $file)
                extension="${filename##*.}"
                filename="${filename%.*}"

                dest_file=$dest_sub$filename".m4a"

#               if ! [[ -f $dest_file ]] && (is_video_file $extension = 1);
                if ! [[ -f $dest_file ]] && ([ $extension = "m4v" ] || [ $extension = "mp4" ]);
                    then

                    ffmpeg -i $file -vn -acodec copy $dest_file
            fi
            done; # each file in source sub_folder

            cd ..
    fi # source sub_folder is new or recently modified

done

IFS="$oldIFS"
share|improve this question

migrated from superuser.com Mar 9 '13 at 7:57

This question came from our site for computer enthusiasts and power users.
up vote 2 down vote accepted

Function

The command

is_video_file $extension = 1

just passes the arguments $extension , = and 1 to the function is_video_file.

To check if the return value of is_video_file is 1, do the following:

is_video_file "$extension"

if [ $? = 1 ]; then ...

However, making a function return 1 usually indicates an error; the return value 0 indicates success.

If you swap 1 and 0 in the definition of is_video_file, you can use

if is_video_file "$extension"; then ...

Further problems

  • Every time you do not know the value of a variable with certainty, you should surround it with double quotes.

    Most of your variables are unquoted, while the strings are quoted. The latter is unnecessary.

    For example, you wrote

    [ $ext = "m4v" ]

    which might fail if $ext contains unanticipated characters.

    The commands

    [ "$ext" = "m4v" ]

    and

    [ "$ext" = m4v ]

    will both work fine.

Suggestions

  • There is not need to restore the IFS and the end of the script. Modifying the IFS will only affect the script itself.

  • Setting IFS=$'\n' gives a false sense of security. It's still necessary to quote all variables if their values are unknown.

share|improve this answer
    
Beware that the OP is on OS X, so stat's -f and -t options have arguments. – slhck Mar 9 '13 at 8:00
    
Ah, that makes sense. The OP didn't mention OS X anywhere. – Dennis Mar 9 '13 at 10:25
    
Thank you for the help. It is so weird to quote variables but not strings... Also, the function based verification works now. – baiano Mar 9 '13 at 21:52
up vote 1 down vote
  • There is no need for the parentheses in the first if statement.
  • You only need a semicolon at the end of an if statement if you put the then on the same line.
  • then and else belong on the same indentation as if.
  • Functions return the exit code of the last statement.
  • As @Dennis mentioned, return code 0 is "true" and you should always quote variables.

So instead of

    if ([ $ext = "m4v" ] || [ $ext = "mp4" ]); 
        then
            return 1
        else
            return 0
    fi

you can write simply

    [ "$ext" = "m4v" ] || [ "$ext" = "mp4" ]

To avoid nasty problems with strange filenames you should use ./ before any glob starting with *, as in for file in ./*.

You don't need a semicolon after done - It's only ever needed if you want to add another command on the same line or (of course) if the semicolon is an actual parameter to the command, as in find . -exec grep foo {} \;

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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