Hot answers tagged

32

using System; If you're going to say using System then don't be putting lots of System.s in the file; the point of this is to eliminate those. internal class Queue Is the class intended to be subclassed? If not, seal it. You can always unseal it later if you need to design it for extension. Why do you not implement IEnumerable<string> ? Surely one ...


24

None of your QUEUE_* functions validate their input arguments before using them. NULL pointers will be a problem, particularly for the pointer-to-pointer arguments. C's memory-allocation functions return a void*, which implicitly converts to any other pointer type. This means that you don't need to typecast the result of calloc. Doing so can actually ...


16

A few points that nearly jump out at me: You've implemented it as a linked list of individually allocated nodes. This nearly guarantees poor locality of reference and for small types wastes quite a bit of space. Your destructor leaks memory: template <typename T> queue<T>::~queue() { node *curr = new node(); while(head) { curr = ...


16

Don't do this in a header file: using std::cout; You are now messing with everybody else's code that uses your header file. That will not please them and can lead to issues down the road for them for which your name will be cursed for a thousand years. No need to make the node public. The people using your class do not need to know the internal workings ...


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&...


15

Bug? The queue destroys itself during a resize because public void Enqueue(System.String item) { if(Count == _container.Length) ReSize(Count * 2); _container[_topHook++] = item; } this allows me to enqueue a null string but it gets lost here for(var i = 0; i < _container.Length; i++) if(_container[i] != null) container[j++] = ...


13

First off Never, ever use a using directive in a header as that can cause problems for your client code. As this is a code review I'm not going to comment on how to implement the C++11 features you mentioned. You can get help with this over at StackOverflow. Public interface Why is node a public type in queue? The user should never have to deal with the ...


13

The lack of curly braces here has a potential for errors: if (argc < 2) /*TODO daemon */ cout << "Starting server\n"; else for(int i=1; i<argc; i++) if (!strcmp(argv[i], "-s")) shell(); else if (!strcmp(argv[i], "-s")) exit(0); else usage(argv[0]); The if statement may ...


13

This looks really nice! Here we go: Is the API well thought and idiomatic? Mostly. For a library as simplistic as this, you probably want to avoid creating a special enum when returning NULL on error will suffice. For this you could make the QUEUE_initialize() function return a pointer to the queue_t instead of having a queue_t** passed as an argument. ...


12

You are guarding the state of one variable std::queue<unsigned char*> _queue_; So you only have one mutex. pthread_mutex_t push_mutex; pthread_mutex_t pop_mutex; If you have two then push and pop can modify queue at the same time. Make sure your mutex locking is exception safe by using RAII. Note all operations were you do start operation stop ...


12

1. Failed requirements The program fails to implement the following details from the specification: The specification says, "Input parameters are ..." but these parameters as implemented as global variables and not as inputs. One of the parameters is "number of engineers", but the program makes no use of this number; it does not even represent the ...


12

This is a partial review about some things I saw reading through your code fast. The setter of your Count property shouldn't be public, because I could set it to one million when there's only one element in the queue. You should specify the accessibility modifier for your member field, I guess it is private int startingPos, and I think you should put it at ...


12

A few minor stylistic bits, adding to what other reviews already mentioned: __RING_QUEUE_H__ include guard uses the double underscore prefix, which is reserved for implementation and Standard Library names. Refer to this SO question for a complete listing of potentially reserved named and prefixes. Technically, size_t is a member of namespace std in C++, e....


12

On testing... Your tests are incomplete, certainly. Your test on isEmpty only tests two conditions: a newly instantiated queue which has never had anything added to it should return true, and after adding just one thing, it should return false. How does isEmpty do when the queue is full? How does isEmpty do when the queue has had two items added and ...


12

The model has failed I would expect from an animal shelter to actually store animals. (Well, in programming terms, a representation of animals.) Your implementation is not an animal shelter, it's an animal-relative-arrival-time shelter. I cannot add animals to it, and cannot retrieve animals from it. I can only add animal types, and retrieve meaningless ...


11

It would be better to do for (Integer element; (element = queue.poll) != null;){ } Because you don't want element to be in the scope after the loop has ended


