40

LinkNode Assuming this is not an internal class: A huge problem (probably the biggest) with this is that everything is public. The three data members should be private. Data members should always be private, otherwise any outside code can access them. This defeats the purpose of using a class as opposed to a struct. You should also not have any ...


31

Please. Oh please stop doing this. using namespace std; If this was a header file you just polluted the global namespace for anybody that uses your file. This will get it banned from any serious project. This is done in textbooks for some reason and is fine for short ten line example programs. But once you get past 10 lines it has issues. Stop using it; it ...


30

Your iterator is missing a few important details: You should provide the pre- and post-increment operators (++it and it++). Currently, you only have the pre-increment version. It might also be good to provide the -> operator, since some people prefer the it->something syntax over the (*it).something one. The comparison and dereference operators should ...


27

For example, is it normal for the class to have at least one node? In other words, this implementation cannot have an empty linked list. The length is always 1. No, a linked list can be empty (i.e. size 0). But there are designs that intentionally always have at least one member in the list. This special member is referred to as a sentinel and should ...


27

When I run your test code on my machine, it crashes. 1st node on the list contains '10' 2nd node on the list contains '0' 3rd node on the list contains '2' rightNode element of the 3rd node contains a garbage address => exception when trying to dereference the 4th node So I'll try to clean this up. Following from Jamal's answer, the following reworks your ...


22

Just a few quick remarks: Creating a linked list from another enumerable stores items in reverse order. That's probably not what most people would expect. Note that System.Collections.Generic.LinkedList does preserve the original order. A similar case can be made for Add: it adds items to the front. People familiar with List<T> would expect it to add ...


21

