Wednesday, November 30, 2011

Feedback Requested: Is there any valid usage for the ‘new’ keyword?

Yesterday I ended up being part of a discussion about using the ‘new’ keyword to hide base-class members. A colleague of mine used it to alter a base-class property in a derived class with the purpose of making it more strongly typed.

I’ve always rationalized guideline AV1010 (Don’t hide inherited members with the new keyword) by referring to the Liskov Substitution Principle and claiming that if you need it, then you’re probably facing a design smell. In this particular case that was indeed the issue, so after fixing it, the keyword wasn’t necessary at all.

But, his arguments did make sense. In fact, he was so convinced about it that he sent me a proposal for an exception to the C# Coding Guidelines. As part of the discussion, he also sent me some background info on the keyword’s origin by C# co-author Eric Lippert.

This is the example he claims is a valid and useful exception to the guideline. The purpose was to ensure that a manager always has a smart phone rather than any other type of phone.

    public class Phone
{
}

public class SmartPhone : Phone
{
}

public class Employee
{
public Employee(Phone phone)
{
Phone = phone;
}

public virtual Phone Phone { get; }
}

public class Manager : Employee
{
public Manager(SmartPhone phone) : base(phone)
{
}

public new virtual SmartPhone Phone
{
get { return (SmartPhone)base.Phone; }
}
}

As far as I see it, the Manager class is violating the contract defined by the Employee class, and thus violates the LSP. If an employee can have any type of phone, and a Manager cannot, than clearly, a Manager is not an Employee. They may share some characteristics, but that might be solved by a common base-class.


Small Liskov Substitution Principle poster


I don’t easily loosen guidelines unless there’s a really sensible exception, but I do like to give any suggestion serious consideration. So please provide me your feedback. Is there any valid reason for using the ‘new’ keyword other than for legacy purposes (where you can’t change some base-class code) and which is not a design smell?


Let me know by commenting on this post, by sending me email or tweeting me at @ddoomen.

3 comments:

Pavlo Glazkov said...

Actually the example that you provided doesn't violate LSP according to its definition:

LSP "states that, in a computer program, if S is a subtype of T, then objects of type T may be replaced with objects of type S (i.e., objects of type S may be substitutes for objects of type T) without altering any of the desirable properties of that program (correctness, task performed, etc.)"

An instance of Manager can be used without a problem in places where an instance of Employee is expected. And the code that works with Employee doesn't need to downcast in order to work with instances of Manager.

Furthermore, when you apply the "new" keyword you effectively say that "this method doesn't have anything to do the method from the base class, it just happens to have the same name". It just hides the method from the base class. So by definition it cannot violate Liskov.

That said, the provided example looks like a valid use of the "new" keyword. It just adds some convenience.

An example of a "smelly" use of the "new" keyword would be if the new Phone property would return something different than "base.Phone". In that case the code that works with instances of Manager would get one data and the code that works with Employee would get different data.

Gitte Vermeiren said...

Why don't you solve this with some generics and interface magic. You can easily make an EmployeeBase (although I hate the Base extensions ... and which can also be a WP7 phone ;) ).

Gitte Vermeiren said...

Why don't you solve this with some generics and interface magic. You can easily make an EmployeeBase (although I hate the Base extensions ... and which can also be a WP7 phone ;) ).