Re: [PATCH] unnesting Property.Maker and rollling datatypes into thier properties.

[Finn Bock]

[Simon Pepping]

I have the following considerations:
...
3. In code where the datatype aspect is used, the code may become less
   logical. This happens in the parsers and in the RTF renderer. An
   example from render/rtf/TextAttributesConverter.java:
        // Cell background color
        ColorTypeProperty colorTypeProp = 
(ColorTypeProperty)propertyList.get(Constants.PR_COLOR);
        if (colorTypeProp != null) {
            ColorTypeProperty colorType = colorTypeProp.getColorType();
            if (colorType != null) {
                if (colorType.getAlpha() != 0
                        || colorType.getRed() != 0
                        || colorType.getGreen() != 0
                        || colorType.getBlue() != 0) {
                    rtfAttr.set(
                        RtfText.ATTR_FONT_COLOR,
                        convertFOPColorToRTF(colorType));
                }
   Here colorTypeProp and colorType denote the same, and the code is
   not quite logical. That is because the method getColorType now more
   acts as an assertion. In the new situation it could be made more
   logical as follows:
        // Cell background color
        ColorTypeProperty colorType = 
(ColorTypeProperty)propertyList.get(Constants.PR_COLOR);
        if (colorType != null && colorType.getColorType() != null) {
                if (colorType.getAlpha() != 0
                        || colorType.getRed() != 0
                        || colorType.getGreen() != 0
                        || colorType.getBlue() != 0) {
                    rtfAttr.set(
                        RtfText.ATTR_FONT_COLOR,
                        convertFOPColorToRTF(colorType));
                }
Yes, I hadn't though of that, perhaps because such constructs are rare 
in layoutmgr and area renderers. But I think it should be rewritten to:

        // Cell background color
        ColorTypeProperty colorType =
                   propertyList.get(Constants.PR_COLOR).getColorType();
        if (colorType.getAlpha() != 0
                || colorType.getRed() != 0
                || colorType.getGreen() != 0
                || colorType.getBlue() != 0) {
            rtfAttr.set(RtfText.ATTR_FONT_COLOR,
                        convertFOPColorToRTF(colorType));
        }
without the cast and without testing for a null value because there is 
*always* some computed value for PR_COLOR and it is *always* a 
colorType. Oh, and the comment should be fixed as well <wink>.

All in all I think that this change simplifies the code, and would be
a good change.
Thanks.

Allow me to make some notes:

1. Would it not be a good idea to move Property.java from fo to
   properties?
Yes, I agree with you and Glen on this.

2. TableColLength and LinearCombinationLength do not have the word
   'Property' in their name. I would advocate making these names
   consistent with the others.
Perhaps, I don't have strong feelings about naming.

3. Should ToBeImplemented.java also be removed?

A lot of good work.
Thanks, but eclipse did all the work for me. It's a great tool for this 
kind of refactoring.

regards,
finn