Jeff Schwab
9/26/2008 4:53:00 AM
Gaijinco wrote:
> I'm trying to do a template class Node.
The short answer is that template definitions have to be seen by the
compiler before they are used in a given translation unit, or else the
compiler will not know enough to instantiate them for you automatically.
In your case, rather than putting the Node class template definition
in a .cpp file that includes the header, you might put it in a .tpp file
that is included by that header. My own preference is just to define
templates inline, as most standard library implementations do.
Keep reading if you're bored enough to want some semi-pedantic advice. :)
Firstly, it looks like you're implementing a linked list. Is this just
a learning exercise, or are you planning to use your Node type in
production code? If you just need a linked list, the std::list type is
probably what you want, or else the singly-linked slist that some
library implementations also provide.
> My node.hpp is:
>
> #ifndef _NODE_HPP_
> #define _NODE_HPP_
Just a matter of style: Leading underscores are reserved to the
implementation under various circumstances, so they're generally best
avoided. The above would be fine as NODE_HPP_, or
COM_MNYA_CARLOS_NODE_HPP_ if you prefer. Double underscores are also
worth avoiding.
> namespace com { namespace mnya { namespace carlos {
>
> template <typename T>
> class Node
> {
> private:
> T data;
> Node* next;
> public:
> Node(const T& data);
> ~Node();
> void setData(const T& data);
> T getData() const;
> void setNext(Node<T>* next);
> Node<T>* getNext() const;
There is no need to refer to the type as Node<T> within the template
definition. The name of the template, within its own definition,
implicitly refers to the instantiated type.
> };
>
> }}}
>
> #endif
>
> My node.cpp is:
>
> #include "node.hpp"
>
> namespace com { namespace mnya { namespace carlos {
>
> template <typename T>
> Node<T>::Node(const T& data):data(data){};
That trailing semicolon is illegal. You need semicolons after type
definitions, but not after function definitions.
You also did not initialize the pointer member that will be deleted in
the destructor. If someone creates a node, but does not set its "next"
pointer, the node's destructor may cause a seg. fault or other severe
problem.
To tell you the truth, I'm not sure what the scoping rules are for the
"data" in your member initializer list, because calling the member the
same thing as the constructor's parameter is very unusual. It is widely
considered good programming style to reserve the nicer names for
parameters and public members, and to decorate private fields with
either leading "m_" or (as I prefer) trailing underscores. You used the
this->member syntax elsewhere, and that is fine, but it does not work in
the member initializer list.
> template <typename T>
> Node<T>::~Node()
> {
> if(next!=0)
> delete next;
> }
There's no need to check for null pointers before deleting them.
Deleting null is a no-op. The above should just be "delete next;".
> template <typename T>
> void Node<T>::setData(const T& data)
> {
> this->data = data;
> }
>
> template <typename T>
> T Node<T>::getData() const
> {
> return data;
> }
>
> template <typename T>
> void Node<T>::setNext(Node<T>* next)
> {
> this->next = next;
> }
>
> template <typename T>
> Node<T>* Node<T>::getNext() const
> {
> return next;
> }
>
> }}}
>
> When I try to use the class with:
> Node<int> n(10);
>
> The compiler says: "[Linker error] undefined reference to
> com::mnya::carlos::Node<int>::Node(int const&)"
>
> What am I doing wrong?
Below is what your code would look like with the above suggestions, plus
a few others. To wit:
- Two-space indentation is kind of nasty. It implies that you're
nesting too deeply, which in turn implies that you're not factoring your
code into sufficiently generic components. Ideally, each function
definition should fit in an 80x24 terminal with four-space indentation,
and braces around the bodies of all if-statements and loops. I can't
tell you how much reusable code I've gotten by imposing this restriction
on myself, and later finding that code I factored out of one function is
also useful in several other functions.
- CamelCase does not lend itself as well as snake_case to generic
programming with the standard library. There is also a very clean,
well-defined set of conventions set by Boost.MPL, for naming things that
represent various kinds of ideas. The conventions are not hard to learn
or use; check out David Abrahams' book on C++ Metaprogramming. (The
metaprogramming itself, unlike the naming conventions, is kind of tricky.)
- "get" and "set" methods are popular in other languages, but C++ has a
convention of just calling the accessor ("getter") method by the
property name, and calling the mutator ("setter") method "assign". If
you find yourself with multiple fields that could have separate
mutators, it's probably time to factor your class into multiple, smaller
types.
- If you always put "const" to the right of the thing to which it
refers, its meaning will always be clear: The thing to the left of
"const" is const. Putting const all the way at the left of a type is
common in practice, but is a little tougher for the eye to parse in the
context of more complicated declarations, being the only situation in
which "const" modifies the thing to its right.
- When a template parameter becomes part of the instantiated class's
public interface, it is a matter of good style to provide direct client
access to the parameter, e.g. via typedef.
- Your nodes do not create their "next" brethren, and the setNext method
does not delete the old pointer, but ~Node does delete its next-pointer.
Unless you have a good reason to do otherwise, acquire all resources
in constructors, and release them in the destructors of the same objects
that originally acquired them. This policy is called Resource
Acquisition Is Initialization, or RAII. In your example, it means that
the nodes are not responsible for deleting their "next" pointers, which
in turn means that an explicitly defined destructor is unnecessary.
Managing ownership is probably the hardest part of good C++ programming,
with a correspondingly great pay-off in terms of efficiency, clarity,
reliability, and ease of software maintenance.
/** @file node.hpp defines a singly linked list node type */
#ifndef COM_MNYA_CARLOS_NODE_HH_
#define COM_MNYA_CARLOS_NODE_HH_
namespace com {
namespace mnya {
namespace carlos {
template<typename T>
struct node {
typedef T value_type;
typedef node* pointer;
node( value_type const& value, pointer next =pointer( ) ):
value_( value ),
next_( next ) { }
void assign(value_type const& value) {
value_ = value;
}
value_type value() const {
return value_;
}
void set_next(pointer next) {
next_ = next;
}
pointer next() const {
return next_;
}
private:
value_type value_;
pointer next_;
};
}
}
}
#endif
/** @file main.cc demonstrates the use of com::mnya::carlos::node */
#include "node.hpp"
#include <iostream>
/** Create and traverse a short linked list. The value of each
* node is printed during the list traversal. */
int main() {
typedef com::mnya::carlos::node<int> node_type;
node_type c( 3 );
node_type b( 2, &c );
node_type a( 1, &b );
for (node_type::pointer node = &a; node; node = node->next()) {
std::cout << node->value() << '\n';
}
return 0;
}