CODEWITHSUNDEEP
c#.net
Xaisoft
Xaisoft

Reputation: 46651

Why does this cause a StackOverFlow error?

This is causing a StackOverFlow error and I have an idea why, but I would like a little more detail on why and is my solution for it the way it should be handled. Ok, first things first, the following code causes a StackOverFlow error when I try to assign a value to the property:

private List<Albums> albums
{
    get
    {
        if (Session["albums"] != null)
            return (List<Albums>)Session["albums"];
        else
            return AlbumCollection.GetAlbums();
    }
    set
    {
    albums = value;
    Session["albums"] = albums;
    }
}

To resolve the above, I changed the name of the property and added another variable to hold the value of the property which resolved the StackOverFlow issue:

private List<Albums> albums = null;
private List<Albums> Albums
{
    get
    {
        if (Session["albums"] != null)
            return (List<Albums>)Session["albums"];
        else
            return AlbumCollection.GetAlbums();
    }
    set
    {
    albums = value;
    Session["albums"] = albums;
    }
}

Also, am I doing the setter correct, assigning the value and then assigning the Session["albums"] the value in albums? Could I have just done, Session["albums"] = value instead?

Upvotes: 0

Views: 467

Answers (8)

George Polevoy
George Polevoy

Reputation: 7681

It's a common mistake to call Session["Albums"] twice. Session indexing is a relatively expensive operation involving a dictionary lookup.

private List<Albums> Albums {    
get
{        
     object stored = Session["albums"];
     if (stored != null)
          return (List<Albums>) stored;
     else
     {      
        var newValue = AlbumCollection.GetAlbums();
        Albums = newValue;
        return newValue;
     }
}    
set    
{    
    Session["albums"] = value;
}}

Upvotes: 0

Charles Bretana
Charles Bretana

Reputation: 146607

Because in your setter, you are calling ... the setter, which goes to the setter, and calls ... the setter ... and ... 

 set
 {    
    albums = value;       // < --- This line calls itself again.. 
    Session["albums"] = albums;
 }

What you need to do is just use the the Session["albums"] as the persistent storage for the value... You don't need a private field - that's just creating a redundant copy of the value. Eliminate it entirely, and just put...

private List<Albums> Albums
{    
    get    
    {        
         if (Session["albums"] != null)
              return (List<Albums>) Session["albums"];        
         else            
            return (Session["albums"] = AlbumCollection.GetAlbums());    
    }    
    set    
    {    
        Session["albums"] = value;    
    }
}

In certain scenarios where you do not have a persistent store, it's perfectly acceptable for a public property to have just a private member backing field.

For more info on C# properties in general, check out the MSDN tutorial.

Upvotes: 16

Adriaan Stander
Adriaan Stander

Reputation: 166596

You were reassigning to the property itself.

In your case, you are only using the Session.

So this should be fine

private List<Albums> albums
{
    get
    {
        if (Session["albums"] == null)
            Session["albums"] = AlbumCollection.GetAlbums();
        return (List<Albums>)Session["albums"];
    }
    set
    {
        Session["albums"] = value;
    }
}

Upvotes: 11

SoapBox
SoapBox

Reputation: 20609

You are correct, the issue is when ever you use "albums" in the first code block you are referring to this setter/getter. Thus when you do albums = value in the setter you are recursively calling the setter again.

Internally, the compiler converts accessors to functions, and it may help you to see your error by doing this yourself:

private List<Albums> albums
{
    set
    {
    albums = value;
    Session["albums"] = albums;
    }
}

When compiled, becomes:

private void set_albums(List<Albums> value)
{
    set_albums(value);
    Session["albums"] = albums;
}

Upvotes: 1

Paolo
Paolo

Reputation: 22646

You are correct, in the first example you are recursively calling the album setter infinite times, hence the stack overflow. (C# Properties and methods should always start with an uppercase letter btw).

In the second example you could simply use:

Session["albums"] = value;

if you wanted to.

Upvotes: 1

Justin Niessner
Justin Niessner

Reputation: 245489

This line is causing your issue, because it winds up calling your getter recursively in an infinite loop:

albums = value;

Upvotes: 3

David M
David M

Reputation: 72930

Your setter is calling itself recursively in the first example. Your second fixes this.

Yes, you could have done.

Upvotes: 1

Mehrdad Afshari
Mehrdad Afshari

Reputation: 422260

The problem is with this line:

albums = value;

You are recursively setting the property to value which will call the setter again and again, until it stackoverflows. There's no point in that line of code. Just get rid of it.

I guess there's a false misconception that a property needs to be bound to a field or something. It's not. A property, by itself, is just a couple unrelated methods, that are not required to have any specific relation to each other or to a field. When you retrieve the property value, you just call its get method and use the return value and when you set its value, you call its set method with the appropriate value argument. You don't need to somehow "change" the property value in setter. The semantics are automatically enforced as you are changing the value that get is going to return so the next time you call get, it will return Session["..."] which you have already changed.

Upvotes: 5

Related Questions