UIntArray class in PHP (part 2)

Question

This question is the follow up of the question:

UIntArray class in PHP

@Brythan provided a really nice review and I have improved my class.

I have addressed almost all the issues he pointed out (the only one I didn't do anything was about the UIntArray::pop() function, that was intentional) and still made some improvements on my own.

final class UintArray implements \Countable,\ArrayAccess,\IteratorAggregate
{
    private $data = array();   //all the saved data will be here
    private $maximum_length = 0;  //maximum length of the array
    //current count, can't use $count because accessing $this->count might give troubles
    private $current_count = 0; 

    //numbers of bytes to store
    const UIntDefault = 0; //default value
    const UInt8 = 1;
    const UInt16 = 2;
    const UInt24 = 3;
    const UInt32 = 4;

    //the index is kept here only for readability.
    private static $bit_masks=array(
        0 => 0xFFFFFFFE, //default bit mask
        1 => 0xFF,
        2 => 0xFFFF,
        3 => 0xFFFFFF,
        //0xFFFFFFFF is -1 on 32bit systems, but 4294967295 on 64 bits
        4 => 0xFFFFFFFE
    );

    //used to be sure the value doesn't go above the maximum value
    private $bit_mask;

    private static function sanitize($value, $bit_mask = 0)
    {
        //sanitize the value, to ensure it is less or equal to the desired mask
        return $value & ( $bit_mask ? $bit_mask : self::$bit_masks[self::UInt32]);
    }

    public function __construct($maximum_length, $bytes_per_element = 0)
    {
        //set the length to a 32bit integer
        $maximum_length = self::sanitize($maximum_length);

        //stores the maximum length, check if it higher than 0
        $this->maximum_length = ( $maximum_length > 0 ) ? $maximum_length : 1;

        //sets the bit mask to be used
        $this->bit_mask = self::$bit_masks[ ( $bytes_per_element >= 1 && $bytes_per_element <= 4 ) ? $bytes_per_element : self::UIntDefault];

        //fill the array ahead, so it's space will be all reserved
        //in theory, this will be faster than creating elements each time
        $this->data = array_fill(0, $maximum_length, 0);
    }

    //countable
    public function count(){return $this->current_count;}

    //arrayaccess
    public function offsetSet($offset, $value)
    {
        $this->__set($offset, $value);
    }
    //used with isset($arr[<offset>]);
    public function offsetExists($offset)
    {
        $offset = self::sanitize($offset);

        //if the offset is within the limits
        if($offset > 0 && $offset <= $this->maximum_length)
        {
            return isset($this->data[$offset]);
        }
        return false;
    }
    //used with unset($arr[<offset>]);
    public function offsetUnset($offset)
    {
        $offset = self::sanitize($offset);

        //if the offset is withing the limits
        if($offset > 0 && $offset <= $this->maximum_length)
        {
            $this->data[$offset]=0;

            if($offset == $this->current_count-1)
            {
                //if we are unsetting the last element, we can safely reduce the count
                --$this->current_count;
            }
        }
    }
    //used with $arr[<offset>];
    public function offsetGet($offset)
    {
        return $this->__get($offset);
    }

    //iteratoraggregate
    //used on the foreach loop
    public function getIterator(){return new ArrayIterator($this->toArray());}

    //magic methods
    public function __toString()
    {
        //replicated the default behavior of converting an array to string
        if(error_reporting() & E_NOTICE)
        {
            @trigger_error('Array to string conversion', E_USER_NOTICE);
        }
        return 'Array';
    }
    public function __invoke(){return $a=&$this->data;}
    public function __set_state(){return $a=&$this->data;}
    public function __set($offset, $value)
    {
        //allows to set $arr[]=<value>;
        if(is_null($offset))
        {
            //verifies if the array is full. returns false if it is.
            if($this->current_count >= $this->maximum_length)
            {
                return false;
            }

            //provides the offset to set the value
            $offset = $this->current_count++;
        }
        //verifies if the $offset is within the allowed limits
        else if( $offset < 0 || $offset > $this->maximum_length)
        {
            return false;
        }

        $this->data[ self::sanitize($offset) ] = self::sanitize($value, $this->bit_mask);
    }
    public function __get($offset)
    {
        $offset = self::sanitize($offset);

        //returns a dummy variable, just in case someone uses the increment (++) or decrement (--) operators
        $dummy = isset($this->data[$offset]) ? $this->data[$offset] : null;
        return $dummy;
    }
    public function __sleep(){return $this->data;}

    //other functionality methods
    public function push()
    {   
        //retrieve all the arguments, saving one variable
        foreach(func_get_args() as $value)
        {
            //if the array still have space
            if( $this->current_count < $this->maximum_length )
            {
                //add to the array, increasing the count
                $this->data[ $this->current_count++ ] = self::sanitize($value, $this->bit_mask);
            }
            //if the array is full, exit the loop
            else break;
        }

        //returns the number of elements
        //this replicated the behaviour of the function array_push()
        //Documentation: http://php.net/manual/en/function.array-push.php
        //Test-case (using array_push()): http://ideone.com/PrTo8m
        return $this->current_count;
    }

    public function pop()
    {
        //if the array is empty
        if($this->current_count < 1)
        {
            return null;
        }
        else
        {
            //decreases the count and stores the last value
            $value = $this->data[ --$this->current_count ];

            //stores 0 on the last value
            $this->data[ $this->current_count ]=0;

            //returns the last element
            return $value;
        }
    }

    public function maxlen(){return $this->maximum_length;}
    public function bitmask(){return $this->bit_mask;}
    public function toArray(){return array_slice($this->data, 0, $this->current_count);}
}

In terms of functionality and usage, it works exactly the same.

The only change was the function UIntArray::bits() was renamed to UIntArray::bitmask(), to address the name change.

Is there anything, both in coding style and performance, that I should address and fix?

Answer 1

Your foreach loop can be simplified by making the break a guard clause:

    foreach(func_get_args() as $value)
    {
        //if the array still have space
        if( $this->current_count < $this->maximum_length )
        {
            //add to the array, increasing the count
            $this->data[ $this->current_count++ ] = self::sanitize($value, $this->bit_mask);
        }
        //if the array is full, exit the loop
        else break;
    }

Instead, do it like:

    foreach(func_get_args() as $value)
    {
        //if the array is out of space
        if( $this->current_count >= $this->maximum_length )
        {
            break;
        }

        //add to the array, increasing the count
        $this->data[ $this->current_count++ ] = self::sanitize($value, $this->bit_mask);
    }

That makes the conditions simpler to follow (no else statement).

Similarly, when you have a return statement, there's no need for an else, this code:

    //if the array is empty
    if($this->current_count < 1)
    {
        return null;
    }
    else
    {
        //decreases the count and stores the last value
        $value = $this->data[ --$this->current_count ];

        //stores 0 on the last value
        $this->data[ $this->current_count ]=0;

        //returns the last element
        return $value;
    }

could instead be:

    //if the array is empty
    if($this->current_count < 1)
    {
        return null;
    }
    //decreases the count and stores the last value
    $value = $this->data[ --$this->current_count ];

    //stores 0 on the last value
    $this->data[ $this->current_count ]=0;

    //returns the last element
    return $value;

Finally, even for 1-liner if-statements, you should indent things correctly. This last block:

public function maxlen(){return $this->maximum_length;}
public function bitmask(){return $this->bit_mask;}
public function toArray(){return array_slice($this->data, 0, $this->current_count);}

should be:

public function maxlen()
{
    return $this->maximum_length;
}

public function bitmask()
{
    return $this->bit_mask;
}

public function toArray()
{
    return array_slice($this->data, 0, $this->current_count);
}

Answer 2

Why is the class named UintArray, while the constants are named like UInt8? Even if PHP is case-insensitive, it would be nice for you to be consistent in capitalization.

UIntDefault = 0 seems weird to me. Why not define UIntDefault = UInt32, since that's what you mean? Why is the bitmask for UInt32 0xFFFFFFFE instead of 0xFFFFFFFF? That rounds your data to even numbers.

When defining $bit_masks, there is no obvious correspondence with the UIntn constants that were just defined. I suggest:

private static $bit_masks = array(
    UInt8  => 0xFF,
    UInt16 => 0xFFFF,
    …
);

Why do offsets need to be sanitize()d? You want to validate the offset, not sanitize it. I don't recommend trying to make sense of bad input, because garbage in = garbage out. Some languages interpret negative array indices as counting backwards from the end — maybe you want to do that? But it's not idiomatic for PHP, so I recommend either emulating the standard behaviour for PHP arrays or explicitly failing.

The way you keep track of $current_count is buggy. In a normal array, you should be able to do $a = array(); $a[0] = "first"; $a[] = "second";. However, if you try that with your UintArray, the second entry will overwrite the first, because you only maintain a correct $current_count if you stick to the push()/pop()/offsetUnset() interface.

__sleep() is supposed to serialize the array, but you only serialize the data, and discard the type.