6
\$\begingroup\$

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!

\$\endgroup\$
  • \$\begingroup\$ The sentence continues on the next line with a link to en.cppreference.com/w/cpp/iterator/iterator#Example :) \$\endgroup\$ – Zathorix Sep 26 '18 at 13:13
  • 1
    \$\begingroup\$ Please do not update the code in your question to incorporate feedback from answers, doing so goes against the Question + Answer style of Code Review. This is not a forum where you should keep the most updated version in your question. Please see what you may and may not do after receiving answers. \$\endgroup\$ – Sᴀᴍ Onᴇᴌᴀ Sep 26 '18 at 19:31
  • \$\begingroup\$ Sorry, I've posted it as an answer now. @TobySpeight oops, I was looking at a different part of my post. I'll fix that bit, thanks. \$\endgroup\$ – Zathorix Sep 26 '18 at 19:39
5
\$\begingroup\$

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.

\$\endgroup\$
1
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ I like the change from overloading operator<<() to the notify method. The former made sense in an earlier version, but doesn't really apply anymore now. However, I'm not sure about resetting the count each time begin() is called. The class is meant as a thin wrapper around the original iterator, but this change gives it extra duties that I'm not sure it should perform. I'll keep this version for now, but it doesn't really seem satisfying. \$\endgroup\$ – Zathorix Sep 27 '18 at 13:51
0
\$\begingroup\$

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.

\$\endgroup\$
  • 1
    \$\begingroup\$ You can initialize size_, too (rather than assigning after it has been default-initialized). You might be able to make begin() and end() const by taking a pointer instead of a reference, but that seems... unclean. It's slightly weird that advancing an iterator changes the state of its owner, so maybe we have to accept that they can't be const. Alternatively, perhaps we should be storing a pair of wrapped iterators, rather than keeping the originals around? That's a whole different approach I don't have time to investigate right now, but it might be worth exploring. \$\endgroup\$ – Toby Speight Sep 26 '18 at 20:13
  • \$\begingroup\$ I had it take a pointer before; same issue there. Probably the best way to get begin()/end() const is to rework the design like you suggested. On the other hand, I don't think they're always const for STL iterators either (example). I'll have to think about it a bit more. In any case. thanks for all the help, it's been beyond useful! \$\endgroup\$ – Zathorix Sep 26 '18 at 21:01
  • \$\begingroup\$ True (about constness of standard iterators) - there are both const and non-const versions, returning different kinds of iterator. I think we shouldn't need to deal with that, because a progress bar can wrap Container::const_iterator or Container::iterator equally well. Which pushes me towards the view begin() and end() don't need to be const (but I'm still somewhat "on the fence"). \$\endgroup\$ – Toby Speight Sep 27 '18 at 7:35

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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