Making time lapse screenshots using Bash

My experience in bash is... well, around the Bourne shell on AIX. I wrote a semi-complex script in Bash, and I feel that it could be improved in 100 different ways.

The Github project is here: https://github.com/mercmobily/lapser

Questions:

• Does anything stand out in terms of things done horribly wrong, in any way?

• In the script I have a bunch of global variables defined at the top of the file; I then re-set them depending on the profile picked by the user. I set them with set_profile_dirs() which is run once the user has picked her profile. Is this a horrific way to go about it?

• To deal with files with spaces, I did something like convert ... "$SHOTS_CURRENT_DIR"/"$now".jpg . Is this normally enough?

• I remember having problems with IFs if one of the operands were empty. Did I make my IF statements strong enough?

• The part file='ls -d "$SHOTS_CURRENT_DIR"/ | head -1' looks fragile to me, because if there are no files, it returns a "". Any hints?

• In the program I have something like working_on=echo $res | cut -d '|' -f 1`. Is this the right way of doing things? Is there a way to get all of the fields at once?






• with delta='expr $new_timestamp - $timestamp'; -- is expr still the way to go?






• When I have a yad command like this res=$(yad, I end up with two results: the standard output and the error code. The ifs underneath those commands are kind of clunky. Is there a better way of checking the results?




The code:

``
#!/bin/bash

# Global variables
DATA_DIR="$HOME/.lapser"

PROFILE_DIR=''
LABEL_FILE=''
CONFIG_FILE=''
SHOTS_CURRENT_DIR=''
SHOTS_NOT_CONVERTED_DIR=''
SHOTS_CONVERTED_DIR=''
MOVIES_NOT_UPLOADED_DIR=''
MOVIES_UPLOADED_DIR=''

set_profile_dirs(){
profile=$1
PROFILE_DIR="$DATA_DIR"/profiles/"$profile"
LABEL_FILE="$PROFILE_DIR"/working_on.txt
CONFIG_FILE="$PROFILE_DIR"/config.cfg
LOG_FILE="$PROFILE_DIR"/log.txt
SHOTS_CURRENT_DIR="$PROFILE_DIR"/current_shots
SHOTS_NOT_CONVERTED_DIR="$PROFILE_DIR"/shots_arch

Your questions

• Does anything stand out in terms of things done horribly wrong, in any way?

It's nicely written, easy to read. Many things can be improved, but the way you've written it is very easy to review.

• In the script I have a bunch of global variables defined at the top of the file; I then re-set them depending on the profile picked by the user. I set them with set_profile_dirs() which is run once the user has picked her profile. Is this a horrific way to go about it?

It's fine. But instead of functions expecting global variables, it's easier to test when functions take parameters. For example, testing first_current_shot looks basically like this:

SHOTS_CURRENT_DIR=some_dir
first_current_shot
# check output

SHOTS_CURRENT_DIR=some_other_dir
first_current_shot
# check output

If the function was taking parameters, testing would look more like this:

first_current_shot some_dir
# check output

first_current_shot some_other_dir
# check output

One statement less per test. It's not a big problem, but this is why I tend to prefer functions taking parameters rather than expecting global variables.

• To deal with files with spaces, I did something like convert ... "$SHOTS_CURRENT_DIR"/"$now".jpg . Is this normally enough?

You mean, you enclosed them in double-quotes? Yes, that's it. In this example you could have enclosed the whole thing in double quotes, "$SHOTS_CURRENT_DIR/$now.jpg", the result is equivalent.

• I remember having problems with IFs if one of the operands were empty. Did I make my IF statements strong enough?

I guess you're referring to this kind of situation:

path=
if [ -d $path ]; then
    echo path is a directory: $path
else
    echo path is not a directory: $path
fi

This will not work, because $path is empty, and [ -d ] is missing an operand. Changing to [ -d "$path" ] fixes that. If the operand is guaranteed to never be empty, you can omit the double-quotes.

• The part file='ls -d "$SHOTS_CURRENT_DIR"/ | head -1' looks fragile to me, because if there are no files, it returns a "". Any hints?

You need to add a check in case there are no files.

• In the program I have something like working_on=echo $res | cut -d '|' -f 1`. Is this the right way of doing things? Is there a way to get all of the fields at once?





