sudeep87 / uimafit

Automatically exported from code.google.com/p/uimafit
0 stars 0 forks source link

CasUtil & JCasUtil - method names #29

Closed GoogleCodeExporter closed 8 years ago

GoogleCodeExporter commented 8 years ago
This issue is to discuss the API (i.e. method names) for the recently 
introduced CasUtil & JCasUtil.

Philip: Most of the method names seem like they could be changed simply to 
"get" (e.g. getCoveredAnnotations and getAnnotationIterator) without any loss 
of clarity.  I also don't see the purpose of the "iterate" methods and also 
find the name to be confusing since they return an Iterable and not an 
iterator.  

Richard: I named them iterate() so they do not take up a lot of space in an 
extended for loop. Also I think of them more like extended control functions 
then getters. The JCasUtils is not a bean - neither do I think of it as a 
factory - which would mean to call them create*. As I said - more like a 
language extension - in conjunction with the static imports I am so fond of ;)

Original issue reported on code.google.com by pvogren@gmail.com on 30 Jun 2010 at 3:30

GoogleCodeExporter commented 8 years ago
I still don't see why getCoveredAnnotations() is better than simply get() even 
if JCasUtil isn't a bean.  It's longer and the word "covered" is a bit strange. 
 Maybe the name retrieve() would be better.  I think the methods are great - 
but its worth reconsidering the names since they have the potential to be 
widely used.  

Original comment by pvogren@gmail.com on 30 Jun 2010 at 3:31

GoogleCodeExporter commented 8 years ago
I guess I should read stuff twice before answering. Actually I was not 
remembering that getAnnotationIterator and getCoveredAnnotations are public. I 
exclusively use one of the "iterate" or "subiterate" methods.
I guess the gerAnnotationIterator() method could possibly be renamed to get() - 
or iterator().

As for the getCoveredAnnotations() - I'm not for renaming these to get(). These 
are two methods of a probably upcoming family of methods, that allow to 
navigate from one annotation to a set of other annotations. In this case. In 
the case of getCoveredAnnotations() one navigates to all annotations within the 
begin/end of the given annotation. There may be other methods soon like getting 
all annotations covering the given, occurring before or after the given, etc. 
I'm also for renaming, I am not sure to what though. selectCovered()? 
selectWithin()? Or even define some enum listing the possible navigation axes, 
e.g. select(jcas, someToken, COVERED), select(jcas, someToken COVERING), etc.? 

Original comment by richard.eckart on 30 Jun 2010 at 7:03

GoogleCodeExporter commented 8 years ago
It might be worth mentioning that we did a fair bit of work along these lines 
in ClearTK.  

http://code.google.com/p/cleartk/source/browse/trunk/ClearTK-framework/src/main/
java/org/cleartk/util/AnnotationRetrieval.java

We can move/borrow whatever code that may be of use to uimaFIT and merge it 
with the code you're working on as we see fit.  If I remember right, our unit 
tests for these methods had a number of tricky edge cases and so it might be 
worth refactoring some of those tests to work with whatever api ends up in 
uimaFIT regardless of whether any of the code in AnnotationRetrieval ends up 
being useful.  

Original comment by pvogren@gmail.com on 30 Jun 2010 at 7:26

GoogleCodeExporter commented 8 years ago
I like iterator() over getAnnotationIterator().  

I like select() better than get() or getCoveredAnnotations().  I don't 
understand how an enum would help here.  Does this tell some single entry point 
method called select how to delegate to some other method which does the 
selection?  I think that the presence of a "window" annotation as a parameter 
would make the method's intended use clear.  e.g. I think select(Class<T> type, 
AnnotationFS windowAnnotation) would be easy to understand.  

Original comment by pvogren@gmail.com on 30 Jun 2010 at 7:31

GoogleCodeExporter commented 8 years ago
There is definitely stuff in the ClearTK AnnotationRetrieval that should go 
into uimaFIT.

If we just call it "select", then the criteria by which we select is not clear 
- even if there is a window annotation. Imagine for example that you want to 
select all annotations that are _covering_ the window annotation, instead of 
all annotations that the window annotation covers. So either we'd need a couple 
of methods, one for each axis of selection, or a single one which also takes 
the axis as a parameter. I guess I'd prefer having one for each axis instead of 
having the enum parameter. So I would suggest e.g. selectCovered, 
selectCovering, selectOverlapping, selectRightOverlapping, 
selectLeftOverlapping, selectBefore, selectAfter, selectLeftAdjacent, 
selectRightAdjacent, etc.

Original comment by richard.eckart on 30 Jun 2010 at 7:39

GoogleCodeExporter commented 8 years ago
good points.  I agree.  

Original comment by pvogren@gmail.com on 30 Jun 2010 at 7:51

GoogleCodeExporter commented 8 years ago
also works better for static imports besides

Original comment by pvogren@gmail.com on 30 Jun 2010 at 7:52

GoogleCodeExporter commented 8 years ago
I changed the getAnnotationIterator methods to iterate() and the 
getCoveredAnnotations to selectCovered.  I also changed the get() method to 
selectByIndex - which I think is in-line with the naming convention discussed 
above and is more descriptive.  

Original comment by pvogren@gmail.com on 2 Jul 2010 at 4:42