Reflection
I don't like it either. It is complex and it is using reflection.
Assuming the generic type being used has a parameterless constructor, you can call new T()
without needing to resort to reflection, by putting the related generic constraint on the generic type.
public class MyClass<T> where T : new()
{
protected T DoStuff()
{
var newObject = new T();
}
}
There is currently no equivalent approach for parametered constructors.
Factories
Factories have two main use cases. In either case, the goal is to give the factory an "expertise" that is necessary for object creation, and which is complex enough to warrant its own class.
The first use case, which I call a "technical" factory, has the factory create the precise type you want, and its expertise lies in knowing how to construct such an object.
public class TechnicalFactory()
{
public FooService GetFooService()
{
return new FooService(
new SomeComplexDependency(),
new AVeryObscureDependency(),
new SecretObject()
);
}
}
Note: I omitted a possible IFooService
interface as it's not the focal point right now.
The second use case, which I call a "smart" factory, has the factory decide which concrete type to instantiate, and its expertise lies in knowing how choose the right type for the situation.
public class SmartFactory
{
public IVehicle GetVehicleToTransport(Cargo cargo)
{
if(cargo.IsExplosive)
{
return new BlastProofTruck(cargo);
}
else if(cargo.Weight > 500)
{
return new Truck(cargo);
}
else if(cargo.Weight < 5)
{
return new Bicycle(cargo);
}
else
{
return new Van(cargo);
}
}
}
It is of course possible to combine the two, though for sufficiently complex scenarios it starts turning into a strategy pattern instead of a factory pattern.
Your scenario
Your use case fits with neither use case for a factory. A ProductCollection
is not hard to instantiate, nor are you making the factory choose between different concrete types. A factory simply isn't the right fit here, at least not for the way you're going about things right now.
The ProductCollection should be instantiated from the data from the scraped web pages.
I think you've skipped a step there. The ProductCollection
shouldn't be instantiated from the data, it should be instantiated from the Product
s (which, in turn, come from the data).
A ProductFactory
makes more sense here than a ProductCollectionFactory
. Note that the ProductFactory
is still likely to return a list of products rather than a single one (due to the nature of scraping a whole webpage), but the "expertise" of the factory lies in the creation of the product, rather than the list of products, and the factory name should reflect that.
If you look at your current structure, it feels very confused. Every involved class seems to just pass the buck on to the next dependency:
- The factory, while having a
ScrapeProducts
method, doesn't actually know how to scrape. It gets passed a ScrapingCommon
for that purpose.
- The factory just creates a
new ProductCollection()
.
- The
ProductCollection
, while having a Scrape
method, doesn't actually know how to scrape. It gets passed a ScrapingCommon
for that purpose.
- During this entire process, both the webpages and the
ScrapingCommon
(which presumably will use those webpages as input data) are being passed together, but in separate parameters. They are like two people travelling separately but on the same train, on their way to a date with each other, but they're not starting a conversation with each other until they get to where they said they would meet.
I can only hope that the ProductCollection
actually calls the ScrapingCommon
and that ScrapingCommon
does in fact know how to scrape.
Although you've been light on details, it seems that ProductCollection
is the actual ProductFactory
here. At least, that is my inference, since you didn't really show that part of the logic.
Instead of juggling these dependencies back and forth, keep things simple:
public class ProductScraper
{
private readonly ScrapingCommon scrapingCommon;
public ProductScraper(ScrapingCommon scrapingCommon)
{
this.scrapingCommon = scrapingCommon;
}
public List<Product> Scrape(HtmlDocument webPage)
{
// Whatever the logic is, e.g.:
var divs = scrapingCommon.GetElementFromPage(webPage, "div");
var productsOnPage = divs.Select(div =>
new Product()
{
Name = div.InnerText,
IsActive = div.Classes.Contains("active")
});
return productsOnPage;
}
}
Some closing notes:
- Notice how
ScrapingCommon
and the HtmlDocument
do not travel together anymore. The scraping logic is an injected dependency, the document is a method parameter. They start their "conversation" from the moment they meet, much like those people on the same train.
- I opted to return
List<Product>
instead of ProductCollection
, because your question doesn't show added value for the ProductCollection
class. If there are other reasons for it to exist, feel free to change that.
- I chose to pass a singular webpage as there is no logical benefit from passing a collection of pages all the way down the call stack. Simply call the
Scrape
method for each page and combine the results.
- I think
ScrapingCommon
is either badly named, badly designed, or both. But that's not the focal point right now so I used it for the sake of consistency with your question.
ProductScraper
can arguably be renamed to ProductFactory
, but "factory" is a vague term and I think "scraper" is a bit more precise and appropriate here.