Checking and converting [[HH:]MM:]SS input format

Question

I would like to use the following routine in my job submission bash script which expects the allowed walltime in the format [[HH:]MM:]SS. (Brackets indicating optionality.) For easier readability I want to convert any input value into the fixed format HH:MM:SS where the value for hours is open ended.
I think it works okay, but I am a bit worried if I have overlooked possible points where a user could make a mistake. This could lead to submitting an invalid jobscript and a lot of computation jobs not starting.
I also ask myself whether or not my code is optimal.

#!/bin/bash
#
# If there is an unrecoverable error: display a message and exit.
#
printExit ()
{
 case 1ドル in
 [iI]) echo INFO: "2ドル" ;;
 [wW]) echo WARNING: "2ドル" ;;
 [eE]) echo ERROR: "2ドル" ; exit 1 ;;
 *) echo "1ドル" ;;
 esac
}
#
# Test if a given value is an integer
#
isnumeric ()
{
 if ! expr "1ドル" : '[[:digit:]]*$' > /dev/null; then
 printExit E "Value for 2ドル (1ドル) is no integer."
 fi
}
#
# Test if a given value is a proper duration
#
istime ()
{
 local tempTime=1ドル
 # Split time in HH:MM:SS
 theTempSeconds=${tempTime##*:}
 isnumeric "$theTempSeconds" "seconds"
 if [[ ! "$tempTime" == "${tempTime%:*}" ]]; then
 tempTime="${tempTime%:*}"
 theTempMinutes=${tempTime##*:}
 isnumeric "$theTempMinutes" "minutes"
 fi
 if [[ ! "$tempTime" == "${tempTime%:*}" ]]; then
 tempTime="${tempTime%:*}"
 theTempHours=${tempTime##*:}
 isnumeric "$theTempHours" "hours"
 fi
 if [[ ! "$tempTime" == "${tempTime%:*}" ]]; then
 printExit E "Unrecognised format."
 fi
 theSeconds=`expr $theTempSeconds % 60`
 theTempMinutes=`expr $theTempMinutes + $theTempSeconds / 60`
 theMinutes=`expr $theTempMinutes % 60`
 theHours=`expr $theTempHours + $theTempMinutes / 60`
 printf -v theWalltime "%d:%02d:%02d" $theHours $theMinutes $theSeconds
}
theWalltime="1ドル"
echo "$theWalltime"
istime $theWalltime
echo "$theWalltime"
exit 0

I use the first two functions quite a lot in my script so I used them for the target function also.

Answer 1

Here are my suggestions:

• The printExit() prints an error message, but does not exit. It would be better to actually exit or rename the function.

• Your regex in isnumeric() has two minor issues. Putting a ^ at the beginning will make it clear that the regex matches the whole string. While this is implicit with expr it does not fit with how most implementations work. Another issue is that the * quantifier means 0 or more so it might match 0 or none of the what you're looking for. So an empty string would match what you have since it would be zero digits followed by the end of the string. I couldn't reproduce this failure mode, but I'd recommend this line instead which works for my tests:

  if ! expr "1ドル" : '^[[:digit:]]\+$' > /dev/null; then
  

• It would be nice to explain what you're trying to do with those string manipulations with a few comments
• More of your variables could be localized so they don't leak out of the function they are used in.
• Using the in variable names to avoid conflicting with generic names works, but if you can find more descriptive names it would be easier to follow. Along the same lines all variables are temporary so that doesn't help explain why the variable exists. Maybe checkTime or even check_time.
• All of the math is integer math. You won't get any decimals with bash or expr math. https://stackoverflow.com/questions/12722095/how-can-i-get-float-division explains that you can work around with bc.
• Optimization Using expr is fine, but it calls an external program. Using bash's builtin math would avoid spawning a new process and would cut out a lot of the script's time. ((just_minutes = just_minutes + trunc_seconds/60))

• Martin - マーチン: Thanks for the comments, I'll put them to good use. The printExit() name is a remnant from a time where every case it was used for actually exited the program. By the time I worked the other cases in, I used it so often, I did not want to change it any more. With the third bullet point you just mean that I should be more talkative in the code, explaining what I do?

Answer 2

I have updated the code with some of the suggestions from chicks answer.

• while the prinExit() routine was initially intended to actually exit in any scenario given, it transmuted over the course of a few iterations in other scripts into a somewhat more advanced info routine. I have now changed the name to reflect that and will slowly roll this out in all of my scripts in the future
• the regular expression now matches the whole expression and cannot be zero
• the variable names reflect now what they are actually doing and they were set as local ones wherever possible to prevent leaking (which could happen if the user applies the duration option twice)
• the transformation is now done by the bash built in $(()), it is integer maths as is the intention
• probably the biggest improvement in optimising the code is nesting the if statements while checking the input statement. If the duration was given in SS it is now only checking once if [[ ! "$foo" == "${foo%:*}" ]] and the same applies to minutes MM:SS (2) and hours HH:MM:SS (4, as it needs checking if there are leftovers).

---

Here is the complete code (when the whole script is done I'll make it available through a third party website):

#!/bin/bash 
#
# Print information and warnings.
# If there is an unrecoverable error: display a message and exit.
#
printInfo ()
{
 case 1ドル in
 [iI]) echo INFO: "2ドル" ;;
 [wW]) echo WARNING: "2ドル" ;;
 [eE]) echo ERROR: "2ドル" ; exit 1 ;;
 *) echo "1ドル" ;;
 esac
}
#
# Test if a given value is an integer
#
isnumeric ()
{
 if ! expr "1ドル" : '^[[:digit:]]\+$' > /dev/null; then
 printInfo E "Value for 2ドル (1ドル) is no integer."
 fi
}
#
# Test if a given value is a proper duration
# Code reviewed: http://codereview.stackexchange.com/q/114144/92423
#
isduration ()
{
 local checkDuration=1ドル
 # Split time in HH:MM:SS
 # Strips away anything up to and including the rightmost colon
 # strips nothing if no colon present
 # and tests if the value is numeric
 # this is assigned to seconds
 local truncDuration_Seconds=${checkDuration##*:}
 isnumeric "$truncDuration_Seconds" "seconds"
 # If successful value is stored for later assembly
 #
 # Check if the value is given in seconds 
 # "${checkDuration%:*}" strips shortest match ":*" from back
 # If no colon is present, the strings are identical
 if [[ ! "$checkDuration" == "${checkDuration%:*}" ]]; then
 # Strip seconds and colon 
 checkDuration="${checkDuration%:*}"
 # Strips away anything up to and including the rightmost colon
 # this is assigned as minutes
 # and tests if the value is numeric
 local truncDuration_Minutes=${checkDuration##*:}
 isnumeric "$truncDuration_Minutes" "minutes"
 # If successful value is stored for later assembly
 #
 # Check if value was given as MM:SS same procedure as above
 if [[ ! "$checkDuration" == "${checkDuration%:*}" ]]; then
 #Strip minutes and colon
 checkDuration="${checkDuration%:*}"
 # # Strips away anything up to and including the rightmost colon
 # this is assigned as hours
 # and tests if the value is numeric
 local truncDuration_Hours=${checkDuration##*:}
 isnumeric "$truncDuration_Hours" "hours"
 # Check if value was given as HH:MM:SS if not, then exit
 if [[ ! "$checkDuration" == "${checkDuration%:*}" ]]; then
 printInfo E "Unrecognised format."
 fi
 fi
 fi
 # Modify the duration to have the format HH:MM:SS 
 # disregarding the format of the user input
 local finalDuration_Seconds=$((truncDuration_Seconds % 60))
 # Add any minutes that had overflow to the minutes given as input
 truncDuration_Minutes=$((truncDuration_Minutes + truncDuration_Seconds / 60))
 # save what cannot overflow as hours
 local finalDuration_Minutes=$((truncDuration_Minutes % 60))
 # add any minutes the overflew to hours
 local finalDuration_Hours=$((truncDuration_Hours + truncDuration_Minutes / 60))
 # Format string and saveon variable
 printf -v requestedWalltime "%d:%02d:%02d" $finalDuration_Hours $finalDuration_Minutes \
 $finalDuration_Seconds
}
requestedWalltime="1ドル"
echo "$requestedWalltime"
isduration $requestedWalltime
echo "$requestedWalltime"
exit 0

Answer 3

An alternative to printExit

As @chicks already pointed out, the printExit is not a good name, as it doesn't always exit, and it's also not obvious what a "print exit" method will do. A related point, I find the i-w-e parameters a bit cryptic. I recommend an alternative I use for this purpose:

msg() {
 echo $*
}
warn() {
 msg WARNING: $*
}
fatal() {
 msg FATAL: $*
 exit 1
}

That is:

• warn "some message" is more natural than printExit W "some message"
• warn some message also works (without double quotes)
• If I want to search for all occurrences of warnings in the code, it's a bit easier to type "warn" than "printExit [wW]". (Actually, since I use vim to edit scripts, I can just press * on the word "warn" to find all occurrences, so that becomes literally a single keystroke.)

More on naming things

Functions prefixed with the word "is" imply some boolean operation, returning true or false. In case of Bash, returning success or failure. As opposed to that:

• the isnumeric function exits the program if the param is not numeric
• the istime function may lead to exiting in calls to isnumeric, or else if everything was fine it prints time

It would be better to rename them. For example, validate_numeric would be a better name, as the "validate" prefix is commonly used for operations that raise exception if the required tests fail.

Lastly, it's good to emphasize visually the word boundaries of the distinct terms in function names, for example is_numeric or isNumeric.

Improving isnumeric

I suggest making is_numeric a real boolean function, and introduce a new function validate_numeric to exit in case of not numeric:

is_numeric() {
 expr "1ドル" : '[[:digit:]]*$' > /dev/null
}
validate_numeric() {
 if ! is_numeric "1ドル"; then
 fatal "Value for 2ドル (1ドル) is no integer."
 fi
}

Improving is_numeric

It's not recommended to use expr anymore today. It spawns an extra process, it's not portable, and better alternatives are usually available.

In this example, you can rewrite using [[ ... =~ ... ]] like this:

is_numeric() {
 [[ 1ドル =~ ^[[:digit:]]+$ ]]
}

Nitpicks

The final exit 0 is unnecessary.

• Martin - マーチン: is there anything that would be an argument against using error instead of fatal as the name for the function? I feel that it would probably come more natural to call it error.
• janos: @Martin-マーチン This may be subjective. To me, fatal clearly implies that the program will exit. I can imagine an implementation of error that just prints a message on strderr but doesn't necessarily exit.