Reassigning member variables of a struct through properties

Question

So I have a struct that is defined in the following manner:

    public struct Item
{
    public string _name { get; set; }
    public double _weight
    {
        get 
        {
            return _weight;
        }
        set
        {
            _weight = value;

            //Shipping cost is 100% dependent on weight. Recalculate it now.
            _shippingCost = 3.25m * (decimal)_weight;

            //Retail price is partially dependent on shipping cost and thus on weight as well.  Make sure retail price stays up to date.
            _retailPrice = 1.7m * _wholesalePrice * _shippingCost;
        }
     }
    public decimal _wholesalePrice
    {
        get
        {
            return _wholesalePrice;
        }
        set
        {
            //Retail price is partially determined by wholesale price.  Make sure  retail price stays up to date.
            _retailPrice = 1.7m * _wholesalePrice * _shippingCost;
        }
    }
    public int _quantity { get; set; }
    public decimal _shippingCost { get; private set; }
    public decimal _retailPrice { get; private set; }


    public Item(string name, double weight, decimal wholesalePrice, int quantity) : this()
    {
        _name = name;
        _weight = weight;
        _wholesalePrice = wholesalePrice;
        _quantity = quantity;
    }
//More stuff

I also have an instance of Item in a different class. When I try to invoke the weight property through the following command, the program crashes:

currentUIItem._weight = formattedWeight;

A descriptive error is not provided. Note that at this point, currentUIItem has been newed with the parameterless default constructor. Now, here is the weird part. When I remove the custom implementation of weight's set property and replace it with a generic { get; set; }, the assignment works flawlessly.

Does anyone know what's going on here? Is this a quirk of structs that would work fine with classes?

Structs should be immutable : http://stackoverflow.com/a/3753640/588868 — Steve B, Jul 29 '12 at 20:47
Everything about this says it should be a `class`, not a `struct`. It won't impact the infinite recursion, but: worth noting. — Marc Gravell, Jul 29 '12 at 21:01
Why do you say that this should definitely be a class? I read that structs *should* be immutable, but since I am only working with a single thread, I do not see how I am going to inadvertently experience problems. — Nick, Jul 29 '12 at 21:09
I disagree with the notion that it should be a class, but I don't like the design of the struct. Whenever practical, structs should be designed so that their state can be contained entirely in public fields which will behave sensibly, and should have no members, other than constructors, that modify `this`. I would eliminate the `_ShippingCost` and `_RetailPrice` fields, but instead have them as read-only properties which are computed based on `Weight` and `WholesalePrice` unless you want to allow the retail price to be explicitly set to 'override' that computation. In that case... — supercat, Aug 13 '12 at 19:15
...I would suggest perhaps having a `RetailPriceOverride` field with defined semantics, or perhaps using an immutable struct or class. I would generally recommend against using a mutable class, since their semantics can be murky unless they're used in a way very different from mutable ones. — supercat, Aug 13 '12 at 19:18

score 3 · Answer 1 · answered Jul 29 '12 at 20:50

3

You have recursive call to _weight property in your _weight property's set method.

answered Jul 29 '12 at 20:50

Embedd_0913

16,125
37
97
135

Glen Hughes · Accepted Answer · 2012-07-29T20:58:18.083

It looks like this would cause a StackOverflowException because you have an infinite recursion.

If you put a breakpoint in your setter:

public double _weight
{
    set
    {
        _weight = value;
    }
 }

You will see that the breakpoint continues to get hit. This is because the setter tries to set a value to _weight. But _weight is not a variable... so when you try to set its value, you are only calling back into the setter method. This continues to happen infinitely. The same applies to the _wholesalePrice property... you probably want something more like this:

public struct Item
{
    public string _name { get; set; }

    private double _weightInternal;
    public double _weight
    {
        get 
        {
            return _weightInternal;
        }
        set
        {
            _weightInternal = value;

            //Shipping cost is 100% dependent on weight. Recalculate it now.
            _shippingCost = 3.25m * (decimal)_weightInternal;

            //Retail price is partially dependent on shipping cost and thus on weight as well.  Make sure retail price stays up to date.
            _retailPrice = 1.7m * _wholesalePriceInternal * _shippingCost;
        }
    }

    private decimal _wholesalePriceInternal;
    public decimal _wholesalePrice
    {
        get
        {
            return _wholesalePriceInternal;
        }
        set
        {
            //Retail price is partially determined by wholesale price.  Make sure  retail price stays up to date.
            _wholesalePriceInternal = value;
            _retailPrice = 1.7m * _wholesalePriceInternal * _shippingCost;
        }
    }
    public int _quantity { get; set; }
    public decimal _shippingCost { get; private set; }
    public decimal _retailPrice { get; private set; }


    public Item(string name, double weight, decimal wholesalePrice, int quantity) : this()
    {
        _name = name;
        _weightInternal = weight;
        _wholesalePriceInternal = wholesalePrice;
        _quantity = quantity;
    }
    //More stuff
}

I didn't realize that the property was invoked every single time the weight variable was referenced. That's good to know! Thank you! — Nick, Jul 29 '12 at 21:10

score 1 · Answer 3 · answered Jul 29 '12 at 20:53

The Setter of _weight property is calling its self recursively and causing StackOverFlow exception

The easiest way to fix this will be have a backing field for the Property this way

private double  _weight;
public double Weight
    {
        get 
        {
            return _weight;
        }
        set
        {
            _weight = value;

            //Shipping cost is 100% dependent on weight. Recalculate it now.
            _shippingCost = 3.25m * (decimal)_weight;

            //Retail price is partially dependent on shipping cost and thus on weight as well.  Make sure retail price stays up to date.
            _retailPrice = 1.7m * _wholesalePrice * _shippingCost;
        }
     }

score 1 · Answer 4 · edited May 23 '17 at 11:48

In the setter for _weight, you are trying to set _weight, so this results in an infinite recursion.

Try something like (not including the additional logic you have for retail and shipping costs):

private double _weight;
public double Weight
{
     get { return _weight; }
     set { _weight = value; }
}

Using just get; set; tells the compiler to generate a backing field automatically for you and has the same effect as the above code.

Note also the naming convention used, fields (_weight) are prefixed with an underscore and should always be private. The outside world interacts with fields using properties (Weight). This pattern is in place all throughout the .NET framework, see What is the difference between a Field and a Property in C#? for more information.

Reassigning member variables of a struct through properties

4 Answers4