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