Placement of '*' and '&'
In C++ the '*' and '&' are part of the type information and usually belong beside the type not the variable. This is the opposite of normal behavior for C.
const SimpleVector &obj
Is more normally written as:
const SimpleVector& obj
Personally I prefer to put the const on the right. There are one or two corner cases were this makes a difference and when doing complex C++ templates this has saved my butt.
SimpleVector const& obj
const always binds to the left, unless it is on the very left then it binds right. So the above two have the same meaning.
Some people argue that they put the '*' next to the variable because there is the use case.
x is a pointer and
y is not. My counter argument is that you should never declare more than one variable per line. This is echoed in every coding standard I have ever seen. And nobody will let you get through a code review with two variables on a line. So this is invalid straw man argument.
Initialize all members in the intializer list.
SimpleVector::SimpleVector(const SimpleVector &obj)
// All members should be initialized here.
// It is a good habit to get into
// So that a quick glance shows that you have made sure
// that all members are in a nice valid state
Declare an initialize members at the same point.
// Many lines later.
oldNodePtr = obj.head;
Why not use a single line.
Node* oldNodePtr = obj.head;
This also goes to the point of declaring your variables as close to the point of first use as you can. This is not ancient C where you need to declare all the variables at the top of the function.
This has several benifits
- You can see the type at the point where you are using the variable.
- When an object has a constructor/destructor they are only being run at the point where they need to. If you exit early then the constructor is not even run.
Don't manually copy object.s
The object should know how to copy itself.
// Create new node. This is the first node in the new list
Node *newNode = new Node;
newNode->value = oldNodePtr->value;
This should be:
Node* newNode = new Node(*oldNotePtr);
By moving this code out of the
Node you are opening yourself up to a whole bunch of potential maintenance problems. If you keep this in the
Node class then if you modify the way
Node is used then you will automatically update the way it is copied. With your current implementation you have to search for all the places that
Node is used and update them as well.
Don't do extra work
Node *lastNode = new Node;
// Why are you setting this to null
// it leaves scope and no longer exists
// after the next line and thus this is
// just superfluous work that has no meaning.
lastNode = nullptr;
I would have simplified to:
SimpleList::SimpleList(SimpleList const& rhs)
Node** tailPtr = &head;
for(Node* list = rhs.head; list; list = list->next)
(*tailPtr) = new Node(*list);
tailPtr = &((*tailPtr)->next);