How can this this URL Checker be made cleaner?

Question

I have a table full of URLs, some of which will occasionally become invalid. As part of a system I'm writing to go through the links periodically, and test if they are valid (or redirect to valid sites), I wrote the following class. The feedback I'm looking for is primarily related to clean code and best practices--how should the URL be passed into this class, for example? (I have not included the test cases, but they were written first. :) )

Originally, I wrote this with the URL passed in on the constructor, but I realized that if I set it later, I could both make the class a dependency of another class without needing to know the URL ahead of time, and use the same object to test multiple URLs.

But as it is, my URLisValid function is doing two things: setting a class variable and evaluating the URL. That doesn't seem ideal to me, but neither does having a separate Setter, nor does passing the URL around the class as an argument, nor does passing it in on the constructor.

(obviously, I think this works fine as-is, although feel free to point out any real bugs. I'm improving this class as a lesson to myself in making my code as clean as possible).

class URLChecker
{
private $curl;
private $url;

const HTTP_RESPONSE_OK = 200;

public function __construct()
{
    $this->curl = curl_init();
}

public function __destruct()
{
    curl_close( $this->curl );
}

public function URLisValid( $url )
{
    $this->url = $url;
    $httpcode = $this->getHttpCodeForURL();
    return $this->httpCodeIsValid( $httpcode );
}

private function getHttpCodeForURL()
{
    $this->executeCurlSessionForHeadersOnly();
    return $this->getHttpCodeFromLastTransfer();
}

private function executeCurlSessionForHeadersOnly()
{
    $this->setCurlOptions();
    curl_exec( $this->curl );           
}

private function getHttpCodeFromLastTransfer()
{
    return curl_getinfo( $this->curl, CURLINFO_HTTP_CODE );
}

private function setCurlOptions()
{
    curl_setopt($this->curl, CURLOPT_URL, $this->url);
    curl_setopt($this->curl, CURLOPT_NOBODY, true);
    curl_setopt($this->curl, CURLOPT_RETURNTRANSFER, true);
    curl_setopt($this->curl, CURLOPT_FOLLOWLOCATION, true);     
}   

private function httpCodeIsValid( $httpcode )
{
    return $httpcode == self::HTTP_RESPONSE_OK;
}   
}

EDIT: version 2, based in part on some feedback:

class URLChecker

{
private $session;

const HTTP_RESPONSE_OK = 200;

public function __construct()
{
    $this->session = curl_init();
    $this->initializeSessionOptions();
}

public function urlIsValid( $url )
{
    $this->setSessionUrl( $url );
    $this->executeSession();
    $httpcode = $this->getHttpCodeFromLastSession();
    return $this->httpCodeIsValid( $httpcode );
}

private function executeSession()
{
    curl_exec( $this->session );            
}

private function getHttpCodeFromLastSession()
{
    return curl_getinfo( $this->session, CURLINFO_HTTP_CODE );
}

private function setSessionUrl( $url )
{
    curl_setopt($this->session, CURLOPT_URL, $url);
}

private function initializeSessionOptions()
{
    curl_setopt($this->session, CURLOPT_NOBODY, true);
    curl_setopt($this->session, CURLOPT_RETURNTRANSFER, true);
    curl_setopt($this->session, CURLOPT_FOLLOWLOCATION, true);      
}   

private function httpCodeIsValid( $httpcode )
{
    return $httpcode == self::HTTP_RESPONSE_OK;
}

function __destruct()
{
    curl_close( $this->session );
}       
}

I've removed $url as a class variable entirely, I've changed some method names to remove references to "curl" where the implementation of the session is not relevant to the logic. I kept the one line functions, because I want someone who does not know exactly how curl works to still be able to read the code. It is now clear that $url is not a property of the class--it is rather a variable to be passed in once, and tested.

Answer 1

one line functions make your logic hard to follow, I would advise strongly against them. Especially if you only call that function only once, which is the case in most of your code.

As a very minor comment, I would put

curl_setopt($this->curl, CURLOPT_NOBODY, true);
curl_setopt($this->curl, CURLOPT_RETURNTRANSFER, true);
curl_setopt($this->curl, CURLOPT_FOLLOWLOCATION, true);   

inside __construct(), it should be a tiny bit more efficient.

One more minor comment, $url could easily be passed as a parameter, why make it a property?

All that taken into account, I would rather maintain something like this:

class URLChecker
{
private $curl;

public function __construct()
{
    $this->curl = curl_init();
    curl_setopt($this->curl, CURLOPT_NOBODY, true);
    curl_setopt($this->curl, CURLOPT_RETURNTRANSFER, true);
    curl_setopt($this->curl, CURLOPT_FOLLOWLOCATION, true);    
}

public function __destruct()
{
    curl_close( $this->curl );
}

public function URLisValid( $url )
{
    $httpcode = $this->getHttpCodeForURL( $url );
    return  $httpcode == 200; //HTTP_RESPONSE_OK
}

private function getHttpCodeForURL( $url )
{
    curl_setopt($this->curl, CURLOPT_URL, $url);
    curl_exec( $this->curl ); 
    return curl_getinfo( $this->curl, CURLINFO_HTTP_CODE );
}
}

Answer 2

Few things about code style:

• Capitalize your function name the same way

  URLisValid vs setCurlOptions

  One is capitalized (CamelCase), the other is not (camelCase). I advice you choose the latter, which is the Java standard and I must say it is a pretty solid one in terms of code clarity. You may chose the other if you want, but the most important thing : be sure to keep the same pattern everywhere.

• Build your function names in the same way

  URLisValid vs setCurlOptions

  One starts with the verb, the other one with a name. Well I do agree that if makes nicer ifs if(URLisValid($url) vs (if(isURLValid($url) BUT when using an IDE or autocompletion(, and also when reviewing a class file), it's easier if like lets the function name follow the same pattern. Right now if you type in autocomplete is you will have no result. And you have to know that the two function that checks for validity exists. By putting isURLValid and isHTTPCodeValid, you can easily find the function is now returns both of them and you keep some sort of similarity between your function names.

• Capitalization patterns generally

  URL vs Http

  You capitalize URL but not HTTP. Well writing URL or Url doesn't really matters. But be sure to keep the same pattern for every type. If you write URL then write HTTP and vice versa.

---

Something you pointed out:

But as it is, my URLisValid function is doing two things: setting a class variable and evaluating the URL. That doesn't seem ideal to me, but neither does having a separate Setter, nor does passing the URL around the class as an argument, nor does passing it in on the constructor.

Well, setters are meant for object encapsulation and part of this is validation. It would probably be clearer to validate in a setter than to set in a validator. Setters are meant to block people from directly modifying and accessing a variable, else you could just put it as public. You could easily rename your URLisValid function to setURL and throw an exception or return false or w/e that suits your need in the setter function, and that would be clearer (to me) than having a validator that sets a value.

About function length, well I do think it's OK. It makes it easy to test the class.

---

About your edit :

To me your class looks pretty good. One thing you could do is catching the cURL exceptions to throw your own exception based on what happened (be sure to keep the old one in the stack trace though). By throwing your own exception you increase encapsulation because the person using your class doesn't have to know whats underlying in it. They don't have to get the cURL exceptions directly. But that is not necessary.