4
\$\begingroup\$

I want to create an object, that represents a progress output.

In a GUI this would be a progress bar, in a console application it may be a text output, that can only be written, but not changed anymore (e.g. the progress bar cannot rewind).

Other possible implementations could include sending a e-mail every 10% of progress and in the best case the API should be open for any other idea how to communicate progress as well.

When I start using the API in programs, it will be hard to change it later on, so it should be well-thought. I hope to get helpful feedback here and hints which use cases the current API may not cover.


My current idea for the API is this one:

class Progress{
public:
        // Called by the function that reports progress
        virtual void setMaxProgress(int value);
        virtual void setCanStop(bool can_stop);
        virtual void start(std::string description);
        virtual void progress(int value);
        virtual void incProgress(int steps = 1);
        virtual void info(std::string message);
        virtual bool shouldStop();
        virtual void end();
        virtual bool wasStopped();
private:
        // Called by the progress implementation
        virtual bool canStop();
        virtual int currentProgress();
        virtual int maxProgress();
};

The functions in detail:

  • The program sets the maximum progress, e.g. for processing 50 items: setMaxProgress(50)
  • The program sets that the process can be interrupted (e.g. by rolling back a transaction or returning a partial result): setCanStop(true).
  • The program starts progress: start("I am calculating foo").
  • The program reports that 11 items in total were processed: progress(11)
  • The program reports that it processed 1 item: incProgress().
  • After each item the program asks if the user requested the operation to be interrupted (e.g. using a cancel button) with: shouldStop().
  • The program displays some status message with info("Second step ...").
  • The program reports that the task is finished (the progress implementation should hide the progress bar): end().
  • The progress implementation asks how many steps are needed with: maxProgress().
  • The progress implementation asks how many steps are already finished with: currentProgress().
  • The progress implementation asks if it should show a cancel button (may not be implemented): canStop().
  • The program queries if the user clicked cancel to determine if the algorithm finished or was interrupted with: wasStopped().

An implementation with a progress bar (GUI or TUI) may set the maximum value to 50 and display a cancel button when canStop() is true.

[====      ] 40% [cancel]

A text mode implementation may calculate the percentage and display a simple progress bar, ignoring canStop()

0 =============== 100
  |||||

I currently use two implementations that look almost like this one:

  • One counts the progress steps and every time the percentage is increased by a certain amount (depending on the console width), it outputs a dot. When some info message is printed, it outputs the current progress after it again.
  • The other one updates the global Maya progress bar. On each progress update it sends a script command to Maya to ask if cancel was pressed and sends the updated progress information. When cancel was pressed, an internal variable is set that makes shouldStop return true. The info method uses the Maya status bar to display a message.

