Stephan Schmidt [2004-09-24 07:54 UTC] Hi,
1. You seem to have some indentation problems (are you using tabs?)
2. Please remove all methods that you marked with @ignore
3. Please change System_Serial_Win32COMconnection::newConnection() to System_Serial_Win32COMconnection::singleton()
4. Please consider providing a PHP4 version. Except visibility you are not using any PHP5-specific features
5. Please follow the coding standards:
if (!$result) $this->_initialized = TRUE;
should be
if (!$result) {
....
}
Stephan
Ian Eure [2004-09-24 08:28 UTC] Some additional issues:
1. Why do you have a System_Serial_Factory class, instead of a System_Serial class with a factory method?
2. What is the point of the switch in System_Serial_Factory::NewConnection? You seem to drop-through to an if statement, so the switch appears to just be bloat.
3. Why is there a function which only returns the PHP_OS constant? Why not use it directly?
4. More CS stuff; FALSE should be changed to false.
5. I think there may be path-related security implications to calling "mode" in System_Serial_Win32COMconnection::init() without a full path. Is "mode" a builtin function, or an external command? If it's external, it should have the full path added.
6. Any reason why System_Command couldn't be used to execute these commands? I believe it handles the path-related issues I raised in point #5.
7. I think the class names are misleading. COM connetions could work on 64-bit Windows, not just Win32. And many *NIX variants use /dev/(whatever) syntax for serial ports.
8. I'm not convinced that seperate classes are necessary, in any case, since it's only initialization that's platform-specific. Why not have a single class with e.g. _unixInit() and _winInit()?
Martin Jansen [2004-09-24 08:57 UTC] In addition to the other comments a bit more:
1) PEAR has the so called "one-class-per-file" convention, which means that multiple classes should end up in multiple files.
2) Why don't you simply use
return PHP_OS;
in _determineOS()?
3) I'd suggest to rename getLine() to readLine() and getArray() to readLines().
4) I'm by no means an expert in serial communication, but shouldn't there be a way to change the value of $badCommandResponse in getArray()? (There may be systems that use a different protocol dialect.)
5) I'm missing documentation for the package. Are you going to write some?
Generally this looks like a useful addition for PEAR.
Stig Bakken [2004-09-25 00:07 UTC] Nice class, I'll try it out on my X.10 controller!
What about taking the device name as a parameter to singleton()? This would make it work for more than one device at a time.
Many of the stty settings used in the unixInit() method are non-POSIX and will cause portability issues. It is probably better to omit the non-POSIX settings by default, and include them on OSes where they are known to be implemented (check PHP_OS, php_uname() or posix_uname()).
Alan Knowles [2004-09-25 00:37 UTC] Considering this is a PHP5 proposal and most of ext/dio is available on Windows in PHP, it would appear this is mostly redundant.
|
