Review my model method

PHP
1

I would like to post a public method of one of my models for your review. What would you add/change to improve readability and/or testability and code quality.

Application_Procedure_Model and Application_Customer_Model are external services simply wrapped by model objects and will eventually be brought into context via service locator so to mock the objects as opposed to stubbing them as I currently do.

What would you test for/validate? Would you use a repository to abstract the query construction? Can you show me examples?


public function createWorkOrder
(
  $id_type = self::TYPE_OVERHAUL, 
  $id_options = 0, 
  $id_customer = 0, 
  $id_manufacturer = 0, 
  $serial_number = 'NSN', 
  $quantity = 1, 
  $model_number = 'NA', 
  $part_number = '', 
  $part_name = 'UNKNOWN', 
  $purchase_order = '', 
  $rpi_number = '1', 
  $notes = ''
)
{
  $rpi_revision = 0;

  $id_stage = self::STAGE_PREQUOTING;
  $id_warehouse = self::WAREHOUSE_WIP;

  $time_received = time();
  $time_approved = ($id_options & self::OPTION_APPROVED ? $time_received : 0);

  $params = array
  (
    'id_type' => $id_type,

    'id_station' => Application_Sequence_Model::STATION_RECEIVING, 
    'id_stage' => $id_stage,
    'id_options' => $id_options,

    'id_warehouse' => $id_warehouse,
    'id_customer' => $id_customer,
    'id_manufacturer' => $id_manufacturer,

    'date_received' => $time_received,
    'date_approved' => $time_approved,
    'date_promised' => 0,
    'date_inspected' => 0,
    'date_scheduled' => 0,
    'date_completed' => 0,

    'quantity' => $quantity,
    'rpi_revision' => $rpi_revision,
    'rpi_number' => $rpi_number,
    'purchase_order' => $purchase_order,
    'serial_number' => $serial_number,
    'model_number' => $model_number,
    'part_number' => $part_number,
    'part_name' => $part_name,
    'status' => '',
    'quote' => '',
    'notes' => $notes
  );

  $id_workorder = $this->erp->getGateway()->createRecord(self::T_ERP_WORKORDER, $params);

  $procedure = new Application_Procedure_Model();
  $master = $procedure->selectProcedureMaster($rpi_number);
  $this->buildWorkOrderFromRpi($procedure, $id_workorder, $rpi_number, $master['quote_step']);

  // NOTE: Update RPI revision on work order according details returned by $procedure object
  //       there is no way of us knowing before we clone RPI so we do it now :)
  $this->erp->getGateway()->updateRecord
  (
    self::T_ERP_WORKORDER,
    array(
      'cost_overhaul' => $master['cost_overhaul'],
      'rpi_revision' => $master['revision']
    ),
    array('id_primary' => $id_workorder)
  );

  // NOTE: Notify the RPI system via a automated task of the incoming part
  if($rpi_number == '1'){
    $customer = new Application_Customer_Model();
    $customer = $customer->returnCustomer($id_customer);

    $procedure->createProcedureTaskForRpi($id_workorder, $part_name, $part_number, $customer['name']);
  }

  $time_estimate = $this->scheduleWorkOrder($id_workorder, $time_received);

  return $id_workorder;
}

Cheers,
Alex

2

A quick question but considering the dependencies of this method (and model) how would I use a DiC such as Phemto? Consider the customer model and procedure model are external and require configuration details out side of the current DB context.

3

In my opinion you know you are doing something wrong (not the best way) when a method has 12 parameters. Hell… 4 is a stretch for me. Anything behind four should probably either become an array or separate application level object that is passed as a dependency with type hinting.

That said, the only direct dependency I see there is Application_Procedure_Model. In which case you could either pass an instance as a parameter or use a factory which exists inside a di container as a property of the object. To do that would either require delaying the construction phase until after all dependencies have been injected or passing the container to the constructor upon instantiation of whatever class this method belongs to.

4

Finally a reply :smiley:

In my opinion you know you are doing something wrong (not the best way) when a method has 12 parameters. Hell… 4 is a stretch for me. Anything behind four should probably either become an array or separate application level object that is passed as a dependency with type hinting

At one time I felt very much the same way until I started implementing models as the fat and keeping controllers thin, which I assume you do as well based on our previous discussions. So its interesting you have not come across model API where the parameter list grows to unusual length.

Let me ask (while in general I agree with your assessment - models are my exception) how would you prefer to see things done? Also the parameters are passed to the model as associative array my framework handles the rest.

