lucenenet-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From NightOwl888 <...@git.apache.org>
Subject [GitHub] lucenenet issue #207: API: Added ReferenceManager<G>.AcquireContext()
Date Thu, 06 Jul 2017 22:39:45 GMT
Github user NightOwl888 commented on the issue:

    https://github.com/apache/lucenenet/pull/207
  
    Looks nice. Although, I tend to favor the "all methods are verbs" and "all properties
are nouns or adjectives"  methodology. For example, I would expect a method named `Buffer()`
to buffer something, not return a buffer. In this case that argument is a bit weaker, but
it still feels like `GetContext()` would be more clear than `Context()`.
    
    I primarily named it `AcquireContext()` because it is essentially the same functionality
as the existing `Acquire()` method (from Java), so the similar names seem natural. But `GetContext()`
is also clear (and more concise).
    
    Hmm, that wasn't what I meant by "other subclasses". Actually, I made the constructor
of `ReferenceContext` internal so it is effectively sealed to the outside world. What I was
referring to is the fact that SearcherManger is just one of a few (with many more possible)
subclasses of `ReferenceManager<G>`. 
    And since `ReferenceManager<G>` is generic, the `Reference` property is also generic
and the cast is not necessary - its type is specific to the subclass of `ReferenceManager<G>`.

    
    However, we can't very well name the property `Searcher` (so we have `context.Searcher`)
because it wouldn't be generic enough for the other subclasses of `ReferenceManager<G>`.
Well, we could by making extension methods and a custom "holder" type for each subclass of
`ReferenceManager<G>`. But the disadvantage there is repetitive similar code. Maybe
it would be worth it for such an integral part of the API, though.
    
    The purpose of the using block here isn't to dispose anything really - it is just to help
ensure the variable isn't utilized after its reference is no longer tracked. The context is
just a "holder" for the reference to implement `Dispose()` on, which tells the parent `ReferenceManager<G>`
class that we are done using the reference. You can see an example of how it looks with the
Java functions [here](https://github.com/synhershko/LuceneNetDemo/blob/master/LuceneNetDemo/GitHubIndex.cs#L157-L184)
with a more verbose finally block. Same thing, but shaves a few lines of code off of the block.
You don't gain much if you need to catch an exception, though because you still need to put
a try catch inside of the using block.
    
    The using block does do one nice thing. Since it is a block, the variable goes out of
scope when it is done.
    
    ```c#
    using (var context = searcherManager.AcquireContext)
    {
        var foo = context.Reference;
        foo.Search();
    } // foo goes out of scope
    ```
    The only issue is that it is not 100% obvious that this is a problem:
    
    ```c#
    Searcher foo;
    
    using (var context = searcherManager.AcquireContext)
    {
        foo = context.Reference;
    } // foo doesn't go out of scope - BAD
    
    foo.Search(); // Not allowed here
    ```
    
    But this just seems to be a scenario where there is no real way to force the end user
to follow the rules. You just have to document the right way and hope they follow the documentation.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

Mime
View raw message