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

[Martin K. <Mar...@St...>]

Hi folks,
=20
   I am currently trying to extend the framework by supporting =
contributions.=20
Just to see how it feels.

So I made some investigations in the sourcecode. I don't want to start a =
war=20
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=20
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=20
horrible API? You write a horrible implementation! And thats how it =
looks.=20
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 -----=20
  From: Martin Kersten=20
  To: spr...@li...=20
  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.=20
  Ok :-) sorry was a strange day for me, I guess.

  Thanks,

  Martin (Kersten)
  ----- Original Message -----=20
    From: Juergen Hoeller=20
    To: spr...@li...=20
    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 =3D=3D 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 =
=3D=3D makes=20
      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=20
      start with if(this=3D=3Dobject) return true;. How huge is the =
estimated
      performance gain?


      Cheers,

      Martin (Kersten)
        ----- Original Message -----=20
        From: Juergen Hoeller=20
        To: spr...@li...=20
        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 =3D=3D 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.=20
          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 =3D=3D =
this.parameterQueue.getLast()

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