Skip to main content
Code Review

Return to Answer

Commonmark migration
Source Link

A few fairly obvious points:

Prefer nullptr over NULL

Unless you really need to support older compilers, you're generally better off using nullptr in new code. If you do use nullptr, you can still support older compilers if really necessary--it's basically just a class that provides conversions for pointer to T and pointer to member.

###Prefer member initializer lists over assignment in a constructor

Prefer member initializer lists over assignment in a constructor

The most obvious example here would be to replace your node constructor:

Node(T val, Node *nxt=NULL) {
 value = val;
 next = nxt;
}

...with one using a member initializer list:

Node(T value, Node *next = nullptr) :
 value(value),
 next(next)
{}

Note that yes, in this case, although the parameter being used to initialize it have the same name, but the compiler can keep track of which is which anyway.

###Rule of three/five/zero

Rule of three/five/zero

Note that in virtually every case, if you provide a destructor and a copy ctor, you also want a copy assignment operator. If you want the better efficiency you can get from move construction/move assignment available in C++11 and later, you'll probably want to add overloads of those that accept rvalue references. I consider it open to debate whether the rule of zero really applies well in this case--I've written a linked list that used smart pointers, but I was never entirely happy with it. It's clumsy but workable in the case of a singly linked list. For a doubly linked list, it's...basically unmanageable.

###Side Note

Side Note

Contrary to your statement, if you don't include a copy ctor, you probably won't get correct results with this sort of class. In particular, the compiler will generate code that does a shallow copy, so only the head pointer in the LinkedList object will be copied. This means if you copy a linked list, you'll end up with both pointing to a single head node. In itself that's not necessarily a problem, but when either of those is destroyed, all the nodes in the list will be destroyed, so in essence you've just destroyed both linked lists, not just the one you intended to. Attempting to use or destroy the one that wasn't supposed to have been destroyed yet will give undefined behavior.

A few fairly obvious points:

Prefer nullptr over NULL

Unless you really need to support older compilers, you're generally better off using nullptr in new code. If you do use nullptr, you can still support older compilers if really necessary--it's basically just a class that provides conversions for pointer to T and pointer to member.

###Prefer member initializer lists over assignment in a constructor

The most obvious example here would be to replace your node constructor:

Node(T val, Node *nxt=NULL) {
 value = val;
 next = nxt;
}

...with one using a member initializer list:

Node(T value, Node *next = nullptr) :
 value(value),
 next(next)
{}

Note that yes, in this case, although the parameter being used to initialize it have the same name, but the compiler can keep track of which is which anyway.

###Rule of three/five/zero

Note that in virtually every case, if you provide a destructor and a copy ctor, you also want a copy assignment operator. If you want the better efficiency you can get from move construction/move assignment available in C++11 and later, you'll probably want to add overloads of those that accept rvalue references. I consider it open to debate whether the rule of zero really applies well in this case--I've written a linked list that used smart pointers, but I was never entirely happy with it. It's clumsy but workable in the case of a singly linked list. For a doubly linked list, it's...basically unmanageable.

###Side Note

Contrary to your statement, if you don't include a copy ctor, you probably won't get correct results with this sort of class. In particular, the compiler will generate code that does a shallow copy, so only the head pointer in the LinkedList object will be copied. This means if you copy a linked list, you'll end up with both pointing to a single head node. In itself that's not necessarily a problem, but when either of those is destroyed, all the nodes in the list will be destroyed, so in essence you've just destroyed both linked lists, not just the one you intended to. Attempting to use or destroy the one that wasn't supposed to have been destroyed yet will give undefined behavior.

A few fairly obvious points:

Prefer nullptr over NULL

Unless you really need to support older compilers, you're generally better off using nullptr in new code. If you do use nullptr, you can still support older compilers if really necessary--it's basically just a class that provides conversions for pointer to T and pointer to member.

Prefer member initializer lists over assignment in a constructor

The most obvious example here would be to replace your node constructor:

Node(T val, Node *nxt=NULL) {
 value = val;
 next = nxt;
}

...with one using a member initializer list:

Node(T value, Node *next = nullptr) :
 value(value),
 next(next)
{}

Note that yes, in this case, although the parameter being used to initialize it have the same name, but the compiler can keep track of which is which anyway.

Rule of three/five/zero

Note that in virtually every case, if you provide a destructor and a copy ctor, you also want a copy assignment operator. If you want the better efficiency you can get from move construction/move assignment available in C++11 and later, you'll probably want to add overloads of those that accept rvalue references. I consider it open to debate whether the rule of zero really applies well in this case--I've written a linked list that used smart pointers, but I was never entirely happy with it. It's clumsy but workable in the case of a singly linked list. For a doubly linked list, it's...basically unmanageable.

Side Note

Contrary to your statement, if you don't include a copy ctor, you probably won't get correct results with this sort of class. In particular, the compiler will generate code that does a shallow copy, so only the head pointer in the LinkedList object will be copied. This means if you copy a linked list, you'll end up with both pointing to a single head node. In itself that's not necessarily a problem, but when either of those is destroyed, all the nodes in the list will be destroyed, so in essence you've just destroyed both linked lists, not just the one you intended to. Attempting to use or destroy the one that wasn't supposed to have been destroyed yet will give undefined behavior.

Source Link
Jerry Coffin
  • 34.1k
  • 4
  • 77
  • 144

A few fairly obvious points:

Prefer nullptr over NULL

Unless you really need to support older compilers, you're generally better off using nullptr in new code. If you do use nullptr, you can still support older compilers if really necessary--it's basically just a class that provides conversions for pointer to T and pointer to member.

###Prefer member initializer lists over assignment in a constructor

The most obvious example here would be to replace your node constructor:

Node(T val, Node *nxt=NULL) {
 value = val;
 next = nxt;
}

...with one using a member initializer list:

Node(T value, Node *next = nullptr) :
 value(value),
 next(next)
{}

Note that yes, in this case, although the parameter being used to initialize it have the same name, but the compiler can keep track of which is which anyway.

###Rule of three/five/zero

Note that in virtually every case, if you provide a destructor and a copy ctor, you also want a copy assignment operator. If you want the better efficiency you can get from move construction/move assignment available in C++11 and later, you'll probably want to add overloads of those that accept rvalue references. I consider it open to debate whether the rule of zero really applies well in this case--I've written a linked list that used smart pointers, but I was never entirely happy with it. It's clumsy but workable in the case of a singly linked list. For a doubly linked list, it's...basically unmanageable.

###Side Note

Contrary to your statement, if you don't include a copy ctor, you probably won't get correct results with this sort of class. In particular, the compiler will generate code that does a shallow copy, so only the head pointer in the LinkedList object will be copied. This means if you copy a linked list, you'll end up with both pointing to a single head node. In itself that's not necessarily a problem, but when either of those is destroyed, all the nodes in the list will be destroyed, so in essence you've just destroyed both linked lists, not just the one you intended to. Attempting to use or destroy the one that wasn't supposed to have been destroyed yet will give undefined behavior.

lang-cpp

AltStyle によって変換されたページ (->オリジナル) /