HiveBrain v1.2.0
Get Started
← Back to all entries
patternphpMinor

UIntArray class in PHP

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
phpclassuintarray

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;
if(is_null($offset))
{
$this->arr[++$this->cnt]=$value&$this->bits;
}
//stores $arr[]=;
else
{
$this->arr[$offset>>0]=$value&$this->bits;
}
}
//used with isset($arr[]);
public function offsetExists($offset){return isset($this->arr[$offset>>0]);}
//used with unset($arr[]);
public function offsetUnse

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[]=;
    if(is_null($offset))
    {
        $this->arr[++$this->cnt]=$value&$this->bits;
    }
    //stores $arr[]=;
    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[] = ; 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

Code Snippets

private $arr=array();   //all the saved data will be here
private $maxlen=0;  //maximum length of the array
private $cnt=0; //current count
private $saved_data = array();
private $maximum_length = 0;
private $current_count = 0;
private $bits=0xFFFFFFFF;
public function __construct($maxlen,$b=4)
public function __construct($maximum_length, $bytes_per_element = 4)

Context

StackExchange Code Review Q#68756, answer score: 7

Revisions (0)

No revisions yet.