axis-java-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Shahi, Ashutosh" <Ashutosh.Sh...@ca.com>
Subject RE: [Axis2] Some comments on SAAJ toy
Date Thu, 31 Mar 2005 05:59:36 GMT
See comments Inline

  _____  

From: Eran Chinthaka [mailto:chinthaka@opensource.lk] 
Sent: Thursday, March 31, 2005 8:20 AM
To: axis-dev@ws.apache.org
Subject: [Axis2] Some comments on SAAJ toy

 

 

Hi Jaya, Ashu and Venkat,

 

I went through the code that you have put in here are some of my
comments.

 

1.	its better not to use new OMElementImpl() sort of things within
the code. We have introduced OMFactory to create objects, simply to
smooth switching of OM implementations. Mind you that OMElementImpl() is
specific to linked list model only. So its better if you can use
omFactory.createOMElement() methods. 

[Ashu] We were thinking along the lines of using omFactory with the
MessageFactory in SOAP. Since we haven't implemented MessageFactory yet,
we hardcoded impl classes. But I think it's a good suggestion and I'll
make these changes

 

2.	SOAPElementImpl.java  addChildElement(Name) method.

I think code should be changed a bit. 

 

org.apache.axis.om.OMElement newOMElement = new
org.apache.axis.om.impl.llom.OMElementImpl(new QName(name.getURI(),
name.getLocalName(), name.getPrefix()), omElement);

omElement.addChild(newOMElement); 

return (SOAPElement)(new SOAPElementImpl(newOMElement));

 

Better be changed to 

 

org.apache.axis.om.OMElement newOMElement =
OMFactory.newInstance().createOMElement(new QName(name.getURI(),
name.getLocalName(), name.getPrefix()), omElement);

omElement.addChild(newOMElement); 

return new SOAPElementImpl(newOMElement);

 

[Ashu] Will do; this is an example of 1 I guess

 

3.	All the addChildElement methods have explicit type cast to
SOAPElement from SOAPElementImpl, which IMHO is not required.
4.	removeContents has a comment with a question mark. Well what you
can do is get the children of the current node and call the detach
method of all of them. 

[Ashu] We had left empty implementation for many methods in the
SOAPElement class on which we had questions (as it was too DOM
specific), removeContents included; and thought will come back to them
once we get feedback and suggestions on how to implement. 

5.	the getVisibleNamespacePrefixes() currently returns declared
namespaces only. But according to the javadoc found on the SAAJ
interfaces, this should return all the "visible" namespaces. So better
get the namespace of the parents as well.

[Ashu]  Agreed. Apart from this I have a question. The OMNamespace used
in OM doesn't have a concept of LocalName. Also omElement allows to
declare the namespace (more than 1) in the context of current element
and stores them in a hashmap, but I could not see a way to find out
which one was used to create the current node and which others are just
declared in current context. Because of this could not implement methods
like getNamespaceURI(), getPrefix() in NodeImpl class. Any thoughts on
this?

 

Sorry I didn't have enough time go through all the code, due to the
summit. I went through only the SOAPELement class. Let me have some more
time to go through this again.  

Anyway, can I also write some code in to your effort ? 

 

Sure, you are most welcome to help us with code :). I'll also make some
of the changes suggested by you and post them.

 

 I wonder whether I can do that as its under the name jaya_ashu_vankat
scratch ;) ;) 

 

 

Regards,

Eran Chinthaka


Mime
View raw message