Re: [PEPr] -1 for Caching::phpfastcache
| From: | Khoa Bui | Date: | Mon, 05 Aug 2013 23:09:14 +0000 |
| Subject: | Re: [PEPr] -1 for Caching::phpfastcache | ||
| References: | 1 2 | Groups: | php.pear.dev |
| Request: | Send a blank email to pear-dev+get-55256@lists.php.net to get a copy of this message | ||
Oh yeah.
Thanks Bretrand again for your comments. Now i Have a list to work on it ^_^
Will get back to u guys again, when I am ready again with this one.
Next time, I will send you the code first, before I post on pear, Bretrand. Will you look at it next
time for me?
________________________________
From: Bertrand Mansion <php@mamasam.net>
To: PEAR developer mailinglist <pear-dev@lists.php.net>
Cc: Bertrand Mansion <php@mamasam.net>; Khoa Bui <khoaofgod@yahoo.com>
Sent: Monday, August 5, 2013 4:51 PM
Subject: [PEPr] -1 for Caching::phpfastcache
Bertrand Mansion (http://pear.php.net/user/mansion) has voted -1 on the proposal for
Caching::phpfastcache.
Proposal information:
http://pear.php.net/pepr/pepr-proposal-show.php?id=692
Vote information:
http://pear.php.net/pepr/pepr-vote-show.php?id=692&handle=mansion
Comment:
I vote -1 because your code is not ready yet, in my opinion.
Have a look at :
- accessors methods to set/get options in drivers
- call_user_func_array instead of eval()
- You might use __invoke() instead of your first function
- use an autoloader for drivers or you can load all drivers at once instead
of hidding require_once in methods, and get rid of isExistingClass...
- follow pear coding standards
- use an interface for drivers and maybe a common abstract class
- spelling mistakes in sqlite driver
- isExistingClass ? heard of
http://php.net/manual/en/function.class-exists.php
- define property and methods scopes explicitly
- rename $this->method to $this->driver or storage
- don't die(), use Exceptions or trigger_error
- have each driver define the options it needs, not the main class
- clean up your code at the bottom of the class
- You might have to rename your class if you want it to fit PEAR standards
(there is already 2 cache packages in pear)
I haven't looked at the drivers code, there might be things to fix there
too.