Review Board 1.7.16


Option for Read to be able to accept #

Review Request #2354 - Created Feb. 23, 2013 and updated

Birger Harzenetter
11/trunk
ASTERISK-18454
Reviewers
asterisk-dev
Asterisk
Option for Read to be able to accept #
https://issues.asterisk.org/jira/browse/ASTERISK-18454

 
Review request changed
Updated (Feb. 25, 2013, 8:03 a.m.)
changes as pointed out by Sean Bright
Posted (Feb. 25, 2013, 8:08 a.m.)
In addition to what I have pointed out, there are some coding guidelines violations here. The most common ones are that spaces are missing around operators and there are braces missing for one-line if statements. I know that you were just copying and pasting from other portions of app_read, but those were written before certain coding guidelines had been put in place (Read is a very old application)
I don't like the comparison of to and maxdigits to their initial values here. A better comparison would be:

if (ast_test_flag(&flags, OPT_HASH) && (ast_strlen_zero(arglist.timeout) && ast_strlen_zero(arglist.maxdigits)))

This way, you're explicitly checking that the user provided an argument instead of checking for a change from the initial value. This is good for two reasons:

1) If, for whatever reason, the initial values change, there will be no need to change the if statement any
2) More bizarrely, if a user were to specify maxdigits as 255, then we not would mysteriously claim invalid input was provided.
Putting the ast_test_flag() statement in the while loop test is odd because it does not change on each round of the loop. You should put it in an outer if statement and only run the while loop if the flag tests true.
http://svn.asterisk.org/svn/asterisk/trunk/apps/app_read.c (Diff revision 1)
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
This loop is more-or-less the same as the loop above that is used if a tonezone sound is played. You should factor the logic into a function of its own.
It is impossible for this if statement to evaluate true. The reason is that if the code has reached this point, it is guaranteed that the OPT_HASH flag is set. You can eliminate this if statement and its body entirely.

Of course, if you take the advice of factoring this loop out into its own function, then you'll have to keep this if statement in so that it can remain generic.

https://reviewboard.asterisk.org/ runs on a server provided by Digium, Inc. and uses bandwidth donated to the open source Asterisk community by API Digital Communications in Huntsville, AL USA.
Please report problems with this site to asteriskteam@digium.com.