Ytdyklly

I have written a voting class in Laravel. It uses the voting sites API to check if the user has voted today. A user is only allowed to vote once every day.

All votes reset at midnight (i.e. 12:00 AM Chicago time) and I create a cookie to not bother wasting a curl response till then, so the cookie expires when a new vote is allowed.

The API returns response codes: 2 means the user hasn't voted for them in the past 24 hours, and 3 means they have voted for them in the past 24 hours. Just think of 2 as 0 and 3 as 1 in a false/true situation.

The reason I am asking this question is I don't think I am handling cookies the best way I can. Could anyone advise a better way to create them? I wasn't sure how to do it without a response, as Laravel says you have to send it with a response.

If the user needs to vote, they are sent to the voting page. For now I've just put a temp link as http://need2vote.com until I finish the class.

<?php



namespace AppHttpControllers;



use AppHttpControllersController;

use Auth;

use Redirect;

use IlluminateSupportFacadesRoute;

use Cookie;

use IlluminateHttpRequest;

use Config;



class VotingController extends Controller

{

    protected $pageUsername;

    protected $requestTimeout;

    protected $usingCloudflare;

    protected $apiUrl;



    public function __construct() 

    {

        $this->pageUsername = Config::get('voting.username');

        $this->requestTimeout = Config::get('voting.timeout');

        $this->usingCloudflare = true;

        $this->apiUrl = Config::get('voting.api_url');

    }



    public function checkVote(Request $request) 

    {

        if (Config::get('voting.enabled') == false) {

            return;

        }



        if ($this->isVoteCookieSet()) {

            return;

        }



        $urlRequest = $this->apiUrl . 'user=' . $this->pageUsername . '&ip=' . $request->ip();



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



        if ($result == 3) {

            $this->redirectToVote();

        }

        else {

            $this->setVoteCookie();

        }

    }



    private function makeCurlRequest($url, $timeout) 

    {

        if (function_exists('curl_version')) 

        {

            $curl = curl_init();



            curl_setopt($curl, CURLOPT_URL, $url);

            curl_setopt($curl, CURLOPT_RETURNTRANSFER, 1);

            curl_setopt($curl, CURLOPT_SSL_VERIFYPEER, 0);

            curl_setopt($curl, CURLOPT_SSL_VERIFYHOST, 0);

            curl_setopt($curl, CURLOPT_CONNECTTIMEOUT, 10);

            curl_setopt($curl, CURLOPT_TIMEOUT, $timeout);



            $requestData = curl_exec($curl);



            curl_close($curl);

        }

        else

        {

            $requestData = stream_context_create(array('http' => array('timeout' => $timeout))); 

            return file_get_contents($url, 0, $requestData); 

        }



        return $requestData;

    }



    public function redirectToVote() {

        header('Location: http://needtovote.com');

        exit();

    }



    private function setVoteCookie() {

        $rankingsResetTime = $this->getVoteResetTime();

        setcookie('vote_timestamp', $rankingsResetTime, $rankingsResetTime);

    }



    private function isVoteCookieSet() {

        if (isset($_COOKIE['voting_timestamp'])) {

            if ($_COOKIE['voting_timestamp'] == $this->getVoteResetTime()) {

                return true;

            }

            else{

                setcookie('voting_timestamp', '');

                return false;

            }

        }



        return false;

    }



    private function getVoteResetTime() {

        $serverDefaultTime = date_default_timezone_get();

        date_default_timezone_set('America/Chicago');



        $rankingsResetTime = mktime(0, 0, 0, date('n'), date('j') + 1);

        date_default_timezone_set($serverDefaultTime);



        return $rankingsResetTime;

    }

}

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

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.