#include
The code was in error until I prefixed it with
#include <vector> #include <iostream>
~CircularBuffer()
There's no need to define a destructor that does nothing - just let the compiler generate a default one.
If you wanted this to be a base class, you would provide a virtual destructor, but you can explicitly default it:
virtual ~CircularBuffer() = default;
I'd say that you are right to omit copy constructor and assignment operator, as the compiler-provided ones will work correctly.
empty()
The reason containers have empty() is so that it can be quicker than testing size is non-zero. So implementing empty() in terms of size() should be a last resort. In this case, we set head negative to indicate an empty buffer, so we can just test that:
bool empty() const { return head < 0; }
Note that I've made this a const method, as we may want to call it on a buffer we don't intend to modify.
size()
There's no need for the sz variable, as we set it only to return immediately:
size_t size() const { if (head < 0) return 0; if (next > head) return next - head; return next + (data.size() - head); }
I've made this const, too, and changed the return type to be more in line with what users will expect. If I were to be really pedantic, I'd consider std::vector<char>::size_type, and adjust the head and next members accordingly.
You could write a single return with a tertiary expression here, but I think the if ladder is slightly clearer - that's obviously a judgement call.
push()
As in empty(), it's inefficient to call size() when we just want to know whether the container is full. Instead, we can just test next == head. If you must produce output, send it to std::cerr, not std::cout! Generally, in a library, it's better to report the error to the caller (by exceptions, or perhaps as a return value) rather than to output diagnostics ourselves, but I'll let that stand for now.
It wasn't obvious at first reading that if head was negative, then we could depend on next being zero. I would probably not made that assumption, and written something like:
data[next] = val; if (head < 0) head = next; if (++next == data.size()) next = 0;
pop()
Most of the critique of push() applies here, so I won't repeat it. It's not clear why -1 is chosen as the value to return on failure - was this specified in the requirements?
main()
You provide no main() function, which is a shame - it would have been nice to use your unit tests when refactoring.
Modified version
I changed the index types to size_t, and defined a constant to indicate empty (as size_t doesn't have negative values).
#include <vector> #include <stdexcept> class CircularBuffer { static const size_t EMPTY = ~0u; std::vector<char> data; size_t next = 0; // index at which next insert will occur size_t head = EMPTY; // index of next pop, or EMPTY marker public: CircularBuffer(size_t size) : data(size) { } virtual ~CircularBuffer() = default; bool empty() const { return head == EMPTY; } size_t size() const { if (empty()) return 0; if (next > head) return next - head; return data.size() - head + next; } void push(char val) { if (next == head) throw std::logic_error("Buffer overrun"); data[next] = val; if (empty()) head = next; if (++next == data.size()) next = 0; } char pop() { if (empty()) throw std::logic_error("Buffer underrun"); const char popVal = data[head]; if (++head == data.size()) head = 0; if (head == next) head = EMPTY; return popVal; } };
Future directions
If you want to set yourself some more challenges, I'd recommend looking at the standard containers (especially std::queue) and seeing how you can make the interface look more like theirs. Implement front() and back() (easy!), emplace(), and some iterators.
Also, you might want to make this a template class, so it can hold any kind of element, not just char.
