As the senior developer in our company, I am currently starting to move our commercial php-mysql e-commerce solution (which takes data from a specific ERP-system) from procedural spaghetti-code which uses globals for pretty much everything and has very little separation of concerns to OOP.
The title describes the basic problem rather well – almost every user-interaction will have to check lots and lots of things (state), and do different things based on that. How can I avoid bloating my controllers & services or coding structural knowledge about everything into my classes (constructor over-injection, service-locator, law-of-Demeter violations etc)?
More information about the current situation:
I have already successfully developed a basic architectural style and implemented an RMA-module with this design.
A brief summary: For every basic business-entity (the rows of our mysql-tables), I have a class for data-storage and retrieval, as well as for getting NULL-versions of that object, a configuration-class that holds information about which fields are mapped from db, what a potential alternative unique key besides the db-id is, as well as the name of the table (or view) to get data from. Furthermore, I have specific collection-classes which ensure that they only contain instances of the data-classes and specific repositories also implementing the identity-map pattern which manage both basic retrieval & persistence as well as entity-specific methods. When non-persistent business-entities are needed, they can be stand-alone, in a dyad with collection or a triad with collection and repository.
On top of this, I have quasi-“front controllers” for modules which use services for processing-tasks. In case of multi-step processes (currently RMA, later also user-account, order etc), the module “front controller” checks the current step (action) & user-input (delegate validation to services where sensible).
If multiple actions are possible on a view, and an action only changes data within the view, and has no potential to change which view has to be chosen – the “front-controller” will pass that action to a presenter for the view. If the action can change what page is displayed (re-routing on validation error, progressing to different view on successful input-validation), the business-logic is done by the module “front-controller” itself with the help of a service or services.
As a rather standard php-application, the application is created with user input via URL, POST & GET, has this input from the beginning, constructs a response, outputs it and then is destroyed. New user input will create the entire application anew – with the exception of one or two places where Ajax-code is put into the site by the application, which is basically just creating a mini-application and integrating its output into the browser-window, and isn’t an interaction with the application-instance that created the page. So the application cannot update views during its lifetime – actually creating a view in the UI is the end of the application lifetime, meaning actual MVC / MVVM / MVP implementations as originally conceived are not strictly possible… only partial mappings are. I have tried a modest implementation as described above.
RMA is working nicely, and has already achieved huge increase in separation of concerns and thus “debuggability”. Using traits for fields and field-related methods that different classes need to fulfill common interfaces (the business-entity specific collections, configs, data-objects and repositories/identity-maps), I was able to avoid code duplication quite neatly.
After many months of reading all about best practices, patterns and anti-patterns, architectural styles etc and trying out different things – I’m still left with some rather pressing problems.
The coupling between the classes related to a single business-entity (data-object, config, collection and repository) is acceptable, in fact quite beneficial: in virtue of this tetrad-pattern and the use of traits, new business-entities are added (and provide extremely powerful methods for handling them) in a matter of minutes. Schema-modifications merely necessitate changing the respective fields in the data-object and their description in the config (two lines of code for every field, plus a potential further two if the non-id unique-key changes… which does’t happen).
What’s bothering me mostly is that I have found no way to avoid falling prey to at least one of the set of bad practices related to constructor over-injection, service locator plus law-of-Demeter violation and tight coupling (too much knowledge) in general for the controllers and services.
That’s mainly because pretty much every single controller will have to check with huge amounts of distributed state… mainly, what should be done and displayed on a certain URL with a certain set of POST & GET data depends on a so many things: Is the shop b2c, b2b or salesperson? Is the visitor logged in? Does the form-data identifying a record to act upon match the records company-code, shop-code, shop-language-code? If we are in b2b, does the user have permission for the module? Does the user have permission for the record? What is the content of the text-snippet with the given code in the local language?
All this information is distributed in different model-entities. Wherever a procedure concerns only the data of the object itself, it is in the data-class or (when meta-information is needed) in a service that knows about the data-class and its config. When it contains nothing but operations on a given collection/aggregate of a business-entity, it’s in the collection-class, if it concerns retrieval, it’s in the repository/identity-map.
But since practically every module and nearly every action the user can perform with respect to a module is dependent upon so many different things, I currently can’t seem to find a way to avoid either bloating objects and their constructors beyond the pale, or using (as I do now with the RMA-module) Configuration-Objects such as “CurrShopConfiguration” which contain the current shop, shop-language, a provider of local text-constants, the current visitor, user and customer and a “god”-like helper-service for a controller which has the CurrConfig-Object and implements all procedures the “front-controller” needs to perform beyond choosing actions, models, views and linking them.
I would very much like to write more SOLID code – yet can’t see how, since the essential decision-logic of the modules and actions have so many necessary dependencies.
Perhaps you have some ideas on how to approach this issue?
Here is some example code – the part of the RMA-“Front Controller” that is executed when the ‘save’ action on a a form with a returnable shipment is called:
//Check for existence of shipment-id in input
if (!($this->inputHeaderID > 0)) {
//add user-faced error-message for view-models
$this->errors[] = $this->templateEngine->getText('record_not_set');
//go to search-view (where errors are displayed)
$this->_showSearchPage();
} else {
//Try to get shipment from Repository (extracted from helper), get either target shipment or NULL-object (unset fields)
$shipment = $this->retShipmentRepository->findByID($this->inputHeaderID);
//Check for NULL-Object
if (!((int)$shipment->id) > 0) {
$this->errors[] = $this->templateEngine->getText('record_not_set');
$this->_showSearchPage();
} else {
//We have valid shipment. Check for user permission on shipment
if (!$this->helper->checkUserShipmentPermission($this->user,$shipment,$this->inputEMail,$this->inputPostCode)) {
$this->errors[] = $this->templateEngine->getText('record_not_permitted');
$this->_showSearchPage();
} else {
//We have permission, attempt saving (shop-context based switching and further validation inside service)
if(!($this->helper->saveReturnOrder($shipment,$this->inputReference,$this->inputLines))) {
$this->errors = $this->helper->getErrors();
$this->_showShipmentPage($shipment);
} else {
//The Shipment could be appended with the return order information and saved.
//Check if it was deemed to require verification via link with token in email to owner of shipment
//Show either success view or confirmation-required view based
$saveStatus = $this->helper->getSaveStatus($shipment->id);
if($saveStatus == RMAOrderHelper::SAVE_STATUS_WITHOUT_TOKEN) {
$this->_showSuccessPage();
} elseif($saveStatus == RMAOrderHelper::SAVE_STATUS_WITH_TOKEN) {
$this->_requestTokenConfirmation($shipment);
}
}
}
}
}
The user-input variables are stored on __construct of the controller (which is injected with the helper – it accesses the helper’s “CurrShopConfig” and extracts information it needs directly) via filter_var.
The above code smells pretty badly to me … but there’s far more to the decision-logic, which is is in the helper-service and accesses the current shop-configuration, user, visitor etc to perform the tasks of its public methods.
Here are the first 2/3rds of the helper-services saveReturnOrder-Method
public function saveReturnOrder( RetShipmentDocument $shipment, $yourReference, $inputLines ) {
//Check if document isn't already marked with a return order request
//that hasn't been synchronized with the ERP-System (and thus cleared) yet
if (!$shipment->isReturnOrderSettable()) {
$this->errors[] = $this->templateEngine->getText('internal_error');
return FALSE;
}
//Check if the input for the shipment-lines (id, return quantity and return-reason code)
//belong to the shipment and are valid (e.g. return quantity <= un-returned quantity)
if (!$this->_validateReturnOrderLineInput($shipment, $inputLines)) {
$this->errors[] = $this->templateEngine->getText('internal_error');
return FALSE;
}
//ValidatedLineRequests are stored for the shipment by the _validateReturnOrderLineInput-Method
//that has just been executed
$requestLines = $this->validatedLineRequests[$shipment->id];
$shipmentLines = $shipment->getLines();
$token = '';
$markAsConfirmed = TRUE;
//If visitor is not logged in and we have a content for a confirmation-request mail
//we must not mark it as confirmed and generate a token
if (!$this->shopConfiguration->visitorLoggedIn() && ($this->getConfirmMailTextModule()->id > 0)) {
$token = md5(uniqid(mt_rand(), TRUE));
$markAsConfirmed = FALSE;
//Otherwise if visitor is not logged in and there is no such mail-content
//we have an error and cannot proceed
} elseif (!$this->shopConfiguration->visitorLoggedIn() && !($this->getConfirmMailTextModule()->id > 0)) {
$this->errors[] = $this->templateEngine->getText('internal_error');
return FALSE;
}
$lineRepository = $this->shipmentRepository->getLineRepository();
$committedLineRequests = 0;
foreach ($requestLines as $lineRequest) {
$lineID = $lineRequest['line_id'];
$returnQuantity = $lineRequest['return_quantity'];
$returnReasonCode = $lineRequest['return_reason_code'];
foreach ($shipmentLines as $shipmentLine) {
if ($shipmentLine->id == $lineID) {
//The shipment has a line matching the request, attempt setting the return-order data
if (!$shipmentLine->setReturnOrder($markAsConfirmed, $returnQuantity, $returnReasonCode)) {
$this->errors[] = $this->templateEngine->getText('internal_error');
$this->_unsetReturnOrder($shipment);
return FALSE;
}
//Data could be set in object - now try persisting it
if (!$lineRepository->updateSingle($shipmentLine)) {
$this->errors[] = $this->templateEngine->getText('internal_error');
$this->_unsetReturnOrder($shipment);
return FALSE;
}
//Persistence succeeded, increment counter
++$committedLineRequests;
}
}
}
As you can see… I’ve already made many discrete sub-processes into methods and located them with objects to which they belong, but I’m still left with huge ugly nested conditionals and an explosion of dependencies. Splitting up every part (or even most parts) of such nested conditionals into different objects is unfeasible… and unhelpful, since practically every conditional already uses methods that are with objects where they belong, and since such a splitting would likely increase re-usability very little and would have to be done for many dozens such situations, resulting in a class- and file-explosion.
7
If you have too many dependencies being passed around, the general technique is to eliminate those dependencies higher up in the call stack, by changing the order decisions are made. This is easiest to explain with an example:
getPath(config, user) {
if (config.isB2B())
return b2bpath(config, user);
else
return b2cpath(config, user);
}
b2bpath(config, user) {
if (!config.allowedToAccessPath(user))
return accessDeniedPage();
else
return "My fancy b2b page";
}
b2cpath(config, user) {
if (!config.allowedToAccessPath(user))
return accessDeniedPage();
else
return "My fancy b2c page";
}
You are repeating the authorization check down at the lowest levels of the call stack, so move it up:
getPath(config, user) {
if (!config.allowedToAccessPath(user))
return accessDeniedPage();
if (config.isB2B())
return b2bpath();
else
return b2cpath();
}
b2bpath() {
return "My fancy b2b page";
}
b2cpath() {
return "My fancy b2c page";
}
Then repeat to see if you can move some of the decisions into the code that calls getPath. This is a simple example, but I see the former kind of code all over the place with more layers. Start with decisions at your lowest layers, and try to figure out ways to move them up. Sometimes this requires judicious use of inheritance, like:
getPath(config, user) {
module = config.isB2B() ? B2BModule() : B2CModule()
if (!config.allowedToAccessPath(user))
return module.accessDeniedPage();
return module.getPath();
}
It’s very rare not to be able to simplify dependencies this way. It’s a matter of trying different arrangements until you find one that works.
If you have workflow-type dependencies, not just data dependencies, as in your first example, you can separate them out using something like this:
step1 = new ValidateHeaderId(inputHeaderId);
step2 = new FindShipment(retShipmentRepository);
step3 = new ValidateUserPermission(user, inputPostCode, inputEmail);
step4 = new SaveReturnOrder(inputLines, inputReference);
step5 = new CheckSaveStatus();
notSet = new NotSetPage(templateEngine, searchPage);
notPermitted = new NotPermittedPage(templateEngine, searchPage);
saveErrors = new SaveErrorsPage();
success = new SuccessPage();
requestTokenConfirmation = new RequestTokenConfirmation();
steps = [step1, [notSet, step2], [notSet, step3], [notPermitted, step4],
[saveErrors, step5], [success, requestTokenConfirmation]];
executeSteps(steps);
This recognizes you have a series of steps which each produce some sort of result and choose the next step. executeSteps abstracts away the repetition of calling run() on each step, and passing the output from the previous step into the next step. This allows the steps to be stored in a data structure instead of a function, which can then be built up in several different ways, including by some sort of registration process or config file. Once each step object has been created, its dependencies no longer need to be tracked outside it. I believe the rules engines from BobDalgleish’s answer are basically pre-existing libraries to help you do this more easily.
6
The general rule in complicated programs is
Separation of Data and Control
The more that you can keep these two aspects separate, the easier the software will be to maintain. This what you are trying to do in your refactoring.
I would suggest that you take a more drastic step and use a “business rules engine” to drive your program. I don’t mean one of those fancy tools designed for non-programmers, but something that quite clearly separates out the decision making process from the actual collection or transformation of data.
I often recommend JBoss Drools for applications that are Java-based. I see that there is a similar type of engine available for PHP. Looking over it, it accomplishes many of the things you are trying to do. -> Ruler.
1