Assigning browser cookies for A-B testing

Question

The following code creates a cookie on the client browser such that 25% of the browsers will have a value 0, and 75% a value of 1. Can anyone review the following code and point out any problems in it?

// AB testing code starts;
$abTestValues = array(1,1,0,1);
$abTestValue = 0;
$abTestCurrentValue = $abTestValues[$abTestValue];
$cookieName = 'ab-ranking-20160301';
$cookieLifeTime = time() + (86400 * 30);
$cookiePath = "/";
$abTestCookieValue = $_COOKIE[$cookieName];
if(trim($abTestCookieValue) != '' && $abTestCookieValue != null){
$abTestCurrentValue = $abTestCookieValue;
}else{
$abTestFile = 'abranking.txt';
if(is_file($abTestFile)){
$abTestLastValue = file_get_contents($abTestFile);
if( ($abTestLastValue + 1) == sizeof($abTestValues) ){
$abTestValue = 0;
}else{
$abTestValue = $abTestLastValue+1;
}
$abTestCurrentValue = $abTestValues[$abTestValue];
}
file_put_contents($abTestFile , $abTestValue);
$ranking_cookieval = $abTestCurrentValue;
setcookie($cookieName,$abTestCurrentValue,$cookieLifeTime,$cookiePath); //cookie set for 30 day
}
// AB testing code ends;

Answer 1

That code, without indentation, is a chore to read. The same code, with just indentation and whitespace changes, is easier to follow.

// AB testing code starts;
$abTestValues = array(1,1,0,1);
$abTestValue = 0;
$abTestCurrentValue = $abTestValues[$abTestValue];
$cookieName = 'ab-ranking-20160301';
$cookieLifeTime = time() + (86400 * 30);
$cookiePath = "/";
$abTestCookieValue = $_COOKIE[$cookieName];
if (trim($abTestCookieValue) != '' && $abTestCookieValue != null) {
 $abTestCurrentValue = $abTestCookieValue;
} else {
 $abTestFile = 'abranking.txt';
 if (is_file($abTestFile)) {
 $abTestLastValue = file_get_contents($abTestFile);
 if (($abTestLastValue + 1) == sizeof($abTestValues)) {
 $abTestValue = 0;
 } else {
 $abTestValue = $abTestLastValue + 1;
 }
 $abTestCurrentValue = $abTestValues[$abTestValue];
 }
 file_put_contents($abTestFile, $abTestValue);
 $ranking_cookieval = $abTestCurrentValue;
 setcookie($cookieName, $abTestCurrentValue, $cookieLifeTime, $cookiePath); //cookie set for 30 day
}
// AB testing code ends;

Answer 2

You've used 11 variables, which is a lot of variables for any code snippet, let alone a simple task like this. One of them, ranking_cookieval, appears to be completely useless. Some of them, like $abTestCurrentValue, are assigned several times, making it even more confusing.

Since this code needs to coexist with other code on the page, these 11 variables are going to be part of a larger problem. You need to make clear which of these variables should be considered hard-coded constants, which variables are transient, and which are important outputs from this routine. I recommend putting all of this code in a function so that the purpose, the inputs, and the outputs are clear.

The array seems pointless. You can just use a counter and test for equality with 0.

Bootstrapping the system, when the abranking.txt file does not exist, can be simpler. Just suppress any error that comes out of file_get_contents(). If file_get_contents() fails, it returns FALSE, and adding 1 to it will produce 1.

/**
 * If the cookie is not set, then set it to "0" with 25% probability
 * and to "1" with 75% probability.
 */
function abTest($cookie='ab-ranking-20160301', $roundRobinFile='abranking.txt') {
 if (0 == strlen($abTestCurrentValue = $_COOKIE[$cookie])) {
 $ab_counter = (@file_get_contents($roundRobinFile) + 1) % 4;
 file_put_contents($roundRobinFile, $ab_counter);
 $abTestCurrentValue = (int)($ab_counter != 0);
 setcookie($cookie, $abTestCurrentValue, # name, value
 time() + (86400 * 30), # 30 days
 '/'); # path
 }
 return $abTestCurrentValue;
}

Note that this is a rather complicated system. I'm not sure what the behaviour will be if two instances of the code try to write to the file at once. If your goal is to generate 0 25% of the time and 1 75% of the time, it might be better just to do (int)(mt_rand(0, 3) != 0). It won't be as predictable or precise as this deterministic round-robin scheme, but it might be good enough for arbitrarily assigning users for A-B testing.