Making a copy constructor more flexible for ADT queue

I have constructed a copy constructor for an ADT queue. The copy constructor works fine. I would want to improve my code, but I don't really know how to shorten it to make it more flexible.

template 
Queue ::Queue(const Queue & other)
{
    if (other.first == nullptr)
    {
        first = nullptr;
        nrOfElements = 0;
    }

   else
   {
    Node* saveFirst;
    Node* walker;
    first = other.first;
    walker = new Node(first->data);
    saveFirst = walker;
    while (first->next != nullptr)
    {
        walker->next = new Node(first->next->data);
        walker = walker->next;
        first = first->next;
    }
    walker->next = nullptr;
    first = saveFirst;
    }
this->nrOfElements = other.nrOfElements;
}

The class Queue also contains an inner private Node class which contains the pointers first, next, etc:

private:
    int nrOfElements;
    class Node
    {
    public:
        Node* next;
        T data;
        Node(T data)
        {
            this->data = data;
        }
    };
    Node* first;

So, I would appreciate any suggestions/examples of how the copy constructor code above could be improved, as I'm a bit lost on the task.

Remove redundancy

{
    Node* saveFirst;
    Node* walker;
    first = other.first;
    walker = new Node(first->data);
    saveFirst = walker;
    while (first->next != nullptr)
    {
        walker->next = new Node(first->next->data);
        walker = walker->next;
        first = first->next;
    }
    walker->next = nullptr;
    first = saveFirst;
    }
this->nrOfElements = other.nrOfElements;

You create a temporary variable saveFirst. You then use a class field as temporary variable. When done, you set the class field to the value set in the temporary variable. Why not do it the other way around?

{
       Node* current = new Node(other.first->data);
       first = current;

       for (Node *otherCurrent = other.first; otherCurrent->next != nullptr; otherCurrent = otherCurrent->next)
       {
           current->next = new Node(otherCurrent->next->data);
           current = current->next;
       }

       current->next = nullptr;
       nrOfElements = other.nrOfElements;
    }

This uses the temporary variable to hold the changing current variable. It sets first to the correct value immediately and directly. It changes the loop to say for every node in other. Now you don't have to set the class field at the end.

Note that I also moved the number of elements assignment inside the else. In the original it would have tried to dereference a null other.

Reuse other methods

You don't mention other functions in the class, but there are certain ones that could be used here (assuming C++11).

{
        for (auto i : other)
        {
            this.append(i.data);
        }
    }

This would replace the entire else block. If you have to create the begin, end, and append functions anyway, then this drops the amount of code that you have to maintain considerably. Note that begin and end are required to make the range-based for loop work.

StackExchange Code Review Q#90815, answer score: 2