Re: Re: Net Traceroute still needs some votes :-)

On 12 Aug 2003 at 15:43, Alan Knowles wrote: > Generally +1, however thank you. > Please make sure you escapeshellargs() all the arguments going to the > command line! - it's a biggy security hole.. So changing this line: ${$option} = $this->_argRelation[$this->_sysname][$option]." ".$value." "; in _createArgList from $value to escapeshellarg($value) should do the trick, right? Thank you! > 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.. Well, but this is the way encapsulation is intended to, right? On the one had you might say "well, if anybody dares to change variables it's his own fault. But isn't this the way of handing objects is designed to be? I've been told multiple times during school and university "to better use get-and-set-Methods" :-) > * variable methods? > > $this->{'_parseResult' . $this->sysname}(); > rather than.. > call_user_func(array(&$this, '_parseResult'.$this->_sysname)); > > = it's marginally clearer.. :) confirmed / changed :-) > * method_exists? > in_array('_parseresult'.$sysname, > array_values(get_class_methods('Net_Traceroute_Result'))); > == try > method_exists( > 'Net_Traceroute_Result', > '_parseresult'.$sysname > ); okay, good point also. makes it a bit more readable :-) > * 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 Fixed those also (locally here - waiting for further fixes to go public again). Well, for small while-loops like while (($searchIdx < count($tempparts)) && ((int) $tempparts[$searchIdx] <= 0)) { $searchIdx++; } it seems strange to me to use brackets - but if that's the commonly agreed standard okay. But my if ... else-statements are okay - or not? if ((int) $tempparts[$searchIdx] > 0) { $this->_ttl = (int) $tempparts[$searchIdx]; // TTL might be written in next line; e.g. on Windows 98 } else if (!empty($this->_raw_data[$dataRow+1])) > (unlike functions/method defs.) = constants should be NET_TRACEROUTE*, You mean TRACEROUTE_HOST_NOT_FOUND becoming now NET_TRACEROUTE_HOST_NOT_FOUND Right? Is that also "coding standard"? If yes, this is also an issue with Net_Ping from which this naming-style was adopted. > * I would make trigger errors a configurable option? -- introduce > option arrays in constructors/factory?? How would you do it? And what e.g. in "factory" would you return if you don't throw an error? In this case just NULL, right? > * '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..... > } changed from: $traceroute_path = exec("which traceroute", $output, $status); if ($status != 0) { to your extended version: $traceroute_path = exec("which traceroute", $output, $status); if ($status != 0) { foreach(array('/usr/bin','/bin','/usr/local/bin') as $test) { if ($status != 0) { $traceroute_path=$test.'/traceroute'; if (file_exists($traceroute_path)) { $status = 0; } } } } if ($status != 0) { Agreed? > * 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(); Also adopted from Net_Ping and I believed that Net_Ping was generally agreed upon. Well if factory doesn't directly throw an error I'd need to use a variable for storing if a traceroute-prog was found, right? Or should I change the return PEAR::throwError(NET_TRACEROUTE_CANT_LOCATE_TRACEROUTE_BINARY_MSG, NET_TRACEROUTE_CANT_LOCATE_TRACEROUTE_BINARY); into an: $this->_traceroute_path=''; return NULL; This way you wouldn't return an error but could optionally throw an error. Is that a good solution? Well, I'm currently a bit in doubt about the error-handling-things mentioned above. From your experience with pear ... what do you think would most people generally agree upon? I believed Net_Ping was already agreed upon so the factory-things were generally good. Thank you very much for your feedback and for taking the time to have a look at it. Will wait a few days for further suggestions before releasing a new minor version. Stefan > 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 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... > >>> > >>> > >>>>Hey 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 > >>>> > >>>>Stefan > >>> > >>> > >>>Don't spam too much please. > >>> > >>>But +1.