Proposal Auth_WIKID Extra Comment (QA)

Hello all, this is an email to do extra QA on
http://pear.php.net/pepr/pepr-proposal-show.php?id=321


First of all Greg, good work on the package, I really like
the overall
structure, the code is very clean, and I like the comments
themselves
and it looks good.

However, I must add a couple points:

The package is noted as PHP5, so the variable properties
should be
respected. I see in the documentor's code that all the
variables
access is noted as access private, therefore, you should
usually make
the variables
private $_isConnected  instead of var $_isConnected; Because
by
default var is now E_STRICT compliant, however it will make
the
variables public.

Also, making the variables private is not recommended as it
makes them
less flexible, therefore it is recommended to use *
protected for pear
packages.


Next point:
In the constructor of your class (which is not defined
using
__construct() { }) I see that you are doing :

       $min_version = "5.0.0";
        if (version_compare($min_version, phpversion(),
">=")) {

            echo "Incompatible version - Auth_WiKID
requires PHP >=
$min_versionn";
            return null;
        }

This is overhead because the pear installer is handling all
this using
the package.xml file (dependencies).

Next point:
In php5, you can use __destruct() in order to execute the
destroy and
all your close() function.

Next point:
As stated you have to make a dependency for openssl

Next point:
Nothing else to say else than good work for this quality
package 

Thanks,

P.S. Any other input would be appreciated guys..
(wink)(wink)

-- 
David Coallier,
Founder & Software Architect,
Agora Production (http://agoraproduction.com
)
51.42.06.70.18

