Method naming convention error

[cxallan]

this method on the Dataset object:

dataset.addToDatasetAnnotations(DatasetAnnotation annotation)

should probably be:

dataset.addToDatasetAnnotation(DatasetAnnotation annotation)

[cxallan]

in addition this should also probably be changed:

image.addToAnnotations(ImageAnnotation annotation)

further, what is the proper convention for these?

type.addToAnnotation(Type type)  // this?
type.addAnnotation(Type type)  // or this?
type.addTypeAnnotation(Type type)  // or even this?

[jmoore]

Some other examples we have are:

• Pixels.addToSettings
• Pixels.addToThumbnails
• Pixels.addToPlaneInfo

Here I see PlaneInfo? as actually being the odd sheep out. It's a Collection-valued field that's not in plural.

On the other hand, there's also:

• Instrument.addToLightSource
• Instrument.addToDetector
• Instrument.addToObjective

which I see as being weird. The current definition of Instrument is:

        <type id="ome.model.acquisition.Instrument">
                <properties>
                        <required name="microscope" ...
                        <onemany name="detector" ...
                        <onemany name="objective" ...
                        <onemany name="lightSource" ...
                        <onemany name="filter" ...
                </properties>
        </type>

Those should be plural. That would then make sense with the "addTo" prefix, which was originally intended to make the methods somewhat grammatically correct without resorting to de-pluralization (addDatasetAnnotations() --> addDatasetAnnotation())

In general, we should align the namings, but for the example given, I'd prefer addToDatasetAnnotations() the current convention being:

• Add a single item to a set: addTo<SetName?>(Type type)
• Add a signle item over a link: add<Type>(Type type)

[anonymous]

FYI, I'm going to use the Pixels API as an example.

I don't particularly like "addTo" at all as it is currently used as I believe it implies a particular order of linkage. In addition, for Pixels in particular, I believe it regularly implies a wrong or certainly a misrepresentative order of linkage.

If I want to create a Pixels object and assign it some rendering settings, I look at the representative Pixels and RenderingDef? APIs:

public void setPixels(ome.model.core.Pixels pixels);  // RenderingDef

public void addToSettings(ome.model.display.RenderingDef target); // Pixels

Looking at this it seems perfectly logical to assume that the API suggests the following:

RenderingDef def = new RenderingDef();
...
Pixels p = new Pixels();
def.setPixels(p);
iUpdate.save(def);

I understand that this is partially an artifact of the way the linkages happen in the database, but I find it strangely similar to the following, which while programatically correct make little sense and are quite unintuitive to someone using the API:

Seeds s = new Seeds();
...
Orange o = new Orange();
s.setOrange(o);
iUpdate.save(s);

// or even worse
Seeds s = new Seeds();
...
Orange o = new Orange();
o.addToSeeds(s);
iUpdate.save(o);

It seems to me logical to drop the "To" from these linkages altogether, which results in the following:

RenderingDef def = new RenderingDef();
...
Pixels p = new Pixels();
p.addRenderingDef(def);
iUpdate.save(p);

// and the more logical...
Seeds s = new Seeds();
...
Orange o = new Orange();
o.addSeeds(s);
iUpdate.save(o);

Further, the API currently contains the "addAllTo<Type>" convention which results in really strange methods that don't really do what they say they're going to do. For example:

public void addAllToSettings(java.util.Set targets); // RenderingDef
public void removeAllFromSettings(java.util.Set targets); // RenderingDef

Surely this implies something very different than what is going to happen and should be:

public void addToAllSettings(java.util.Set targets); // RenderingDef
public void removeFromAllSettings(java.util.Set targets); // RenderingDef

So, to close I'd guess we're sort of on par with the following:

• Add a single item to a set: addTo<SetName?>(Type type)
• Add a single item: addType(Type type)

We may also be able to get away with adding a suffix like so:

• addToDatasetAnnotationCollection()
• addToObjectiveCollection()
• addToDetectorCollection()

certainly saves us having to worry about pluralization.

[cxallan]

Gah, posted as anonymous.

[jmoore]

This is turning into something of an email-like thread, but let's see it to its end. From what I understand, the linkage that your are referring to is the fact that saving a RenderingDef can save a Pixels but not the other way around, whereas one usually thinks of Pixels "containing" RenderingDefs and not vice versa (as with Oranges and Seeds).

