Stack overflow error in C# set/get

Question

I was working on a public comments part of an application on Friday when I got a stack overflow error, which confused me so I thought I'd ask for help. And searching the web using the expression 'stack overflow' is a bit self-defeating!

I wanted to do an HtmlEncode on the set statement of the field in the class, before sending an instance of the class to be added to the database:

public class Feedback
{

    public Feedback() { }

    public string FeedbackComment
    {
        get { return FeedbackComment; }
        set {System.Web.HttpUtility.HtmlEncode(value); }
    }

    // other fields 

    // methods
}

This was causing StackOverflow errors, I've fixed the error by changing the code to look like this:

public class Feedback
{

    public Feedback() { }

    private string feedbackComment;

    public string FeedbackComment
    {
        get { return feedbackComment; }
        set { feedbackComment = System.Web.HttpUtility.HtmlEncode(value); }
    }

    // other fields 

    // methods
} 

But I just wanted an explanation of why the first get/set statements were so recursive that they caused a stack overflow but when reverting the code to look more like c#2.0 worked? Can this be achieved with the shorter syntax and if so how?

This is my first question on SO - please try to be gentle!

Answer 1

The getter of the first example is returning the property itself, not a backing field.

// The property name is "FeedbackComment"
public string FeedbackComment
{
    // And here you are returning "FeedbackComment" which is
    // creating the stack overflow
    get { return FeedbackComment; }
}

Unfortunately there is no way to shorten what you have, automatically implemented properties (i.e. public String FeedbackComment { get; set; }) must have empty getter and setter blocks to be syntactically correct. There is nothing wrong with your second example - yes, it is a bit verbose but it is clear, concise, and gets the job done.

Answer 2

You're getting a stack overflow in the first code because your property getter is returning the property itself.

This will cause the getter to be invoked again, and again until your stack overflows.

Answer 3

The code that was causing a StackOverflowException doesn't provide a backing field for the property, the get accessor is returning the property itself which is what causes the stack overflow. The second example does provide a backing field and returns its content.

Answer 4

It sounds like you are trying to use the auto-implemented properties feature introduced in C# 3.0, but making a bit of a mess of the syntax.

Returning FeedbackComment in the get accessor of the FeedbackComment propery is creating a self-referential loop that keeps 'getting' the property, so no surprise about the stack overflow there!

The correct syntax for an auto-implemented property is the following. However, it cannot perform any processing in either the get or set accessor (by definition really).

public class Feedback
{
    public Feedback() { }

    public string FeedbackComment
    {
        get;
        set;
    }

    // other fields 

    // methods
}

In your case, since you want to do processing on the 'set' accessor, doing it the standard way of using a backing field is what you want.

Answer 5

The getter references itself (as Andrew points out) but the setter is also wrong.

This code:

set { System.Web.HttpUtility.HtmlEncode(value); }

...doesn't actually set anything. The HtmlEncode method returns the encoded value, it does not actually change value.

Another thing you should keep in mind is that if you are HtmlEncode-ing on the way in, you need to HtmlDecode on the way out, otherwise you can end up with multiple encodings (which are not idempotent). If you're trying to "automate" the encoding processes then the class normally looks something like this:

public class Foo
{
    private string bar;

    public string Bar
    {
        get { return HttpUtility.HtmlDecode(bar); }
        set { bar = HttpUtility.HtmlEncode(value); }
    }

    public string SafeBar
    {
        get { return bar; }
    }
}

Or you can reverse the safe/unsafe logic, for example:

public string Bar
{
    get { return bar; }
    set { bar = HttpUtility.HtmlEncode(value); }
}

public string UnsafeBar
{
    get { return HttpUtility.HtmlDecode(value); }
}

Either way your class should make explicit the fact that it is doing some sort of encoding, otherwise if you write code like this:

Foo foo1 = new Foo();
foo1.Bar = "<test>";
Foo foo2 = new Foo();
foo2.Bar = foo1.Bar;

...then you'll start seeing a bunch of ugly escape characters in the output of foo2.Bar. Make your class's contract clear, it should either perform both the encoding and decoding or do neither one.

Answer 6

I think you lack fundamental understanding of properties. A property cannot hold any data, it is just a pair of a getter method and a setter method (there are also properties which only have getters or setters).

The getter method is basically a method which takes no arguments and returns a value of the property's type, the setter on the other hand is a method with no return value and an argument of the property's type called value. C# hides this both methods and combines them to a property and you can call them like a normal field.

Your first implementation is equivalent to:

public class Feedback
{
    public string get_FeedbackComment()
    {
        return get_FeedbackComment();
    }

    public void set_FeedbackComment(string value)
    {
        System.Web.HttpUtility.HtmlEncode(value);
    }
}

You may see now where the recursion is and where your error lies. In addtion, when you take a look at the setter, you will notice that it does not set anything. The return value of HtmlEncode will not be saved anywhere. You need to provide a backing field (like the one in your second piece of code).

There are however automatically implemented properties in C# 3.0 and higher, which you declause in the following way. Please be also aware that the C# compiler will create a backing field automatically, so basically both ways are the same but you gain more flexibility with the first because with automatically implemented properties you cannot add more complex behaviour than just simple setting and retrieving of values (at least in the class where you declare it, making it virtual opens possibilites to extend property logic in subclasses..).

public class Feedback
{
    public string FeedbackComment
    {
        get;
        set;
    }
}

Best Regards,
Oliver Hanappi

Answer 7

Remember that properties are really methods - they compiler will convert them to T get_Property() and set_Property(T value) calls - there is no storage (unless using automatic proprieties, but what happens there is the compiler creates a backing field automatically. So what your stack overflow example looks like is:

public class Feedback
{

    public Feedback() { }

    // Getter
    public string get_FeedBackComment() {
        return get_FeedBackComment();
    }
    // Setter
    public void set_FeedBackComment(string value) {
         System.Web.HttpUtility.HtmlEncode(value);
    }
}

So your getting was a function call that called it self forever, so the stack overflowed because each call is a stack push. And the setting called a function, but never stored its value anywhere.

Answer 8

This one is great example for elaborating misuse Recursive programming ;)

Problem is not with respect to properties but in general rtreiving value from any method like:

public int GiveMeValue()
{
  return GiveMeValue();
}

public void SetValue(int value)
{
   SetValue(value);
}

Anyway DotNet properties are special type of methods. ain't they?