11
\$\begingroup\$

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
*/
\$\endgroup\$
  • \$\begingroup\$ This does exist for PHP, as an extension written by nikic, a contributor to the PHP source. The extension is up for graps on github here \$\endgroup\$ – Elias Van Ootegem Nov 4 '14 at 12:44
  • \$\begingroup\$ @EliasVanOotegem I was not aware of that. But what if I can't install extensions? This is still useful for those. \$\endgroup\$ – Ismael Miguel Nov 4 '14 at 13:09
  • \$\begingroup\$ That's why I posted this as a comment, not an answer, and didn't vote to close. Your question is still valid, but just so you know: with the buffer extension, you can have UIntArray's \$\endgroup\$ – Elias Van Ootegem Nov 4 '14 at 13:15
  • \$\begingroup\$ @EliasVanOotegem Can you edit the question and add that information you provided, please? I don't want to simply put a reference saying about you, but to actually have a "mark" there. \$\endgroup\$ – Ismael Miguel Nov 4 '14 at 13:23
  • \$\begingroup\$ @EliasVanOotegem Wow, thanks for the edit! However, you forgot to link to the extension. But it's alright, you really improved the question a lot! \$\endgroup\$ – Ismael Miguel Nov 4 '14 at 13:44
7
+50
\$\begingroup\$
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.

\$\endgroup\$
  • \$\begingroup\$ First of all, thank you A LOT for the review. I see lots of things I didn't though about and then I see some things that I completely disagree and a few that won't work well. You rewrote part of the code of the __constrctor() function into this: $this->maximum_length = (int)$maximum_length > 0 ? (int)$maximum_length >> 0 : 1;. The reason why I didn't do this way but used a bitshift is simple: When you convert to (int), it returns a platform-dependent integer. Using the >>0 forces is to be a 32bit integer (which is VERY IMPORTANT). \$\endgroup\$ – Ismael Miguel Nov 16 '14 at 14:28
  • \$\begingroup\$ On this line I verify exactly the same: if ( 0 > $offset || ( (int)$offset >= $this->maximum_length ) ). I don't want people doing stuff beyond the highest 32bit integer. I know that offsetSet doesn't return a value, but I'm using it in case someone tries to access it directly or in case the magic method __get() is triggered or accessed directly. Perhaps the code should be moved to inside __get() instead? Another thing is offsetGet returning a reference. No, it doesn't. It shouldn't even be allowed to do so. Check the documentation: php.net/manual/en/arrayobject.offsetget.php \$\endgroup\$ – Ismael Miguel Nov 16 '14 at 14:36
  • \$\begingroup\$ On the rewrite of my setting function, you have this bit: $this->data[$this->count++] = sanitize($value);. The function sanitize doesn't exist outside the scope of the class. You should either use self::sanitize($value,$this->bit_mask); and declare it as private static function sanitize($value, $bit_mask){return $value & $bit_mask;} or use $this->sanitize($value) instead. One other thing I noticed is this: if ( $this->count <= 0 ). I didn't do this because the $this->count should never be below 0. \$\endgroup\$ – Ismael Miguel Nov 16 '14 at 14:40
  • \$\begingroup\$ I completely disagree with your rewrite of the pop function. This is your code: return $this->data[--$this->count];. It will leave important data behind. The array is all initialized to 0. Why would I leave a value behind? Getting rid of that value and setting to 0 is a security bonus (except in the case where the value was already 0). This is all I have to say about the review. Tell me when you finished it so I can rewrite my code. I will present the rewritten code in another answer. After you finish, I will give you the reputation bonus. \$\endgroup\$ – Ismael Miguel Nov 16 '14 at 14:44
  • 1
    \$\begingroup\$ "$this->count should never be below 0" Sure, but what if it is? Once you get past 0, the original code will continue doing the wrong thing. It's better if it stops doing the wrong thing when it gets into a buggy state like that. Unfortunately, code does things that it is not supposed to do all the time for a variety of reasons. There may be times to rely on your code having correct behavior, but that's not one of them. It's just as easy to do the broader check, so you might as well do it. \$\endgroup\$ – Brythan Nov 16 '14 at 14:59

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.