Extension method not setting value

Question

I have a product class that looks something like this -

public class Product
{
    public int ProductId { get; set; }
    public string Name { get; set; }
}

I have an extension class that looks like this

public static class ProductExtension
{
    public static void FixProduct(this Product product)
    {
        product = new Product(){Name = product.Name.ToUpper()};
        //product.Name is now UPPERCASE
    }
}

In my Main method I have -

static void Main(string[] args)
{
    Product p = new Product() {ProductId = 1, Name = "steve"};
    p.FixProduct(); 
    System.Console.WriteLine(p.Name);
}

This prints "steve" and not what I wanted it to print: "STEVE". Why doesn't the assignment in the extension method work?

In your `FixProduct` you create a new instance of `Product`. It will be dropped by GC and does not affect your original object — Aleks Andreev, Oct 14 '17 at 19:13
`product = new Product()` redirects the variable to refer to a *new* object, not the one that was passed in as a parameter (and it should also be unnecessary, why not simply `product.Name =`) — UnholySheep, Oct 14 '17 at 19:13
You would need to pass `this product` as a `ref` parameter for the change in its value to be reflected outside the extension method. However, extension methods [do not support `ref` parameters](https://stackoverflow.com/q/2618597). You could use a [fluent style](https://en.wikipedia.org/wiki/Fluent_interface) such as `var p = new Product {ProductId = 1, Name = "steve"}.FixProduct();` — dbc, Oct 14 '17 at 19:17

John Wu · Accepted Answer · 2017-10-14T20:16:56.377

I suggest a small change to follow a fluent interface pattern. Instead of void, return the new product instead. Don't use ref, that is weird.

public static class ProductExtension
{
    public static Product FixProduct(this Product input)
    {
        return new Product
            {
                Name = input.Name.ToUpper(),
                Id = input.Id
            }
        //product.Name is now UPPERCASE
    }
}

Then use it like this:

static void Main(string[] args)
{
    var p = new Product()
        {
            ProductId = 1, 
            Name = "steve"
        }
        .FixProduct();
    System.Console.WriteLine(p.Name);
}

A neat advantage of this approach is (if you think you will need it) you can support several product classes while preserving their precise type, e.g.:

public static class ProductExtension
{
    public static T FixProduct<T>(this T input) where T: Product, new
    {
        return new T
            {
                Name = input.Name.ToUpper(),
                Id = input.Id
            }
    }
}

Now you could use it on any derived product class while keeping exactly the same syntax.

class DeluxeProduct : Product
{ }

static void Main()
{
    var p = new DeluxeProduct
        {
            Id = 1,
            Name = "Steve"
        }
        .FixProduct();
    Console.WriteLine(p.GetType().Name)); //outputs "DeluxeProduct"
}

Now on the other hand, if all you want to do is "fix" the product's name, you could just wrap it in a property.

class Product
{
    private string _name;

    public int Id { get; set; }

    public string Name
    {
        get { return _name; }
        set { _name = value.ToUpper(); } //Automatically "fix" it the moment you set it
    }
}

...and then you don't need an extension method at all.

I did this too, but it feels a little odd to have an extension method return the type of the type it is extending. — Bryan, Oct 14 '17 at 19:48
No, it is very common. In fact all of the LINQ extension methods use this pattern, as well as objects that follow a builder pattern, e.g. most IoC containers. — John Wu, Oct 14 '17 at 19:51
So in the main method: p = p.FixName(). Makes it feel like product is immutable, like when you are doing something to a string. I must be doing this all the time with LINQ and just got used to it — Bryan, Oct 14 '17 at 20:13
I figured this is just an example and there is more going on. But if you just want to force the name to uppercase, maybe wrap in a property-- I'll edit my answer to suggest this at the end. — John Wu, Oct 14 '17 at 20:20
This is a gross simplification of what I'm really doing, but it has validated that what i was originally trying can not work. Thanks for you suggestions. — Bryan, Oct 14 '17 at 20:24

Gilad Green · Answer 2 · 2017-10-14T19:47:47.823

Extension methods cannot be used that way. In your method you create a new instance of Product and then assign it to product which is a local reference to the passed object, and not the original reference p.

When you first enter the function what you have is two references referencing the same object in memory.

Then just before exiting the method you have two objects, one referred by each reference, with the product reference, referencing a local variable being cleaned by the GC at the end of the method call.

Solutions:

To correct this and have it closest to what you were trying to do, change your method to get a ref parameter:

public static void FixProduct(ref Product product)
{
    product = new Product() { Name = product.Name.ToUpper() };
    //product.Name is now UPPERCASE
}

and then:

ProductExtension.FixProduct(ref p);

I believe a better approach all together will be (by having it a member function or an extension method) to update the object instead of instantiating a new one:
```
public static void FixProduct(this Product product)
{
    product.Name = product.Name.ToUpper();
}
```

A more elegant way would be to place it in the original `Product` class (if possible), such as `Product.Fix(prod);` — Ian H., Oct 14 '17 at 19:17
@IanH.- I agree, or still to use an extension method but jut update the object instead of creating a new one — Gilad Green, Oct 14 '17 at 19:19
@Bryan - you are welcome :) I'd suggest to go with the second solution I've listed — Gilad Green, Oct 14 '17 at 19:50

score 4 · Answer 3 · answered Oct 14 '17 at 19:21

4

In your extension method, you are assigning a new Product to the variable product. This doesn't end up affecting the original referenced Product.

Modify the method to the one below to set the name on the original passed in object.

public static void FixProduct(this Product product)
{
    product.Name = product.Name.ToUpper();
}

answered Oct 14 '17 at 19:21

jdphenix

15,022
3
41
74

Cool, I think you were editing your post while I was writing my answer. – jdphenix Oct 14 '17 at 19:23

Tom Blodget · Answer 4 · 2017-10-15T14:48:41.147

Parameters are passed by value unless they are ref or out. this doesn't change that. You can understand this syntactically because ref and out require a variable reference; otherwise only an expression is required.

Unfortunately, you can't combine this with ref or out.

You can change the value of any parameter variable, though, except in the case of ref or out, it's best avoided or limited to quick touch-ups to the passed-in value that simplify later algorithmic code.

A method is permitted to assign new values to a value parameter. Such assignments only affect the local storage location represented by the value parameter—they have no effect on the actual argument given in the method invocation. — C# Language Specification

So, the assignment does work, just not in the ref or out way.

Extension method not setting value

4 Answers4