12
\$\begingroup\$

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?

\$\endgroup\$
  • \$\begingroup\$ why check if(error_reporting() & E_NOTICE) in your __toString method, why not simply return (string) []; (return a array to string cast), so you get the same behavior, with a lot less code \$\endgroup\$ – Elias Van Ootegem Nov 19 '14 at 9:25
  • \$\begingroup\$ @EliasVanOotegem That is a good one, but less obvious. I would need to add (another) comment. And I wouldn't use [] in a PHP context (not yet, at least while PHP 5.3 is around). \$\endgroup\$ – Ismael Miguel Nov 19 '14 at 9:34
  • \$\begingroup\$ PHP 5.3 is no longer supported, officially, and the short array syntax is available in 5.4 and up, so all of the supported versions can handle it. Also: comments are fine, your code posted here contains, if anything, too little comments \$\endgroup\$ – Elias Van Ootegem Nov 19 '14 at 9:51
  • \$\begingroup\$ @EliasVanOotegem Sadly, I'm still trapped with PHP 5.3 :/ And to be honest: I prefer this way to create an array. Using array() for PHP and [] for Javascript. I've always been used to that too. \$\endgroup\$ – Ismael Miguel Nov 19 '14 at 9:57
5
\$\begingroup\$

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);
}
\$\endgroup\$
3
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ "Why is the bitmask for UInt32 0xFFFFFFFE instead of 0xFFFFFFFF?" -> That's addrfessed in the comment, just above. "Why do offsets need to be sanitize()d? Do you not trust the caller to give you offsets that are integers?" -> Never trust user input. Also, It is to make sure that the integer values match the bitmask. About the last point, it is a really good one. I did that on purpose. The idea is to really return an array. The bug you mentioned, I need to make some testing. \$\endgroup\$ – Ismael Miguel Jun 19 '15 at 19:13
  • \$\begingroup\$ Nice catch on the 0xFFFFFFFE bug. It should be 0xEFFFFFFF instead. \$\endgroup\$ – Ismael Miguel Jun 19 '15 at 19:19
  • \$\begingroup\$ One thing I've noticed is that you have to use self::UInt8 instead of only UInt8 in the array. \$\endgroup\$ – Ismael Miguel Jun 19 '15 at 19:34
  • \$\begingroup\$ How would sanitizing an invalid offset make it any better, though? \$\endgroup\$ – 200_success Jun 19 '15 at 19:57
  • \$\begingroup\$ Sanitizing would limit the offset to integer-only and to prevent that the offset it outside the boundaries. Also, I've fixed the mentioned bug. It really was a nice catch. \$\endgroup\$ – Ismael Miguel Jun 19 '15 at 20:04

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.