If that is this implication for you of "addTo" then we can certainly look into alternative method namings.

Your first suggestion of dropping the "To", however, is problematic. The problem is still the plurality (which is the whole reason that the "To" was added). There's no reliable way for the code generation to know that the field called "settings" should produce a method called "addSetting", and even if it may work for this Pixels API, it has to be a solution which will work for all many-to-one mappings. If the suggestion is to actually use the class name rather than the field name (defined in the UML), there are two issues: 1) the field name provides a certain amount of context, and 2) there are (or certainly could be) cases of two of the same types being linked to the same third type. In that case, the generated method names would conflict.

The second suggestion of suffixing the methods with "Collection" seems doable. Again, because of the plurals, it would be:

• addToSettingsCollection();

As for the helper methods "addAllTo<Type>" I disagree. The two versions you mentioned are:

1. addAllToSettings(Set targets)
2. addToAllSettings(Set targets)

(1) for me implies that all the members of "targets" will be added to the "Settings" collection. This is like the addAll(Collection) and removeAll(Collection) methods on the java.util.Collection API, in which effectively the add/remove method is called on "all" of the values in the Collection argument. (2) "addToAllSettings", however, maybe explains how we see these differently. Am I right in saying that this means for you "add this Pixels to all of those Settings" ? If so, then, yes, the naming is confusing.

Let's see, our suggestions are now:

addToAll rather than addAllTo - I'd have to say I disagree on this point. addToSettingsCollection seems long, but clear. What's to do with the All?

Other posssibilities? append(All)ToSettings?() since append is perhaps clearer? Or we could just mit All.

My suggestion would be to add "Collection" (or the more specific "Set" and "List") if that clears things up. Otherwise, we should probably keep searching.

[cxallan]

Okay, I think there's a fundamental misconception (read: language problem) that I have when I look at this:

public void addToSettings(RenderingDef target);
public void addAllToSettings(Set targets);

what I read is:

• Add the pixels object to the object target.
• Add the pixels object to each element in the Set targets.

which is why the following seems more logical to me:

public void addSettings(RenderingDef target);
public void addToSettingsCollection(Set targets);

However, if I read you right, what the first set of methods actually does is:

• Add the object target to the class variable settings.
• Add each element in the Set targets to the class variable settings.

which seems to make sense. All this said, and going back to the "what I read" statements, I guess what I'm voting for is using the class name as the suffix:

• add<Class>(Class object)

To the outsider this appears to be the way we've been doing things since the beginning as there are dozens of methods that appear to follow this convention. I suppose this is the real reason why I didn't really pick up on what the methods were actually trying to say. From my read of the Pixels datatype API, the "Settings" family of methods and are the only methods which don't actually follow the "add<Class>(Class object)" convention.

Given the choice between having to deal with these multiple instances of pluralization or changing a few methods like "addToSettings" to "addRenderingDef" I'd choose the latter but I'm certainly willing to be convinced otherwise.

[jmoore]

Indeed the problem may start with unnamed relationships in the UML. Perhaps we need to examine that. For all relationships for which there is not a name defined, the DSL files simply use the class name which explains the behavior that you see most of the time. Unifying/specifying that behavior may make this clearer.

But, in general, yes, addToXyz( target ) adds target to the instance variable named xyz (which is a Collection) by calling xyz.add( target ). It also does the inverse, by calling target.setXyz( xyz ).

That being said, I'm still against the use of the class unless it's specified as such in the UML. I'm cc'ing Jean-Marie now since this is essentially his domain. As long as the code generation can guarantee that there will be only unique field names produced, then I have no worries. And changing a few methods is currently not feasible. Whatever solution we find has to work for all of our code generation. Perhaps the new modelling/codegen framework (#96) will allow selective renaming, but that remains to be seen.

[cxallan]

After face-to-face and audio conversations with Josh and Jean-Marie, the decision to go with the add<Class> convention seems to have been accepted. The extraneous attribute names (like "settings" from the Pixels/RenderingDef? API) are going to be removed from the UML model definitions, as their presence is more for informational purposes rather than explicit attribute and method naming.

[jmoore]

Closed by r736. I tried to get all the tests, etc. but there may be some initial compilation problems.