[ale] Anyone want to crtique a shell script?

[Scott Plante]

I'll take a whack at it. Sorry for the delay, I wanted to wait until I had time to really have a good look. 

One thing that's often overlooked in shell scripts is that you can redirect the output 
of a block of statements at once. I often see stuff like 
  echo a > filename 
  echo b >> filename 
  echo c >> filename
instead of 
  {
    echo a
    echo b
    echo c
  } > filename
The latter also has the advantage of only opening the file once, instead of 3 (or x) times. It may not
seem more concise & readable here (2 extra lines!) but in practice it usually is. Or especially in cases
like yours where the block (for loop) is already there.

Also, for simple "if" conditionals where there isn't both a true and false statement, 
and especially where there's only a single statement to execute conditionally, you can 
use the handy && and || connectors. && means execute the next statement if the previous
returned 0 exit code (true/no error), and || when non-zero/false.

e.g. [ -f "$file" ] && true > "$file"

And for short sanity checks, I often do this kind of thing even though it's two statements:
  [ -f "$reqfile" ] || 
      { echo "$reqfile is required" 1>&2; exit 1; }
or in your case:
  [ -z "$RELEASE"  ] &&
      { echo "Please provide a release number." 1>&2; exit 2; }
Somebody mentioned about the non-zero exit codes already. Also, the 1>&2 sends the 
message to std err, which would be the convention.

An aside:
  I often use the simple 
    > $file
  and I've seen 
   : > $file" 
  Note ":" is the null statement. In my e.g. I used "true" as it's concise and emits no output 
  but is unlikely to be overlooked. If you think cat /dev/null is more readable then that's 
  fine, but I don't know if I'd say any are very much better than the others. I think the cat /dev/null
  is mostly more readable if you're used to seeing it and not used to seeing the others, but there
  are no absolute answers in stylistic questions.

I'm a bit unsure what's meant to be in default, but it seems like the "-r RELEASE" option is not meant
to be optional. If that's true, you shouldn't have it in square brackets in your help message. But maybe
you mean it's optional if it's in the default?

I'm not a big getopts user so I'm going to assume that's all working correctly. I have a vague memory that
it had some kind of notation for which options were required, but I could be confused.

There is a handy little dev file /dev/stdout you can use for standard out when you need a file placeholder. 
I don't see exactly in your script where OUTFILE could be empty, so I just added a check before the "for file in":

So you could make the last block a lot more concise like this:

[ -z "${OUTFILE}" ] && OUTFILE=/dev/stdout
for file in install network services partitions packages pre post
do
  if [ -f "${file}.${RELEASE}.${SUFFIX}" ]
    cat "${file}.${RELEASE}.${SUFFIX}"
  elif [ -f "${file}.${RELEASE}" ]
    cat "${file}.${RELEASE}"
  else
    cat "${file}"
  fi
done > "${OUTFILE}"

In Linux we don't often get files/directories with white space in them, but I always try to account 
for the possibility. Hence the added double quotes around "$OUTFILE" etc.

Ok I lied--not always--typing all those quotes does get old. But usually at least if it's meant to be a 
long-lived script and/or other people will use it. I have the same problem w/ the curly braces on 
variables. At least with them, it's not dependent on the value of the variable--if it works without the 
curlies, it'll always work.

One minor stylistic/convention point. I notice you sometimes use all caps variable names and sometimes 
lower case. I was taught that environment variables were all-caps to differentiate them from the ones 
in your scripts. So I believe OUTFILE, RELEASE, etc. should all be lowercase. The first shell variables
people learn about are environment variables so they seem to think all shell variable should be caps, 
and this old convention is widely ignored or unknown, or maybe I was taught wrong :-P get off my lawn!

FWIW, I've written many shell scripts over the years and never known a missing line-end semicolon to 
cause problems. Well at least if you actually end the line. You need them if you want to type 
{ cmd-a; cmd-b; } 
instead of 
{
  cmd-a
  cmd-b
}
Not to say there isn't some edge case, but I'd be very curious to see it. Then again, 
I sometimes add the superfluous semis out of habit from other languages. 

Scott

p.s. One last thought occurred to me. After fixing whatever typos, you could try something like this:

for prefix in install network services partitions packages pre post
do
  for file in "${prefix}.${RELEASE}.${SUFFIX}" "${prefix}.${RELEASE}" "${prefix}"
  do
    [ -f "$file" ] && { cat "${file}"; continue 2; }
  done
done > "${OUTFILE}"


----- Original Message ----- 

From: "Leam Hall" <leamhall at gmail.com> 
To: "Atlanta Linux Enthusiasts" <ale at ale.org> 
Sent: Saturday, April 11, 2015 10:22:59 AM 
Subject: [ale] Anyone want to crtique a shell script? 

Could use critique on a shell script that writes Kickstart files. You 
don't have to understand Kickstart to critique, the output files are 
plain text in a certain order. 

https://github.com/LeamHall/SecComFrame/tree/master/tools/ks_writer 

The script is ks_writer.sh and the other files there are what gets 
pulled in and what an output might look like. 

Thanks! 

Leam 
_______________________________________________ 
Ale mailing list 
Ale at ale.org 
http://mail.ale.org/mailman/listinfo/ale 
See JOBS, ANNOUNCE and SCHOOLS lists at 
http://mail.ale.org/mailman/listinfo