Don't use namespace std
Starting top down
using namespace std;
on file level is a very(!) bad idea. for a header file this is an absolute no go as you leak the namespace everywhere. in a cpp file you can get away with it but you increase the chance of a name collision. in a mixed file (like here) you should avoid it as it makes later refactoring more difficult. use it in implementation bodies only.
Use uppercase template types parameters
template<typename t> shall be
template<typename T>, that is de facto standard
next=NULL; shall be
next = nullptr;.
NULLis a C-language leftover tha was finally fixed in C++11. Also, for the sake of readability, start using whitespace reasonably.
node<t> n; is orphaned and shall be deleted
node<t> *head=NULL; again
NULL and whitespace but you also use a different way to initialize compared to
prev shall be named
tail to pair with
Improve method naming scheme
list_delete(). you should always name functions with a verb. So
print_list() would be a better name than
list_print(). however the
list_ prefix is completely needless and distracting duplication in
one_way_linked_list<T>::list_print(). it is even more distracting in
list_create(T dat) as this does not create a list bit inserts a node.
Also, you should try to use common names that can be interpreted by anybody. there are lots of containers in the standard library, you should follow the standard.
list_create(t) should be named like
push_back() or so
list_delete() should be named
Don't implement printing to a stream
list_print() should not be there at all. you introduce a dependency on
std::cout. it is for debugging only and it won't help you when writing to a file or serialize to a string. you could implement
operator<< (member or non-member) to be more generic.
Issues with method/function implementation
if(head==NULL) (again whitespace!) is more readable if written as
if (!head). also you want to assign ptr to two variables but you use the previously assigned
headin the next statement. therfore you did not see the statement is the same in both branches and may be moved outsie the if condition
should look like
head = ptr;
tail->next = ptr;
tail = ptr;
While it should not be there at all - here in this small scope you could use the std namespace for convenience
using namespace std;
You should not clutter this function with
std::cout. while you may do this during development it is inappropriate in code to be reviewed. For logging in production code use logging libraries.
Despite logging to cout the implementation is a complete mess. Starting with the first lines
node<t> *ptr=new node<t>();
where a new node is constructed. nobody needs a new node for deletion. the value is saved for what?
and there it is. ptr gets assigned, the newly created node from before is not accessible anymore and can't be deleted, we leak memory.
wait, what was assigned to ptr? the default value of the newly created nodes member next which is the nullptr, right? For some reasons the gods of C++ decided this should not segfault. any other value than NULL would.
now, as we safely deleted the nullptr our loop ends as the ptr is NULL. we haven't deleted anything but leaked a node. let's go on after the loop.
cool. we have successfully deleted one node, the head node. the rest of the list is now dangling in memory. no one pointing to the second element, we leak another (n-1) nodes, altogether n nodes. but at least the list looks empty.
what we miss is
which potentially dangerous.
- For what functionality is missing compare to other containers from the standard library
- learn about
std::shared_ptr and friends to avoid memory leaks
- learn to write unit tests to get at least the basic implementation tested and right
- care about readability (whitespaces, naming conventions, ...) to avoid causing pain to others