4
\$\begingroup\$

I would like to use the following routines in my submission script for GAMESS calculations. Before I do, I would like to get some input if I can improve it somehow.

The three messaging routines (message,warning,fatal) are taken from my earlier review request, although I have chosen a more verbose name.
Since I am not posting the whole script due to it's length, I only include the lines that are necessary to run it. I will describe what the routines do in a little more detail below.

The input to the script should accept either a job title or a file name. The corresponding counterpart needs to be found automatically. It then should assign a output file destination based on the supplied (or found) input file ending. It checks weather this file exists and creates a backup.
Based on the job title it also checks if any auxiliar files exist from a previous run. If they could potentially stop the execution the script will abort. If they are in the current directory, they will be backed up like the output file.

Even though it seems unnecessary, username="$USER" is included because the username from the queue it is sent to may not be equal to the username for the linux system.

  • is_readableFile is used to make the following code more readable.
  • exist_File is the backup procedure.
  • exist_GamessAuxfiles checks if there are files leftover from a previous (incomplete) run.
  • validate_GamessFiles is the main part.
  • last line is to check if it is actually working. It does (at least in the cases I was able to come up with.)

#!/bin/sh

# Following Variables will be set by the main script
username="$USER"
requestGamessAuxOutDir="/home/$username/scr"

# Read Input from cmdline
submittedInputfile=$1

#
# Print logging information and warnings nicely.
# If there is an unrecoverable error: display a message and exit.
#

message ()
{
    echo INFO   : $*
}

warning ()
{
    echo WARNING: $*
}

fatal ()
{
    echo ERROR  : $*
    exit 1
}

# files must exist, be readable and regular
is_readableFile ()
{
    [[ -r "$1" && -f "$1" ]]
}

#
# Check if file exists and prevent overwriting
#
exist_File ()
{
    if [ -e "$1" ]; then
      local filecount=1
      while [ -e "$1.$filecount" ]; do
        ((filecount++))
      done
      message "File \"$1\" exists."
      message "It has been moved to  \"$1.$filecount\"."
      mv $1 $1.$filecount
    fi
}

#
# Check if there are auxiliar files from a Games run
#

exist_GamessAuxfiles ()
{
    local possibleAuxfiles
    for possibleAuxfiles in "dat" "trj" "rst" "efp"; do
      if [ -e $requestGamessAuxOutDir/$1.$possibleAuxfiles ]; then
        fatal "Please save, rename, or erase \"$requestGamessAuxOutDir/$1.$possibleAuxfiles\" \
               from a previous run and try again."
      fi
      if [ -e $1.$possibleAuxfiles ]; then
        exist_File "$1"
      fi
    done
}

# 
# Validate that inputfile exists and is readable,
# extract the suffix and jobtitle
# generate name for outputfile
# test if outputfile exists
#

validate_GamessFiles ()
{
    local allowedInputSuffix=(com in inp COM IN INP)
    local matchOutputSuffix=(log out log LOG OUT LOG)
    local choices=${#allowedInputSuffix[*]}

    # Check if supplied inputfile is readable, extract suffix and title
    if is_readableFile "$1"; then
      jobname="${1%.*}"
      inputSuffix="${1##*.}"
      # Assign matching outputfile
      for ((count=0; count<choices; count++)); do
        if [[ "$inputSuffix" == "${allowedInputSuffix[$count]}" ]]; then
          outputSuffix="${matchOutputSuffix[$count]}"
          break 
        fi
      done
      # Abort when input-suffix cannot be identified
      if [ -z $outputSuffix ]; then
        fatal "Unrecognised suffix of inputfile \"$1\"."
      fi
    else
      # Check if only the jobtitle was given
      for ((count=0; count<choices; count++)); do
        if is_readableFile "$1.${allowedInputSuffix[$count]}"; then
          jobname="$1"
          inputSuffix="${allowedInputSuffix[$count]}"
          outputSuffix="${matchOutputSuffix[$count]}"
          break
        fi
      done
      # Abort if no suitable inputfile was found
      if [ -z $inputSuffix ]; then
        fatal "Unable to access \"$1\"."
      fi
    fi

    # Check if any auxiliar files exist that could prevent Gamess from executing
    # Also check if any auxiliar files exist in current directory
    # prevent overwriting
    exist_GamessAuxfiles "$jobname" 

    # Check if an outputfile exists and prevent overwriting
    exist_File "$jobname.$outputSuffix"

    # Display short logging message
    message "Will process Inputfile \"$jobname.$inputSuffix\"."
    message "Output will be written to \"$jobname.$outputSuffix\"."

}

validate_GamessFiles $submittedInputfile
\$\endgroup\$

1 Answer 1

2
\$\begingroup\$

Double-quote paths

In many places you correctly enclosed within double quotes the variables containing paths. But in many other places you didn't. You should always enclose these variables.

Naming

Some elements really need better names.

exist_File gives the impression that it checks if a file exists (and do nothing else). But the implementation is much more than that, it's more like making a backup. backupIfExists would be better.

You're using multiple naming schemes, which is unusual. Here are some common naming schemes:

  • camelCase
  • snake_case (all lowercase)

I suggest to adopt one of these and use consistently.

Lastly, it's customary to use plural names for collection types, so it's confusing here to see a plural name for a single value:

for possibleAuxfiles in "dat" "trj" "rst" "efp"; do

BTW, you didn't need to quote those extensions.

Readability

The indentation is a bit inconsistent. I suggest to indent using 4 spaces at every level.

In this code, the double quotes embedded within double quotes are a bit awkward:

  message "File \"$1\" exists."
  message "It has been moved to  \"$1.$filecount\"."
  mv $1 $1.$filecount

This is not a problem, but perhaps you might be interested in the idea of using single quotes instead:

  message "File '$1' exists."
  message "It has been moved to  '$1.$filecount'."
  mv $1 $1.$filecount

Another nitpick I have here is that the message is a bit of a lie, as the file wasn't really moved yet. I propose this alternative:

  message "File '$1' exists, will be moved"
  mv -v "$1" "$1.$filecount"

Thanks to the verbose flag, the command will print the rename, doing the work for you. I also added the recommended double quotes.

\$\endgroup\$
2
  • \$\begingroup\$ Thanks for those helpful comments. I thought about using mv -v before. How can I indent the output, so that it matches the previous INFO : statement? \$\endgroup\$ Commented Apr 1, 2016 at 6:31
  • \$\begingroup\$ @Martin-マーチン if you want to be pedantic about it (nothing wrong with that), then don't rely on the -v, use an extra message line as you did \$\endgroup\$
    – janos
    Commented Apr 1, 2016 at 6:37

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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