Inefficient code inside of getter?

Question

I have two lists of the same type and I want to be able view a list of both lists:

    public List<Page> HeaderPages;
    public List<Page> SurveyPages;

    public IReadOnlyList<Page> AllPages 
    { 
        get 
        {
            List<Page> allPages = new List<Page>();
            allPages.AddRange(this.HeaderPages);
            allPages.AddRange(this.SurveyPages);
            return allPages;
        } 
    }

I think it is very inefficient to keep creating a new list object each time I want to get AllPages, so I thought I could store a private allPages list. But then I thought if I am in the class, I would have to remember to use the public AllPages rather than the private allPages - which may not be up-to-date.

Any elegant solutions?

Answer 1

If you have control over the design of the Page class, I would add a "bool IsHeader" property to it and keep one public list of all Page objects and add every Page object to that list. Instead of keeping several other lists that sort your Page objects, you can then perform searches on that master list instead for Page objects with certain properties, preferably with LINQ statements.

Answer 2

If you want to keep the two lists (otherwise check Hackworth's answer), the best solution would be to use LINQ extensions

public IEnumerable<Page> AllPages 
{ 
    get 
    {
        return HeaderPages.Concat(SurveyPages);
    } 
}

This way you do not create any unnecessary collections. It forces you to use IEnumerable<Page> though, but in my opinion it is a good thing.

Answer 3

As you said, keep another List which will contains all the elements and when you add an element inside one of the two update the internal List ... you could make List private and make helper method which update the main list and the internal List.

With your code anyway you will create a List every item it's called and I think it could lead to bugs if you keep the reference sonewhere and you except it's updated (it's isn't, will keep values of when is created) with the latest pages.

Another way is to keep your approch but create the List only once and return it. Then you should find a way to let know to the class if one of the List changed, if changed update the list (clear and readd to avoid a new instance)

I could add code example if you need.

Answer 4

You can use lazy initialization to achieve the same:

private Lazy<List<Page>> _lazyPageList = new Lazy<List<Page>>(GetPageList);

public IReadOnlyList<Page> AllPages
{
    get
    {
        return lazyPageList.Value;
    }
}

private List<Page >GetPageList(){
    List<Page> allPages = new List<Page>();
    allPages.AddRange(this.HeaderPages);
    allPages.AddRange(this.SurveyPages);
    return allPages;
}

Answer 5

Have you profiled your code and determined that this call is problem? If not, do that before optimising anything.

There is a general solution to problems like this, which is to use a dirty flag to track whether the return value is valid. To do this you will want to wrap all the cases when the header and survey pages can be modified in a manner that breaks the allpages list and have these calls set the dirty flag. Then replace your getter with something like this:

private List<page> allPages;
private boolean dirty;

public IReadOnlyList<Page> AllPages 
{ 
    get 
    {
        if (dirty)
        {
           allPages = new List<Page>();
           allPages.AddRange(this.HeaderPages);
           allPages.AddRange(this.SurveyPages);
        }
        return allPages;
    } 
}

Unfortunately dirty flags can be error prone so tread with care.

Answer 6

Two alternatives to Hackworth's answer if you don't have control of the Page class:

• Either, create two subclasses HeaderPage and SurveyPage, bother/either of which you can store in the List<Page> AllPages
• Or, add a Dictionary<Page,bool> in which to remember the type of page (the 'type of page' is shown here as a bool, but could be an enum) for each page in your single List<Page> AllPages

Access to that dictionary could be defined as an extention method:

static class PageExtensions
{
    // Or this could be a non-static member of your container class and passed
    // as a parameter into the GetPageType and SetPageType extension methods.
    static Dictionary<Page, bool> dictionary = new Dictionary<Page, bool>();

    public static bool? GetPageType(this Page page)
    {
        bool rc;
        return (dictionary.TryGetValue(page, out rc)) ? rc : null;
    }
    public static void SetPageType(this Page page, bool type)
    {
        dictionary[page] = type;
    }
}

Answer 7

What are people going to want to do with AllPages, and are they going to have any expectations about how it should behave if the underlying data changes?

If the intention is that code which reads AllPages isn't going to use it after the next time anything changes, you could have each instance of your self create (or lazily create) an instance of AllPageWrapper, which holds a reference to its creator along with a "change count" and possibly the number of items in HeaderPages. Methods and properties of AllPageWrapper would ensure the wrapped object had not been modified, and would then use that wrapped object as a source of data. The indexed getter might look something like:

public Page this(int index)
{ get
  {
    if (changeCount != parent.changeCount)
      throw InvalidOPerationException(...);
    if (index > headerCount)
      return parent.SurveyPages(index-headerCount);
    else
      return parent.HeaderPages(index);
  }
}

Such an approach would allow the AllPages property to simply return a wrapper object, without having to do any real "work". Code which needs a detached copy of the list could use use thing.AllPages.ToList(), but code which will need it only briefly need not bother.