GPDB Bug Fixed
- Fixed bug in domain_ranges.sql script
- Finished Create site
- Added replica_name field
- Fixed naming error with snapto_dir and owning_group
- Expanded size of snap_notify to 1024 bytes
« November 19, 2006 - November 25, 2006 | Main | December 10, 2006 - December 16, 2006 »
By and large I feel that massively indented and thus multi level conditional statements are not handled well by most people. You see as your evaluating the code you have to "put on the mind stack" each condition and bear it in mind as you successively consider each of the nestings of code. As such I always "look for ways out" of the script/function/routine that I'm in. The idea is to avoid nesting by handling error conditions and other "if this happens then we're done" first, often die'ing, erroring out, returning the script/function or subroutine. The net effect is that the rest of the code, the code that normally executes, is shifted to the left a level or two. Additionally complicated, repeated or complex code can be written into a well named subroutine and called from the main or other functions when needed. To me this all makes the code a lot easier to read.
As an example, and again, I'm not trying to pick on your script specifically and I realize that this might not be the final version, but in reality it contains only a call to GetOptions and 2 if statements. The first if statement merely calls usage if -h is specified. BTW this could be re-written as:
GetOptions (
'f=s', \$facility,
'p=i', \$port,
'a=s', \$XAID,
'w=s', \$password,
'n=s', \$projName,
'g=s', \$unixGroup,
's=s', \$serverName,
'c=s', \$cachePath,
'h', sub { usage },
) || usage;
IOW rather than define an $h variable and then set it via GetOptions then test it afterwards only to call usage, define an anonymous subroutine can calls usage directly from GetOptions. Or, since anything other than the stated options displays usage (as per the "|| usage") simply drop the "h" line entirely. If the user specifies -h then it's unknown and usage is displayed. Then $h can be removed as well as the if test following GetOptions.
The next if statement tests that all of the above variables have been defined. If so then all real processing happens inside that if (and other nested if's therein). Otherwise usage is called. In following with the "looking for a reason to get out" philosophy how about:
usage unless ( defined $password && defined $XAID && defined $projName && defined $serverName && defined $unixGroup && defined $cachePath && defined $port && defined $facility );
Basically this says, "if any of these are not defined call usage" and since usage doesn't return everything inside the old if statement can be shifted to the left. Other opportunities exist to continue to apply this philosophy of "getting out while you can" and reducing the nesting level of the program. For example, inside the old if statement you check to see if the login of GPDB successfully made you an administrator. If not you're gonna stop the script right there. So then how about something like:
my $login = gpdb_login ($XAID, $password); error "$XAID is not an administrator", 1 if $login !~ /administrator/;
gpdb_login returns a string that will contain "administrator" if you were able to login as an administrator. Here I use error, a routine that I often use (in fact I'd like to publish it to TI as a common utility) that writes an error message to STDERR and optionally exits if the second parameter is not 0.