Refactoring #2621
open
Database class API is too complex
0%
Description
The nested tree stuff does not belong to the Database class' natural responsibilities. It ought to be its own class.
Furthermore, the database class has way too many methods to do the same thing.
The "retrieve" group of methods is particularly large: retrieve_objects, retrieve_result, retrieve_object_set, retrieve_record_set, retrieve_max_sort_value, retrieve_next_sort_value, retrieve_record, retrieve_row, retrieve_object, retrieve_distinct (and count_objects, count_result_set, count_distinct could also be considered part of the retrieval group depending on how you look at it)
The API itself is also rather unclear; what's supposed to be for internal use and what isn't? There are almost no private or protected methods.
Files
Updated by Anonymous over 12 years ago
To refactor the API itself I think it's important that no MDB2 objects leak out so as to make the abstraction complete. That means anything returning MDB2 objects should either be marked protected/private (since those can only be helper methods) or wrapped in Chamilo-specific classes.
This would also make it easier to move to PDO or some other abstraction layer when MDB2 finally dies.
Updated by Hans De Bisschop over 12 years ago
Please keep in mind that PDO in and of itself is not an abstraction layer in the same way MDB2 is.
Updated by Anonymous over 12 years ago
delete() and delete_objects() are identical. One should be removed, or they should be differentiated somehow.
Updated by Goulwen Reboux over 12 years ago
- File mdb2.patch mdb2.patch added
When starting digging in the code, I got lots of warnings from MDB2 about Strict Standards when calling non static methods statically. I've done a patch, but I'm not sure it's OK to change this in Chamilo, as it's more the responsability of MDB2, but in the other hand, MDB2 activity is pretty low now. At least it allows to develop with default error reporting without having lots of message in the top of the page.
Updated by Anonymous over 12 years ago
Goulwen Reboux wrote:
When starting digging in the code, I got lots of warnings from MDB2 about Strict Standards when calling non static methods statically. I've done a patch, but I'm not sure it's OK to change this in Chamilo, as it's more the responsability of MDB2, but in the other hand, MDB2 activity is pretty low now. At least it allows to develop with default error reporting without having lots of message in the top of the page.
First off, this is unrelated to the issue at hand; this should probably be a separate issue.
But more to the point, I ran into the same issue while writing tests for the Database class, and actually had to hack a workaround into the tests to temporarily disable the strict checking so the tests didn't fail prematurely on those issues, so I think fixing this is rather important. Your patch looks good to me and I think you should commit this, after filing a bugreport with this patch on the MDB2 issue tracker. Then you can refer to your MDB2 ticket number (and/or bugtracker URL) in the commit message.
My tests should help you to find these problems more easily, but unfortunately I still haven't been able to commit that because it's part of my Postgres support changes. I can't commit those until the new stable/dev different repos are set up, which should be by the end of this week I hear.
Updated by Goulwen Reboux over 12 years ago
Yep sorry, I've started write a new issue, but before post it, I made a new research for "MDB2" and found the ticket. I assume wrongly that it's related.
I create a new ticket on MDB2 issue tracker and then will add a new ticket here with the patch (I don't have the commit access currently)
Updated by Stefaan Vanbillemont over 12 years ago
- Project changed from Chamilo LCMS Connect to Repository
Updated by Anonymous over 12 years ago
- Project changed from Repository to Common
Updated by Stefaan Vanbillemont over 12 years ago
- Target version changed from 7 to 2.1.0
Updated by Stefaan Vanbillemont over 12 years ago
- Target version changed from 2.1.0 to 31
Updated by Stefaan Vanbillemont over 12 years ago
- Target version changed from 31 to Backlog (default)