Comments for "System_Launcher"

» Submit Your Comment
Comments are only accepted during the "Proposal" phase. This proposal is currently in the "Finished" phase.
» Comments
  • Michael Gauthier  [2010-08-18 01:32 UTC]

    Great start, thanks for providing unit tests and documentation. Here's my feedback so far:

    1.) run phpcs on the code. It will pick up many formatting errors.
    2.) I'm not a big fan of hungarian notation. I don't know of any other PEAR packages that use it and IMO it doesn't make a lot of sense for a dynamically typed language.
    3.) Use exceptions instead of triggering errors. PEAR packages are not allowed to use trigger_error.
    4.) Package might fit better in the "System" category than the "File" category. The File category is for file formats. I'd suggest renaming the package as System_Launcher.
  • Daniel O'Connor  [2010-08-18 03:20 UTC]

    Consider structuring your package slightly differently:
    File/
    File/Launcher.php

    That lets you run tests straight out of svn/git without doing:
    require_once dirname(__FILE__).'/../File/Launcher.php';

    Even better - it'll slot right into the unit test framework :)


    I'd avoid doing work in the constructor (it makes unit testing harder):
    public function __construct()
    {
    $this->nCurrentOS = $this->detectOS();
    }


    I'd consider splitting it into driver type classes if you see more in depth functionality being required. This also would allow people to easily mock out the launch command.

    class File_Launcher {
    // Snip:
    public function getCommand() {
    return $this->os->getCommand();
    }

    public function detectOS() {
    foreach ($this->drivers as $driver) {
    if ($driver->applies()) {
    $this->os = $driver;
    return;
    }
    }
    throw new Exception(":(")
    }
    }

    class File_Launcher_Foo implements File_Launcher_Driver {
    /** What's the command to launch things? */
    public function getCommand() {}
    /** Does this apply to the current operating system? */
    public function applies() {}

    }

    Final one: static methods make unit testing harder - I know you want to make it trivial to do File_Launcher::launchBackground() as a one liner, but I'd urge against it - $foo->launch(..., true or false) tends to cover it.

    Overall: I like it
  • Till Klampaeckel  [2010-08-18 22:22 UTC]

    Just curious what do you use this for? Like a real world example.

    All in all, this is pretty interesting, e.g. I didn't know of gnome-open before. Thanks for broadening my horizon. :-)
  • Jesús Espino  [2010-08-19 17:36 UTC]

    Nice package, only two comments.

    Test coverage is near the 50%, not bad, but not good.

    And the which command not have -s parameter in all systems, for example, my system (Ubuntu 10.4) don't have the -s parameter in which.
  • Christian Weiske  [2011-03-14 11:30 UTC]

    Any updates?
  • Olle Jonsson  [2011-07-12 12:03 UTC]

    All the above feedback has been incorporated, and I just renamed the repository to System_Launcher at Github, to reduce confusion.

    I went with Mr O'Connor's package structure. Very small classes.