> I incorporated the patch that adds the ErrorDescriptor
> mechanism to the RedirectorPlugin interface, along with a few
> changes [1].
Thanks
>
> A couple of substantive comments:
>
> * Some of the ErrorDescriptor methods have checks for invalid
> parameters, and return errors if they are not valid, but they
> don't log anything. Return codes alone are pretty useless. I
> think that an out-of-range error for parameters like
> these that
> are hard-coded in the caller is a CRIT level log (it would be
> lower if, for example the parameter was something that was
> expected to be from a SIP message or other external input).
> bool ErrorDescriptor::setStatusLineData(
> const int statusCode, const UtlString& reasonPhrase )
> {
> bool result = false;
> if( statusCode >= SIP_4XX_CLASS_CODE &&
> statusCode < SIP_7XX_CLASS_CODE )
> {
> mStatusCode = statusCode;
> mReasonPhrase = reasonPhrase;
> result = true;
> }
> <<<<<<<<<<<<<<<<<<<<<<<<< need an 'else' to do some
> logging here.
> return result;
> }
I'll fix that and provide you with another diff sometime today.
>
> * I don't understand why we need the dontAppendRequestToResponse
> method - as the comment notes, it has no effect unless you've
> already called appendRequestToResponse, and since only one
> error should ever be returned I don't see why that would ever
> happen. Unless you can come up with such a scenario, I'd say
> take the method out.
>
Usually, when I provide a method to set something, I try to provide the
'unset' counterpart but I do not have a use case in mind that would
illustrate a specific scenario where such a method would be useful,
nonetheless I would like to keep it around for symmetry and
completeness.
> A few style comments:
...
>
> * I much prefer documenting method parameters using individual
> comments after the parameter itself in the code to the way you
> did it (using @param elsewhere in the comments):
>
> I think that's easier to read and maintain in the C++.
Personally, I hate that way of doing comments. They often fall off the
screen if you have long variable names; for lengthy comments, they
create that narrow column of text that is hard to read and you have to
clean them out after a copy-paste if you do not want to carry the
comments in your code. Who do I have to bribe to get both methods
accepted as part of the coding guidelines :)
_______________________________________________
sipx-dev mailing list
[email protected]
List Archive: http://list.sipfoundry.org/archive/sipx-dev
Unsubscribe: http://list.sipfoundry.org/mailman/listinfo/sipx-dev
