Return to Answer

##The Question

##General Feedback ###Passing instance/member variables to method Why pass $this->timeout to makeCurlRequest from the checkVote method? If the method was called outside this class (and the scope of the method changed to protected for sub-classes or public for anywhere else in the code) then it might make sense to accept that parameter.

###Misleading Method Name The method makeCurlRequest appears to check if the curl function curl_version() exists and then either makes a cURL request or utilizes file_get_contents(). Thus the method might not always make a cURL request and a better name might be makeRequest. The code in the block when the curl_version() function does exist could be moved to a new method called makeCurlRequest.

###return from makeCurlRequest() The method makeCurlRequest has two return statements. One is in the else block, and the other is at the end of the method - i.e. return $requestData;. That variable,$requestData, appears to be the response from the API when the curl_version() function exists (and thus the cURL request is made), yet in the else block, $requestData is assigned a resource from stream_context_create(). While this code likely functions correctly, it could be confusing to a teammate who had to update it. A more appropriate name for the return value from the call to stream_context_create() might be something like $streamContext. Then it might be simpler to either assign the return value from the call to file_get_contents() to $requestData and utilize the return of that variable at the end of the method, or when making the curl request, return the response at the end of that block.

###Redundant return false

###Constants for Response codes It would be wise to define (class) constants for the response codes, like the ones below. While there currently appears to only be one place in your code where that value appears, there may arise a need to have it appear in other logic and thus it would be useful to reuse the constant(s). Then if the value would ever need to be updated (e.g. if the API ever changes) then it can be updated in your code in one spot.

The Question

General Feedback

Passing instance/member variables to method

Why pass $this->timeout to makeCurlRequest from the checkVote method? If the method was called outside this class (and the scope of the method changed to protected for sub-classes or public for anywhere else in the code) then it might make sense to accept that parameter.

Misleading Method Name

The method makeCurlRequest appears to check if the curl function curl_version() exists and then either makes a cURL request or utilizes file_get_contents(). Thus the method might not always make a cURL request and a better name might be makeRequest. The code in the block when the curl_version() function does exist could be moved to a new method called makeCurlRequest.

return from makeCurlRequest()

The method makeCurlRequest has two return statements. One is in the else block, and the other is at the end of the method - i.e. return $requestData;. That variable,$requestData, appears to be the response from the API when the curl_version() function exists (and thus the cURL request is made), yet in the else block, $requestData is assigned a resource from stream_context_create(). While this code likely functions correctly, it could be confusing to a teammate who had to update it. A more appropriate name for the return value from the call to stream_context_create() might be something like $streamContext. Then it might be simpler to either assign the return value from the call to file_get_contents() to $requestData and utilize the return of that variable at the end of the method, or when making the curl request, return the response at the end of that block.

Redundant return false

Constants for Response codes

It would be wise to define (class) constants for the response codes, like the ones below. While there currently appears to only be one place in your code where that value appears, there may arise a need to have it appear in other logic and thus it would be useful to reuse the constant(s). Then if the value would ever need to be updated (e.g. if the API ever changes) then it can be updated in your code in one spot.

###return from makeCurlRequest() The method makeCurlRequest has two return statements. One is in the else block, and the other is at the end of the method - i.e. return $requestData;. However,$requestData is only defined when the curl_version() function exists (and thus the cURL request is made). In the else block, $requestData is assigned a resource from stream_context_create()

###return from makeCurlRequest() The method makeCurlRequest has two return statements. One is in the else block, and the other is at the end of the method - i.e. return $requestData;. That variable,$requestData, appears to be the response from the API when the curl_version() function exists (and thus the cURL request is made), yet in the else block, $requestData is assigned a resource from stream_context_create() . While this code likely functions correctly, it could be confusing to a teammate who had to update it. A more appropriate name for the return value from the call to stream_context_create() might be something like $streamContext. Then it might be simpler to either assign the return value from the call to file_get_contents() to $requestData and utilize the return of that variable at the end of the method, or when making the curl request, return the response at the end of that block.

