[asterisk-dev] [Code Review] [confbridge] Behavioural correction for hold-music state when users join/part conferences in varying combinations

[Matt Jordan]

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



/trunk/apps/app_confbridge.c
<https://reviewboard.asterisk.org/r/1869/#comment11549>

    There's no sense in returning a failure result if things that use this method aren't going to check it.
    
    Since a failure to stop MOH isn't a reason to cancel most subsequent actions, I'd change this method to return a void.  You should probably instead log a WARNING if the user cannot be suspended from the bridge.



/trunk/apps/app_confbridge.c
<https://reviewboard.asterisk.org/r/1869/#comment11547>

    This should probably be:
    if (conference_bridge->users > 1)
    
    There's no reason you wouldn't turn off MOH if there were three people in the room.



/trunk/apps/app_confbridge.c
<https://reviewboard.asterisk.org/r/1869/#comment11548>

    All three of these if statements can be combined to reduce indentation



/trunk/apps/app_confbridge.c
<https://reviewboard.asterisk.org/r/1869/#comment11551>

    These actions are repeated a lot throughout the code.  At this point, it would probably be a good idea to make a small method that suspends a channel from the bridge, starts/stops MOH on the channel, and unsuspends them back.



/trunk/apps/app_confbridge.c
<https://reviewboard.asterisk.org/r/1869/#comment11550>

    Don't put these on separate lines.



/trunk/apps/app_confbridge.c
<https://reviewboard.asterisk.org/r/1869/#comment11552>

    Don't put brackets on a line by themselves



/trunk/apps/app_confbridge.c
<https://reviewboard.asterisk.org/r/1869/#comment11554>

    We don't need the number of other participants here, we just need to know whether or not we're alone in the conference.  If we're alone, we should consider starting MOH - otherwise, we shouldn't start it.



/trunk/apps/app_confbridge.c
<https://reviewboard.asterisk.org/r/1869/#comment11555>

    The same finding applies here.



/trunk/apps/app_confbridge.c
<https://reviewboard.asterisk.org/r/1869/#comment11556>

    1. A number of these if statements can be combined.  Having multiples doesn't necessarily add to the clarity
    2. I'm not sure why nonwaiting_participants would be >= 2 here.  If there are any participants that are not marked users, and no marked users exist, we probably need to start MOH on them.
    3. A number of comments in this block serve no purpose in clarifying the intent of the code, e.g., "All conditions needed to enable speech are satisfied"


- Matt


On May 9, 2012, 4:35 p.m., Neil Tallim wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/1869/
> -----------------------------------------------------------
> 
> (Updated May 9, 2012, 4:35 p.m.)
> 
> 
> Review request for Asterisk Developers, David Vossel and jcolp.
> 
> 
> Summary
> -------
> 
> This is the "everything" patch attached to issue https://issues.asterisk.org/jira/browse/ASTERISK-19562
> 
> Details about what it does may be found in the issue's description.
> 
> 
> Diffs
> -----
> 
>   /trunk/apps/app_confbridge.c 359977 
> 
> Diff: https://reviewboard.asterisk.org/r/1869/diff
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Neil
> 
>

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