(I don't really use the info method, but it was in the code base that contained the first implementation


Example implementation:

class SimpleProgress {
public:
        SimpleProgress(int progress_bar_length = 50, int max_progress = 100): progress_bar_length(progress_bar_length), max_progress(max_progress) {};

        virtual void start(std::string describtion) {
                std::cerr << "Starting: " << describtion << std::endl;
                _description = describtion;
        };

        virtual void end() {
                std::cerr << "Finished: " << _description << std::endl;
        };

        virtual void setMaxProgress(int value) {
                max_progress = value;
        };

        virtual void progress(int progress) {
                assert(max_progress >= 0);
                assert(progress <= max_progress);
                for(int i = 0; i < (progress - current_progress) / max_progress * progress_bar_length; i++) {
                        std::cerr << "=";
                }
                current_progress = progress;
        };

        virtual void incProgress(int steps = 1) {
                if(max_progress >= 0) {
                        assert(current_progress <= max_progress);
                        progress(current_progress + steps);
                } else {
                        // Progress without a known maximum value.
                        std::cerr << "." << std::endl;
                }
        };

        virtual int maxProgress() override {
                return max_progress;
        };

        virtual void info(std::string message) {
                std::cerr << message << std::endl;
                for(int i = 0; i < current_progress / max_progress * progress_bar_length; i++) {
                        std::cerr << "=";
                }
        };

        virtual void setCanStop(bool can_stop) {
        };

        virtual bool canStop() {
                return false;
        };

        virtual bool shouldStop() {
                return false;
        }

private:
        int current_progress = 0;
        int max_progress = -1;
        int progress_bar_length;
        std::string _description;
};

(Note that this is only a prototype as I am currently rewriting the class from a more complex project)

For a non-interactive implementation, you could for example want to stop when a certain runtime is exceeded, implementing the methods like this:

        virtual void setCanStop(bool can_stop) {
                _can_stop = can_stop;
        };

        virtual bool canStop() {
                return _can_stop;
        };

        virtual bool shouldStop() {
                if(_can_stop && runtime >= max_runtime) {
                        return true;
                } else {
                        return false;
                }
        }

Example usage:

void doSomething(Progress *progress = nullptr) {
    DummyProgress dummyProgress;
    if(progress == nullptr) {
        progress = &dummyProgress;
    }
    progress->setMaxProgress(50);
    progress->canStop(true);
    progress->start("Calculating");
    for(int i=0; i < 50; i++) {
        progress->progress(i);
        //
        // Calculate something for step i
        //
        if(progress->shouldStop()) {
            progress->end();
            break;
        }
    }
    if(progress->wasStopped()) {
        std::cerr << "Not all items were processed." << std::endl;
    } else {
        progress->end();
    }
}

I would like to get feedback about what is possibly missing in my API, what could be improved and what may be a bad idea?


Feedback on suggestions:

  • Replace setMaxProgress and other setters with parameters for start(). This will help to avoid code that did not set theses properties.
  • Making getters const
  • I cannot purely rely on callbacks for the cancel button, as I need to support one implementation that requires an active query if cancel was clicked, i.e., running progressBar -query -isCancelled in Maya (docs).
\$\endgroup\$
7
  • 2
    \$\begingroup\$ The code is not complete and just with the class definition you are only get improvements on the class that could be wrong because you didn't share the code on the post. \$\endgroup\$ – camp0 Feb 28 '20 at 13:20
  • 1
    \$\begingroup\$ Consider adding at least one implementation and one usage of this interface to help reviewers generate more useful answers. \$\endgroup\$ – L. F. Feb 28 '20 at 13:53
  • \$\begingroup\$ @camp0 I am mostly interested in feedback for designing the interface, i.e., what functions are useful and what functions I will want to remove later because of bad design, when my other projects already depend on them. I want to have the good design right from the start so I do not need to change a lot of projects later on to use a changed API. \$\endgroup\$ – allo Feb 28 '20 at 14:56
  • 1
    \$\begingroup\$ I don't think this question is a good fit for CodeReview.SE. It's not really a review of code that has already been fully implemented and instead asks "Is this a good design?" before most of the code has actually been written. And while there undoubtedly is value to posing that question, I feel like it might be better suited to another site of the stack exchange network (possibly Programmers.SE?). \$\endgroup\$ – hoffmale Feb 28 '20 at 21:32
  • 1
    \$\begingroup\$ Again, have you had a look at Software Engineering (previously programmers.SE)? One of the focal points of that site is "requirements, software architecture and design", so your question would seem a lot more on topic there. \$\endgroup\$ – hoffmale Mar 1 '20 at 0:53
3
\$\begingroup\$

I encourage the use of interfaces, but this is not an interface. In an interface all the methods are public, and your code has private methods (which I'm not sure can actually compile given that they're private). The ideas of interfaces is that you only define the communication between objects, and don't define anything about the implementation details. What you have here is intended to be an abstract base class (assuming you change every private method to protected).

In most progress-bar interfaces I saw there weren't that many methods. Usually what you use is something like start(...), setProgress(double ...) and stop(), and pass whatever arguments you need in the method call. It's simpler to pass all the arguments that are needed in one call instead of knowing that you actually have to call setSomething and setSomethingElse before calling start().

Also keep in mind that one challenge of progress-bars are that they're usually a singleton (whether or not they follow the design pattern) so you need to think about how to handle many different objects accessing the same progress-bar. When you hold state, you're gonna have a lot of carry-on state from previous call and have unexpected behavior unless of course you complicate your progress-bar calls a little more by relying on clients to use some clear method.

I would look at your class from a YAGNI perspective. I don't think you should have one interface for progress-bars and for sending emails whenever a progress of something reaches some point. I don't think it's gonna be the progress-bar responsibility in most cases. In most cases you have some object performing some operation and updating the progress in the progress bar (completion% and message if needed), so that class would be the one sending emails. It would be much easier if you can subscribe to some event on the progress-bar to handle any progress changes. Events are a great tool when you want to be able to respond to something without adding responsibility to classes.

\$\endgroup\$
8
  • \$\begingroup\$ Sorry about the "interface" I thought more about the concept than the language feature. I reduced my existing code a lot to keep the question as open as possible and added the public/private while writing the question to make more clear what I intend to do. I do not need inheritance and may implement it using templates, but before fixing anything I need to know a good set of methods. \$\endgroup\$ – allo Feb 28 '20 at 14:32
  • \$\begingroup\$ My current projects needs to update a singleton progress bar from a blocking algorithm. I have one implementation that scripts a GUI progress bar with cancel button (disabled when the algorithm does not support it) and one that outputs one dot per percent on a console. Now I want to design a class that can be passed either as base class pointer or using templates to any future algorithm in which I need it. \$\endgroup\$ – allo Feb 28 '20 at 14:36
  • \$\begingroup\$ Unless I'm missing something, all the points remain. Use a simple interface with few simple methods. You don't need templates, and your domain objects don't need to know if they're updating the progress of some console window or some GUI application. It doesn't matter. You can have an option to enable a "cancel" button and to enable/disable it during the progress-bar run, but you don't want the progress-bar itself to be concerned with the reasons for enabling/disabling the button \$\endgroup\$ – asaf92 Feb 28 '20 at 15:02
  • 1
    \$\begingroup\$ Merging them into start is a good suggestion. Btw. should I add suggestions to the question when I like them or should they remain in answers / comments on this site? \$\endgroup\$ – allo Feb 28 '20 at 15:32
  • 1
    \$\begingroup\$ @allo Once you receive an answer to your question, you should not make any substantial edits to it as this can invalidate the answer. See What should I do when someone answers my question? \$\endgroup\$ – 1201ProgramAlarm Feb 28 '20 at 18:13
2
\$\begingroup\$

Methods should be const where possible (currentProgress, maxProgress, various "stop" related functions - but see below).

This class should have a constructor. In particular, besides having a default constructor, having one that can take the initial state (like max progress and/or an initial message) would streamline its use without having to call multiple set type functions.

Since your class is intended to be derived from, a virtual destructor, even if empty or defaulted, is necessary to avoid problems when destroying progress bars via a pointer to the base class.

Asking a progress bar if the current calculation or task should stop seems counter intuitive to me. Not all users of progress will need this capability, and it gets away from the Single Responsibility Principle. The progress bar should just handle progress. The calculation or task can handle the stopping. In some cases (like a GUI progress bar with a "stop" button) the progress class may need to keep track if a stop is requested. This can be handled by the derived class, in cooperation with the task that is using the Progress object. Should the stop be pushed to the task? Should the task query the progress to see if the stop button was hit? What about multiple threads processing the task?

\$\endgroup\$
2
  • \$\begingroup\$ The getters should of course be const. I probably do not want to use the constructor like the start function, as the object is a singleton representing the progress display. \$\endgroup\$ – allo Feb 29 '20 at 19:38
  • \$\begingroup\$ One of the implementations I use is scripting the Maya progress bar by running MEL commands from C++. The Maya API is not using callbacks for clicks on the cancel button, but you need to query if cancel was clicked. So my implementation for Maya calls the MEL command progressBar -query -isCanceled in my shouldStop() function. See the progressBar docs for details on this implementation. This is probably not the best design in the Maya API, but I need to be able to deal with it in my progress bar wrapper. \$\endgroup\$ – allo Feb 29 '20 at 19:51

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.