How to prevent Factory Method pattern causing warning about virtual member call in constructor?
-
18-06-2021 - |
Frage
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.
Lösung
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.
Andere Tipps
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;
}
}
}
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 allPages
at construction time, then it can do it without factory method since it knows exactly all the Pages to be createdbut if
Document
must createPages
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 fromResume
constructor, sinceDocument
object is fully constructed and ready to be used.Method
Add
is protected, so pages can only be be added fromDocument
or derived classes.
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.