Problem
I have seen UInts being natively supported in other languages. PHP allows you to install a third party extension, but I can’t do that ATM, so I’ve decided to create my own class.
In which ways could/should I improve my code. Both in terms of overall code quality aswel as performance?
final class UintArray implements Countable,ArrayAccess,IteratorAggregate
{
private $arr=array(); //all the saved data will be here
private $maxlen=0; //maximum length of the array
private $cnt=0; //current count
//numbers of bytes to store
const UInt8=1;
const UInt16=2;
const UInt24=3;
const UInt32=4;
//used to be sure the value doesn't go above the maximum value
private $bits=0xFFFFFFFF;
public function __construct($maxlen,$b=4)
{
//stores the maximum length, check if it higher than 0
$this->maxlen=$maxlen>>0>0?$maxlen>>0:1;
switch($b)
{
case 1:$this->bits=0xFF;break;
case 2:$this->bits=0xFFFF;break;
case 3:$this->bits=0xFFFFFF;break;
case 4:default:$this->bits=0xFFFFFFFF;
}
//fill the array ahead, so it's space will be all reserved
//in theory, this will be faster than creating elements each time
$this->arr=array_fill(0,$this->maxlen,0);
}
//countable
public function count(){return $this->cnt;}
//arrayaccess
public function offsetSet($offset,$value)
{
//verifies if the offset is valid, and if still have space
if($this->cnt>=$this->maxlen||($offset>>0>=$this->maxlen)||$offset<0)
{
return false;
}
//allows for $arr[]=<value>;
if(is_null($offset))
{
$this->arr[++$this->cnt]=$value&$this->bits;
}
//stores $arr[<offset>]=<value>;
else
{
$this->arr[$offset>>0]=$value&$this->bits;
}
}
//used with isset($arr[<offset>]);
public function offsetExists($offset){return isset($this->arr[$offset>>0]);}
//used with unset($arr[<offset>]);
public function offsetUnset($offset){$this->arr[$offset>>0]=0;}
//used with $arr[<offset>];
public function offsetGet($offset)
{
return isset($this->arr[$offset>>0])
?$this->arr[$offset>>0]
:null;
}
//iteratoraggregate
//used on the foreach loop
public function getIterator(){return new ArrayIterator($this->toArray());}
//magic methods
public function __toString()
{
if(error_reporting()&E_NOTICE)
{
@trigger_error('Array to string conversion',E_USER_NOTICE);
}
return 'Array';
}
public function __invoke(){return $a=&$this->arr;}
public function __set_state(){return $a=&$this->arr;}
public function __set($key,$value){return $this->offsetSet($key,$value);}
public function __get($key){return $this->offsetGet($key);}
public function __sleep(){return $this->arr;}
//other functionality methods
public function push()
{
//we retrieve all the arguments
$args=func_get_args();
foreach($args as &$value)
{
//if we still have space
if($this->cnt<$this->maxlen)
{
//add to the array, increasing the count
$this->arr[$this->cnt++]=$value&$this->bits;
}
//if not, return the size (replicates array_push behaviour)
else return $this->cnt;
}
//returns the size (replicates array_push behaviour)
return $this->cnt;
}
public function pop()
{
//if the array is empty
if(!$this->cnt)
{
return null;
}
else
{
//decreases the count and stores the last value
$r=$this->arr[--$this->cnt];
//stores 0 on the last value
$this->arr[$this->cnt]=0;
//returns the last element
return $r;
}
}
public function maxlen(){return $this->maxlen;}
public function maxint(){return $this->bits;}
public function toArray(){return array_slice($this->arr,0,$this->cnt);}
}
Example of usage:
$arr=new UIntArray(32); //same as $arr=new UIntArray(32,UIntArray::UInt32);
echo $arr->push(1,3,5,6,3,23,3,1,2);
echo $arr[0];
echo $arr->pop();
foreach($arr as $v)echo PHP_EOL,$v;
/*
outputs:
912
1
3
5
6
3
23
3
1
*/
Solution
private $arr=array(); //all the saved data will be here
private $maxlen=0; //maximum length of the array
private $cnt=0; //current count
Rather than use comments that only appear in one place, it is generally better to use descriptive names:
private $saved_data = array();
private $maximum_length = 0;
private $current_count = 0;
Note that $data and $count would probably be enough, as “saved” and “current” are implied.
I also added whitespace between tokens. The compiler won’t care, but this often makes it easier for humans to read.
private $bits=0xFFFFFFFF;
I’d call this a $bit_mask. Also, as a general rule, I prefer to only name collections in the plural. This is a scalar, so I would try to name it in the singular.
I wouldn’t use a comment here, as this isn’t where it’s used.
public function __construct($maxlen,$b=4)
I’d write this
public function __construct($maximum_length, $bytes_per_element = 4)
That makes it clearer why you would pass those parameters.
//stores the maximum length, check if it higher than 0
$this->maxlen=$maxlen>>0>0?$maxlen>>0:1;
As before, I’d rewrite this
$this->maximum_length = ($maximum_length >> 0) > 0 ? $maximum_length >> 0 : 1;
I removed your comment, as that just restates what I can read from the code. I’d still like a comment here though. It’s unclear to me why you are shifting the bits right by zero places. The only reason that I can see is that it forces PHP to treat $maximum_length as an integer. A comment would be helpful, but I wouldn’t put it here. Instead use a helper function here.
$this->maximum_length = UIntArray::make_int32($maximum_length > 0 ? $maximum_length : 1);
which says what it is doing explicitly rather than relying on a side effect. Also, I put it all the way around, as you want to force it to be a 32 bit integer all the time, not just when the value is not the default.
private static function make_int32($value)
{
// Doing a shift forces PHP to treat the value as a 32 bit integer.
// Shifting by 0 means that this is otherwise a no-op.
return $value >> 0;
}
Now there may be a reason why the no-op right shift is better than an explicit cast, but if so, tell me. Also, this would be a great time to define a unit test that shows me what can go wrong. That way if someone optimizes the right shift out of the code, then at least it will fail unit testing immediately.
case 4:default:$this->bits=0xFFFFFFFF;
This is more commonly written
case 4:
default:
$this->bits=0xFFFFFFFF;
which makes it clearer that you are directing multiple cases to the same result.
//fill the array ahead, so it's space will be all reserved
should be
// fill the array ahead, so its space will be all reserved
Should use the possessive “its” there rather than the contraction “it’s”.
//verifies if the offset is valid, and if still have space
if($this->cnt>=$this->maxlen||($offset>>0>=$this->maxlen)||$offset<0)
So if the array is full, we can no longer set any elements? That seems like a bug. We should not be able to add elements in that case, but we should still be able to change existing elements.
Write instead
if ( 0 > $offset || ( UIntArray::make_int32($offset) >= $this->maximum_length ) )
Also
return false;
is not correct. offsetSet does not return a value, so this won’t work. It’s not clear what you should do instead. Throwing an exception is one possibility.
//allows for $arr[]=<value>;
if(is_null($offset))
{
$this->arr[++$this->cnt]=$value&$this->bits;
}
//stores $arr[<offset>]=<value>;
else
{
$this->arr[$offset>>0]=$value&$this->bits;
}
Ok, now I see why you wanted to check that the array wasn’t full, but I still think that you shouldn’t have done it where you did. Instead
// $uint_array[] = <value>; will set the $offset to null
if ( is_null($offset) )
{
if ( $this->count >= $this->maximum_length )
{
// panic somehow and do not proceed
}
$offset = $this->count++;
}
$this->data[UIntArray::make_int32($offset)] = $this->sanitize($value);
This only checks count against maximum_length when you would be increasing the count.
When the $offset is null, we set $offset to equal count (which will be one greater than the index of the last element) and increment count. Your original version had a bug where it incremented the count first, which actually sets the offset just beyond the end of the array.
Doing things this way gets rid of the else and reduces repeated code. We do the same thing for either case. We just have to prime the $offset if it wasn’t explicitly passed.
Note that this also requires a helper function:
private function sanitize($value)
{
// only return the bits possible for this size of element
return $value & $this->bits;
}
You’re doing this a lot, so should define it in one place.
public function offsetGet($offset)
{
return isset($this->arr[$offset>>0])
?$this->arr[$offset>>0]
:null;
}
I don’t think that will work. The offsetGet function returns by reference, but null is a value. I think that you have to do something like
: $dummy = null;
so that it has a variable to reference.
This also brings up another problem. The offsetGet function will return a reference to the underlying data structure which can then be used in assignments. If the caller does this, they can bypass the bit mask that you use in assignments. For that matter, the caller could store a non-integer in the array. So maybe what you want to do is
$offset = UIntArray::make_int32($offset);
$dummy = isset( $this->data[$offset])
? $this->data[$offset]
: null;
return $dummy;
That always returns a dummy reference. Of course, if you do that, you may block some assignments that you would want to allow. Perhaps there is some way to return a special $dummy that will trigger the proper behavior on assignment or update.
foreach($args as &$value)
{
//if we still have space
if($this->cnt<$this->maxlen)
{
//add to the array, increasing the count
$this->arr[$this->cnt++]=$value&$this->bits;
}
//if not, return the size (replicates array_push behaviour)
else return $this->cnt;
}
//returns the size (replicates array_push behaviour)
return $this->cnt;
I’d write this
foreach ( $args as &$value )
{
if ( $this->count < $this->maximum_length )
{
$this->data[$this->count++] = $this->sanitize($value);
}
else
{
// when out of space, stop processing arguments
break;
}
}
// returns the number of elements in the array (replicates array_push behaviour)
return $this->count;
I got rid of the comments that just said the same thing as the code. I left the comment that explained why we were doing this uncommon thing.
I changed the else because you were doing the same thing in the else as you would if it just fell out of the loop. Doing it this way means that we only have one return statement to maintain.
//if the array is empty
if(!$this->cnt)
This seems more easily written as
if ( $this->count <= 0 )
which shouldn’t require a comment. Also, this covers more potential issues, as the previous check only made sure that the count wasn’t 0. This covers negative offsets as well.
//decreases the count and stores the last value
$r=$this->arr[--$this->cnt];
//stores 0 on the last value
$this->arr[$this->cnt]=0;
//returns the last element
return $r;
This seems overly complicated. Under what circumstance does the value at a particular offset outside the array matter?
return $this->data[--$this->count];
This covers the basic functionality and is easier to read.