lucenenet-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Jens Melgaard (JIRA)" <j...@apache.org>
Subject [jira] [Comment Edited] (LUCENENET-515) bring back TokenStream.GetAttribute(Type).
Date Wed, 12 Dec 2012 21:11:21 GMT

    [ https://issues.apache.org/jira/browse/LUCENENET-515?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13530325#comment-13530325
] 

Jens Melgaard edited comment on LUCENENET-515 at 12/12/12 9:10 PM:
-------------------------------------------------------------------

[~ccurrens]

{quote}
Regarding the check of attribute type twice, I don't think it's necessary. We're looking at
a performance decrease of less than 100 ticks for 1000 calls to the method, so it's not at
all an issue. I was just bringing it up as a reminder that internally we're using the generic
version, so we should either a) not, or b) make sure whatever design decision we make won't
adversely affect its performance.
{quote}

As sugested, under the circumstance that "GetAttribute" and "HasAttribute" is used more often
than adding, AND that the common case is that we find the type in the dictionary, you can
basically add the check without having to perform it each time... 

{code}
public T GetAttribute<T>() where T : IAttribute
{
   return (T) GetAttribute(typeof(T));
} 
public virtual IAttribute GetAttribute(Type key)
{
  if (!this.attributes.ContainsKey(key))
  {
    if(!typeof(IAttribute).IsAssignableFrom(t))
      throw new ArgumentException("Type of T must inherit from IAttribute");

    throw new ArgumentException("This AttributeSource does not have the attribute '" + key.FullName
+ "'.");
  }
  else
    return (T) this.attributes[key].Value;
}
{code}

Because I assume that if the type exist in the dictionary, then it actually complies with
the IAttribute interface, if it does not exist then you can continue to determine what error
to throw... Odd code in a sense...

Ofc. thats besides the point when we take the other issue (portability from java) into account...
I never ported a Java project, so I don't know what tools there is and how they work...
                
      was (Author: jmelgaard):
    [~ccurrens]

{quote}
Regarding the check of attribute type twice, I don't think it's necessary. We're looking at
a performance decrease of less than 100 ticks for 1000 calls to the method, so it's not at
all an issue. I was just bringing it up as a reminder that internally we're using the generic
version, so we should either a) not, or b) make sure whatever design decision we make won't
adversely affect its performance.
{quote}

As sugested, under the circumstance that "GetAttribute" and "HasAttribute" is used more often
than adding, AND that the common case is that we find the type in the dictionary, you can
basically add the check without having to perform it each time... 

{code}
public T GetAttribute<T>() where T : IAttribute
{
   return (T) GetAttribute(typeof(T));
} 
public virtual IAttribute GetAttribute(Type key)
{
  if (!this.attributes.ContainsKey(key))
  {
    if(!typeof(IAttribute).IsAssignableFrom(t))
      throw new ArgumentException("Type of T must inherit from IAttribute");

    throw new ArgumentException("This AttributeSource does not have the attribute '" + key.FullName
+ "'.");
  }
  else
    return (T) this.attributes[key].Value;
}
{code}

Because I assume that if the type exist in the dictionary, then it actually complies with
the IAttribute interface, if it does not exist then you can continue to determine what error
to throw... Odd code in a sense...
                  
> bring back TokenStream.GetAttribute(Type).
> ------------------------------------------
>
>                 Key: LUCENENET-515
>                 URL: https://issues.apache.org/jira/browse/LUCENENET-515
>             Project: Lucene.Net
>          Issue Type: Improvement
>          Components: Lucene.Net Core
>    Affects Versions: Lucene.Net 3.0.3
>            Reporter: Jens Melgaard
>             Fix For: Lucene.Net 3.6
>
>
> I Have noticed that TokenStream.GetAttribute(Type) is gone in favor for TokenStream.GetAttribute<Type>();
> Obviously TokenStream.GetAttribute<Type>() is a better syntax; but it should not
replace TokenStream.GetAttribute(Type), but instead compliment it... But this is a common
pitfall, especially for non-.NET developers and people who have not run into the implications
of this..
> In the case of Lucene.NET, it is properly unlikely that this will affect anyone, however,
I consider what has been done bad practice... now why is that?
> Many would think like this:
> Before I had to do this old thing...
> {code}TokenStream.GetAttribute(typeof(TermAttribute));{code}
> And god I hated that, so now I replaced it with the much more beautiful:
> {code}TokenStream.GetAttribute<TermAttribute>();{code}
> And deleted that old hag of a method taking a Type... And now I am happy...
> BUT!...
> What when...
> {code}
> Type myNotDefinedHereType = GetTypeFromSomeWhere();
> TokenSteam.GetAttribute.... ?????!?!?!?... Uhm... What now???? 
> {code}
> Now you have to write a whole lot of reflection mess, use a dynamically compiled delegate
using IL-Emit or the Mono Compiler as a Service...
> All of those 3 workarounds are generally just ugly... 
> If you keept both...
> {code}
> Type myNotDefinedHereType = GetTypeFromSomeWhere();
> TokenSteam.GetAttribute(myNotDefinedHereType);
> {code}
> While it might be unlikely that it will ever be used, there is always the off case, and
API's should support both...
> So instead of:
> {code}
> public virtual T GetAttribute<T>() where T : IAttribute
>     {
>       Type key = typeof (T);
>       if (!this.attributes.ContainsKey(key))
>         throw new ArgumentException("This AttributeSource does not have the attribute
'" + key.FullName + "'.");
>       else
>         return (T) this.attributes[key].Value;
>     }
> {code}
> Do:
> {code}
> public T GetAttribute<T>() where T : IAttribute
> {
>    return (T) GetAttribute(typeof(T));
> } 
> public virtual IAttribute GetAttribute(Type key)
>     {
>       //Note: Since Type is required to implement IAttribute, I would properly check
that as well to be able to provide a more meaningfull error... However to speed things up,
I would do it inside the if bellow...
>       if (!this.attributes.ContainsKey(key))
>         throw new ArgumentException("This AttributeSource does not have the attribute
'" + key.FullName + "'.");
>       else
>         return (T) this.attributes[key].Value;
>     }
> {code}

--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators
For more information on JIRA, see: http://www.atlassian.com/software/jira

Mime
View raw message