7
\$\begingroup\$

How can I improve this code?

template<class T>
class Queue {
public:
    Queue(const std::initializer_list<T>& 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 <T>::iterator begin() {
        return elem.begin();
    }
    typename std::vector <T>::iterator end() {
        return elem.end();
    }

private:
    std::vector<T> elem;
};

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

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

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

template<class T>
T& Queue<T>::operator[](std::size_t i) {
    if (empty() || i < 0 || i >= size()) {
        throw std::out_of_range("underflow");
    }
    return elem[i];
}
\$\endgroup\$
3
  • 2
    \$\begingroup\$ use std::queue<T> instead? Then you get iterators too. \$\endgroup\$
    – Alnitak
    Dec 29, 2014 at 17:09
  • 2
    \$\begingroup\$ Or std::deque if you need efficient insertion/removal at both ends. \$\endgroup\$
    – glampert
    Dec 29, 2014 at 17:43
  • 1
    \$\begingroup\$ good point, especially since std::queue by default just wraps a std::deque. I achieved substantial performance improvements recently by replacing a queue with a deque. \$\endgroup\$
    – Alnitak
    Dec 29, 2014 at 17:58

1 Answer 1

6
\$\begingroup\$

I assume that you are familiar with std::queue and std::deque and wrote this class as a 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<T>. 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 <T>::iterator should be replaced by a typedef or using alias:

      using iterator = std::vector<T>::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;
    
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Not the answer you're looking for? Browse other questions tagged or ask your own question.