Re: svn commit: r558045 - in /xmlgraphics/fop/trunk/src/java/org/apache/fop/fo/properties: EnumNumber.java EnumProperty.java NumberProperty.java StringProperty.java

[Vincent Hennebert <vi...@anyware-tech.com>]

Hi Andreas,

Sorry for a late comment on this.

> Author: adelmelle
> Date: Fri Jul 20 09:48:55 2007
> New Revision: 558045
> 
> URL: http://svn.apache.org/viewvc?view=rev&rev=558045
> Log:
> Slight correction: 
> - make NumberProperty, EnumProperty, EnumNumber and StringProperty final, so instanceof suffices in the check for equality
> - instead of subclassing NumberProperty, make EnumNumber implement the Numeric interface

<snip/>
> -    public int getValue() {
> -        log.error("getValue() called on " + enumProperty + " number");
> -        return 0;

That may be discussed, but I have strong feelings against that. If the 
method shouldn’t be called, why not throw an IllegalStateException or 
so? If it is called that means there’s a bug somewhere. Perhaps the code 
won’t crash, perhaps nothing will even be noticeable on the output, but 
I doubt it as there can only be a serious problem, that should be 
corrected sooner rather than later.

All that said... if most methods of the Numeric interface aren’t 
applicable to EnumNumber, should that class still be considered as 
a Numeric object? Does that make sense to cast an EnumNumber into 
a Numeric?

WDYT?
Vincent

Re: svn commit: r558045 - in /xmlgraphics/fop/trunk/src/java/org/apache/fop/fo/properties: EnumNumber.java EnumProperty.java NumberProperty.java StringProperty.java

[Andreas L Delmelle <a_...@pandora.be>]

On Aug 11, 2007, at 09:17, Vincent Hennebert wrote:

>> <snip />
>> Good idea, much better than the above! Didn't quite occur to me to do
>> that. Then again, in case you didn't notice: those lines are  
>> removed...(?)
>
> Ok, I isolated the wrong snippet... Go below in the commit message,  
> and
> you’ll find plenty of such lines with plus instead of minus :-P

Yeah, I knew that... ;-)

> <snip/>
>>> All that said... if most methods of the Numeric interface aren’t
>>> applicable to EnumNumber, should that class still be considered as
>>> a Numeric object? Does that make sense to cast an EnumNumber into
>>> a Numeric?
>>
>> Well, apparently, a long time ago, someone felt it necessary to  
>> have a
>> Property type that stored an enum but in the end it's only a number
>> (values like "no-limit").
>> The idea of an EnumNumber itself always seemed somewhat ugly to  
>> me, but
>> I never took the time to come up with a decent alternative.
>
> So we can consider that this class will eventually be removed/changed?
> Then this hack is more acceptable. What about a “TODO this is ugly and
> shall be removed”?

Also a fine suggestion. Now that the point has been raised, I'm  
trying to assess why it currently needs to exist in the first place.  
Maybe we could even do without, which would be even better. It just  
looked a little like EnumNumber is an artificial construction to have  
an Enum that can act as a Numeric (?), and in that case it seemed  
cleaner to me to implement the Numeric interface than have it extend  
NumberProperty just to override those methods that aren't supposed to  
be called anyway...

EnumLength is another one of those hybrids, BTW.


Cheers

Andreas

Re: svn commit: r558045 - in /xmlgraphics/fop/trunk/src/java/org/apache/fop/fo/properties: EnumNumber.java EnumProperty.java NumberProperty.java StringProperty.java

[Vincent Hennebert <vi...@anyware-tech.com>]

Hi Andreas,

Andreas L Delmelle a écrit :
> On Aug 8, 2007, at 18:30, Vincent Hennebert wrote:
>
>>> -    public int getValue() {
>>> -        log.error("getValue() called on " + enumProperty + " number");
>>> -        return 0;
>>
>> That may be discussed, but I have strong feelings against that. If the
>> method shouldn’t be called, why not throw an IllegalStateException or
>> so?
>
> Good idea, much better than the above! Didn't quite occur to me to do
> that. Then again, in case you didn't notice: those lines are removed...(?)

Ok, I isolated the wrong snippet... Go below in the commit message, and
you’ll find plenty of such lines with plus instead of minus :-P

<snip/>
>> All that said... if most methods of the Numeric interface aren’t
>> applicable to EnumNumber, should that class still be considered as
>> a Numeric object? Does that make sense to cast an EnumNumber into
>> a Numeric?
>
> Well, apparently, a long time ago, someone felt it necessary to have a
> Property type that stored an enum but in the end it's only a number
> (values like "no-limit").
> The idea of an EnumNumber itself always seemed somewhat ugly to me, but
> I never took the time to come up with a decent alternative.

So we can consider that this class will eventually be removed/changed?
Then this hack is more acceptable. What about a “TODO this is ugly and
shall be removed”?

Cheers,
Vincent

Re: svn commit: r558045 - in /xmlgraphics/fop/trunk/src/java/org/apache/fop/fo/properties: EnumNumber.java EnumProperty.java NumberProperty.java StringProperty.java

[Andreas L Delmelle <a_...@pandora.be>]

On Aug 8, 2007, at 18:30, Vincent Hennebert wrote:

>> -    public int getValue() {
>> -        log.error("getValue() called on " + enumProperty + "  
>> number");
>> -        return 0;
>
> That may be discussed, but I have strong feelings against that. If the
> method shouldn’t be called, why not throw an IllegalStateException or
> so?

Good idea, much better than the above! Didn't quite occur to me to do  
that. Then again, in case you didn't notice: those lines are  
removed...(?)

> If it is called that means there’s a bug somewhere. Perhaps the code
> won’t crash, perhaps nothing will even be noticeable on the output,  
> but
> I doubt it as there can only be a serious problem, that should be
> corrected sooner rather than later.
>
> All that said... if most methods of the Numeric interface aren’t
> applicable to EnumNumber, should that class still be considered as
> a Numeric object? Does that make sense to cast an EnumNumber into
> a Numeric?

Well, apparently, a long time ago, someone felt it necessary to have  
a Property type that stored an enum but in the end it's only a number  
(values like "no-limit").
The idea of an EnumNumber itself always seemed somewhat ugly to me,  
but I never took the time to come up with a decent alternative.


Cheers

Andreas