How to prevent Factory Method pattern causing warning about virtual member call in constructor?

Question

On www.dofactory.com I found a real world example of the Factory Pattern. But the code generates a warning in ReSharper about a virtual member call in the constructor.

The code causing the warning is the following:

abstract class Document
{
    private List<Page> _pages = new List<Page>();

    // Constructor calls abstract Factory method
    public Document()
    {
        this.CreatePages(); // <= this line is causing the warning
    }

    public List<Page> Pages
    {
        get { return _pages; }
    }

    // Factory Method
    public abstract void CreatePages();
}

class Resume : Document
{
    // Factory Method implementation
    public override void CreatePages()
    {
        Pages.Add(new SkillsPage());
        Pages.Add(new EducationPage());
        Pages.Add(new ExperiencePage());
    }
}

In the consuming code, you can then simply use:

Document document = new Resume();

I do understand why it's a bad idea to call a virtual member in the constructor (as explained here).

My question is how you can refactor this in order to still use the factory pattern, but without the virtual member call in the constructor.

If I'd just remove the call to CreatePages from the constructor, the consumer would have to explicitly call the CreatePages method:

Document document = new Resume();
document.CreatePages();

I much more prefer the situation where creating a new Resume is all that's needed to actually create a Resume containing pages.

@EkoostikMartin what if the OP will wake up tomorrow wanting to declare `StylishResume : Resume`? — s.m., Jun 29 '12 at 21:21
@EkoostikMartin: I don't just want the warning to go away, I want to know how else this could be implemented. Still making it possible to subclass `Resume`. — comecme, Jun 29 '12 at 21:39

score 2 · Accepted Answer · answered Jun 29 '12 at 21:21

One way to refactor this would be passing the pages upfront, and passing them to a protected constructor:

public abstract class Document {
    protected Document(IEnumerable<Page> pages) {
        // If it's OK to add to _pages, do not use AsReadOnly
        _pages = pages.ToList().AsReadOnly();
    }
    // ...
}

public class Resume : Document {
    public Resume() : base(CreatePages()) {
    }
    private static IEnumerable<Page> CreatePages() {
        return new Page[] {
            new SkillsPage(),
            new EducationPage(),
            new ExperiencePage()
        };
    }
}

P.S. I am not sure what this has to do with the factory method. Your post illustrates the Template Method Pattern.

According to [dofactory.com](http://www.dofactory.com/Patterns/PatternFactory.aspx) it is an example of a factory method. Are they wrong? — comecme, Jun 29 '12 at 21:37
@comecme Yes, I am reasonably certain that they are wrong: for one thing, factory method cannot be `void`, it has to return something. — Sergey Kalinichenko, Jun 29 '12 at 21:47

GETah · Answer 2 · 2012-06-29T21:50:24.483

1

What about this? It uses lazy initialization where the pages are created only when needed (instead of creating them in the constructor)

Also, note the factory method visibility is changed to protected to hide it from public usage.

abstract class Document{
    protected List<Page> _pages = new List<Page>();

    // Constructor calls abstract Factory method
    public Document(){}

    public List<Page> Pages
    {
        get { CreatePages(); return _pages; }
    }

    // Factory Method
    protected abstract void CreatePages();
}

class Resume : Document{
    // Factory Method implementation
    protected override void CreatePages(){
       if(pages.Count == 0 ){
        _pages .Add(new SkillsPage());
        _pages .Add(new EducationPage());
        _pages .Add(new ExperiencePage());
       }
    }
}

EDIT Suggestion: I personally do not like having that global _pages variable as it might get you into trouble if shared among many methods and threads. I would rather go for the factory method pattern as described in the GoF book. Here is my suggestion:

abstract class Document{
    public IEnumerable<Page> Pages{
        get { return CreatePages();}
    }

    // Factory Method
    protected abstract IEnumerable<Page> CreatePages();
}

class Resume : Document{
    // Factory Method implementation
    protected override IEnumerable<Page> CreatePages(){
         List<Page> _pages = new List<Page>();
        _pages .Add(new SkillsPage());
        _pages .Add(new EducationPage());
        _pages .Add(new ExperiencePage());
        return _pages;
       }
    }
}

