Would this linked list be considered clean if written in an hour-long interview?

If you were interviewing me, and saw this code written in a little under an hour, would you consider me as a candidate for a position at your company if you knew I had 2 years of C++ experience?

```
#include
#include

/*! \class Node to be used in likned list like constructors
*/
template
class node
{
private:
T data;
node * next;

public:
/*! \brief Default constructor
* Initialize next pointer to NULL and let data's default constructor
* be called
*/
node() :
next(NULL)
{}

/*! \brief Parametered constructor
* Initialize next pointer to NULL and set data to t
*/
node(T t) :
data(t),
next(NULL)
{}

/*! \brief Copy constructor
*/
node(const node & other) :
data(other.data),
next(other.next)
{}

/*! \brief Destructor
*/
~node()
{}

/*! \brief Assignment operator
*/
node & operator=(const node &other)
{
data = other.data;
next = other.next;

return (*this);
}

/*! \brief Set the next pointer
*/
void set_next(node *next_node)
{
next = next_node;
}

/*! \brief Set the nodes data
*/
void set_data(T t)
{
data = t;
}

/*! \brief Get the nodes data
*/
T get_data() {
return data;
}

/*! \brief Get the nodes next pointer
*/
node * get_next()
{
return next;
}

/*! \brief Equivalence operator
*/
bool operator==(const node &other)
{
return (data == other.data && next == other.next);
}

/*! \brief Not equivalence operator
*/
bool operator!=(const node &other)
{
return (data != other.data || next != other.next);
}
};

/*! \class Linked List
*/
template
class linked_list
{
private:
node * head;

public:
/*! \brief Default constructor
* Initialize head pointer to NULL
*/
linked_list() :
head(NULL)
{}

/*! \brief Parametered constructor
* Initialize head to a node
*/
linked_list(node &n) :
head(&n)
{}

/*! \brief Copy constructor
*/

I can't speak for others, but I'd personally have some reservations. To be fair, perhaps I'm not being lenient enough for doing this under pressure in an hour, but there are multiple issues that I'd want to address with this code.

Questions and Small Problems

Why is node not an internal class of linked_list? Generally, the user should neither know nor care about exactly how a node is represented. I'd let this slide pretty easily though, because it's a small oversight and can be fairly easily corrected.

You're not passing to your node constructor by const T& which I would have expected. If this is constructing from a type that is expensive to copy construct, this adds quite a bit of overhead.

Usage of NULL vs nullptr. Not a big deal, but it'd make me push you a little bit on knowledge of C++11 constructs.

Pain Points

The interface for your list is (somewhat) setup, but the implementation is very, very incomplete. As it currently stands, it doesn't really represent a list at all, but something that holds the address of a single element that exists outside the list, and is therefore tied to the lifetime of that object (which is a big, big problem).

Your linked_list constructor takes a node as a parameter. This goes back to the first point about node being internal, but here it is more serious in my mind. This shows that you do expect the user to know/care about the node type. It also takes it by reference, and then simply makes the head node point to that address. This is all a bit worrying. It should be taking an element of type T, which it then wraps up in a node, and that is stored as the head element:

linked_list(const T& initial)
  : head(new node(initial))
{ }

You never new or delete anything - a linked_list that uses absolutely no dynamic storage is very strange indeed. It'd certainly make me want to press you on things like object lifetimes, stack and heap allocation, dangling references and the like.

There is no way to modify a list once it is created; something as basic as adding an element isn't possible. Even if these functions weren't finished in time, I'd expect just the declarations to be there, something like:

void push_front(const T& data)
{ }

Because of the lack of dynamic allocation, your copy constructor and assignment operator don't really do anything, and you don't have a destructor. Implementations of these would be something I'd heavily scrutinize. For a start, do they do what they are supposed to do? A copy constructor should be doing a deep copy of the list it is passed - many people get this wrong and end up simply doing a shallow copy, which generally means pointers pointing to deleted memory, and undefined behaviour.

operator= should either be using the copy-swap idiom or at least doing basic checks like (this != &rhs), clearing the current data in the list, and copying the data from what is passed in as the argument to operator=. With your current implementation, none of this can be demonstrated properly.

Why is get_head declared as node *? Why are you returning a node instead of simply returning the data? This should have two overloads, one for const T& and one for T&.

Operators shouldn't be member functions, but that's pretty minor, and I wouldn't really care too much. However, if you do make them member functions, they should definitely be const:

bool operator!=(const linked_list &other) const
                                        //^^^^^

All in all, there is a lot lacking for an implementation, even given the short time span. The lack of any kind of dynamic memory allocation would probably be a deal-breaker for me personally. Without being able to hear your reasoning about this, I'd make the assumption that you didn't understand allocations or object lifetimes, which is of critical importance when programming in C++.

Edit: If given a limited time (such as one hour) to write something like a list, I'd probably approach it in the following way: firstly, sketch out the minimal interface I want to develop from. Given the pretty strict time requirements, I'd keep it as simple as possible:

• A private internal node struct with all public members (this cuts down on boilerplate code that needs to be written, and since it's private, it's not user facing, so I'd argue it's fine anyway).

• Private members for list, which I'd keep to a node *head and std::size_t size.

• Basic constructors, copy assignment, copy constructor, destructor.

• Insert, update and delete. Since this is a singly linked list, insert would be limited to adding something to the front (push_front if we're following standard lib naming), something to update a given value, and something to delete a given value. Usually these would use iterators, but that's too complex given the tight time restrictions.

• Something to get the size, if the list is empty, and maybe some basic comparison operators.

Sketch

StackExchange Code Review Q#26557, answer score: 6