Re: [PEPr] Comment on PHP::Mailman

Here are the changes I've made based on your input: https://github.com/hm2k/PHP_Mailman/commit/c396ce36d37393a9dba7c8dd6f6caad4c28e8380 Thanks. On Wed, Sep 7, 2011 at 2:32 PM, Vasil Rangelov <boen.robot@gmail.com> wrote: > Hi James. > > Here are my impressions from an initial overview of the source: > > For the most part, the code itself seems fine. The only notable exception > is $invite at the subscribe() method. Why integer? Why not accept booleans, > and conver the value to integer? > > Also, for the sake of performance, consider getting rid of the _fetch() > method. If all you do in it is file_get_contents, you might as well do it > without the overhead of a method call. > > But those are minor issues... the bigger problem IMHO is in the docs. You > should keep in mind that tools like PhpDocumentor or similar are going to > be generating documentation out of them which would be presented > differently, and would not include any source code. > > e.g. in the constructor you have > /** > * The class constructor > * > * @param string $adminurl Sets the class variable > * @param string $list Sets the class variable > * @param string $adminpw Sets the class variable > * > * @return void > */ > And all I'm thinking is "what class variable? What is it for?". Your > default values are making things even more misleading. Unless I had read > the docs on the class variables, I wouldn't have realized that $list and > $adminpw are supposed to be strings ("false" leads me to think those are > booleans). > > Also, avoid relying on any order. Use links instead. e.g. instead of > "Set digest (you have to first subscribe them using URL above, then set > digest)" > use > "Set digest. Note that the $email needs to be subsribed first, e.g. by > using the {@link subsribe()} method." > > -- > http://pear.php.net/pepr/pepr-proposal-show.php?id=665 >