Stack overflow error in C# set/get [duplicate]

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

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.