[asterisk-dev] [Code Review] Option for Read to be able to accept #

[Mark Michelson]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/2354/#review7938
-----------------------------------------------------------


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)


http://svn.asterisk.org/svn/asterisk/trunk/apps/app_read.c
<https://reviewboard.asterisk.org/r/2354/#comment15047>

    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.



http://svn.asterisk.org/svn/asterisk/trunk/apps/app_read.c
<https://reviewboard.asterisk.org/r/2354/#comment15048>

    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
<https://reviewboard.asterisk.org/r/2354/#comment15050>

    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.



http://svn.asterisk.org/svn/asterisk/trunk/apps/app_read.c
<https://reviewboard.asterisk.org/r/2354/#comment15049>

    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.


- Mark


On Feb. 25, 2013, 2:03 p.m., Birger Harzenetter wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/2354/
> -----------------------------------------------------------
> 
> (Updated Feb. 25, 2013, 2:03 p.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Summary
> -------
> 
> Option for Read to be able to accept #
> https://issues.asterisk.org/jira/browse/ASTERISK-18454
> 
> 
> This addresses bug ASTERISK-18454.
>     https://issues.asterisk.org/jira/browse/ASTERISK-18454
> 
> 
> Diffs
> -----
> 
>   http://svn.asterisk.org/svn/asterisk/trunk/apps/app_read.c 381515 
> 
> Diff: https://reviewboard.asterisk.org/r/2354/diff
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Birger
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20130225/69aa828b/attachment-0001.htm>