Breaking the workorder table into several distinct tables would result in more code and complexity. This table has been re-designed about 50 times since it’s inception 2 years ago. Added to, removed, re-ordered, etc. Everything that is in there now is truly part of a workorder entity - right now (never know what the future holds) moving fields to another table does not make sense and does not align with what I have gathered from daily communication with the business experts of the company I work.

Although I can appreciate and understand your concern. :slight_smile:

That said, the only direct dependency I see there is Application_Procedure_Model. In which case you could either pass an instance as a parameter or use a factory which exists inside a di container as a property of the object. To do that would either require delaying the construction phase until after all dependencies have been injected or passing the container to the constructor upon instantiation of whatever class this method belongs to.

I believe there are two dependencies - Procedure and Customer objects - the former is a web service and the later is being accessed externally in another DB until the time comes a RESTful API is also provided at which point both will be through REST.

After having read of various DiC’s I am not entirely convinced that is what I need just yet. Rather my focus is on re-organzing my models so these monolithic massive objects can be broken into more logical units. Having read up on DDD lately I find myself compelled to experiment with the idea of aggregate root objects (which is what my model is now) but also implement aggregate sub objects possibly using a data mapper to fully abstract data access.

Ideally the single model class with a dependency on three tables should be refactored to a 1:1 relationship - the root object providing the API that exists now but the implementation for each being split into sub-models which are not accessible directly but only through the root object - which essentially delegates to sub-models.

This is where I understand the repository pattern comes into play? The repository will store root aggregates as a collection but may provide a declarative interface to the root objects which then delegate to the sub-models.

Each of the sub-models would implement their CRUD functionality and validations, etc. The relationships between objects would be handled by the root - and transactional operations could be bundled up into a Unit of Work. Sure this results in more classes but the code clarity would improve tremendously I think. I just am not totally clear on how or what to get started.

As an exercize - this is what I am thinking:

WorkOrder is the domain model - providing the API I currently have

Header is the entity of which you have seen the interface for - it contains information that is on every workorder printed.

There are about 20 other entities such as sequences, certifications, repairorders, reprocessess, additionals, cerilogs, etc

Each of these are models that require their own CRUD functionality and more but rely strictly on the existence of a workorder. Each of which I believe should be implemented as their own root aggregates and aggregate objects - according to my understanding of DDD nomenclature.

For instance repairorders is a header record which than has N number of line items dictating what needs to be done.

RepairOrderRoot would encapsulate at least two tables (repairorder & repairorder_item) both tables would ideally have their own CRUD model with validation and various finder methods. But neither should be accessible directly only being used indirectly through the RepairOrder root object - the root model. This model would also be responsible for handling the relationships. So when you query a repair order, you also get the line items returned to you if there are any.

Sorry for being so verbose but this topic confuses me and interests me and I have a great deal on my mind on how to best re-structure my fat models - following DDD patterns and principles - at least my current interpretation of them. :stuck_out_tongue:

Love to hear more comments or experiences, fascinating subject :slight_smile:

Cheers,
Alex

5

I’m with oddz on the concern of multiple parameters. I’m guessing this is a factory object of some sort (at least by pattern), so taking that amount of arguments in makes sense, but it also makes for hard to parse code once you revisit it in a few months, or for someone new. Consider an array argument. PHP’s syntax makes this clumsier than JavaScript, but it’s still powerful and effective.


protected $defaultParams = array (
  'type' => self::TYPE_OVERHAUL,  
  'options' => 0,  
  'customer' => 0,  
  'manufacturer' => 0,  
  'serial_number' => 'NSN',  
  'quantity' => 1,  
  'model_number' => 'NA',  
  'part_number' = '',  
  'part_name' = 'UNKNOWN',  
  'purchase_order' = '',  
  'rpi_number' = '1',  
  'notes' => ''
)
 
/**
 * array $params
 */
public function createWorkOrder ( $params) {
  $params = array_merge($params, $this->defaultParams);

  ...
}

// And we can now invoke the function as...
$order = $factory->createWorkOrder( array( 
  'type' => FACTORY::SOME_NON_DEFAULT_TYPE_CONSTANT,
  'options' => 4
));

And that’s a start. It’s a pain to have to name the parameters outside the object, but in my experience it’s more of a pain to get the sequence of arguments just right. This is the real problem with a 12 argument function - what if the only parameter of your call that isn’t default is the last one?? You’re forced into repeating the default arguments for all previous 11 arguments when you make the call. Modern IDE’s like Zend Studio can lessen the pain somewhat, but it’s still there.

