You are viewing a plain text version of this content. The canonical link for it is here.
Posted to java-dev@axis.apache.org by Eran Chinthaka <ch...@opensource.lk> on 2005/03/31 04:50:15 UTC
[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.
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);
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.
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.
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 ?
I wonder whether I can do that as its under the name jaya_ashu_vankat
scratch ;) ;)
Regards,
Eran Chinthaka
RE: [Axis2] Some comments on SAAJ toy
Posted by Eran Chinthaka <ch...@opensource.lk>.
Jaya, got you. That's the same reason why I wanted to help you. (Anyway
Venkat is next to me, and he wanted me to comment ;) )
Some more comments ..
Global Comments :
. I saw in lot of places, not only in SOAPElementImpl, that you guys
have used new OMElementImpl(). Shall we all stick to OMFactory methods to
create new OM objects. This is sort of a good practice as its giving
flexibility to change OM impls smoothly.
. There were some explicit cast which is not required to do. For
example DetailEntryImpl implements DetailsEntry and there were casting to
DetailEntry from DetailElementEntry when returning in methods.
Anyway, Can I help you by implementing some stuff in your scratch area, if
time permits ?
-- Chinthaka
> -----Original Message-----
> From: jayachandra [mailto:jayachandra@gmail.com]
> Sent: Thursday, March 31, 2005 10:45 AM
> To: axis-dev@ws.apache.org; chinthaka@opensource.lk
> Subject: Re: [Axis2] Some comments on SAAJ toy
>
> Thanks Eran for the inputs. Actually, this effort was more in the
> direction of creating a proof of concept trail. So, many methods were
> deliberately left uncoded or partially coded. So far, what is present
> is some minimal code that's required for the test case to pass and
> thereby convince ppl of this idea of SAAJ wrapping. Nevertheless, the
> (SOAPElement) typecasting and other such things can be cleaned up. And
> if the team gets a clean chit to go ahead and make this a
> comprehensive enough implementation we can all work together and make
> it as solid as possible :)
>
> Jaya
>
> On Thu, 31 Mar 2005 08:50:15 +0600, Eran Chinthaka
> <ch...@opensource.lk> wrote:
> >
> >
> >
> >
> > Hi Jaya, Ashu and Venkat,
> >
> >
> >
> > I went through the code that you have put in here are some of my
> comments.
> >
> >
> > 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.
> > 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);
> >
> >
> > All the addChildElement methods have explicit type cast to SOAPElement
> from
> > SOAPElementImpl, which IMHO is not required.
> > 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.
> > 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.
> >
> >
> >
> >
> >
> > 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 ?
> >
> >
> >
> > I wonder whether I can do that as its under the name jaya_ashu_vankat
> > scratch ;) ;)
> >
> >
> >
> >
> >
> > Regards,
> >
> > Eran Chinthaka
>
>
> --
> -- Jaya
Re: [Axis2] Some comments on SAAJ toy
Posted by jayachandra <ja...@gmail.com>.
Thanks Eran for the inputs. Actually, this effort was more in the
direction of creating a proof of concept trail. So, many methods were
deliberately left uncoded or partially coded. So far, what is present
is some minimal code that's required for the test case to pass and
thereby convince ppl of this idea of SAAJ wrapping. Nevertheless, the
(SOAPElement) typecasting and other such things can be cleaned up. And
if the team gets a clean chit to go ahead and make this a
comprehensive enough implementation we can all work together and make
it as solid as possible :)
Jaya
On Thu, 31 Mar 2005 08:50:15 +0600, Eran Chinthaka
<ch...@opensource.lk> wrote:
>
>
>
>
> Hi Jaya, Ashu and Venkat,
>
>
>
> I went through the code that you have put in here are some of my comments.
>
>
> 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.
> 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);
>
>
> All the addChildElement methods have explicit type cast to SOAPElement from
> SOAPElementImpl, which IMHO is not required.
> 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.
> 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.
>
>
>
>
>
> 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 ?
>
>
>
> I wonder whether I can do that as its under the name jaya_ashu_vankat
> scratch ;) ;)
>
>
>
>
>
> Regards,
>
> Eran Chinthaka
--
-- Jaya