Comments for "Mailman"

» Submit Your Comment
Comments are only accepted during the "Proposal" phase. This proposal is currently in the "Finished" phase.
» Comments

  • Vasil Rangelov  [2011-09-07 13:32 UTC]

    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."

  • Till Klampaeckel  [2011-09-07 19:15 UTC]

    Hey,

    a few suggestions:

    1) Don't use private, use protected.

    2) Update your docblocks (phpdoc.org has a lot of documentation -- e.g. a __construct doesn't return void, it returns the actual class.)

    3) Consider ext/dom to parse html (instead of preg_match)

    4) Get rid off user_error(): if the error can be neglected. I'd collect them in the class, add hasError(), getError() style methods. If those errors should be addressed, use an exception.

    5) Instead of file_get_contents(), use HTTP_Request2 so people can use different transports and customize it, etc.. E.g. right now it's not just hiding possible errors but I'm also unable to use a different extension, proxy or similar.

    6) I'd make adminurl and adminpw protected and add set-methods for various reasons: it looks like both require a little validation (e.g., is this a URL, does this password not contain spaces, etc.). Setting the public var could lead to unexpected things.


    All in all, I like the idea very much. I had the need for something like that a couple times before.

    Cheers,
    Till