Friday 18 January 2013

Re-factoring bash

Sometimes one has to perform maintenance on someone else's bash scripts. At such times one keeps one's mouth shut and gets on with it.

Sometimes one gets to re-write them to add new features, and some re-factoring brings great relief.

See this function wrapper to run a command more than once in case it fails the first time. (Don't get distracted on thoughts of the validity of this approach).

do_cmd()
{
  retval=0
  for i in `seq 1 2`;
  do
    cmd 2>/dev/null $@
    retval=$?
    if [ $retval -eq 0 ];
    then
         break
    fi

    sleep 1
  done

  return ${retval}
}

Can we express that function any more concisely or correctly?

The first thing that comes to mind is local retval=0

...but that is closely followed by the wonder of `seq 1 2` which emits 1 2 and so that line could become:

for i in 1 2

At this point it becomes very obvious that the function will execute cmd up to twice, with a 1 second delay if it fails the first time. Any bash coder given that description could come up with a 1 liner; but let's approach it by degrees.

This chunk:

cmd 2>/dev/null $@
    retval=$?
    if [ $retval -eq 0 ];
    then
         break
    fi

becomes:

cmd 2>/dev/null $@
    retval=$?
    test $retval -eq 0 && break

but we don't need to remember retval if we return right away, so this might become:

cmd 2>/dev/null $@ && return
    retval=$?

(we still remember retval so that we can return the last failure code if the loop exits.

But who needs a loop for 2 iterations? How about this:

do_cmd()
{
  cmd 2>/dev/null $@ && return
  sleep 1
  cmd 2>/dev/null $@
} 

Which is pretty clear. But compare that to a fresh re-write based on the original description above:

do_cmd()
{
  cmd "$@" || sleep 1 && cmd "$@"
} 2>/dev/null

I also added in the missing quotes around $@ and added the stderr redirector to the function body definition.

The function will exit with either 0 because the first invocation succeeds, or it will return with the exit code of the second invocation 1 second later - presuming sleep doesn't fail.

If we are afraid that sleep might fail, we could make the second invocation not dependant on the success of the sleep command:

do_cmd()
{
  cmd "$@" || { sleep 1 ; cmd "$@" ; }
} 2>/dev/null

Of course this doesn't quite match the original function because any stderr resulting from a failure in executing the sleep command would also be hidden.

1 comment:

  1. Heh, for a moment the naming of do_cmd() made me think, "Gosh, did I write that at some point?". But I'm hoping I didn't, based at least on the lack of "local" for retval, the loop structure, and the return value checking style.

    This reminds me of the discussions around more "Pythonic" ways of achieving a goal in Python. A true Bashista would prefer the refactored do_cmd(), but I've seen plenty of people write do_cmd() in the same fashion as the original.

    Really, though, the key issue with refactoring Bash scripts is that they rarely have much in the way of test harnesses or documentation. So it's sometimes tricky to figure out whether your refactorings have been beneficial or not :-|

    ReplyDelete