academe / SagePay

SagePay Protocol v3
GNU General Public License v3.0
11 stars 12 forks source link

Use Data Mapper model rather than up-side-down Active Record #25

Open judgej opened 10 years ago

judgej commented 10 years ago

And active record normally starts with a data storage layer, and models inherit from that layer and add their own fields and business logic.

The Transaction object here starts with the data properties and business logic, then allows you to use one of several data storage methods that inherit from the model.

That is a kind of up-side-down active record, and could make integration a little cumbersome.

Instead, we should use a data mapper. The models already are pretty much dumb data with some business rules. We just need to take the storage out of the line of inheritance and keep it as a separate mapper. Each storage type would then be an adapter for the mapper. I don't think it is a complicated change, but will make more sense for people familiar with more standard patterns, and will be easier to test (the model and the storage can be tested independently, while at the moment the storage can only be tested with a model).

judgej commented 10 years ago

Here is a good (from what I can see, though I'm no expert) example of a data mapper:

https://github.com/iio/datamapper

Now, this also leads to another interesting idea. We have one big "Transaction" model at the moment that persists transactional data from one page to another. Depending on the action being performed, different fields need to be completed (are used or not used). Maybe instead of one Transaction model, we could use separate models - one for each service, or one for each message type. The model would tell the data mapper which fields to save and retrieve, so some (or all) of the central Transaction metadata would be moved out to the models. We would still need to find a way to define the storage structure (the table columns) which needs to be able to store any of the multiple models (or perhaps they could be stored in separate tables, if that is what the mapper adapter chooses to do.