###Passing instance/member variables to method Why pass $this->timeout to makeCurlRequest from the checkVote method? If the method was called outside this class (and the scope of the method changed to protected for sub-classes or public for anywhere else in the code) then it might make sense to accept that parameter.

$result = $this->makeCurlRequest($urlRequest, $this->requestTimeout);

Since that method is not static, it could simply refer to $this->requestTimeout instead of accepting the parameter for it.

###Misleading Method Name The method makeCurlRequest appears to check if the curl function curl_version() exists and then either makes a cURL request or utilizes file_get_contents(). Thus the method might not always make a cURL request and a better name might be makeRequest. The code in the block when the curl_version() function does exist could be moved to a new method called makeCurlRequest.

###return from makeCurlRequest() The method makeCurlRequest has two return statements. One is in the else block, and the other is at the end of the method - i.e. return $requestData;. However, $requestData is only defined when the curl_version() function exists (and thus the cURL request is made). In the else block, $requestData is assigned a resource from stream_context_create()

###Redundant return false

In isVoteCookieSet() There is an else block to the nested if statement that contains return false;. That could be removed, since the last line of the method does the same thing.

###Constants for Response codes It would be wise to define (class) constants for the response codes, like the ones below. While there currently appears to only be one place in your code where that value appears, there may arise a need to have it appear in other logic and thus it would be useful to reuse the constant(s). Then if the value would ever need to be updated (e.g. if the API ever changes) then it can be updated in your code in one spot.

const RESPONSE_VOTE_PLACED_IN_PAST_24_HOURS = 3;
const RESPONSE_VOTE_NOT_PLACED_IN_PAST_24_HOURS = 2;

That way, the code lines like below:

if ($result == 3) {

Can be updated like this:

if ($result == self::RESPONSE_VOTE_PLACED_IN_PAST_24_HOURS) {

Which would hopefully allow somebody else reading your code to have a better idea of what that logic means. Though if those names are too long, feel free to shorten them.

##The Question

Could anyone advise a better way to create [cookies]?

Like your code already uses, setcookie() is the traditional way of setting cookies. And yes, since the cookie data is sent in a header, there must be a response, otherwise the browser/user won't really be able to receive the cookie.

##General Feedback ###Passing instance/member variables to method Why pass $this->timeout to makeCurlRequest from the checkVote method? If the method was called outside this class (and the scope of the method changed to protected for sub-classes or public for anywhere else in the code) then it might make sense to accept that parameter.

$result = $this->makeCurlRequest($urlRequest, $this->requestTimeout);

Since that method is not static, it could simply refer to $this->requestTimeout instead of accepting the parameter for it.

###Misleading Method Name The method makeCurlRequest appears to check if the curl function curl_version() exists and then either makes a cURL request or utilizes file_get_contents(). Thus the method might not always make a cURL request and a better name might be makeRequest. The code in the block when the curl_version() function does exist could be moved to a new method called makeCurlRequest.

###return from makeCurlRequest() The method makeCurlRequest has two return statements. One is in the else block, and the other is at the end of the method - i.e. return $requestData;. However, $requestData is only defined when the curl_version() function exists (and thus the cURL request is made). In the else block, $requestData is assigned a resource from stream_context_create()

###Redundant return false

In isVoteCookieSet() There is an else block to the nested if statement that contains return false;. That could be removed, since the last line of the method does the same thing.

###Constants for Response codes It would be wise to define (class) constants for the response codes, like the ones below. While there currently appears to only be one place in your code where that value appears, there may arise a need to have it appear in other logic and thus it would be useful to reuse the constant(s). Then if the value would ever need to be updated (e.g. if the API ever changes) then it can be updated in your code in one spot.

const RESPONSE_VOTE_PLACED_IN_PAST_24_HOURS = 3;
const RESPONSE_VOTE_NOT_PLACED_IN_PAST_24_HOURS = 2;

That way, the code lines like below:

if ($result == 3) {

Can be updated like this:

if ($result == self::RESPONSE_VOTE_PLACED_IN_PAST_24_HOURS) {

Which would hopefully allow somebody else reading your code to have a better idea of what that logic means. Though if those names are too long, feel free to shorten them.