11

You can abstract out the thing that changes, the comparison expression, into a predicate for a template function. class PriorityMatch { int priority_; public: explicit PriorityMatch(int priority) : priority_(priority) {} bool operator()(const Job& job) { return job->priority == priority_; } }; class ...


11

As far as I can tell, this code seems pretty easy to follow, especially with the comments. Here are several things that stood out to me: Since you haven't provided your own queue implementation, I assume you're using std::queue. If so, remove using namespace std and use std:: where necessary. This entire method should be const. You're modifying three ...


11

I think this question would actually benefit from both a C review (just change the cout's to printf, etc) and a C++ review since you seem to be using a very C-orientated programming style. This post will be a C++ review. Please note that the Turbo C++ compiler is not available to me, so my suggestions will revolve around the C++98/03 standard. My first ...


11

Overall the code looks functional to me. Everytime you pop it will wait on the condition variable if the queue is empty. Everytime you push, a notification will be sent to any thread waiting in the pop method because the queue is empty and let it know that the queue is not empty anymore. I can't speak for your intentions but that is generally how people want ...


11

!The Queue About the not-the-queue things: your Employee and HourlyEmployee classes have public fields (firstname, etc.) You should instead have 'getters' for them (like getFirstName()). I would recommend making those values 'final' as well, but I don't think this is central to your question about queues. Your code is generally well formatted. It's easy to ...


11

This class looks reasonably complete, and fully usable. There are some issues I see that may affect the experience though, and also some suggestions about a better way to do it... Issues: Why do you reject null values? Your implementation works well even if the item is null, so why exclude it? The NullPointerException in the constructor is not a problem, ...


11

I won't talk about the big picture (design), but here are some details where you can enhance your code: The "iterator" for loop was idiomatic a few years ago: for(std::list<Queue>::iterator it = listQueue.begin(); it != listQueue.end(); ++it) But now we have the awesome range-based for: for(auto&& elem: listQueue) It will do almost the ...


11

Uh, that's not a ring queue. The O(n) implementation of dequeue jumps out immediately. In an actual circular buffer, you can't use the underlying buffer directly; you have to do math (add and modulus) on all indexing operations (if you even supply them! normally I would expect only a .front() and .back(). Likewise, you can't reuse the underlying contain's ...


11

You've independently discovered how to implement a (double-ended) queue using two stacks. See for example How to implement a queue using two stacks? or google that phrase for lots of hits. If you've implemented them efficiently, each of the implementations you mentioned should be equally efficient: amortized constant time for insertions and deletions. The ...


10

That is a lot of code for the job. It can be done significantly more easily in two areas: use an array instead of a queue use the POSIX function, getline, which will handle lines of any size. If you don't want to use getline (which admittedly might not have existed when K&R wrote their book), concentrate on writing its equivalent. Here is a version ...


10

Here's a bug: var queue = new Queue<int>(); queue.Enqueue(1); Console.WriteLine(queue.Dequeue()); Console.WriteLine(queue.Dequeue()); This prints 1 0, but the second call to Dequeue should throw an exception, since the queue is empty. I would follow System.Collections.Generic.Queue<T> and throw an InvalidOperationException. If we call Dequeue ...


10

The PriorityQueue was no good idea. What are you doing? After the nth element, you copy all the previous n-1 elements into a temporary queue, which has complexity \$O(n)\$. Then you drain half the queue, which has complexity \$O(n \log n)\$. Summed up you get \$O(n^2 \log n)\$. You're just misusing the queue as a sorting vehicle. That's not optimal as ...


10

A single ferry?! Why is this Ferry class (mostly) static? There is such thing as multiple ferries. And, since this information that you are putting inside the Ferry class isn't going to be useful to anything but this one class, then you should turn this into a class that is instantiated and stores data in it's own instance. As Quill noted in a comment: ...


10

Structure So your structure is definitely a bit off. First of all, you made your Node generic, which is good, as it could theoretically work with any content - which is what you want from a queue. But then, you have fields which just don't belong there, such as animalName (and type, which is just not needed). So first of all, lets move animalName outside ...


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