Re: Re: Net Traceroute still needs some votes :-)
| From: | Alan Knowles | Date: | Tue, 12 Aug 2003 07:43:02 +0000 |
| Subject: | Re: Re: Net Traceroute still needs some votes :-) | ||
| References: | 1 2 | Groups: | php.pear.dev |
| Request: | Send a blank email to pear-dev+get-19549@lists.php.net to get a copy of this message | ||
Thanks -
Generally +1, however
Please make sure you escapeshellargs() all the arguments going to the command line! - it's a biggy security hole..
Other Comments... (less critical :)
There seemed very little reason for making the variables in the Result object Private ... - you could get rid of 4 methods (and 4 pages of documentation :) - and users can understand it quicker..
* variable methods?
$this->{'_parseResult' . $this->sysname}();
rather than..
call_user_func(array(&$this, '_parseResult'.$this->_sysname));
= it's marginally clearer.. :)
* method_exists?
in_array('_parseresult'.$sysname, array_values(get_class_methods('Net_Traceroute_Result')));
== try
method_exists(
'Net_Traceroute_Result',
'_parseresult'.$sysname
);
* There are a few CS fixes needed..
if ((int) $tempparts[$searchIdx] > 0)
$this->_ttl = (int) $tempparts[$searchIdx];
else
if (!empty($this->_raw_data[$dataRow+1]))
{
= all control structures should use brackets.
= the brackets on go on the same line as the control structures (unlike functions/method defs.)
= constants should be NET_TRACEROUTE*,
* I would make trigger errors a configurable option? -- introduce option arrays in constructors/factory??
* 'which' is not 100% reliable (If it's not in the PATH') - a fallback to a standard unix lookup would probably work.
foreach(array('/usr/bin','/bin','/usr/local/bin') as $test) {
if (file_exists.....
}
* I'm not sure about the use of a factory constructor, when it doesnt actually follow a classic factory pattern is a great idea.. - perhaps just throw the 'no traceroute prog error when you actually run $traceroute->traceroute();
Regards
Alan
Stefan Neufeind wrote:
Done that. Available on: http://pear.speedpartner.de/ Direct link: http://pear.speedpartner.de/packages/unpacked/Net_Traceroute- 0.11/Traceroute.php.txt Stefan On 12 Aug 2003 at 14:56, Alan Knowles wrote:-- Can you help out? Need Consulting Services or Know of a Job? http://www.akbkhome.comCan you post a .phps or php.txt view of the code, It makes reviewing it alot easier (and voting on it easier :) Regards Alan nicos@php.net wrote:"Stefan Neufeind" <Neufeind@speedpartner.de> a écrit dans le message de news:3F37B4D6.31370.1808EFD@localhost...-- Can you help out? Need Consulting Services or Know of a Job? http://www.akbkhome.comHey Tarjei, didn't intend to make fun of your mail. But the same problem here. Net Traceroute is an adoption from Net Ping to a wrapper for traceroute. We still need 3 votes to get it in. Please review at (latest changes to windows-parser only a few hours ago): http://pear.speedpartner.de StefanDon't spam too much please. But +1.