Friday, December 9, 2005

C# Type Conversion Operators Considered Harmful

I see Fritz just posted a code sample that makes use of C# conversion operators. I don't like it. No sir, I don't like it one bit. :)

 

Here's the deal. A conversion operator is just a way of writing a method that you invoke using the C# casting syntax. The thing is, since it's a method, why not just make it a method. If I were writing this code, I'd call it ToUpdateItem() or perhaps emphasize its a factory nature by calling it something like CreateFromUpdateItem.

 

There are two problems with the type conversion operator approach as I see it. First, you're providing something that implies type compatibility, when really no such type relationship exists. Second, the conversion operator syntax implies that no new instance is created, when in fact a new object instance is created every time.

 

With a plain ol' method, people who maintain your code can see exactly what's going on. With a type conversion operator, you're just twisting the language syntax to make it look like one thing is happening, when really something else is.

 

A good rule of thumb is that if you see yourself typing "public static operator", you're doing something wrong. :)

21 comments:

  1. Why don't you say what you really feel Craig? :)



    I can see your point, but I would argue that that type conversion makes sense when you are in fact converting types, which is what I'm doing. I'm not twisting the language syntax if I'm using it's native conversion operator (casting) to change one type into another type.

    ReplyDelete
  2. No, you're not converting type. If you were converting types, you'd still have the same object reference at the end of the conversion. That's always true when doing type conversion where the conversion operator has not been overloaded. It is not true in your case.



    More to the point, why *wouldn't* you use a method? There's no loss of performance or functionality, and the meaning is perfectly clear.

    ReplyDelete
  3. ...just thought I'd chime in and point out that you can't (to my knowledge) define cast operators on 'interfaces'. This means that the cast (or 'conversion' in this case, in the real "you get a new object on the heap" sense) is a function of the *implementation*, and not a function of the *interface*. So, a method is better for that reason too, as it makes the interface clear (separate from the implementation, with regard to what it's possible to define on an 'interface' should you so desire).



    (The parentheses betray me, don't they.. ;)





    ReplyDelete
  4. Whether it's the same object reference or not is an implementation detail. Overloading the type conversion operator is just a way of granting clients the ability to turn your types into other types in a sanctioned way.



    Of course you could write a method, but the same could be said of properties, constructors, interface method dispatching, etc. These are language features you can leverage to make the types you build more intuitive to work with. I personally dislike nested chains of method calls:

    ws.UpdateItem(item.ToUpdateItem());



    and think the casting syntax is a bit cleaner:

    ws.UpdateItem((UpdateItem)item);

    ReplyDelete
  5. > Whether it's the same object reference or not is an implementation detail. <



    That would be true if your objects were immutable. But they're not. Making this an even worse idea in my opinion.







    ReplyDelete

  6. "First, you're providing something that implies type compatibility"



    As a good friend pointed out to me its called a "conversion" operator for a reason. You are *converting* from one type to another -- who said they had to be compatible?

    ReplyDelete
  7. I have no problem with what he's done from the standpoint that this isn't some general framework code that will be reused across projects or anything -- it's helper code to simplify web service calls. His type converters aren't changing between types that have wildly different meanings, as in the case of calling myDate.ToString(). It's simply changing the type to allow an object to be passed to a method that has an arbitrary requirement for a different type (a WSDL generated method). I don't see much difference between what he does and converting from an int to a double, for example, so that you can pass an int to a function that wants a double. In fact, I think an implicit operator would be fine in his scenario. As long as web service proxies insist on generating new types without letting you override this behavior, might as well provide the simplest way to access the methods.

    ReplyDelete
  8. >> Whether it's the same object reference or not is an implementation detail. <<



    >That would be true if your objects were immutable. But they're not. Making this an even worse idea in my opinion.<



    Ok, that is a good point. If you had a reference to one type, and you cast it to another, you typically expect them to still be the same object, so that changes in one are reflected in the other. Implementing the conversion operator nullifies this assumption, and arguably makes it more confusing for the client of the class.



    So is the general consensus the same as Craig's - "if you see yourself typing 'public static operator', you're doing something wrong" ? Anyone have a good example of using conversion operators effectively in C#?

    ReplyDelete
  9. I can't comment on the consensus, but I think a math library is probably one area where operators make sense. If I wrote a BigInt library, I think it's much more reasonable to define (for example) conversion operators to and from int, addition, etc.



    But even there I think you should *also* have the normal methods: ToInt32(), Add(), etc.



    Dilip: they don't *have* to be compatible, of course, in the sense that the code will still work. But it's about expectations. "Type conversion" operator implies type compatibility to most people. If it didn't, it would be legal to cast a string to an int (like it is in C++).



    For example, let's say I wrote this:



    public static operator==(Foo a, Foo b)

    {

    if (IsTuesday()) { return false; }

    else { return a.Equals(b); }

    }



    This is a case of reducto ad absurdum, of course, but it makes the point clear - it's a bad idea to make things work in a non-obvious way, because you're going to force people to have to understand the code in a deep way. Here, we've overloaded the == operator to something that *almost* means "equality". In the same way, I contend that Fritz has overloaded the type conversion operator to something that *almost* means type conversion.

    ReplyDelete
  10. > So is the general consensus the same as Craig's - "if you see yourself typing 'public static operator', you're doing something wrong" ? <



    I'd say you'd only ever use 'conversion' to go from one immutable value type to another. You'd never want conversions for classes. You already have a type system for that (i.e. inherit from a class, or implement the appropriate interface, then just 'cast' if necessary). If a 'cast' isn't appropriate (such as when you do actually want a new and different reference type), then you should just use a method. Like Craig said, something like UpdateItem.CreateFromPrototype( Item ), or UpdateItem.Convert( Item )...



    I'd say 'conversions' are there just for the sake of value types (which aren't 'proper' objects).







    ReplyDelete
  11. I'm with Craig on this one. I think the "ToString()" analogy is an apt one. MS could have used a "cast-to-string" approach rather than a ToString() method, but didn't, and code is easier to understand because of it.



    I worked on a C++ project long ago where the developers had used overloaded type-conversion extensively to implement a token parsing system. It was some of the most ungodly hard to follow code I've worked on. Sometimes "convenient" and "clever" just translates to "hard to maintain".

    ReplyDelete
  12. Another Son of SmartPart QuickStart [Via: Jan

    Tielens ]

    C# Type Conversion Operators Considered ...

    ReplyDelete
  13. I agree with what Craig states. Redefining operators is a 'nice' thing only at first sight. From a long-term maintenance point of view, I always found out that these were more nasty beasts, carrying too much implicit meaning to be clear enough.

    ReplyDelete
  14. A first post on the benefits&nbsp;of the usage of C#'s "conversion operator": http://pluralsight.com/blogs/fritz/archive/2005/12/09/17343.aspx...

    ReplyDelete

  15. Since FireFox just

    stopped responding (i.e. opening up new pages, but at least didn't

    die (like it...

    ReplyDelete
  16. Please explaing the follwing C# concepts with examples:

    1- 'is' Operator

    2- 'as' Operator

    3- Boxing

    4- Delegates

    5- unsafe



    may another questoin that what is the difference between Method Hiding and Method Overriding regarding C# ?



    Regards

    ReplyDelete
  17. Why would I take the time to explain these concepts when you obviously haven't bothered to search out one of the many, many, many places where this information is available on the Internet?

    ReplyDelete
  18. Generally, I agree with Fritz that type conversion is nicer looking than methods.



    But here's a problem with type conversion I just ran into. The behavior of a reference type changes when it's converted while being passed. Consider the following code (which is poor, I know, but brings out the case I have in mind).



    public class Foo2

    {

    public string Property1;

    }



    public class Foo1

    {

    public string Property1;



    public static implicit operator Foo2( Foo1 data )

    {

    Foo2 foo = null;



    if (data != null)

    {

    foo = new Foo2();

    foo.Property1 = data.Property1;

    }



    return (foo);

    }



    public static implicit operator Foo1( Foo2 data )

    {

    Foo1 foo = null;



    if (data != null)

    {

    foo = new Foo1();

    foo.Property1 = data.Property1;

    }



    return (foo);

    }

    }



    public void DoStuff( Foo2 foo )

    {

    foo.Property1 = "New Value";

    }



    [STAThread()]

    public static int Main( string[] args )

    {

    Foo1 foo = new Foo1();

    foo.Property1 = "Old Value";

    DoStuff(foo);

    Console.WriteLine(foo.Property1);

    }



    Without running it, one might be inclined to expect that it would print "New Value". But it won't because it passed a new object (i.e. not your original foo object) to the method. You have to specify it as a ref or out parameter for it to work -- which is a little counter-intuitive until you realize what's going on in the code IMO.

    ReplyDelete
  19. Guess I'm a little late to this thread - but how would you solve this without a conversion operator:

    Some of the controls that come with the framework like ComboBox take only an "object" type, yet usually developers want to store an object and a string to be displayed in the combobox.

    If you add a string conversion operator to your class you can just add them to the combobox and you are done.

    The item text is automatically displayed correctly, and the object is retrievable at at any time during a selection event of via enumeration.

    Is there another way to do this without adding extra classes or methods?

    Regards -

    ReplyDelete
  20. I believe most of the controls offer properties that let you bind the display string to one of the properties of the object, while you bind the value to a different one. So that's mostly been thought of for the visual stuff.

    In many other contexts, when someone wants to display an object, they look first for an implementation of IFormattable::Format, and then for an implementation of ToString. I'm not sure how you'd ever pull this off "without adding extra classes or methods" - there's no useful universal algorithm for displaying an object.

    ReplyDelete
  21. Agree with Craig here, Casting means you still have to reference to the same object, and implies there is some kind of relationship between the two object. I will go for an old plain static method, easier to read and maintain.

    ReplyDelete