This is called a doubly linked list. List<T> is basically a wrapper around an array. The only operations where you can hope to be faster are insertions and deletions from the middle of the list. Using Marshal unless you absolutely have to is a bad idea, if not a plain crazy one. This code has memory leaks (how is that destructor meant to release all ...


20

It looks good and readable IMHO. For brownie points, you could: modify the structure a bit. Not every LinkedList will contain books. Similarly, not every Node in a LinkedList will have an AuthorName. You could define 4 separate classes : Node, Book, LinkedList, and Bookstore(LinkedList). define a data attribute for Node. define __str__ for Node, which just ...


18

A stack should not know about the first element. A stack only know about the last element that was pushed, so the Stack object itself should only have one Node called last. Because of this, the constructor should be changed as well to only take one Node ex: public Stack(Node node). Edit: As @vnp says, Node is private, so it cannot be created outside this ...


18

Time complexity The time complexity of push and pop operations should be \$O(1)\$, and so it is in your case too. It doesn't matter how many elements you have, these operations should take constant time. (UPDATE: you've edited your original post, and made pop wipe out the entire stack. That's not normal! Normally, the pop operation on a stack should return ...


18

Don't Repeat Yourself (DRY) public void add(Object data) { Node temp = new Node(data); Node curr = head; if (head == null) { head = temp; } else { while (curr.getNext() != null) { curr = curr.getNext(); } curr.setNext(temp); } } You have a size field, so you should update it in this ...


18

Not a bad first crack at a linked list. Here's some things that are obvious issues to me: Your getters and setters for list_node_generic aren't really encapsulating much. They allow unchecked access to private fields. Essentially rendering those fields public, but with much verbosity. Since you don't intend to let any code other than the enclosing ...


17

I had tagged your question reinventing-the-wheel, because you're recreating a LinkedList, and Java already has one. @200_success has removed this tag for some reason, allowing me to post this answer: Use Java's built-in LinkedList. There's no reason for you to be writing your own implementation. Using Java's own LinkedList (full classified name: java.util....


17

Starting at the top //any suggestions Yes, plenty. But don't add that to the code. using namespace std; Nearly every beginner includes this (and it's understandable because this is what they do in books). In actual practice this is a bad idea. Never include this line in real code. The reason books include it is to save space (because each printed page ...


17

First, you need to realize that Python is not Java. Python is a "consenting adult language." (Watch Raymond Hettinger's talk for more.) What that means is the bizarre fetish for mutators/accessors/getters and setters that permeates Java and C# doesn't exist in Python. You don't need to write: class Node: def getData(self): def getNext(self): ...


16

One quite important thing to do when you work with linked lists is to link and unlink the previous/next nodes as you add/remove nodes. If you can only add at the end, that's good, but if you can insert a node anywhere you have to be careful. The same goes with removing nodes. +--------+ +--------+ +--------+ | A | | B | ...


16

BUG: public void removeFirst() { Node temp = first; if (first.next == null) last = null; first = first.next; } This crashes if you have an empty list. Add a check for first == null. Additionally... what's that temp variable for? Don't define the Node as T, define the List to contain T. public class LinkList<T&...


16

File extensions Separating the interface from the implementation is great. However, the .h/.hpp split is not obvious. The most frequent file extensions used for implementation files that cannot go into .cpp files are .inl (for inline) and .tpp (for template cpp). I know that at least some code editors or IDEs recognize the extension .inl (at least Code::...


16

All things considered this looks like a really good beginner's attempt at the problem. The really good things I can see are: you have used generics it appears to all look functional it has advanced concepts (you have a double-linked list, not a single-linked list). The generics look right, and the double-link is used well for locating data. There are two ...


16

I see a number of things that may help you improve your code. Prefer references to raw pointers In modern C++, we tend not to use raw pointers very often. In this case, It would probably be better to have two different classes, one would be a LinkedList class and the other would be a Node class. That way, instead of starting with a pointer, you can start ...


16

Collection != enumerator The main problem I see here is that LinkedList<T> implements IEnumerator<T> instead of IEnumerable<T>. That's the wrong interface, which makes this class quite difficult to use: you now have to manually call MoveNext() and Current, instead of being able to use foreach. This also prevents you from using Linq methods,...


16

Next time you're in such a scenario, ask them to clarify what "academic" means. For me it means a solution that concentrates on the essence of data structure, not an implementation of a specific API. But we probably never know so let's not concentrate on that. Instead, let's look at the red flags... (I have ignored the comments as I do not understand the ...


15

New Linked List Class This is a review of the new "New Linked List Class". Here you wrote: //Have to use new template. Does not run if I try to use T template<class E> class LinkNode { public: E data; LinkNode<E> *leftNode; LinkNode<E> *rightNode; LinkNode() { } LinkNode(E e) { this->data = e; } ...


15

Here are some things that may help you improve your code. Don't abuse using namespace std Putting using namespace std at the top of every program is a bad habit that you'd do well to avoid. Use nullptr rather than NULL Modern C++ uses nullptr rather than NULL. See this answer for why and how it's useful. Use more whitespace Lines like this one: ...


15

I don't see anything glaring, except for the size method. It has 2 major flaws: Every time you want to check the size, you must traverse the entire list, giving it a time complexity of O(n) (linear). This would likely be an unacceptable complexity for real world use. I'd expect that a size method would be O(1), meaning the length of time the operation takes ...


15

There are a number of things that could be improved, and I hope you find this list of suggestions useful. Don't abuse using namespace std Putting using namespace std at the top of every program is a bad habit that you'd do well to avoid. Don't use system("pause") There are two reasons not to use system("cls") or system("pause"). The first is that it ...


14

Your code is buggy, if you have two Nodes with the same value in succession it will corrupt the list.... prevNode will be set to the deleted node tempNode, and if the next value also matches the input value you will be working with the wrong node as prevNode. Also, why is d a good name for the input search value? To avoid future bugs it is convenient to ...


14

As usual, revewing a design rather than code. As coded, pop takes time proportional to the current stack size. It could and should take constant time. Hint: reverse the semantics of node.next (BTW, first becomes redundant). I don't think that pop/peek returning -1 on empty stack is a good idea. A value of -1 is a valid result of popping from a stack of ...


14

In addition to other answers: Add a __len__ method (or use .__len__(self) instead of .size(self)): by convention, .__len__(self) is used to calculate the 'size' of an object. Using __len__ also has the benefit that users of your class can call len() on a linked list instance, in the same way as they would any other sequence. def __len__(self): return ...


14

A little late here as I just got back in front of a non-mobile device, so let me try to add some new points: Most importantly (with the caveat that I'm super pedantic), the format of this assignment concerns me. I'd be weary of a professor that assigns something like this. Assuming you copied the instructions verbatim, it seems like this person would ...


Only top voted, non community-wiki answers of a minimum length are eligible