----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/706/#review654 -----------------------------------------------------------
http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQAnyDestination.java <https://reviews.apache.org/r/706/#comment1312> Not a major issue, but this seems a bit convoluted to me as compared with the more direct: if (getRoutingKey() != null) { return getRoutingKey().toString(); } else if (getSubject() != null) { return getSubject(); } else { return ""; } Also why use 'super' here? Finally, is returning the empty string the right default? Might it be better to simply throw an exception? What does it mean if the topic has a null routing key (and null subject)? http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQSession.java <https://reviews.apache.org/r/706/#comment1315> What happens here if the address doesn't resolve to an existing exchange? http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQSession.java <https://reviews.apache.org/r/706/#comment1313> As above, is it possible the routing key is null? If so does that matter here? http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQTopic.java <https://reviews.apache.org/r/706/#comment1314> Same comments about what seems to be to be convoluted logic and possibly a default where an exception would be clearer... - Gordon On 2011-05-10 03:38:41, rajith attapattu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/706/ > ----------------------------------------------------------- > > (Updated 2011-05-10 03:38:41) > > > Review request for qpid. > > > Summary > ------- > > The attached patch adds code to resolve the address to determine the correct > defualt for the subject field and also to populate the legacy fields. > Also added null checks in both AMQAnyDestination and AMQTopic to prevent any > NPE. > > > Diffs > ----- > > > http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQAnyDestination.java > 1099057 > > http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQSession.java > 1099288 > > http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQTopic.java > 1099057 > > Diff: https://reviews.apache.org/r/706/diff > > > Testing > ------- > > > Thanks, > > rajith > >