Now personally, when I have objects that have argument sequences this sophisticated, those arguments become objects themselves rather than just an array, although they implement ArrayObject so I don’t have to fuss to much about access syntax. The validation and defaulting logic of the parameters then lives in the parameter carrying object. This also helps since if you have an object that takes similar parameters you can extend the parameter object as needed.

The parameter object can also be written to allow chaining if you like that style of coding (I do, but I understand some don’t). I’ll give an example below - in it startWorkOrder is a new method of your factory that simply returns the default parameters object. It will accept an array argument as above, but doesn’t need that argument. The two set methods are merely setters for those parameters, The execute method returns the work order object by calling the createWorkOrder method, it does this by passing itself back into the factory (The factory gave the parameter object a reference to itself when it created it ) and letting the factory create the work order.


$order = $factory->startWorkOrder()->setOptions(4)->setPurchaseOrder($purchase)->execute();

One of the things I’ve picked up here, mostly from Lastcraft, is to try to cap each individual object’s business. Makes the objects individually easier to test.

EDIT (Final edit hopefully): As far as number of arguments a function takes, one factor I take into account is public vs. protected. I try to keep public methods under 3 arguments, but I’ve let protected methods get up to 5 or so. The reasoning here is protected methods, in theory, shouldn’t have code written for them as often.

6

Thanks for the feedback. :slight_smile:

And that’s a start. It’s a pain to have to name the parameters outside the object, but in my experience it’s more of a pain to get the sequence of arguments just right. This is the real problem with a 12 argument function

Not sure but in case I failed to mention the last time - the method is passed an associative array, the parameters are only fixed in this case for the sake of demonstration.

While I am not a proponent of methods with several parameters, the exception I find is best made for a model API. The interface maps to their tables. It’s only the creating/updating methods which often grow large simply because to create a complete records you need all the information - although some of it is defaulted. I have individual accessor/mutators if only a single field or two need to be updated or retrieved as well.

The parameter object can also be written to allow chaining if you like that style of coding (I do, but I understand some don’t).

I do…but I don’t think I would as part of the model interface - remember this API is acting as my root aggregate or primary interface to external interfaces (HTML, REST, CLI, etc). If called within my controller I would feel as though I have extracted some of the business logic back into the controller and outside of the model.

$model->createWorkOrder($params)->setOptions(4)->setPurchaseOrder('77360-34')->execute;

You know, after thinking about this for a few minutes I have revoke my comment and re-consider this a little more.

One concern I do have with models doing this - is that now business logic may not be encapsulated completed within in the model. The chained interface now allows you potentially implement business logic (declaratively) but as part of the interface not implementation - which causes me some concern but I am not convinced it’s entirely justified.

Will have to think more, thanks for the input.

Cheers,
Alex

7

On the topic of multiple parameters…

Your parameters are actually a strict set of data. I’d argue you should declare them a class:


class WorkOrder {
	public $id_type;
	public $id_options;
	//etc
}

//...
public function createWorkOrder (WorkOrder $params) {}

It’s the controllers job to format the data into the format the model is expecting then the model does processing on this data.

E.g. your controller could be doing this:


$workOrder = new WorkOrder;
//using post for demonstration only...
foreach ($_POST as $key => $value) {
	//And probably need some checking here that it's a valid key being set.
	$workOrder->$key = $value;
}
$model->createWorkOrder($workOrder);

This is, after all, controller logic. It doesn’t make it a ‘fat controller’ as all it’s doing is connecting the externally input data to the model.

This would also avoid the use of the $params array inside the model method. This looks messy to me.

The problem I have with multiple parameters and your $params array (and the subsequently introduced $defaultParams array) is that it’s a maintainability nightmare. If you add a single field you need to make 3 changes. Ouch. Use a class and it’s one edit (two if you’re manually assigning the properties in the controller)

8

You have waaay too many parameters in your function. It was hard for me to be able to get the gist of what the function does. To some, it can make your API harder to read. If it’s a factory method, then you should provide the arguments in an array as said above. If you’re just trying to create an object, it should have all those parameters on construction, so it is fully set when created. If you’re at the controller level, it shouldn’t be a problem because as long as you filter the values coming into the object before creating it, it should be fine. From there you can just have some sort of save function in your repository that takes in the said object.