[ 
https://issues.apache.org/jira/browse/LUCENENET-515?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
 ]

Jens Melgaard updated LUCENENET-515:
------------------------------------

    Description: 
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...
TokenStream.GetAttribute(typeof(TermAttribute));
And god I hated that, so now I replaced it with the much more beautiful:
TokenStream.GetAttribute<TermAttribute>();
And deleted that old hag of a method taking a Type... And now I am happy...

BUT!...

What when...
Type myNotDefinedHereType = GetTypeFromSomeWhere();
TokenSteam.GetAttribute.... ?????!?!?!?... Uhm... What now???? 
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...
Type myNotDefinedHereType = GetTypeFromSomeWhere();
TokenSteam.GetAttribute(myNotDefinedHereType);

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:

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

Do:
public virtual 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;
    }





  was:
I Have noticed that TokenStream.GetAttribute(Type) is gone in favor for 
TokenStream.GetAttribute<Type>();

Obviously TokenStream.GetAttribute<Type>(); 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...
TokenStream.GetAttribute(typeof(TermAttribute));
And god I hated that, so now I replaced it with the much more beautiful:
TokenStream.GetAttribute<TermAttribute>();
And deleted that old hag of a method taking a Type... And now I am happy...

BUT!...

What when...
Type myNotDefinedHereType = GetTypeFromSomeWhere();
TokenSteam.GetAttribute.... ?????!?!?!?... Uhm... What now???? 
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...
Type myNotDefinedHereType = GetTypeFromSomeWhere();
TokenSteam.GetAttribute(myNotDefinedHereType);

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:

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

Do:
public virtual 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;
    }





    
> 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...
> TokenStream.GetAttribute(typeof(TermAttribute));
> And god I hated that, so now I replaced it with the much more beautiful:
> TokenStream.GetAttribute<TermAttribute>();
> And deleted that old hag of a method taking a Type... And now I am happy...
> BUT!...
> What when...
> Type myNotDefinedHereType = GetTypeFromSomeWhere();
> TokenSteam.GetAttribute.... ?????!?!?!?... Uhm... What now???? 
> 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...
> Type myNotDefinedHereType = GetTypeFromSomeWhere();
> TokenSteam.GetAttribute(myNotDefinedHereType);
> 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:
> 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;
>     }
> Do:
> public virtual 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;
>     }

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