Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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?

share|improve this question
    
Can you post an example of some code which demonstrates the problem? You're asking us to review a hypothetical class which contains a private allPages list, and a hypothetical method which finds it difficult to remember whether to reference allPages or AllPages ... but it's difficult to understand hypothetical (non-existent) code. –  ChrisW Mar 26 at 10:06
    
I don't know C# enough to write an answer, but what if everytime the get was called you saved the result in an instance variable and set something like "changed = false" alse as an instance variable. Whenever get is called, if changed is false just return your cached list. Then anything in the class that changed the other two lists sets changed to true. –  Cruncher Mar 26 at 20:14
    
Do you actually need to optimise this? How often do you call the getter? –  Jack Aidley Mar 27 at 11:45
add comment

7 Answers 7

up vote 13 down vote accepted

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.

share|improve this answer
    
I think this is a really nice advice. –  Nikita Brizhak Mar 26 at 12:12
    
Even if he has control over Page class, how do you know both HeaderPages and SurveyPages are not non-empty? –  abuzittin gillifirca Mar 26 at 13:11
2  
I would do it completely without HeaderPages and SurveyPages, that's the whole point of my answer. –  Hackworth Mar 26 at 13:24
    
I agree with this because it's a far more elegant solution, however if the OP is worried about the overhead of instantiating a single list, then doing O(n) operations on all data accesses might not be their favourite. They'd be wrong, of course, as n is almost certainly trivial. –  Phoshi Mar 26 at 14:09
    
I do have control of the Page class. In the end I decided this method was best suited to my project. –  chazjn Mar 27 at 13:07
show 1 more comment

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.

share|improve this answer
    
+1 The difference between IReadOnlyList and IEnumerable is (only) that IReadOnlyList also supports Count and Index properties. –  ChrisW Mar 26 at 18:45
    
Thanks, I really like this very clean solution and and will definitely try it out on other projects. –  chazjn Mar 27 at 13:05
add comment

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.

share|improve this answer
add comment

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;
}
share|improve this answer
    
And what good will it do? You will still have to update AllPages somehow when other collections change. –  Nikita Brizhak Mar 26 at 12:11
    
What I understand from the original question the lists do not change. The idea is to avoid creating a new list everytime AllPages property is accessed. @chazjn doesn't want to create a member variable to store the result. Lazy class is best suited for this. –  Sandeep Mar 26 at 12:50
    
from question: "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." Introducing Lazy property does not solve this issue, it only adds extra complexity to the code. (you still create an extra field _lazyPageList) –  Nikita Brizhak Mar 26 at 12:57
    
I agree with you that new changes will not be updated with lazy approach. However whether you use the linq extension or the code in the original post a new list will be created every time the property is accessed. A better approach would be to store the result temporarilty to a variable and update it only if the either of the list has changed –  Sandeep Mar 27 at 6:38
1  
linq does not create new list. It creates new enumeration logic over existing collection(-s). To create a new list you'd have to call ToList() method. –  Nikita Brizhak Mar 27 at 7:19
add comment

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.

share|improve this answer
1  
There's a bug in the sample code: allPages names a local variable and a member. Also, if dirty would you prefer allPages = new List<Page>(); or allPages.Clear();? –  ChrisW Mar 27 at 12:15
    
@ChrisW I would say just create a new list and let the old one get collected. If you try to do the Clear, you need to check to see if the allPages is initialized first. –  BPugh Mar 27 at 12:50
    
@ChrisW: Thanks, I've fixed that bug. I don't know which is more efficient in C#, you'd have to time it. –  Jack Aidley Mar 27 at 15:02
add comment

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;
    }
}
share|improve this answer
1  
Bool means nothing in this context. I would change it to an enum that names the page type. –  Jeff Vanzella Mar 27 at 16:41
    
I did say, "could be an enum" in the answer. Alternatively rename the associated extensions methods, e.g. to SetIsHeader instead of SetPageType. –  ChrisW Mar 27 at 16:58
add comment

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.

share|improve this answer
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.