View Issue Details
ID | Project | Category | View Status | Date Submitted | Last Update |
---|---|---|---|---|---|
15395 | Development | _ Unknown | public | 2019-10-10 17:01 | 2019-10-11 12:12 |
Reporter | ollehar | Assigned To | |||
Priority | none | Severity | minor | ||
Status | new | Resolution | open | ||
Product Version | 4.0.0dev | ||||
Target Version | 4.0.0dev | ||||
Summary | 15395: 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 | - | ||||
Tags | No tags attached. | ||||
@DenisChenu Feel free to leave feedback. Current discussion about how to increase testability of the code-base, in this case our models. | |
Why not , but i need to learn clean difference between services and dao … a sample is OK :) | |
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). | |
We have a lot of Service in current models ? Just some definition ? No ? |
|
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 |
|
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. |
|
Right for copyToCentral … (me never really work with CPDB …) And better understand here :) And [Survey::replacePolicyLink](https://github.com/LimeSurvey/LimeSurvey/blob/c8c8ef6736f604718ec84e91a558683cd771b07f/application/models/Survey.php#L1951) where is the best place ? |
|
New class SurveyPolicyService Btw, "dao" folder should maybe be called "activerecords". Service can inherit from Yii CModel class (not sure what it does, honestly). |
|
> 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. |
|
> A third folder called "forms" or "formmodels" would also be good. Yep :) |
|
Then another possibility: ``` models/ forms/ InstallerForm... activerecords/ User... services/ SurveyPolicyService ``` Putting services outside of model folder since they don't inherit from CModel. |
|
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 |