Re: git commit: ANY23-148 Programmes Ontology and updated WO vocab

[Simone Tripodi <si...@apache.org>]

Hi Lewis!

thanks a lot for putting efforts on that! I just have few minor notes:

> + *
> + * @author lewismc

as already adopted in other ASF projects, I'd suggest to drop all
@author tags, since committers are mentioned in the parent POM

> + */
> +public class Programme extends Vocabulary {
> +
> +  public static final String NS = "http://purl.org/ontology/po/";
> +
> +  private static Programme instance;
> +
> +  public static Programme getInstance() {
> +    if(instance == null) {
> +      instance = new Programme();
> +    }
> +    return instance;
> +  }
> +

this operation is not thread-safe, the instance can be potentially
instantiated more than once

> +  //Resources
> +  /** A version holding an audio description. */
> +  public final URI AudioDescribedVersion  = createClass(NS, "AudioDescribedVersion");
> +
> +  /** A brand, e.g. `Top Gea`r*/
> +  public final URI Brand                = createClass(NS, "Brand");
> +

first char of fields name should be lower-case (as above and below);
as an alternative solution, we could use an Enum for that type, unless
users have to define their own class...

HTH, all the best!
-Simo

Re: git commit: ANY23-148 Programmes Ontology and updated WO vocab

[Peter Ansell <an...@gmail.com>]

On 3 April 2014 23:56, Simone Tripodi <si...@apache.org> wrote:
> Hi Lewis!
>
> thanks a lot for putting efforts on that! I just have few minor notes:
>
>> + *
>> + * @author lewismc
>
> as already adopted in other ASF projects, I'd suggest to drop all
> @author tags, since committers are mentioned in the parent POM
>
>> + */
>> +public class Programme extends Vocabulary {
>> +
>> +  public static final String NS = "http://purl.org/ontology/po/";
>> +
>> +  private static Programme instance;
>> +
>> +  public static Programme getInstance() {
>> +    if(instance == null) {
>> +      instance = new Programme();
>> +    }
>> +    return instance;
>> +  }
>> +
>
> this operation is not thread-safe, the instance can be potentially
> instantiated more than once

Threadsafety is only really relevant if there is either going to be a
memory-leak or a deadlock/livelock between threads. Supporting
multiple equivalent URI instances is required by the OpenRDF URI API
to interact properly. Ie, you can't do "==" equality to check whether
a URI is the same as one in this vocabulary. So, if everyone is using
.equals it shouldn't matter whether there are different physical URI
objects, and proper synchronisation won't hurt, but won't actually
gain anything other than a very very small amount of memory from the
duplicated objects in some cases. The instance variable will also only
ever end up with a single field in it after the initial threaded calls
pass through, and others will end up calling getInstance multiple
times as necessary rather than keeping the reference for the life of
the application, so any multiple instances are likely to come up for
garbage collection before long.

Having said that, if you did strictly require a single instance this
pattern is broken and needs a synchronized(Programme.class) block with
another null check inside. A pattern to avoid if you do need strict
"singleton" behaviour is that a "public static synchronized" is a very
bad thing for threaded applications as it can cause the threads to
block and/or interact with one another, even if temporarily and only
for a short amount of time, it reduces performance unnecessarily.

>> +  //Resources
>> +  /** A version holding an audio description. */
>> +  public final URI AudioDescribedVersion  = createClass(NS, "AudioDescribedVersion");
>> +
>> +  /** A brand, e.g. `Top Gea`r*/
>> +  public final URI Brand                = createClass(NS, "Brand");
>> +
>
> first char of fields name should be lower-case (as above and below);
> as an alternative solution, we could use an Enum for that type, unless
> users have to define their own class...

This good java convention breaks with many ontologies that are case
sensitive and have both upper and lower case URIs defined for classes
and properties respectively.

Cheers,

Peter