Prevent a setter from re-running an action when it's being deserialized with Newtonsoft JSON.NET

Question

So I was creating a Monopoly console app to explore some design concepts as well as to try to understand Test-Driven Development better.

That being the case, I created unit tests to make sure that all of my classes can be serialized and deserialized properly (because I'm going to need that later and if they can't be I'd rather know that sooner than later so I can avoid rework; doing so has, in fact, helped me catch several flaws in my initial design).

This property is part of a larger Property class. Owner is the player that owns the property.

What I'm trying to achieve here is that when a player mortgages a particular property, the property should be marked as "Mortgaged" and the player should receive half of the purchase price in cash. When they unmortgage the property, they pay the money back. There will be a few other checks that I haven't written yet as well; for example, game rules specify that you're not allowed to mortgage a property while it still has houses on it. I also don't yet check to see if the player has enough money to unmortgage the property, but I plan to add that shortly as that's a trivial rule to implement. That's why I'm using a setter here instead of an auto-generated property.

Here's the problem: this serializes and deserializes without throwing an exception, but if I serialize a mortgaged property it'll add the money a second time when I deserialize it (i.e. deserializing basically has the effect of re-mortgaging the property), so needless to say the unit test fails (which it should given that there's an obvious bug). I suppose that that's not surprising given that that's exactly how I wrote it - when you change its value it'll either mortgage or unmortgage the property.

Here's the code:

[JsonProperty(Order = 1)]
public Player Owner
{
    get;
    set;
}

    private bool _isMortaged = false;

    [JsonProperty(Order = 2)]
    public bool IsMortgaged
    {
        get { return _isMortaged; }
        set
        {
            if (!IsOwned)
            {
                throw new InvalidOperationException("You can't mortgage a property that no one owns");
            }

            // If we're setting this property to the same thing that it
            // already was, log it. You could argue that that would indicate
            // a bug (it probably would), but throwing an exception here
            // breaks deserialization.
            Debug.WriteIf(value == _isMortaged, $"Setting IsMortgaged to the same thing as it was before. Stack trace:{Environment.NewLine}{(new StackTrace()).ToString()}");

            // If we're setting this to the same thing that it already was,
            // then the setter should have no effect at all.
            if (value != _isMortaged)
            {
                // The player is mortgaging the property
                if (value)
                {
                    _isMortaged = true;
                    Owner.Money += (PurchasePrice / 2);
                }
                // The player is unmortgaging the property
                else
                {
                    _isMortaged = false;
                    Owner.Money -= (PurchasePrice / 2);
                }
            }
        }
    }

Is there an easy way to have a different setter for deserialization, or to tell whether or not I'm deserializing in the setter? (The latter seems like a "hack"). I suppose that there's also the possibility of writing a custom deserializer, which would solve the problem I guess, but I'd much rather avoid that if possible (it doesn't seem like a particularly "clean" way of doing this).

Alternatively, is this just a design flaw?

Answer 1

This is a design flaw; setters shouldn't have side effects beyond the obvious of updating a value (and those that are expected eg. INotifyPropertyChanged)

If you need special behaviour, then either introduce a special setting method, or think of it the other way around:

You're mortgaging/un-mortgaging a property, so you want the owner of that property's balance to update

Instead of having the property update the owner's balance (which it shouldn't concern itself with: Separation of Concerns; you could instead have the owner's balance property be dynamically computed:

public class Property {
    public bool IsMortgaged {get;set;}
}

public class Player
{
    private List<Property> _properties = new List<Property>();
    private double _cash = 0;

    public int AvailableMoney =>
        _cash + _properties.Where(p => p.IsMortgaged).Select(p => p.MortgageValue).Sum();
}

Now, this will obviously need to be updated to properly match your model, but ideally you want your data to be as dumb as possible; whether a property is mortgaged or not is a simple true/false. It's the balance that is special, and there's no reason not to re-compute it.

Answer 2

This is a violation of the Single Responsibility or Separation of concerns.

The way I would go about this would be to have a private setter on Property.IsMortgaged and use some JSON.NET functionality that exists already to deserialize it.

This would need some new methods on Property and Owner

For Property you would need to add:

public class Property
{
    public double Value {get;} = 5000.00

    private bool _isMortgaged

    public bool SetMortgaged()
    {
        //Validation for if the property can be mortgaged goes here, return false if fails
        if(_isMortgaged == true)
        {
            return false;
        }

        _isMortgaged = true;

        return true;
    }
}

and on the Owner class

public class Owner //or player
{
    private double _cash;

    public double AvailableCash
    {
        get {return _cash;}
    }

    public void MortgageProperty(Property item)
    {
        if(item.SetMortgaged())
        {
            _cash += item.Value / 2
        }
        else
        {
            //Throw an error or something else to let the user know it failed
        }
    }
}

This will encapsulate the logic in the respective classes, the Property will know if it can be mortgaged and the Owner takes care of the money.