Re: [Springframework-developer] I don't like the DefaultXmlBeanDefinitionParser

SourceForge Headquarters 1320 Columbia Street Suite 310 San Diego, CA 92101 +1 (858) 422-6466

Still having a strange day? ;-)

I'm not sure what you mean with "interface belongs to the client", BTW:
Spring uses interfaces at all levels, both for client APIs and for internal
strategies. That seems perfectly feasible to me.

Anyway, regarding DefaultXmlBeanDefinitionParser's implementation, in
particular the use of the DOM API: As Erwin correctly assumed, the main
reason is simply that DOM is part of the JDK. A special XML parser with a
nicer API (like JDOM or DOM4J) would introduce a new required dependency -
even when not using XML bean definitions directly (for example, JdbcTemplate
uses a default SQLErrorCodesFactory that reads internal XML bean definitions
from sql-error-codes.xml).

Admittedly, DefaultXmlBeanDefinitionParser has not really been designed for
extensibility; there is certainly potential for making it more convenient to
subclass. However, I strongly object to introducing a third-party XML
parsing API: the library dependencies of Spring's core have to remain
minimal. This only leaves DOM and SAX, as included in the JDK.

Juergen

  -----Original Message-----
  From: spr...@li...
[mailto:spr...@li...]On Behalf Of
Martin Kersten
  Sent: Tuesday, February 22, 2005 1:22 PM
  To: spr...@li...
  Subject: [Springframework-developer] I don't like the
DefaultXmlBeanDefinitionParser

  Hi folks,

     I am currently trying to extend the framework by supporting
contributions.
  Just to see how it feels.

  So I made some investigations in the sourcecode. I don't want to start a
war
  about proper design rules, since I am a believer in 'Interface belongs to
the
  client' stuff and you are appearently not, but this isn't the issue I want
to
  talk about.

  Th implementation I hate most on first sight is the
  XMLBeanDefinitionParser. I know it does what it should but you can read
this:

   /**
    * Make the horrible DOM API slightly more bearable:
    * get the text value we know this element contains.
    */

  Well I would agree but it's a bit wired also. You think the DOM API is
horrible
  and you are still using it? You know what it means to use a
  horrible API? You write a horrible implementation! And thats how it looks.
  It took me more then a gaze to catch the meaning of the parser and I also
  got blown by the code duplication. Since I am in need to extend this
class,

  So I would like to ask if I may refactor it and commit you a patch (or
maybe
  a complete reimplementation)?

  Cheers,

  Martin (Kersten)

  PS: By the way, how about 'Hidding 3rd party library behind single
interface?'
    ----- Original Message -----
    From: Martin Kersten
    To: spr...@li...
    Sent: Tuesday, February 22, 2005 12:46 PM
    Subject: Re: [Springframework-developer] Please check these two things

    Sorry, thought the agreement goes with the callee.
    Ok :-) sorry was a strange day for me, I guess.

    Thanks,

    Martin (Kersten)
    ----- Original Message -----
      From: Juergen Hoeller
      To: spr...@li...
      Sent: Tuesday, February 22, 2005 12:13 PM
      Subject: Re: [Springframework-developer] Please check these two things

      Actually, I have *not* replaced this with a == comparison of the
arrays: Instead, BatchSqlUpdate is storing clones of the passed-in arrays
now, for execution on flush. This avoids any side effects in the first place
(even if the passed-in arrays are changed afterwards or reused for multiple
update inovcations), and the overhead of cloning an array should be
acceptable (after all, we're talking about database update operations here).

      Juergen

        -----Original Message-----
        From: spr...@li...
[mailto:spr...@li...]On Behalf Of
Martin Kersten
        Sent: Tuesday, February 22, 2005 12:04 PM
        To: spr...@li...
        Subject: Re: [Springframework-developer] Please check these two
things

        But isn't this bogus thinking? I mean replacing .equals with ==
makes
        the implementation more strickt and reduces semantical informations.
        We are thinking about objects and there is no performance gap
        to justify this modification.

        I wouldn't do it. I just would ensure that equals implementations
        start with if(this==object) return true;. How huge is the estimated
        performance gain?

        Cheers,

        Martin (Kersten)
          ----- Original Message -----
          From: Juergen Hoeller
          To: spr...@li...
          Sent: Tuesday, February 22, 2005 10:06 AM
          Subject: Re: [Springframework-developer] Please check these two
things

          Well-spotted!

          ConcurrencyThrottleInterceptor should indeed use an internal
monitor to avoid any potential for side effects. I doubt that this has
caused any issue in practice, but it's nevertheless cleaner.

          That check in BatchSqlUpdate is not supposed to compare the
elements but just the array reference: Repeated update invocations should
not pass-in the same array instance repeatedly, with modified elements. Of
course, a == check would be sufficient for this. I've reworked that part a
bit differently, though: BatchSqlUpdate stores a clone of the passed-in
array now, so there shouldn't be a need for such a check anymore.

          Juergen

            -----Original Message-----
            From: spr...@li...
[mailto:spr...@li...]On Behalf Of
Dave Brosius
            Sent: Tuesday, February 22, 2005 8:08 AM
            To: spr...@li...
            Subject: [Springframework-developer] Please check these two
things

            These may be problems, and then again maybe not. But they seem
odd/wrong to me

            1) In
org.springframework.aop.interceptor.ConcurrencyThrottleInterceptor

            in method invoke

            uses wait on 'this'

            In my mind you are exposing your synchronization strategies as a
public artifact, which leaves this class open to failure due to client code.
            The client code may unwittingly us an instance of this class to
do it's own synchronization, and totally screw up this class.
            I would recommend doing synchronizations (especially the use of
wait/notify) on a private member so client code can not effect it.

            2) In org.springframework.jdbc.object.BatchSqlUpdate

            in method update, you do

              if (!this.parameterQueue.isEmpty() &&
args.equals(this.parameterQueue.getLast())) {

            this is the same as using     args ==
this.parameterQueue.getLast()

            or in other words, are these objects the same object. I assume
you want to compare the elements of the array?