Singly-linked queue in PHP 7 #2

Question

I have improved this post to the code below.

As always, any critique is much appreciated!

<?php

class QueueNode {

    function __construct($item) {
        $this->item = $item;
        $this->next = null;
    }

    function __destruct() {
        $this->item = null;
    }

    function getItem() {
        return $this->item;
    }    
}

class QueueIterator implements Iterator {

    function __construct($queue) {
        $this->queue = $queue;
        $this->current_node = $queue->head;
    }

    public function current() {
        return $this->current_node->item;
    }

    public function key(): \scalar {
        return null;
    }

    public function next(): void {
        $this->current_node = $this->current_node->next;
    }

    public function rewind(): void {
        $this->current_node = $this->queue->head;
    }

    public function valid(): bool {
        return isset($this->current_node);
    }
}

class Queue implements IteratorAggregate {

    function __construct() {
        $this->head = null;
        $this->tail = null;
        $this->size = 0;
    }

    function __destruct() {
        $this->head = null;
        $this->tail = null;
    }

    function push($item) {
        if ($this->size === 0) {
            $this->head = $this->tail = new QueueNode($item);
        } else {
            $new_node = new QueueNode($item);
            $this->tail->next = $new_node;
            $this->tail = $new_node;
        }

        $this->size++;
    }

    function pop() {
        if ($this->size === 0) {
            throw new Exception("Popping from an empty queue.");
        }

        $ret = $this->head->getItem();
        $this->head = $this->head->next;

        if (--$this->size == 0) {
            $this->head = $this->tail = null;
        }

        return $ret;
    }

    function getFirst() {
        if  ($this->size() === 0) {
            throw new Exception("getFirst() on empty Queue.");
        }

        return $this->head->getItem();
    }

    function getLast() {
        if ($this->size() === 0) {
            throw new Exception("getLast() on empty Queue.");
        }

        return $this->tail->getItem();
    }

    function size() {
        return $this->size;
    }

    function isEmpty() {
        return size() === 0;
    }

    public function getIterator(): \Traversable {
        return new QueueIterator($this);
    }
}

$queue = new Queue();

for ($i = 1; $i <= 10; $i++) {
    $queue->push($i);
}

echo "Iteration: ";

foreach ($queue as $item) {
    echo $item . " ";
}

echo "<br>Popping: ";

for ($i = 1; $i <= 10; $i++) {
    echo $queue->getFirst() . ", " . $queue->pop() . " ";
}

echo "<br>Bye!";
?>

Output

Iteration: 1 2 3 4 5 6 7 8 9 10 
Popping: 1, 1 2, 2 3, 3 4, 4 5, 5 6, 6 7, 7 8, 8 9, 9 10, 10 
Bye!

Answer 1

I see three major problems with your code:

1. You dont use PHP 7 type hinting / return types
2. You dont have any scopes on your methods
3. If you wont modify your classes in the future and/or you wont extend from these classes you should use final for your class declarations
4. you should always set declare(strict_types=1); at the beginning of every php script as long as there are no veeeery good reasons to avoid it. It will force you to write your code more typesafe or you will get fatal errors

I will give you an example for each point i mentioned

1. "You dont use PHP 7 type hinting / return types"

Before:

function push($item) {

After:

function push(int $item): void

2. You dont have any scopes on your methods

Before:

function getItem() {

After:

public function getItem(): void

3. class declaration as final

Before:

class QueueNode

After:

final class QueueNode

Sidenote:

Also you should use properties like private $item = null; instead of your constructor setting. Your IDE and also any other maintainer will have trouble finding bugs and/or understanding this code.