My Code?

Andrew · 10-21-2007, 10:56 PM

I'm in the middle of coding a script, and before I want to continue, I want to make sure that my OOP at least conforms to what it should look like. Also, I noticed Haris has posted threads asking if his code looks good, so I figured it's worth a shot to post mine.

Keep in mind, this isn't nearly finished, I still have the whois function to make, and then also add in other features I have planned.

Index:
http://psdtocode.com/index.phps

Whois Class:
http://psdtocode.com/whois.class.phps

A brief explanation on my whois output. I'm a neat freak when it comes to how my code looks, so I wanted to output it to look perfect. However, if I release this (probably unlikely), I'm going to get rid of that so it doesn't look weird in different layouts.

Karl · 10-22-2007, 11:36 AM

Nice start Andrew, there's a few places where I'd like to point out some mistakes. The first mistake is your lack of member declarations, you have assigned a value to 2 member properties but there is no actual declaration for either property. Example:

PHP Code:


			
class Whois
{
   private $domain;
   private $extensions;
   ...

The visibility of those properties may need to be public or some accessor methods adding (that is, a get method for returning their values). The next thing I noticed was that you called validate and passed 2 arguments along with the call, even though:

1) valdidates does not accept any function arguments
2) the arguments you try to pass through are already stored
as member properties, thus you already have access to them
inside the valdiates function

Im guessing that was actually a mistake and wasn't intentional? Overall, nice start, good to see you trying to do this in OOP :)

Andrew · 10-23-2007, 12:20 AM

What do you mean in your bullet points? Should I pass them into the validate function as they are ($this->domain)? Is that supposed to give validate ability to use them? Sorry, new to OOP. :) I also was thinking about declaring those variables, yet wasn't exactly sure what to do for them. Thanks for the tips, will put them in use. :)

Wildhoney · 10-23-2007, 12:32 AM

A couple of things to mention. Concerning your regular expression for checking the domains, I don't think such domains as .co.uk and .org.uk domains will validate. Also the hyphen character (-) is technically a regular expression character for range and therefore should be escaped. Although it would work fine in the instances you've used to it in.

Also, one more thing, just for making your code nice and tidy if you're particular - which it would appear you are, use the single quotes (see this article) when outputting the HTML, or the heredoc concept (which I personally dislike) makes it much neater:

Single Quotes (Yum!)

PHP Code:


			
echo 'Show this';
echo 'And this';
echo 'And this, too!';

Heredoc (Yuk!)

PHP Code:


			
<<<EOF
Show this
And this
And this, too!
EOF;

Andrew · 10-23-2007, 12:58 AM

I did change it to single quotes, but the escaped characters for new lines and such didn't work, and I didn't want to have single quotes, then double quotes after the singles just to make those work.

Also, I was aware of those domains not working. I was talking with Salathe about it for a bit and realized it, which I still need to find a work around for.

Salathe · 10-23-2007, 01:41 AM

Just a little bit of information which might help you. You probably know this already (or wish you did) but the explode function can have an optional third argument called limit. This enables you to limit the number of array elements returned. To put that into something visual:

PHP Code:


			
<?php

$domain = 'salathe.co.uk';

// Normal way of using explode
$parts_normal = explode('.', $domain); // array('salathe', 'co', 'uk')

// Limit the number of elements
$parts_limit = explode('.', $domain, 2); // array('salathe', 'co.uk')

// You could also do
$extension = array_pop(explode('.', $domain, 2)); // $extension = 'co.uk'


// Show some output 
header('Content-Type: text/plain');
var_dump($parts_normal, $parts_limit, $extension);

Another point that I'd like to raise is that your objects (classes) should only really be doing one thing. Personally, I think that you Whois class should be dealing only with the whois information, not handling the output at all (the Whois::display() method).

If somewhere along the line you have 20 classes all with a display() method within them, it'll be a nightmare trying to debug the output and even worse if later on you decide that you want to offer both HTML and another form of output (XML, PDF, whatever).

Wildhoney · 10-23-2007, 02:33 AM

You also really want to check for if they enter the www. part of the domain which they could do, and act accordingly, else your extension is going to be incorrect.

PHP Code:


			
$domain = 'www.salathe.co.uk';
$limit = 2;

if(substr($domain, 0, 4) == 'www.')
{
    $limit = 3;
}

$extension = array_pop(explode('.', $domain, $limit));

Andrew · 10-23-2007, 04:49 AM

Thanks for all the help, I saw the limit part of explode on php.net, but never thought to look at it.

I'm actually thinking of dumping this project. It was a good idea I thought at first, but last night I was thinking maybe I'm doing too much trying to both learn OOP and even more techniques (doing whois with fsockopen), all at once. Thanks for all the help, and it will come in handy in the next project I'm planning. I'm doing to outline this one out first to make sure I have it all in my head.

Haris · 10-23-2007, 06:53 AM

And you forgot the comments.

For including configuration and classes file, why not use include_once instead of include? That'll help producing error if you accidentally include them more than 1 time.

Wildhoney · 10-23-2007, 02:44 PM

I did one of these a year or so ago, and although the code's not that good any more (edit: updated it :)), it does still work. It may come in handy if you do decide to continue.

PHP Code:


			
$szAddress = sprintf('http://www.whois.net/whois_new.cgi?d=%s&tld=%s', $szDomainNoExt, $szDomainExt);



curl_setopt($pCurl, CURLOPT_URL, $szAddress);

curl_setopt($pCurl, CURLOPT_HEADER, true);

curl_setopt($pCurl, CURLOPT_RETURNTRANSFER, true);

curl_setopt($pCurl, CURLOPT_FOLLOWLOCATION, true);



$szData = curl_exec($pCurl);

curl_close($pCurl);

    

if(strpos(strip_tags($szData), 'No match found for'))

{

    return false;

}



return true;

Andrew · 10-23-2007, 11:30 PM

Quote:

Originally Posted by Haris

And you forgot the comments.

For including configuration and classes file, why not use include_once instead of include? That'll help producing error if you accidentally include them more than 1 time.

I went back through and did comments, but since I didn't update any of the code, I didn't reupload the source.