You could do something like this:



IFS=

This the | characters to newlines, and puts each line in the fields array. 

  
  
with delta='expr $new_timestamp - $timestamp'; -- is expr still the way to go?


  

In Bash you can use math within ((...)), like this:

((delta = new_timestamp - timestamp))


  
  
When I have a yad command like this res=$(yad, I end up with two results: the standard output and the error code. The ifs underneath those commands are kind of clunky. Is there a better way of checking the results?


  

I'm not familiar with yad, and I'm not sure what you're asking here.

Creating many directory levels

Instead of this:

for d in "$DATA_DIR" "$DATA_DIR"/profiles "$DATA_DIR"/profiles/default;do
    if [ ! -d "$d" ]; then mkdir "$d"; fi
  done


You could use the -p flag of mkdir to create all missing parent directories:

mkdir -p "$DATA_DIR"/profiles/default


This works even if the target directory already exists.

Finding the first and last shots

Several improvements are possible to these functions:

first_current_shot(){
  file=`ls -d "$SHOTS_CURRENT_DIR"/* | head -1`
  file=`basename $file`
  file="${file%.*}"
  echo "$file"
}

last_current_shot(){
  file=`ls -d "$SHOTS_CURRENT_DIR"/* | tail -1`
  file=`basename $file`
  file="${file%.*}"
  echo "$file"
}


Issues:

Instead of backticks, use $(...) for subshells, for example $(ls ...)



There are many processes here: ls, head, basename. It's good to use as few processes as possible



The stripping of parent directories and extension is duplicated



Doesn't handle empty directories



An alternative implementation that treats all these issues:

filename() {
    test -e "$1" || return
    file=${1#*/}
    file=${file%.*}
    echo $file
}

first_current_shot() {
    for file in "$SHOTS_CURRENT_DIR"/*; do
        filename "$file"
        return
    done
}

last_current_shot() {
    for file in "$SHOTS_CURRENT_DIR"/*; do
        :
    done
    filename "$file"
}


When the directory is empty, these functions output nothing. Callers must either ensure that the directory is not empty, or check the output.

Use $(...) instead of ...

Backticks are obsolete, and troublesome when you need to nest sub-shells.
Replace all backticks with $(...).

Avoid echo with flags

echo is fine for simple printing like this:

echo some message


But the flags of echo` are not portable. So instead of this:

echo -n > "$f"


This is more portable:

printf > "$f"


But in this\n' fields=($(tr '|' '\n' <<< "$res"))

This the | characters to newlines, and puts each line in the fields array.





• with delta='expr $new_timestamp - $timestamp'; -- is expr still the way to go?





In Bash you can use math within ((...)), like this:

%%CODEBLOCK_4%%





• When I have a yad command like this res=$(yad, I end up with two results: the standard output and the error code. The ifs underneath those commands are kind of clunky. Is there a better way of checking the results?





I'm not familiar with yad, and I'm not sure what you're asking here.

Creating many directory levels

Instead of this:

%%CODEBLOCK_5%%

You could use the -p flag of mkdir to create all missing parent directories:

%%CODEBLOCK_6%%

This works even if the target directory already exists.

Finding the first and last shots

Several improvements are possible to these functions:

%%CODEBLOCK_7%%

Issues:



• Instead of backticks, use $(...) for subshells, for example $(ls ...)






• There are many processes here: ls, head, basename. It's good to use as few processes as possible

• The stripping of parent directories and extension is duplicated

• Doesn't handle empty directories

An alternative implementation that treats all these issues:

%%CODEBLOCK_8%%

When the directory is empty, these functions output nothing. Callers must either ensure that the directory is not empty, or check the output.

Use $(...) instead of ...

Backticks are obsolete, and troublesome when you need to nest sub-shells.
Replace all backticks with $(...).

Avoid echo with flags

echo is fine for simple printing like this:

%%CODEBLOCK_9%%

But the flags of echo` are not portable. So instead of this:

%%CODEBLOCK_10%%

This is more portable:

%%CODEBLOCK_11%%

But in this

StackExchange Code Review Q#121345, answer score: 2