6

In the context of MVC sometimes I find myself creating a Factory and injecting the factory with Repository.

While it is certainly possible to use Repository as layer inside the Factory, I wonder if it is an anti-pattern to do so. i.e. is it advisable to keep Factory Repository-free?

Example

For example, my QuoteFactory class is tasked with

  • creating Quote class
  • maintaining Repository methods & generally being Repository-aware

Code

$repository = new QuoteRepository(111); $factory = new QuoteFactory($repository); $quote = $factory->loadQuote(); class QuoteRepository { function __construct($id) { $this->id = $id; $this->em = DoctrineConnector::getEntityManager(); } function getLineItems() { $query = $this->em ->createQuery('SELECT s FROM... where id = :id') ->setParameter('id', $id); return $query->getResult(); } } class QuoteFactory { function __construct(QuoteRepository $repository) { $this->repository = $repository; } function loadQuote() { $quote = new Quote(); $quote->setLines($this->repository->getLineItems()); return $quote; } } class Quote { function setLines(array $lines) { $this->lines = $lines; } }
Improve this question
4
  • 2
    What do you mean by "acceptable?" Note: don't answer this with a tautology like "best practice" or "most commonly used." State your specific criteria for acceptability. Commented Sep 27, 2016 at 22:55
  • 1
    my concern is that ... a class should only be responsible for one thing (single responsibility principles). As soon as I inject Repository into my Factory, the factory becomes Repository-aware and is no longer only responsible for creation of the Quote (in my code example). It has to now also manage the Repository methods. I guess I don't have a very specific concern at this time, just a general one based on the SRP Commented Sep 27, 2016 at 23:00
  • What exactly is $id being used for? Don't see anything reading it. Commented Sep 28, 2016 at 4:14
  • @Candied, fixed - see updated post Commented Sep 28, 2016 at 14:08

2 Answers 2

Reset to default
8

While in general injecting something is not bad and does not automatically result into breaking the SRP (nor it does in your case - you have one class which only fetches data and another constructing an object from it), you have a different problem: wrong understanding of layering and abstraction.

The repository layer is the one to bind data to your domain models, you should not need another layer to do that. Not to mention your solution is overengineered.

Simply construct the quote directly in the repository, unless you have a really good reason for doing otherwise, no need for a factory.

From your comment below:

What do you mean by binding data to domain models? It sounds too abstract and I can't find a way to understand it. Can you give an example?

You have pure data which somehow finds its way into your system. The ways may include:

  • SOAP API,
  • REST API,
  • database,
  • contents from a read file,
  • ...

This data is just data and nothing else, it contains no rules.

You then have your business (the fun parts of the application, where the rules are) core, you domain. The problem is your domain does not understand the pure data. In order to understand the pure data the data shall be transformed to be represented by business objects, domain models.

Also overall you are saying, effectively merge Factory and Repository together (into Repository) but keep Quote as a separate concept, or should Quote since it contains data be bound to Repository as well?

I am not saying you shall merge the factory and repository together, I am saying you should remove the factory completely and instantiate a Quote object directly in the getLines method. And since we're already talking about it, it might be wise to rename the method to something better, such as: getQuoteWithLineItemsById.

Also, the Quote shall have no direct ties to the repository. Why? Repository acts as a gateway to your system - as I have already mentioned - by taking pure data and transforming it to objects.

The proposed design:

class QuoteRepository { /** * @var \Doctrine\ORM\EntityManager */ private $entityManager; public function __construct(\Doctrine\ORM\EntityManager $entityManager) { $this->entityManager = $entityManager; } /** * @param string $quoteId * @return Quote */ public function getQuoteWithLineItemsById($quoteId) { $query = $this->entityManager ->createQuery('SELECT s FROM... where id = :id') ->setParameter('id', $quoteId); $quote = new Quote(); $quote->setLines($query->getResult()); return $quote; } } class Quote { private $lines = []; public function setLines(array $lines) { $this->lines = $lines; } } $quoteRepository = new QuoteRepository(DoctrineConnector::getEntityManager()); $quote = $quoteRepository->getQuoteWithLineItemsById('1');

Your repository is now responsible for transforming the data retrieved from the database to a domain model, the Query. It knows nothing about business logic, the business logic shall be within the domain model.

Besides transforming raw data to business-understandable entities the repository layer also exists for another reason:

  • transforming business-understandable entities back to raw data for persistence purposes.

So in effect, I can end up with a single QuoteRepository class that will contain my data, have business domain functionality and read/write to/from the database?

No. You will end up with repository layer only responsible for reading/writing from/to the database and transforming the data either one way or another.

Then you will have another layer (the domain) which is persistence ignorant, knows absolutely nothing about SQL, is pure PHP (most likely classes), and contains all your business rules - eg. a username must not be empty or longer than 32 characters.

When dealing with business operations your domain models ensure your business rules are preserved. If a domain model exists and is sent to a repository to be saved the repository no longer cares about the state of the domain model, because it simply trusts the domain model is in a valid state. The responsibility of the repository is to save the model, not to check for its state.

Improve this answer
3
  • thank you.. what do you mean by binding data to domain models? It sounds too abstract and I can't find a way to understand it. Can you give an example? Also overall you are saying, effectively merge Factory and Repository together (into Repository) but keep Quote as a separate concept, or should Quote since it contains data be bound to Repository as well? So in effect, I can end up with a single QuoteRepository class that will contain my data, have business domain functionality and read/write to/from the database? Commented Sep 28, 2016 at 14:20
  • @Dennis I added a brief answers to your questions. Hope that helps. Commented Sep 28, 2016 at 16:12
  • thanks I think that helps... Repository is thus not a blind "read/write" instrument but rather "read/write while handling transformation between business model and database model". I didn't need a factory because .. I didn't need to create anything new but just do data reading and translation. Commented Sep 28, 2016 at 16:43
2

Stricly speaking, QuoteFactory doesn't break SRP:

  • maintaining Repository methods & generally being Repository-aware is not a responsibility, it's a mean to the class responsibility.
  • creating Quote class is its sole responsibility.

But QuoteFactory doesn't seem to implement that responsibility.

In my understanding, a factory creates "new" entities, "new" at a functional level. An entity retrieved from a repository is not a "new" one. A constructor creates an object, a technical representation of an entity in memory.

So a loadQuote() method in a QuoteFactory feels bad. Either it really loads an "old" quote from storage, and it should go to a QuoteRepository, or it actually creates a "new" quote from some line items, and should be named createQuote(). In either case, the QuoteFactory doesn't need a QuoteRepository.

Also, your QuoteRepository actually loads line items, not quotes. It's fine if the line items are mainly parts of a quote. Otherwise, shouldn't it be named LineItemRepository ?

Improve this answer
1
  • very good point. I can remove QuoteFactory, move the newly-called loadQuote method into LineItemRepository Commented Sep 28, 2016 at 14:40

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.