Menu
▾
▴
Re: [Springframework-developer] I don't like the DefaultXmlBeanDefinitionParser
|
From: Rob H. <ro...@ca...> - 2005-02-22 13:52:36
|
My thoughts exactly :). We have enough dependencies already.
Erwin Vervaet wrote:
> I think the main reason to use the W3C DOM API directly is to avoid
> the need for an extra dependency (e.g. JDOM) just to parse the XML
> bean definitions. You end up with an "less than elegant"
> implementation in DefaultXmlBeanDefinitionParser, but in this case the
> benifits outweigh the costs.
>
> Erwin Vervaet
> erw...@er... <mailto:erw...@er...>
>
> ----- Original Message -----
> *From:* Martin Kersten
> <mailto:Mar...@St...>
> *To:* spr...@li...
> <mailto:spr...@li...>
> *Sent:* Tuesday, February 22, 2005 1:21 PM
> *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
> <mailto:Mar...@St...>
> *To:* spr...@li...
> <mailto: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 <mailto:ju...@in...>
> *To:* spr...@li...
> <mailto: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
> <mailto:ju...@in...>
> *To:*
> spr...@li...
> <mailto: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?
>
>
>
>
|
