xmlgraphics-general mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Glen Mazza <gma...@apache.org>
Subject Re: A small XMP framework
Date Thu, 15 Jun 2006 21:47:35 GMT
I unfortunately don't know much about XMP (I did look a little bit at 
the Adobe links you had given earlier though), but here are a few 
code-comments independent of the subject material at hand:

1.)  I would recommend any methods named in a JavaBeans fashion that you 
may have to be purely for setting a private member variable or 
retrieving the same private member variable.  For example, in your 
    public void setTitle(String value, String lang) {
        setLangAlt("title", value, lang);

This is not really setting a private String value named Title.  It is 
actually adding to some array of some sort.  I think "addTitle()" or 
similar would be clearer here.  As-is, when I look at the caller, for 
example, MetadataFromScratch:
dc.setTitle("Der Herr der Ringe", "de");
dc.setTitle("Lord of the Rings", "en");

It causes the reader of the code to think that the second call may be 
overwriting the value of the first call.   So something like 
"addTitle()" instead would help readability IMO.

2.)  Offhand, XMPComplexValue looks like a candidate for being an 
interface instead of an abstract base class.  It only has two methods, 
both abstract.

3.)  I noticed several "//" commenting-out of code, some of which looks 
like you will never be restoring it (throwaway code).  If you haven't 
already, it would be nice if you could quickly search on "//" within the 
newly added files to make sure each have a reasonable chance of someday 
being reactivated, else just remove the commented-out code entirely.

4.)  Will the system *ever* run without an instance of XMPSchemaRegistry 
being activated?  Within that class the Singleton instance is 
instantiated only upon the first call to getInstance(), leaving vague 
its necessity.  But if there is always going to be such an instance, you 
might wish to instantiate via

static {
    instance = new XMPSchemaRegistry();

instead to emphasize that this object will always be created.

5.)  In XMPProperty:getEffectiveQName(), I suspect a user-friendly error 
should be given if the namespace is not available within the 
XMPSchemaRegistry instead of an NPE.  (i.e., if 
"XMPSchemaRegistry.getInstance().getSchema(getNamespace());" returns null)

Also, I don't know the logic, but for XMPProperty:getArrayValue() below, 
I think a simple class cast exception or a more user-friendly message 
might be a better idea then just silently returning null, iff the callee 
should know in advance not to call getArrayValue() if it hasn't already 
configured the value to be an XMPArray.

    public XMPArray getArrayValue() {
        return (value instanceof XMPArray ? (XMPArray)value : null);

6.)  Also, for XMPArray.getSimpleValue(), same issue as immediately 
above but what if you have a one-element XMPArray?  I think the below 
will return getValue(0) rather than the null (or error message) I think 
you would like here:

   public Object getSimpleValue() {
        if (values.size() == 1) {
            return getValue(0);
        } else {
            return null;

If I'm correct, I think you would want to test on "values instanceof 
XMPArray" instead.

7.)  In XMPSchemaAdapter.formatISO8601Date():  variables dt, zoh, and 
zom should probably be spelled out to make it more readable.  I'm not 
even sure what zoh and zom mean.  (hours and minutes, OK, but "zo" is 
unclear to me.)


Jeremias Maerki wrote:

>I've had no feedback in a week. Is everybody comfortable if I commit the
>code to the XML Graphics Commons repository as presented in the snapshot
>ZIP? If I hear no objections within 72 hours, I'll go ahead.
>On 07.06.2006 10:53:35 Jeremias Maerki wrote:
>>I've just uploaded my XMP approach for review. Of course, it's far from
>>being complete or bug-free. But it presents a proof-of-concept although
>>I haven't implemented structured properties.
>>Usage examples can be found in the examples directory. The source code
>>is in the org.apache.xmlgraphics.xmp package.
>>Feedback is welcome. We can then decide which way to go.
>Jeremias Maerki
>Apache XML Graphics Project URL: http://xmlgraphics.apache.org/
>To unsubscribe, e-mail: general-unsubscribe@xmlgraphics.apache.org
>For additional commands, e-mail: general-help@xmlgraphics.apache.org

Apache XML Graphics Project URL: http://xmlgraphics.apache.org/
To unsubscribe, e-mail: general-unsubscribe@xmlgraphics.apache.org
For additional commands, e-mail: general-help@xmlgraphics.apache.org

View raw message