CODEWITHSUNDEEP
bashmacos
eozzy
eozzy

Reputation: 68650

for loop in bash just runs once

I'm doing resolution check in bash but this loop just runs once and ends without an error:

for file in *; do \
  WIDTH = $(identify -ping -format '%h' $file) \
  HEIGHT = $(identify -ping -format '%w' $file) \
  if [ "$WIDTH" -ge 500 ]; then \
    echo width greater than 500 \
  elif ["HEIGHT" -ge 500]; then \
    echo height greater than 500 \
  fi \
done

Output:

height greater than 500 fi done

Why doesn't it check all the files?

Upvotes: 0

Views: 57

Answers (2)

Charles Duffy
Charles Duffy

Reputation: 295278

#!/bin/bash
# using bash, not sh, ensures that <(), (( )), and other extensions are available.

for file in *; do
  IFS=: read -r width height < <(identify -ping -format '%w:%h\n' "$file")
  if (( width >= 500 )); then
    printf '%s\n' "$file has width greater than 500"
  elif (( height >= 500 )); then
    printf '%s\n' "$file has height greater than 500"
  fi
done
  • Running identify twice doubles the performance impact of invoking an external command; better to run it only once, and read both variables in a single pass. The syntax used to collect the output of the identify command into a stream is process substitution, whereas the read command is rather comprehensively discussed in BashFAQ #001, or the relevant bash-hackers wiki page.
  • Backslashes are line continuations; they're appropriate when you have a simple command (not a compound command like this one) that spans multiple lines. In this context, they're simply wrong.
  • [ is a command, and command names need to be separated from their arguments by spaces. Just as you run ls -l, not ls-l, you can't run [foo; it needs to be [ foo, as two separate words. (If it's clearer, consider using the synonym test: if test "$width" -ge 500; then ...).
  • All-caps variable names are specified by POSIX-defined convention to be used for names with meaning for the system and shell, whereas names with at least one lower-case character are reserved for application use. (The convention explicitly applies to environment variables, but shell variables share the same namespace: setting a shell variable with a name that overlaps an environment variable will overwrite the latter).
  • Using printf rather than echo has better-defined behavior: the POSIX specification for echo leaves wide ranges of behaviors undefined, and if printing a filename containing literal backslashes, the behavior of echo becomes implementation-dependent and thus nonportable. See in particular the APPLICATION USAGE section of the linked page.
  • Using (( )) puts you into a math context, where variable names can be used without a preceding $, and more natural C-style math syntax can be used, such as >= rather than -ge. (This is a bashism -- POSIX sh specifies only $(( )), which has similar behavior but is a substitution command; thus, in POSIX sh, one could instead write if [ "$(( width >= 500 ? 1 : 0))" = 1 ] to test the results of this substitution).

Upvotes: 1

eozzy
eozzy

Reputation: 68650

Asked too soon, figured it out myself using this awesome helper - shellcheck:

for file in *; do 
  WIDTH=$(identify -ping -format '%h' "$file") 
  HEIGHT=$(identify -ping -format '%w' "$file") 
  if [ "$WIDTH" -ge 500 ]; then 
    echo width greater than 500 
  elif [ "$HEIGHT" -ge 500 ]; then 
    echo height greater than 500 
  fi 
done

Upvotes: 0

Related Questions