Re: [PEPr] -1 for Caching::phpfastcache

Hello Khoa,

I am a bit short on time so I'd prefer if you post your code on Pear list to have others review it as well. My suggestions are not always feasible, so you'll have to see for yourself.

Regards,

Bertrand

Le 6 août 2013 à 01:09, Khoa Bui <[email protected]> a écrit :

> 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 <[email protected]>
> To: PEAR developer mailinglist <[email protected]> 
> Cc: Bertrand Mansion <[email protected]>; Khoa Bui <[email protected]> 
> 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.
> 
>