HiveBrain v1.2.0
Get Started
← Back to all entries
patterncppMinor

Queue with std::vector

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
withstdqueuevector

Problem

How can I improve this code?

template
class Queue {
public:
    Queue(const std::initializer_list& i) :elem{i} {}
    int size() const {
        return elem.size();
    }
    bool empty() const {
        return elem.empty();
    }
    void enqueue(T&&);
    void dequeue();
    T peek();
    T& operator[](std::size_t);
    typename std::vector ::iterator begin() {
        return elem.begin();
    }
    typename std::vector ::iterator end() {
        return elem.end();
    }

private:
    std::vector elem;
};

template
void Queue::enqueue(T&& t) {
    elem.push_back(t);
}

template
void Queue::dequeue() {
    if (empty()) {
        throw std::out_of_range("underflow");
    }
    elem.erase(elem.begin());
}

template
T Queue::peek() {
    if (empty()) {
        throw std::out_of_range("underflow");
    }
    return elem.front();
}

template
T& Queue::operator[](std::size_t i) {
    if (empty() || i = size()) {
        throw std::out_of_range("underflow");
    }
    return elem[i];
}

Solution

I assume that you are familiar with std::queue and std::deque and wrote this class as a reinventing-the-wheel exercise.

There are a few mainly aesthetic points that you could improve:

-
int size() is better if returning a std::size_t, which is the underlying type returned by vector::size().

-
Keep parameter names in function prototypes, it helps self-documenting the code.

-
T peek() doesn't change any internal state, so it should be const too.

-
operator[] should be provided with two overloads, const and non-const:

T& operator[](std::size_t index);
  const T& operator[](std::size_t index) const;


Otherwise you won't be able to access data in a const Queue. The same thing applies to begin()/end(). You will have to provide both const and non-const versions. This is purely boilerplate code, but C++ doesn't offer a better solution.

-
This excessively long name: typename std::vector ::iterator should be replaced by a typedef or using alias:

using iterator = std::vector::iterator;

  // Then you simply use `iterator` now:
  iterator begin() { ... }


-
Shouldn't you provide a constructor that takes no arguments? It is very likely that you will want to declare an empty queue at some point. A default constructor should also do:

Queue() = default;

Code Snippets

T& operator[](std::size_t index);
  const T& operator[](std::size_t index) const;
using iterator = std::vector<T>::iterator;

  // Then you simply use `iterator` now:
  iterator begin() { ... }
Queue() = default;

Context

StackExchange Code Review Q#75144, answer score: 6

Revisions (0)

No revisions yet.