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

Jens Melgaard commented on LUCENENET-515:
-----------------------------------------

Weather you would return IAttribute or just object doesn't matter, however it 
would makes sense to return IAttribute since it seems you require that 
interface...
But actually, no... today you wouldn't need a whole lot of reflection to 
interact with an object of unknown type, you could let the .NET framework 
handle that part for you since 4.0 (dynamics)...
I don't know the nature of the attributes we talk about here... So I wouldn't 
know what kind of interactions that would make sense...
You might not even be the one interacting with it...

I am merely pointing out what I see as bad practice from an API perspective. 
Because I have meet these challenges before where we actually overcame them by 
changing framework...


                
> 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
>
> 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

Reply via email to