[
https://issues.apache.org/jira/browse/SYNAPSE-478?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12645201#action_12645201
]
Asankha C. Perera commented on SYNAPSE-478:
-------------------------------------------
Hi Irantha
Since this is your first patch, let me point you to:
http://cwiki.apache.org/synapse/developer-guidelines.html
Some initial feedback on coding style and logic:
1) Use proper Javadocs : "A JMS header field names need to confirm to java
variable naming specs. "Content-Type" is illigel"
2) Leave spaces between "try {", "catch (Exception ex){", "if(contentType.."
etc.. also we keep the catch block on the same line as the "}" which closes the
try block
+ try{
+ contentType = JMSUtils.getProperty(message,
BaseConstants.CONTENT_TYPE);
+ if(contentType == null)
+ contentType = JMSUtils.getProperty(message,
JMSConstants.JMS_CONTENT_TYPE);
+ }
+ catch(Exception ex){
+ contentType = JMSUtils.getProperty(message,
JMSConstants.JMS_CONTENT_TYPE);
+ }
3) w.r.t. above, the try block will not throw an exception - since the method
JMSUtils.getProperty() returns null hiding a possible JMSException
4) Anyway, generally catch only what you handle.. w.r.t. the above, catching
"Exception" should not be performed, if the block only would through a
JMSException (this is not the case here - but is a general comment)
+ OMOutputFormat format = BaseUtils.getOMOutputFormat(msgContext);
+ MessageFormatter messageFormatter = null;
+ try {
+ messageFormatter = TransportUtils.getMessageFormatter(msgContext);
+ } catch (AxisFault axisFault) {
+ throw new JMSException("Unable to get the message formatter to
use");
+ }
5) Why would you catch an AxisFault, and throw a JMSException? The JMSException
is a from javax.jms package, and we as an application should not be throwing
JMSExceptions for application level faults
- getFirstChildWithName(BaseConstants.DEFAULT_BINARY_WRAPPER);
+
getFirstChildWithName(BaseConstants.DEFAULT_BINARY_WRAPPER);
- BaseConstants.DEFAULT_BINARY_WRAPPER, e);
+ BaseConstants.DEFAULT_BINARY_WRAPPER, e);
6) Check if your IDE is set to use 4 spaces instead of tab character - also,
for continuation, we use 4 tabs in general. At the same time, ensure that your
IDE does not change untouched lines or the line break for files (i.e. turn off
stip trailing spaces option)
+ try{
+ message.setStringProperty(BaseConstants.CONTENT_TYPE, contentType);
+ }
+ catch(Exception ex){
+ message.setStringProperty(JMSConstants.JMS_CONTENT_TYPE,
contentType);
+ }
7) Consider comment #2 and #4
+ try{
...
+ }
+ catch(Exception ignore){}
8) Comment # 2
asankha
> Synapse exceptions with JBossMQ
> -------------------------------
>
> Key: SYNAPSE-478
> URL: https://issues.apache.org/jira/browse/SYNAPSE-478
> Project: Synapse
> Issue Type: Bug
> Components: Transports
> Affects Versions: 1.2
> Environment: JBoss 4.2.3GA
> Reporter: irantha suwandarathna
> Assignee: Andreas Veithen
> Attachments: commons_jms.patch
>
>
> JBossMQ SpyMessage (javax.jms.Message instance) throw exceptions for (
> JMSXDeliveryCount, JMS_DESTINATION, JMS_JBOSS_REDELIVERY_COUNT,
> JMS_REDELIVERED ) properties set inside JMSUtils.setTransportHeaders method.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
---------------------------------------------------------------------
To unsubscribe, e-mail: [EMAIL PROTECTED]
For additional commands, e-mail: [EMAIL PROTECTED]
