View Issue Details

IDProjectCategoryView StatusLast Update
15395Development _ Unknownpublic2019-10-11 12:12
Reporterollehar Assigned To 
PrioritynoneSeverityminor 
Status newResolutionopen 
Product Version4.0.0dev 
Target Version4.0.0dev 
Summary15395: Split models into services and DAO (data access objects)
Description

"Fat models" pattern leads to less testability and too big classes. We should split all models into two categories:

models/
    services/
        ParticipantService.php
        ...
    dao/
        Participant.php
        ...

or similar. DAO extends active record, services do not and can be stateless or statefull.

Much functionality in controllers should be moved into services too.

Steps To Reproduce

-

Additional Information

-

TagsNo tags attached.

Activities

ollehar

ollehar

2019-10-10 17:04

administrator   ~53989

@DenisChenu Feel free to leave feedback. Current discussion about how to increase testability of the code-base, in this case our models.

DenisChenu

DenisChenu

2019-10-10 18:23

developer   ~53994

Why not , but i need to learn clean difference between services and dao … a sample is OK :)

ollehar

ollehar

2019-10-11 11:31

administrator   ~53999

E.g., if you have a method in a class with no "this" and no "self" (and it's static even), one should consider moving it to a service class. What's the difference between a helper function and a service class? A service class can be mocked in a test, meaning that if class A depend on service class B, we can create a dummy class B_dummy when testing A. This is not possible for helper functions (although a function is easier to test than a stateful class).

DenisChenu

DenisChenu

2019-10-11 11:34

developer   ~54000

We have a lot of Service in current models ?

Just some definition ? No ?

ollehar

ollehar

2019-10-11 11:46

administrator   ~54001

Consider copyToCentral method: https://github.com/LimeSurvey/LimeSurvey/blob/master/application/controllers/admin/participantsaction.php#L2307

It is thin, which is good. Basically just dealing with the outside world (request) and delegating work to another class. BUT: That class is an active record model, which is bullshit. Active records should ONLY deal with database input/output. What we need is a layer between models and controllers which can more easily be tested than a controller action. So this method here should be moved to a CPDBCopyService class: https://github.com/LimeSurvey/LimeSurvey/blob/master/application/models/Participant.php#L1941

ollehar

ollehar

2019-10-11 11:47

administrator   ~54002

So instead of "thin controllers, fat models" pattern, it should be "thin controllers, thin models, thin services". :)

All our helper functions could be moved into services, too. But that's another topic.

DenisChenu

DenisChenu

2019-10-11 11:59

developer   ~54003

Right for copyToCentral … (me never really work with CPDB …)
And better understand here :)

And Survey::replacePolicyLink where is the best place ?

ollehar

ollehar

2019-10-11 12:07

administrator   ~54004

New class SurveyPolicyService

Btw, "dao" folder should maybe be called "activerecords". Service can inherit from Yii CModel class (not sure what it does, honestly).

ollehar

ollehar

2019-10-11 12:08

administrator   ~54005

Service can inherit from Yii CModel class

Or maybe not, since a service doesn't hold so much data (state).

A third folder called "forms" or "formmodels" would also be good.

DenisChenu

DenisChenu

2019-10-11 12:10

developer   ~54006

A third folder called "forms" or "formmodels" would also be good.

Yep :)

ollehar

ollehar

2019-10-11 12:12

administrator   ~54007

Then another possibility:

models/
    forms/
        InstallerForm...
    activerecords/
        User...
services/
    SurveyPolicyService

Putting services outside of model folder since they don't inherit from CModel.

Issue History

Date Modified Username Field Change
2019-10-10 17:01 ollehar New Issue
2019-10-10 17:02 ollehar Description Updated View Revisions
2019-10-10 17:02 ollehar Description Updated View Revisions
2019-10-10 17:03 ollehar Description Updated View Revisions
2019-10-10 17:04 ollehar Note Added: 53989
2019-10-10 17:17 ollehar Description Updated View Revisions
2019-10-10 18:23 DenisChenu Note Added: 53994
2019-10-11 11:31 ollehar Note Added: 53999
2019-10-11 11:34 DenisChenu Note Added: 54000
2019-10-11 11:46 ollehar Note Added: 54001
2019-10-11 11:47 ollehar Note Added: 54002
2019-10-11 11:59 DenisChenu Note Added: 54003
2019-10-11 12:07 ollehar Note Added: 54004
2019-10-11 12:08 ollehar Note Added: 54005
2019-10-11 12:10 DenisChenu Note Added: 54006
2019-10-11 12:12 ollehar Note Added: 54007