Sorry that reviewing this has taken so long.
This looks much better, and is very very close. There is one immediate
problem - the unit test fails (which is probably the result of changes
that took place in the interim). It would be useful if you would add
some calls to log the messages in the unit tests after each
transformation; it makes debugging the tests easier.
The other thing I'd like you to improve is that in a number of places
you've added a parameter for the headerName - for example:
/// Constructor which decodes SipXauthIdentity or P-Asserted-Identity from a
received message.
SipXauthIdentity(const SipMessage& message, ///< message to scan for an
identity header
const char* headerName, /**<headerName for the identity,
* either SipXauthIdentity or
P-Asserted-Identity
*/
DialogRule bindRule = requireDialogBinding
);
The addition itself is fine, but I'd like to see it constrained to
actually only be valid to pass one of the two values that you actually
use:
static const char* AuthIdentityHeaderName;
static const char* PAssertedIdentityHeaderName;
There are two reasons for this suggestion - one is that it documents
what is really expected here (an arbitrary header is not going to do
anything useful and may even actually be harmful), but also the
implementation relies on the fact that the pointers match - so even a
pointer to a different character string with the same value would not
work, and that would be hard to debug.
I'd suggest defining a type for these parameters (inside the class but
public):
typedef const char* HeaderName;
and then change the constant definitions to use that type:
static HeaderName AuthIdentityHeaderName;
static HeaderName PAssertedIdentityHeaderName;
and the parameter definitions to use it:
/// Constructor which decodes SipXauthIdentity or P-Asserted-Identity from a
received message.
SipXauthIdentity(const SipMessage& message, ///< message to scan for an
identity header
HeaderName headerName, /**< which header to parse from
the message
*/
DialogRule bindRule = requireDialogBinding
);
This will mean that the references outside the class will need to be
qualified by the class name, but that too is good documentation:
SipXauthIdentity::AuthIdentityHeaderName
One other nit - please check that all the code and comments fit in a
100-column wide window per the coding standard.
http://sipxecs.sipfoundry.org/doc/coding-standard.html#ref3.1.1
If you'll take another crack at this, I promise a quick turnaround on
the next version.
_______________________________________________
sipx-dev mailing list
[email protected]
List Archive: http://list.sipfoundry.org/archive/sipx-dev
Unsubscribe: http://list.sipfoundry.org/mailman/listinfo/sipx-dev
