Progress bar wrapper class in C++

Question

I've recently written a simple progress bar class in C++ to mimic usage of similar libraries I've used in Python. The idea is to take some iterable container (e.g. std::vector), and iterate over the container while printing the progress to stdout. This can be useful when doing some computationally expensive operations on the container in the loop body, such as in physics simulations.

The code as it stands is

#include <iostream>
#include <iterator>

#ifndef __PBAR_H
#define __PBAR_H

namespace pbar {

template<class It>
class ProgressBar {
public:
  ProgressBar(It&& it, It&& it_end, int width, const char symbol='=')
      : pos_(0), 
        width_(width), 
        symbol_(symbol), 
        iter_(it),
        iter_begin_(it),
        iter_end_(it_end) 
  {}

  using value_type = typename It::value_type;
  using reference = typename It::reference;

  class iterator 
    : public std::iterator<typename It::iterator_category,
                           value_type,
                           typename It::difference_type,
                           typename It::pointer,
                           reference>
  {
  private:
    value_type val_ = *iter_;
    ProgressBar<It> *parent_;

  public:
    iterator(ProgressBar<It> *parent, value_type start) 
      : val_(start), parent_(parent) {}

    iterator& operator++();
    iterator operator++(int);

    bool operator==(iterator other);
    bool operator!=(iterator other);
    reference operator*(); 

  };

  iterator begin();
  iterator end();

  template<class I>
  friend std::ostream& operator<<(std::ostream &steam, const ProgressBar<I> &pbar);

private:
  int pos_;
  int width_;
  char symbol_;
  char left_delim_{'['};
  char right_delim_{']'};
  char pointer_{'>'};

  It iter_;
  It iter_begin_;
  It iter_end_;

}; // class ProgressBar


template<class It>
using piter = typename ProgressBar<It>::iterator;

template<class It>
inline bool ProgressBar<It>::iterator::operator==(piter<It> other) {
  return val_ == other.val_;
}

template<class It>
inline bool ProgressBar<It>::iterator::operator!=(piter<It> other) {
  return !(*this == other);
}

template<class It>
inline typename It::reference ProgressBar<It>::iterator::operator*() {
  return val_;
}

template<class It>
inline piter<It>& ProgressBar<It>::iterator::operator++() {
  ++(parent_->iter_); 
  val_ = *(parent_->iter_); 
  auto fraction = static_cast<double>(std::distance(parent_->iter_begin_, 
        parent_->iter_))/std::distance(parent_->iter_begin_, parent_->iter_end_);
  parent_->pos_ = parent_->width_*fraction;
  std::cout << *parent_;
  return *this; 
}

template<class It>
inline piter<It> ProgressBar<It>::iterator::operator++(int) {
  auto retval = *this;
  ++(*this);
  return retval;
}

template<class It>
inline piter<It> ProgressBar<It>::begin() {
  return ProgressBar<It>::iterator(this, *iter_begin_);
}

template<class It>
inline piter<It> ProgressBar<It>::end() {
  return ProgressBar<It>::iterator(this, *iter_end_);
}


template<class It>
inline std::ostream& operator<<(std::ostream &stream, const ProgressBar<It> &pbar) {
  stream << pbar.left_delim_;
  for (int i=0; i<pbar.width_; i++) {
    if (i < pbar.pos_)
      stream << pbar.symbol_;
    else if (i == pbar.pos_)
      stream << pbar.pointer_;
    else
      stream << " ";
  }
  stream << pbar.right_delim_ << int(double(pbar.pos_)/pbar.width_*100) << "%\r";
  stream.flush();
  return stream;
}

}; // namespace pbar

#endif // __PBAR_H

Using the class:

#include <iostream>
#include <vector>
#include "pbar.h"

using namespace pbar;

int main() {
  std::vector<int> v = {1, 2, 3, 4, 5};

  ProgressBar<std::vector<int>::iterator> pbar(v.begin(), v.end(), 50);
  for (auto i = pbar.begin(); i != pbar.end(); i++) {
    ;
  }

  // The constructor allows changing the bar symbol (default '=')  
  ProgressBar<std::vector<int>::iterator> pbar2(v.begin(), v.end(), 50, '#');

  std::cout << "\nRange based loops also work" << std::endl;

  for (auto& i: pbar2) {
    ;
  }
}

While I've been programming in C++ for a while now, I haven't had the opportunity to get a lot of feedback on my code yet, and it's making me anxious that I'm learning bad patterns. So, I'd like to know:

1. Are there any obvious pitfalls in my code?
2. Bad design decisions?
3. General improvements or additions to the code?
4. Should I separate the library into a header and implementation file, or keep it header-only?
5. Is there a better way to implement iterators for custom types? I've taken this implementation from here, but I'm wondering if there's a different way than using nested classes.

I appreciate any and all advice!

Answer 1