-- 
PEAR Development Mailing List (http://pear.php.net/)
To unsubscribe, visit: http://www.php.net/unsub
.php

Thanks!.  I've updated my code with all your suggestions but
the last:  I've
set the dependency on the openssl extension in
package{,2}.xml and on the
project page - do I also need to specify it in the source or
somewhere else?

On 4/5/07, David Coallier <davidcphp.net> wrote:
>
> Hello all, this is an email to do extra QA on
> http://pear.php.net/pepr/pepr-proposal-show.php?id=321

>
> First of all Greg, good work on the package, I really
like the overall
> structure, the code is very clean, and I like the
comments themselves
> and it looks good.
>
> However, I must add a couple points:
>
> The package is noted as PHP5, so the variable
properties should be
> respected. I see in the documentor's code that all the
variables
> access is noted as access private, therefore,
you should usually make
> the variables
> private $_isConnected  instead of var $_isConnected;
Because by
> default var is now E_STRICT compliant, however it will
make the
> variables public.
>
> Also, making the variables private is not recommended
as it makes them
> less flexible, therefore it is recommended to use *
protected for pear
> packages.
>
>
> Next point:
> In the constructor of your class (which is not defined
using
> __construct() { }) I see that you are doing :
>
>        $min_version = "5.0.0";
>         if (version_compare($min_version, phpversion(),
">=")) {
>
>             echo "Incompatible version -
Auth_WiKID requires PHP >=
> $min_versionn";
>             return null;
>         }
>
> This is overhead because the pear installer is handling
all this using
> the package.xml file (dependencies).
>
> Next point:
> In php5, you can use __destruct() in order to execute
the destroy and
> all your close() function.
>
> Next point:
> As stated you have to make a dependency for openssl
>
> Next point:
> Nothing else to say else than good work for this
quality package 
>
> Thanks,
>
> P.S. Any other input would be appreciated guys..
(wink)(wink)
>
> --
> David Coallier,
> Founder & Software Architect,
> Agora Production (http://agoraproduction.com
)
> 51.42.06.70.18
>



-- 
Greg Haygood
ghaygoodgmail.com

Do you think you could upload your new version to

http://dev.wikidsystems.com/docs/php/pear/WiKID.phps ?

Thanks 

On 4/6/07, Greg Haygood <ghaygoodgmail.com> wrote:
> Thanks!.  I've updated my code with all your
suggestions but the last:  I've
> set the dependency on the openssl extension in
package{,2}.xml and on the
> project page - do I also need to specify it in the
source or somewhere else?
>
>
> On 4/5/07, David Coallier <davidcphp.net> wrote:
> > Hello all, this is an email to do extra QA on
> > http://pear.php.net/pepr/pepr-proposal-show.php?id=321

> >
> > First of all Greg, good work on the package, I
really like the overall
> > structure, the code is very clean, and I like the
comments themselves
> > and it looks good.
> >
> > However, I must add a couple points:
> >
> > The package is noted as PHP5, so the variable
properties should be
> > respected. I see in the documentor's code that all
the variables
> > access is noted as access private, therefore,
you should usually make
> > the variables
> > private $_isConnected  instead of var
$_isConnected; Because by
> > default var is now E_STRICT compliant, however it
will make the
> > variables public.
> >
> > Also, making the variables private is not
recommended as it makes them
> > less flexible, therefore it is recommended to use
* protected for pear
> > packages.
> >
> >
> > Next point:
> > In the constructor of your class (which is not
defined using
> > __construct() { }) I see that you are doing :
> >
> >        $min_version = "5.0.0";
> >         if (version_compare($min_version,
phpversion(), ">=")) {
> >
> >             echo "Incompatible version -
Auth_WiKID requires PHP >=
> > $min_versionn";
> >             return null;
> >         }
> >
> > This is overhead because the pear installer is
handling all this using
> > the package.xml file (dependencies).
> >
> > Next point:
> > In php5, you can use __destruct() in order to
execute the destroy and
> > all your close() function.
> >
> > Next point:
> > As stated you have to make a dependency for
openssl
> >
> > Next point:
> > Nothing else to say else than good work for this
quality package 
> >
> > Thanks,
> >
> > P.S. Any other input would be appreciated guys..
(wink)(wink)
> >
> > --
> > David Coallier,
> > Founder & Software Architect,
> > Agora Production (http://agoraproduction.com
)
> > 51.42.06.70.18
> >
>
>
>
>  --
> Greg Haygood
> ghaygoodgmail.com
>


-- 
David Coallier,
Founder & Software Architect,
Agora Production (http://agoraproduction.com
)
51.42.06.70.18

-- 
PEAR Development Mailing List (http://pear.php.net/)
To unsubscribe, visit: http://www.php.net/unsub
.php

David Coallier wrote:
> Hello all, this is an email to do extra QA on
> http://pear.php.net/pepr/pepr-proposal-show.php?id=321

> 
> First of all Greg, good work on the package, I really
like the overall
> structure, the code is very clean, and I like the
comments themselves
> and it looks good.
> 
> However, I must add a couple points:
> 
> The package is noted as PHP5, so the variable
properties should be
> respected. I see in the documentor's code that all the
variables
> access is noted as access private, therefore,
you should usually make
> the variables
> private $_isConnected  instead of var $_isConnected;
Because by
> default var is now E_STRICT compliant, however it will
make the
> variables public.
> 
> Also, making the variables private is not recommended
as it makes them
> less flexible, therefore it is recommended to use *
protected for pear
> packages.

I would add that this advice does not make much sense. I
mean that one
should use private and protected when necessary, there are
very good
reasons to have a private property and not wanting any
derived class
using it. So the advice should be to use what makes sense in
the design
of the class and should be left to the developer to decide.

ps: no offense david 

Arnaud.

-- 
PEAR Development Mailing List (http://pear.php.net/)
To unsubscribe, visit: http://www.php.net/unsub
.php

That's ok  They are
defined as private in this case but used as
public which was my main concern. Personally i'm a private
person,
however following most comments on previous and current
proposals,
there is a protected trend.

Anyways my main concern was that the use of public when was
marked as
access private.

On 4/6/07, Arnaud Limbourg <arnaudlimbourg.com> wrote:
> David Coallier wrote:
> > Hello all, this is an email to do extra QA on
> > http://pear.php.net/pepr/pepr-proposal-show.php?id=321

> >
> > First of all Greg, good work on the package, I
really like the overall
> > structure, the code is very clean, and I like the
comments themselves
> > and it looks good.
> >
> > However, I must add a couple points:
> >
> > The package is noted as PHP5, so the variable
properties should be
> > respected. I see in the documentor's code that all
the variables
> > access is noted as access private, therefore,
you should usually make
> > the variables
> > private $_isConnected  instead of var
$_isConnected; Because by
> > default var is now E_STRICT compliant, however it
will make the
> > variables public.
> >
> > Also, making the variables private is not
recommended as it makes them
> > less flexible, therefore it is recommended to use
* protected for pear
> > packages.
>
> I would add that this advice does not make much sense.
I mean that one
> should use private and protected when necessary, there
are very good
> reasons to have a private property and not wanting any
derived class
> using it. So the advice should be to use what makes
sense in the design
> of the class and should be left to the developer to
decide.
>
> ps: no offense david 
>
> Arnaud.
>


-- 
David Coallier,
Founder & Software Architect,
Agora Production (http://agoraproduction.com
)
51.42.06.70.18

-- 
PEAR Development Mailing List (http://pear.php.net/)
To unsubscribe, visit: http://www.php.net/unsub
.php

done!

On 4/6/07, David Coallier <davidcphp.net> wrote:
>
> Do you think you could upload your new version to
> 
http://dev.wikidsystems.com/docs/php/pear/WiKID.phps ?
>
> Thanks 
>
> On 4/6/07, Greg Haygood <ghaygoodgmail.com> wrote:
> > Thanks!.  I've updated my code with all your
suggestions but the
> last:  I've
> > set the dependency on the openssl extension in
package{,2}.xml and on
> the
> > project page - do I also need to specify it in the
source or somewhere
> else?
> >
> >
> > On 4/5/07, David Coallier <davidcphp.net> wrote:
> > > Hello all, this is an email to do extra QA
on
> > > http://pear.php.net/pepr/pepr-proposal-show.php?id=321

> > >
> > > First of all Greg, good work on the package,
I really like the overall
> > > structure, the code is very clean, and I like
the comments themselves
> > > and it looks good.
> > >
> > > However, I must add a couple points:
> > >
> > > The package is noted as PHP5, so the variable
properties should be
> > > respected. I see in the documentor's code
that all the variables
> > > access is noted as access private, therefore,
you should usually make
> > > the variables
> > > private $_isConnected  instead of var
$_isConnected; Because by
> > > default var is now E_STRICT compliant,
however it will make the
> > > variables public.
> > >
> > > Also, making the variables private is not
recommended as it makes them
> > > less flexible, therefore it is recommended to
use * protected for pear
> > > packages.
> > >
> > >
> > > Next point:
> > > In the constructor of your class (which is
not defined using
> > > __construct() { }) I see that you are doing
:
> > >
> > >        $min_version = "5.0.0";
> > >         if (version_compare($min_version,
phpversion(), ">=")) {
> > >
> > >             echo "Incompatible version -
Auth_WiKID requires PHP >=
> > > $min_versionn";
> > >             return null;
> > >         }
> > >
> > > This is overhead because the pear installer
is handling all this using
> > > the package.xml file (dependencies).
> > >
> > > Next point:
> > > In php5, you can use __destruct() in order to
execute the destroy and
> > > all your close() function.
> > >
> > > Next point:
> > > As stated you have to make a dependency for
openssl
> > >
> > > Next point:
> > > Nothing else to say else than good work for
this quality package 
> > >
> > > Thanks,
> > >
> > > P.S. Any other input would be appreciated
guys.. (wink)(wink)
> > >
> > > --
> > > David Coallier,
> > > Founder & Software Architect,
> > > Agora Production (http://agoraproduction.com
)
> > > 51.42.06.70.18
> > >
> >
> >
> >
> >  --
> > Greg Haygood
> > ghaygoodgmail.com
> >
>
>
> --
> David Coallier,
> Founder & Software Architect,
> Agora Production (http://agoraproduction.com
)
> 51.42.06.70.18
>



-- 
Greg Haygood
ghaygoodgmail.com

David Coallier wrote:
> That's ok  They are
defined as private in this case but used as
> public which was my main concern. Personally i'm a
private person,
> however following most comments on previous and current
proposals,
> there is a protected trend.
>
> Anyways my main concern was that the use of public when
was marked as
> access private.

Indeed that makes sense.

Arnaud.

-- 
PEAR Development Mailing List (http://pear.php.net/)
To unsubscribe, visit: http://www.php.net/unsub
.php