edited Jun 29 '12 at 21:50

answered Jun 29 '12 at 21:24

GETah

20,922
7
61
103

This would keep adding pages on each access of `Pages`. I suppose inside the `Pages` property you would check if `_pages == null` and only call `CreatePages` if it is. – comecme Jun 29 '12 at 21:32
1

Your code doesnt make sense, `CreatePages()` has a void return, so what does `myResume.Pages` give me in this design? – EkoostikMartin Jun 29 '12 at 21:33
@EkoostikMartin Yeah I just saw that! Fixed now. Thanks for the tip – GETah Jun 29 '12 at 21:34
I'd prefer the check for `_pages == null` or `_pages.Count == 0` to be inside the abstract class though. – comecme Jun 29 '12 at 21:34
@comecme Yes that is one option. However, it is more flexible this way as the child class can choose what to do instead of having one common behavior for all subclasses – GETah Jun 29 '12 at 21:37
What's interesting is that the [global description of the pattern](http://www.dofactory.com/Patterns/PatternFactory.aspx) states that the Factory method _returns_ an object of type Product (Document in my example). In their real world example, `CreatePages` doesn't return anything. – comecme Jun 29 '12 at 21:44
@comecme that's what I thought too and that's why I put in the `return CreatePages()` which brought me a -1 for free :D – GETah Jun 29 '12 at 21:45
Your edited code will create new pages each time `Resume.Pages` is accessed. I also think it'd be better to initialize `_pages` in the abstract class. That way, if a derived class would implement an empty `CreatePages` method, you could still use a `foreach` loop on `Pages`. I also don't see what you mean by `_pages` being a global variable. – comecme Jun 30 '12 at 09:31

score 1 · Answer 3 · answered Feb 23 '15 at 20:15

My question is how you can refactor this in order to still use the factory pattern, but without the virtual member call in the constructor.

According to the definition

In class-based programming, the factory method pattern is a creational pattern which uses factory methods to deal with the problem of creating objects without specifying the exact class of object that will be created.

factory method is not intended to be used from the constructor, since the exact class of object that will be created is known at construction time. For example,

if Document must create all Pages at construction time, then it can do it without factory method since it knows exactly all the Pages to be created
but if Document must create Pages later after construction and maybe multiple times, then factory method is useful

I much more prefer the situation where creating a new Resume is all that's needed to actually create a Resume containing pages.

So, if all Pages are to be created at construction time, this concrete example can be rewritten without use of factory method pattern:

public class Document
    private readonly PageList as IList(of IPage)

    public readonly property Pages as IEnumerable(of IPage)
        get
            return PageList
        end get
    end property

    public sub new()
        Me.PageList = new List(of IPage)
    end sub

    protected sub Add(paramarray Pages() as IPage)
        Me.Pages.AddRange(Pages)
    end sub
end public

public class Resume
    inherits Document

    public sub new()
        mybase.add(new SkillsPage, new EducationPage, new ExperiencePage)
    end sub
end class

Method Add is allowed to be used from Resume constructor, since Document object is fully constructed and ready to be used.
Method Add is protected, so pages can only be be added from Document or derived classes.

score 0 · Answer 4 · answered Jun 29 '12 at 21:20

You can do in property itself.

In that case the Pages property can be marked as virtual in base class.

The hypothetic code for Resume could look like this:

    private List<Page> _pages  = null;
    public override List<Page> Pages
    {
          get 
          {  
                if(pages == null) 
                {
                  _pages = new List<Page>();
                  _pages .Add(new SkillsPage());
                  _pages .Add(new EducationPage());
                  _pages .Add(new ExperiencePage());
                }

                return _pages; 
           }

        }
    }

In this case, the pages will be created on first access to Pages property, and not on ctor execution.

I don't think this would qualify as a factory method, since there is no method anymore. — comecme, Jun 29 '12 at 21:36
Well, you use a `property` like a "method". Why you want toughly couple with method members... — Tigran, Jun 29 '12 at 21:41

How to prevent Factory Method pattern causing warning about virtual member call in constructor?

4 Answers4