The constructor takes two forwarding references, but doesn't actually forward them:

  ProgressBar(It&& it, It&& it_end, int width, const char symbol='=')
        iter_(it),
        iter_begin_(it),
        iter_end_(it_end)

It's better to accept it and it_end by value, and move-construct from them:

  ProgressBar(It it, It it_end, int width, const char symbol='=')
        iter_(it),
        iter_begin_(std::move(it)),
        iter_end_(std::move(it_end))

---

The constructor also does no checking of its arguments - what does a zero or negative width mean? Should it even be a signed type at all?

---

std::cout is a strange choice of stream for the ++ operator - progress information like this should normally go to std::clog rather than being mixed with program output.

---

Inheriting from std::iterator is now deprecated - just define the member types directly in the class.

---

It's wrong for iterator to forward the category tag of It, as it's at most a Forward Iterator - it's certainly not a Bidirectional Iterator. We could make iterator inherit from It to exactly forward its category, but I think that would be a mistake: we don't want to try to track progress of a bidirectional iterator. We'll need to be a bit clever when defining its category.

---

Many of the members (of bar and of iterator) are missing const when I'd expect it:

bool operator==(const iterator& other) const;
bool operator!=(const iterator& other) const;
reference operator*() const;

---

Modern GCC (g++-8 -std=c++2a) doesn't believe you can use that template alias to define members of the iterator type:

204396.cpp:73:13: error: no declaration matches ‘bool pbar::ProgressBar<It>::iterator::operator==(pbar::piter<It>)’
 inline bool ProgressBar<It>::iterator::operator==(piter<It> other) {
             ^~~~~~~~~~~~~~~
204396.cpp:42:10: note: candidate is: ‘bool pbar::ProgressBar<It>::iterator::operator==(pbar::ProgressBar<It>::iterator)’
     bool operator==(iterator other);
          ^~~~~~~~

(and many, many more like that)

---

Computation of fraction can be expensive when It is less capable than RandomAccessIterator, due to the std::distance() calls. We can save work by storing the total and progress separately, which means only one std::distance() per ProgressBar instead of two per iteration.

---

The percentage calculation doesn't need to go through double if we multiply before dividing (at least in the absence of overflow):

    stream << pbar.right_delim_ << (pbar.pos_ * 100 / pbar.width_) << "%\r";

---

Assignments through the iterator are lost. For example, this code doesn't work (the vector is unchanged):

for (auto& i: ProgressBar(v.begin(), v.end(), 50, '#')) {
    i *= 2;
}

Really, we want to encapsulate the container iterator inside the progress-bar iterator (instead of a copy of its value), so that operator*() forwards right through to the container.

---

These lines can be within the include-guard:

#include <iostream>
#include <iterator>

Although they almost certainly have include guards of their own, it certainly does no harm to avoid repeating them.

---

Finally, a specific answer to your specific question no. 4: there's nothing in the header that doesn't depend on the template parameter, so nothing can be moved to a separately-compiled implementation file without losing the benefit of templates.

Answer 2

As discussed in comments, an alternative approach would be to construct the wrapped start and end iterators when the progress bar is created, rather than when begin() or end() is called. I've experimented with this, and here's the result (which I think is considerably simpler); it may be a useful starting point:

#ifndef __PBAR_H
#define __PBAR_H

#include <iostream>
#include <iterator>

namespace pbar {

template<class It>          // requires ForwardIterator It
class ProgressBar
{
public:
    ProgressBar(It it, It it_end, size_t width, const char symbol='=')
        : width_(width),
          size_(std::distance(it, it_end)),
          symbol_(symbol),
          iter_begin_(*this, std::move(it)),
          iter_end_(*this, std::move(it_end))
    {
    }

    struct iterator;

    iterator begin()
    {
        count_ = 0;
        return iter_begin_;
    }
    iterator end()
    {
        return iter_end_;
    }

private:
    // callback from iterator
    void notify()
    {
        ++count_;
        auto& stream = std::clog;
        std::size_t pos = width_ * count_ / size_;
        stream << left_delim_;
        for (std::size_t i = 0;  i < width_;  ++i) {
            stream << (i == pos ? pointer_
                       : i < pos ? symbol_
                       : ' ');
        }
        stream << right_delim_ << count_ * 100 / size_ << "%\r";
        stream.flush();
    }

    std::size_t count_ {0};
    const std::size_t width_;
    const std::size_t size_;

    const char symbol_;
    const char left_delim_ {'['};
    const char right_delim_ {']'};
    const char pointer_ {'>'};

    const iterator iter_begin_;
    const iterator iter_end_;

}; // class ProgressBar


template<class It>
struct ProgressBar<It>::iterator
{
    using iterator_category = std::forward_iterator_tag;
    using value_type = typename It::value_type;
    using difference_type = typename It::difference_type;
    using pointer = typename It::pointer;
    using reference = typename It::reference;

    iterator(ProgressBar& parent, It iter)
        : parent_(parent), iter_(std::move(iter))
    {}

    iterator& operator++()
    {
        ++iter_;
        parent_.notify();
        return *this;
    }
    iterator operator++(int)
    {
        auto retval = *this;
        ++*this;
        return retval;
    }

    bool operator==(const iterator& other) const
    {
        return iter_ == other.iter_;
    }
    bool operator!=(const iterator& other) const
    {
        return !(*this == other);
    }

    reference operator*() const {
        return *iter_;
    }

    It operator->() const {
        return iter_;
    }

private:
    ProgressBar<It>& parent_;
    It iter_;

}; // struct ProgressBar<It>::iterator

} // namespace pbar

#endif // __PBAR_H

It's still hard to know what's the Right Thing to do if begin() is called more than once, or if the returned iterator is copied (in particular, what happens if the user calls std::distance(bar.begin(), bar.end()) - we'll interpret that as 100% visited!). I've chosen to reset the counter each time begin() is called, but that's still not foolproof.

A further enhancement would be to remember what we last displayed, to avoid screen flicker when we process many small items. We should write output only when the bar position or the percentage number changes.

Answer 3

Thanks Toby for the advice, I've updated my code to reflect most of your comments to the best of my ability:

#include <iostream>
#include <iterator>

#ifndef __PBAR_H
#define __PBAR_H

namespace pbar {

template<class It>
class ProgressBar {
public:
  ProgressBar(It it, It it_end, size_t width, const char symbol='=')
      : count_(0),
        width_(width), 
        symbol_(symbol), 
        iter_(std::move(it)),
        iter_begin_(iter_),
        iter_end_(std::move(it_end))
  {
    size_ = std::distance(iter_begin_, iter_end_); 
  }

  struct iterator {

    using iterator_category = typename It::iterator_category;
    using value_type = typename It::value_type;
    using difference_type = typename It::difference_type;
    using pointer = typename It::pointer;
    using reference = typename It::reference;

  public:
    iterator(ProgressBar<It>& parent, It& iter) 
      : parent_(parent), iter_(iter) {}

    iterator& operator++();
    iterator operator++(int);

    bool operator==(const iterator& other) const;
    bool operator!=(const iterator& other) const;

    reference operator*() const;

  private:
    ProgressBar<It>& parent_;
    It& iter_;

  };

  iterator begin();
  iterator end();

  template<class I>
  friend std::ostream& operator<<(std::ostream&, const ProgressBar<I>&);

private:
  size_t count_;
  size_t width_;
  size_t size_;

  char symbol_;
  char left_delim_{'['};
  char right_delim_{']'};
  char pointer_{'>'};

  It iter_;
  It iter_begin_;
  It iter_end_;

}; // class ProgressBar

template<class It>
inline bool ProgressBar<It>::iterator::operator==(
    const ProgressBar<It>::iterator& other) const {
  return iter_ == other.iter_;
}

template<class It>
inline bool ProgressBar<It>::iterator::operator!=(
    const ProgressBar<It>::iterator& other) const {
  return !(*this == other);
}

template<class It>
inline typename It::reference ProgressBar<It>::iterator::operator*() const {
  return *iter_;
}

template<class It>
inline typename ProgressBar<It>::iterator& ProgressBar<It>::iterator::operator++() {
  ++(iter_); 
  ++parent_.count_;
  std::clog << parent_;
  return *this; 
}

template<class It>
inline typename ProgressBar<It>::iterator ProgressBar<It>::iterator::operator++(int) {
  auto retval = *this;
  ++(*this);
  return retval;
}

template<class It>
inline typename ProgressBar<It>::iterator ProgressBar<It>::begin() {
  return ProgressBar<It>::iterator(*this, iter_begin_);
}

template<class It>
inline typename ProgressBar<It>::iterator ProgressBar<It>::end() {
  return ProgressBar<It>::iterator(*this, iter_end_);
}

template<class It>
inline std::ostream& operator<<(std::ostream &stream, const ProgressBar<It> &pbar) {
  size_t pos = pbar.width_*pbar.count_/pbar.size_;
  stream << pbar.left_delim_;
  for (int i=0; i<pbar.width_; i++) {
    if (i < pos)
      stream << pbar.symbol_;
    else if (i == pos)
      stream << pbar.pointer_;
    else
      stream << " ";
  }
  stream << pbar.right_delim_ << pbar.count_*100/pbar.size_ << "%\r";
  stream.flush();
  return stream;
}

}; // namespace pbar

#endif // __PBAR_H

One thing I'm puzzled about is how to get begin() and end() to be const. Currently, they return an iterator whose constructor takes the parent class by reference, which drops the const. One way I can imagine fixing it is by moving most of the logic into the iterator subclass, which would essentially turn ProgressBar into a front for the encapsulated iterator, but I'm wondering if there isn't a better way to do it.