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

Finally got round to clearing this off my TODO flags.. :) - responses in-line..

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" :-)

In general it is good advice however you have to bear in mind. = there will always be a speed trade off in scripting languages for method calls over direct access.. (I've never benchmarked this directly, but AFAIK it's about 2x slower at least..) = PHP in theory uses __get_x/__set_x in overload to implement transparent getters/setters.. (more like C#'s get/set methods.) = It can add considerable extra documentation/ increases barrier to entry/usage for a class.. Deciding on this should be a carefull balance.. - On a large project, you may end up adding a huge overhead by implementing this everywhere.. Even in the famous Pattern book, it does discuss the trade off of reducing encapsulation to offer greater flexibility in Visitor type methods.. (eg. the ability to extend an objects' API without actually modifying it or 'A extends B' it.)

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]))

I guess this reply may be outdated now :) - but you should always use {} for any control structure - eg. if/else/while....

It's a usefull habit, especially when your editor has brace matching. - makes finding mismatches easier, and reduces mistakes when you think you are adding a statement to an unbracketed statement.

(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. Sounds like a bug :) - If it's still there file a bug at bugs.php.net...

* 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?

false is a better return than null.. - as you can reliably test for it (see php-dev archives :)

* '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?

sounds good..

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

It's probably survivable to leave it as is..:)

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

.

-- Can you help out? Need Consulting Services or Know of a Job? http://www.akbkhome.com