5
\$\begingroup\$

I have wrote code for a stopwatch that utilizes the abstract design pattern and would like to get some feedback on the code, so be as harsh as you can.

Note: I used ctime instead of chrono because this is not meant for benchmarking. The code will later be used in a console game, and the format of the std::tm struct is easy to work with.

Also, note that the way I wrote the tests in source.cpp will result in a small visual bug from time to time to resolve it lower the waiting time inside of the do while loop from std::chrono::seconds(1) to std::chrono::milliseconds(250); and increase the value of timer, the following action will increase the refresh rate.

EDIT: Note that this question is a followup to Watch that uses the abstract factory design pattern they both use the same Console, Constants and Digits library but both accomplish a different task

StopWatch.h:

#ifndef STOP_WATCH
#define STOP_WATCH
#include"Digits.h"
#include<vector>
#include<memory>
#include<ctime>

class StopWatch
{
public:
    virtual void printTime() = 0;
    void setStopWatchXY(int x, int y);
    bool countDownFrom(int seconds);
    void updateTime();
    void reset();
    void start();
    void stop();
    void lap();

    const std::vector<int>& getLapTimes() const;
    int getElapsed() const;

    virtual ~StopWatch() = default;

protected:
    int m_watchXPos;
    int m_watchYPos;
    int m_seconds;
    int m_minutes;
    int m_hours;

private:
    std::vector<int> m_lapTimes;
    bool             m_running{ false };
    int              m_elapsed{};
    int              m_beg;
    std::time_t      m_now;

    void converter(int seconds);
    void clearTime();
};

class DigitalStopWatch final : public StopWatch
{
public:
    virtual void printTime() override;

    explicit DigitalStopWatch(int x, int y)
    {
        setStopWatchXY(x, y);
    }
};

class SegmentedStopWatch final : public StopWatch
{
public:
    virtual void printTime() override;

    explicit SegmentedStopWatch(int x, int y)
    {
        setStopWatchXY(x, y);
    }

private:
    Digit m_stopWatchDigits[6];

    void printDigitAtLoc(Digit digArr[], int index, int x, int y) const;
    void printColon(int x, int y);
    void printSeconds();
    void printMinutes();
    void printHours();

    void set(Digit digArr[], int startIndex, int unit);
    void setDigitsToCurrentTime();
    void setSeconds();
    void setMinutes();
    void setHours();
};

class Factory
{
public:
    virtual std::unique_ptr<StopWatch> createStopWatch(int stopWatchXPos = 0, int stopWatchYPos = 0) const = 0;
};

class DigitalStopWatchFactory final : public Factory
{
    virtual std::unique_ptr<StopWatch> createStopWatch(int stopWatchXPos = 0, int stopWatchYPos = 0) const override
    {
        return std::make_unique<DigitalStopWatch>(stopWatchXPos, stopWatchYPos);
    }
};

class SegmentedStopWatchFactory final : public Factory
{
    virtual std::unique_ptr<StopWatch> createStopWatch(int stopWatchXPos = 0, int stopWatchYPos = 0) const override
    {
        return std::make_unique<SegmentedStopWatch>(stopWatchXPos, stopWatchYPos);
    }
};
#endif   

StopWatch.cpp:

#include"StopWatch.h"
#include"Console.h"
#include<thread> //for this_thread::sleep_for

namespace
{
    constexpr int maxTime          { 356400 };
    constexpr int digitPadding     { 5 };
    constexpr int secondsIndexStart{ 5 };
    constexpr int minutesIndexStart{ 3 };
    constexpr int timePadding      { 2 };
    constexpr int hoursIndexStart  { 1 };

    enum
    {
        First,
        Second,
        Third,
        Fourth,
        Fifth,
        Sixth
    };
}

/*|---STOP_WATCH_FUNCTIONS_START---|*/
/*|---PUBLIC_FUNCTIONS_START---|*/
void StopWatch::setStopWatchXY(int x, int y)
{
    Console::setXY(x, y, m_watchXPos, m_watchYPos);
}

bool StopWatch::countDownFrom(int seconds)
{
    if (seconds > maxTime) seconds = maxTime;
    while (seconds >= 0)
    {
        converter(seconds);
        printTime();
        if (seconds > 0)
        {
            std::this_thread::sleep_for(std::chrono::seconds(1));
        }//end of if
        --seconds;
    }//end of while
    return true;
}

void StopWatch::updateTime()
{
    long long curTimeInSec{ static_cast<long long>(std::time(&m_now)) - m_beg + m_elapsed };
    if (curTimeInSec > maxTime) curTimeInSec = 0;
    converter(curTimeInSec);
}

void StopWatch::reset()
{
    m_running = false;
    m_lapTimes.clear();
    m_lapTimes.shrink_to_fit();
    clearTime();
}

void StopWatch::start()
{
    if (!m_running)
    {
        m_beg = static_cast<long long>(std::time(&m_now));
        m_running = true;
    }//end of if
}

void StopWatch::stop()
{
    if (m_running)
    {
        m_elapsed += static_cast<long long>(std::time(&m_now)) - m_beg;
        m_running = false;
    }//end of if
}

void StopWatch::lap()
{
    if (m_running)
    {
        stop();
        m_lapTimes.emplace_back(m_elapsed);
        clearTime();
        start();
    }//end of if
}

const std::vector<int>& StopWatch::getLapTimes() const
{
    return m_lapTimes;
}

int StopWatch::getElapsed() const
{
    return m_elapsed;
}
/*|----PUBLIC_FUNCTIONS_END----|*/

/*|---PRIVATE_FUNCTIONS_START---|*/
void StopWatch::converter(int seconds)
{
    m_hours   = seconds / 3600;
    seconds   = seconds % 3600;
    m_minutes = seconds / 60;
    m_seconds = seconds % 60;
}

void StopWatch::clearTime()
{
    m_elapsed = 0;
    m_seconds = 0;
    m_minutes = 0;
    m_hours   = 0;
}
/*|----PRIVATE_FUNCTIONS_END----|*/
/*|----STOP_WATCH_FUNCTIONS_END----|*/

/*|---DIGITAL_STOP_WATCH_FUNCTIONS_START---|*/
/*|---PUBLIC_FUNCTIONS_START---|*/
/*|---VIRTUAL_FUNCTIONS_START---|*/
void DigitalStopWatch::printTime() 
{
    Console::gotoxy(m_watchXPos, m_watchYPos);
    if (m_hours < 10) std::cout << '0';
    std::cout << m_hours << ':';
    if (m_minutes < 10) std::cout << '0';
    std::cout << m_minutes << ':';
    if (m_seconds < 10) std::cout << '0';
    std::cout << m_seconds;
}
/*|----VIRTUAL_FUNCTIONS_END----|*/
/*|----PUBLIC_FUNCTIONS_END----|*/
/*|----DIGITAL_STOP_WATCH_FUNCTIONS_END----|*/

/*|---SEGMENTED_STOP_WATCH_FUNCTIONS_START---|*/
/*|---PUBLIC_FUNCTIONS_START---|*/
/*|---VIRTUAL_FUNCTIONS_START---|*/
void SegmentedStopWatch::printTime() 
{
    setDigitsToCurrentTime();
    printHours();
    printColon(m_watchXPos + 10, m_watchYPos);
    printMinutes();
    printColon(m_watchXPos + 22, m_watchYPos);
    printSeconds();
}
/*|----VIRTUAL_FUNCTIONS_END----|*/
/*|----PUBLIC_FUNCTIONS_END----|*/

/*|---PRIVATE_FUNCTIONS_START---|*/
void SegmentedStopWatch::printDigitAtLoc(Digit digArr[], int index, int x, int y) const
{
    digArr[index].setDigitXY(x + index * digitPadding, y);
    digArr[index].printDigit();
}

void SegmentedStopWatch::printColon(int x, int y)
{
    Console::putSymbol(x, y + 1, '.');
    Console::putSymbol(x, y + 2, '.');
}

void SegmentedStopWatch::printSeconds()
{
    printDigitAtLoc(m_stopWatchDigits, Fifth, m_watchXPos + timePadding * 2, m_watchYPos);
    printDigitAtLoc(m_stopWatchDigits, Sixth, m_watchXPos + timePadding * 2, m_watchYPos);
}

void SegmentedStopWatch::printMinutes()
{
    printDigitAtLoc(m_stopWatchDigits, Third, m_watchXPos + timePadding, m_watchYPos);
    printDigitAtLoc(m_stopWatchDigits, Fourth, m_watchXPos + timePadding, m_watchYPos);
}

void SegmentedStopWatch::printHours()
{
    printDigitAtLoc(m_stopWatchDigits, First, m_watchXPos, m_watchYPos);
    printDigitAtLoc(m_stopWatchDigits, Second, m_watchXPos, m_watchYPos);
}

void SegmentedStopWatch::set(Digit digArr[], int startIndex, int unit)
{
    if (unit < 10) digArr[startIndex - 1] = 0;
    else digArr[startIndex - 1] = unit / 10;
         digArr[startIndex]     = unit % 10;
}

void SegmentedStopWatch::setDigitsToCurrentTime()
{
    setHours();
    setMinutes();
    setSeconds();
}

void SegmentedStopWatch::setSeconds()
{
    set(m_stopWatchDigits, secondsIndexStart, m_seconds);
}

void SegmentedStopWatch::setMinutes()
{
    set(m_stopWatchDigits, minutesIndexStart, m_minutes);
}

void SegmentedStopWatch::setHours()
{
    set(m_stopWatchDigits, hoursIndexStart, m_hours);
}
/*|----PRIVATE_FUNCTIONS_END----|*/
/*|----SEGMENTED_STOP_WATCH_FUNCTIONS_END----|*/

Source.cpp:

#include"StopWatch.h"
#include"Console.h" //for Console::gotoxy
#include<thread>   //for this_thread::sleep_for

int main()
{
    std::unique_ptr<Factory> segFact{ std::make_unique<SegmentedStopWatchFactory>() };
    std::unique_ptr<Factory> digFact{ std::make_unique<DigitalStopWatchFactory>() };

    std::unique_ptr<StopWatch> stoppers[2];
    stoppers[0] = segFact->createStopWatch();
    stoppers[1] = digFact->createStopWatch();

    stoppers[0]->setStopWatchXY(5, 5);

    //to test the second stopper simply change stoppers[0] to stoppers[1]
    stoppers[0]->countDownFrom(12); //test countdown
    //stoppers[0]->countDownFrom(60 * 60 * 60 * 60); //overflow test
    /*
    stoppers[0]->start();
    while (1)
    {
        stoppers[0]->updateTime();
        stoppers[0]->printTime();
        std::this_thread::sleep_for(std::chrono::milliseconds(200)); //this is only responsible for the refresh rate, the lower the better
    }//note that no waiting at all will result in visible reprinting.
    */
    /*
    int timer = 9; 
    stoppers[0]->start();
    do
    {
        stoppers[0]->updateTime();
        stoppers[0]->printTime();
        std::this_thread::sleep_for(std::chrono::seconds(1));
    }while (--timer);
    stoppers[0]->stop(); //test stop
    std::this_thread::sleep_for(std::chrono::seconds(3));
    timer = 9;
    stoppers[0]->start();
    do
    {
        stoppers[0]->updateTime();
        stoppers[0]->printTime();
        std::this_thread::sleep_for(std::chrono::seconds(1));
    }while (--timer);
    */
    /*
    int timer = 9;
    stoppers[0]->start();
    do
    {
        stoppers[0]->updateTime();
        stoppers[0]->printTime();
        std::this_thread::sleep_for(std::chrono::seconds(1));
    }while (--timer);
    stoppers[0]->reset(); //test reset
    std::this_thread::sleep_for(std::chrono::seconds(3));
    timer = 9;
    stoppers[0]->start();
    do
    {
        stoppers[0]->updateTime();
        stoppers[0]->printTime();
        std::this_thread::sleep_for(std::chrono::seconds(1));
    }while (--timer);
    */
    /*
    int timer = 9;
    stoppers[0]->start();
    do
    {
        stoppers[0]->updateTime();
        stoppers[0]->printTime();
        std::this_thread::sleep_for(std::chrono::seconds(1));
    } while (--timer);
    stoppers[0]->lap(); //lap reset
    stoppers[0]->stop();
    std::this_thread::sleep_for(std::chrono::seconds(3));
    timer = 9;
    stoppers[0]->start();
    do
    {
        stoppers[0]->updateTime();
        stoppers[0]->printTime();
        std::this_thread::sleep_for(std::chrono::seconds(1));
    } while (--timer);
    stoppers[0]->lap();
    stoppers[0]->stop();
    Console::gotoxy(0, 0);
    std::cout << "m_elapsed: " << stoppers[0]->getElapsed() << '\n';
    std::cout << "First lap: " << stoppers[0]->getLapTimes()[0] << '\n';
    std::cout << "Second lap: " << stoppers[0]->getLapTimes()[1] << '\n';
    */
    return 0;
}
\$\endgroup\$
  • \$\begingroup\$ can you please explain how it this a duplicate ? I wrote both the watch and the stop watch and the code is completely different for both of them so why the downvote ? \$\endgroup\$ – globalturist Jul 2 '17 at 18:30
  • \$\begingroup\$ the only thing in common is the digits.h and Console.h but StopWatch.h is nothing like watch.h it even has different functions and implementations, note that it doesn't even have a singleton class like the watch.h has \$\endgroup\$ – globalturist Jul 2 '17 at 18:32
  • \$\begingroup\$ Please i ask you to compare StopWatch.h, StopWatch.cpp and source.cpp to the link you marked as a duplicate (Watch.h, watch.cpp and source.cpp) and tell me if both of those codes are exactly the same (which they are not) \$\endgroup\$ – globalturist Jul 2 '17 at 18:38
  • \$\begingroup\$ So can't you handle these variants being reviewed together with your previous question, or at least state that this one is a follow-up (with a link), and leave out the duplicated code? \$\endgroup\$ – πάντα ῥεῖ Jul 2 '17 at 18:52
  • \$\begingroup\$ @πάντα ῥεῖ Sorry about that i'm pretty new here and i didn't see anything written about this case specifically in the rules i will have it in mind next time, as for why i left the duplicate code in here (Console, Constants and digits libraries) I thought that if someone searches for a ready made solution he would want to use all of the code and not only the non duplicate, as to address the followup I don't see the codes as the same, yes they use the same design pattern but they Work completely differently and i wanted to know if i implemented the pattern correctly in both cases. \$\endgroup\$ – globalturist Jul 2 '17 at 19:01
8
\$\begingroup\$

Okay, after I worked my way through the original code, a few things have become clearer. Since I have never done programming with ncurses I was eager to try my hand at a better design.

Here it comes. It's a sketch only in the sense that I didn't create separate translation units. That is basically a tedious exercise and left for the reader.

However, it does implement stopwatch (including lap times and reset), countdown and a bonus "random timer" task.


Big ideas

I noticed that "Stopwatch" was a bit of a "God Object" antipattern. It does too many things (you can't really usefully do a countdown and a lap time simultaneously).¹

I reckoned it would be nice to simply have tasks (without any UI) that expose duration measurements, and views that can display them as they update. This is akin to a publish/subscribe pattern.

To demonstrate this, I made not only the views generic, but also the tasks.

The UI will be an interactive terminal application that supports the following short cut keys:

           Select (multiple) views: [d]igital/[s]egmented
        Launch task: [r]andom [c]ountdown s[t]opwatch [?]any
                Control tasks: [l]ap [!]reset [k]ill
                 Other: [z]ap all views [q]uit/Esc

¹ I am aware of the usual implementation in hardware stopwatch devices where the operation modes form a state machine. I also realize that the implementation in code tried to mimick this. Unfortunately, not only did it fall short, it also conflated things with the UI side of things. Consider this answer a finger exercise on my part.


Code walkthrough

Includes

#include <iostream>
#include <sstream>
#include <iomanip>

We'll be using stream formatting to display hh:mm:ss times.

#include <chrono>
using namespace std::chrono_literals;
#include <memory>
#include <random>
#include <algorithm>
#include <set>

We'll be using standard library containers and algorithms.

#include <boost/signals2.hpp>

A little bit of Boost to aid in the publish/subscribe mechanism. It facilitates multi-cast subscription and automatic (RAII) disconnection.

#include <cursesapp.h>
#include <cursesp.h>

As in the last comment on the old answer, we'll be using ncurses to create an interactive terminal UI (TUI).


Preamble/general declarations

We start at the foundation, some general utilities:

namespace {
    using Clock     = std::chrono::steady_clock;
    using TimePoint = Clock::time_point;
    using Duration  = Clock::duration;
    using Durations = std::vector<Duration>;

Just some convenience shorthands, so we keep our code legible and easy to change.

    struct HMS { 
        int hours, minutes, seconds;

        std::string str() const {
            std::stringstream ss;
            ss << std::setfill('0') << std::setw(2) << hours << ':' << std::setw(2) << minutes << ':' << std::setw(2)
               << seconds;
            return ss.str();
        }
    };

    HMS to_hms(Duration duration) {
        auto count = std::chrono::duration_cast<std::chrono::seconds>(duration).count();
        int s = count % 60; count /= 60;
        int m = count % 60; count /= 60;
        int h = count;
        return {h,m,s};
    }

You had these conversions anyways, but here they are in "functional style" (specifically, side-effect free). You'll notice how much this uncomplicates the stopwatch tasks below.

    using boost::signals2::signal;
    using boost::signals2::scoped_connection;
}

(More shorthands)

There's a subtle point to maintaining Duration at full resolution internally. This means that no rounding errors are introduced when starting a new lap "halfway" a second.


The "Task" hierarchy

struct Task {
    virtual ~Task() = default;
    virtual void tick() {}
};

The foundation of our task class hierarchy. Note that the most generic base class doesn't even presuppose any published events, which makes the framework extensible to tasks other than time measurement. Our time-related tasks share the following abstract base:

struct TimerTask : Task {
    signal<void(Duration, Durations const&)> updateEvent;

    virtual void tick() = 0;
};

As you can see, we promise to publish events carrying one or more durations. These can be subscribed to by any view capable of displaying one or more durations.

Implementing the various timer operations on this is peanuts. Let's for example do a random durations generator in 3 lines of code:

struct RandomTimeTask : TimerTask {
    virtual void tick() {
        updateEvent(rand()%19000 * 1s, {});
    }
};

That might seem a cheap example, but countdown is really not much different:

struct CountdownTask : TimerTask {
    CountdownTask(Duration d) : _deadline(Clock::now() + d) { }

    virtual void tick() {
        updateEvent(std::max(Duration{0s}, _deadline - Clock::now()), {});
    }

  private:
    TimePoint _deadline;
};

Even stopwatch isn't harder, before we add lap times:

struct StopwatchTask : TimerTask {
    StopwatchTask() : _startlap(Clock::now()) { }

    virtual void tick() {
        updateEvent(Clock::now() - _startlap, {});
    }

  private:
    TimePoint _startlap;
};

Adding laptimes and reset() the full-featured stopwatch task becomes:

struct StopwatchTask : TimerTask {
    StopwatchTask() : _startlap(Clock::now()) { }

    virtual void tick() {
        updateEvent(elapsed(), _laptimes);
    }

    void lap() {
        _laptimes.push_back(elapsed());
        _startlap = Clock::now();
    }

    void reset() {
        _startlap = Clock::now();
        _laptimes.clear();
    }

  private:
    Duration elapsed() const { return Clock::now() - _startlap; }
    std::vector<Duration> _laptimes;
    TimePoint _startlap;
};

View hierarchy

We introduce one more parameter object:

struct Bounds {
    int x, y, lines, cols;
};

And then we get down to business: Views will be things that have a TUI element (a panel):

struct View {
    View(Bounds const& bounds) : _window(bounds.lines, bounds.cols, bounds.x, bounds.y) { }
    virtual ~View() = default;

  protected:
    mutable NCursesPanel _window;
};

Without delay, let's present the abstract base TimerView that subscribes to any TimerTask:

struct TimerView : View {

    TimerView(Bounds const& bounds) : View(bounds) { }

    void subscribe(TimerTask& task) {
        _conn = task.updateEvent.connect([this](Duration const& value, Durations const& extra) {
            //if (value == 0s) _window.setcolor(1);
            update(value, extra);
        });
    }

  private:
    scoped_connection _conn;
    virtual void update(Duration const& value, Durations const& extra) const = 0;
};

I haven't figured out how to configure colors in Curses, but you can see how simple it would be to add generic behaviour to timer views there.

A simple view to implement would be the digital timer view. We already have the to_hms() and HMS::str() utilities. Let's decide that the view should show the total elapsed time regardless of lap-times and it should indicate how many laps have been recorded:

struct DigitalView final : TimerView {

    using TimerView::TimerView;

  private:
    void update(Duration const& value, Durations const& extra) const override {
        _window.erase();

        auto total = std::accumulate(extra.begin(), extra.end(), value);
        auto hms = to_hms(total).str();
        hms += "[#" + std::to_string(extra.size()) + "]";

        int x = 0;
        for (auto ch : hms)
            _window.CUR_addch(0, x++, ch);

        _window.redraw();
    }
};

Note that I have foregone a Console like abstraction here. Instead I used _window methods directly, which does tie the implementation to Curses. You may want to abstract this again, probably using a buffered approach like in my other answer.

The SegmentView isn't actually much more complicated. I dropped the bitset<> in favour of more obviously readable code. It has the added benefit of making the code short (except for the constants of course).

Functionally, we show

  • the total elapsed time in a frame caption
  • the current lap time in the 7-segment large display
  • a running list of previous lap-times on the right hand side:

    enter image description here

struct SegmentView final : TimerView {

    using TimerView::TimerView;

  private:
    void update(Duration const& value, Durations const& extra) const override {
        _window.erase();

        // total times in frame caption
        {
            auto total = std::accumulate(extra.begin(), extra.end(), value);
            _window.frame(to_hms(total).str().c_str());
        }

        // big digits show current lap
        {
            auto hms = to_hms(value).str();
            int digits[6] {
                hms[0]-'0', hms[1]-'0',
                    hms[3]-'0', hms[4]-'0',
                    hms[6]-'0', hms[7]-'0',
            };

            auto xpos = [](int index) { return 4 + (index * 5); };

            int index = 0;
            for (auto num : digits)
                printDigit(num, xpos(index++), 1);

            for (auto x : {xpos(2)-1, xpos(4)-1})
                for (auto y : {2, 4})
                    _window.CUR_addch(y, x, '.');
        }

        // previous laptimes to the right
        {
            // print lap times
            int y = 1;
            for (auto& lap : extra) {
                int x = 35;
                _window.CUR_addch(y, x++, '0'+y);
                _window.CUR_addch(y, x++, '.');
                _window.CUR_addch(y, x++, ' ');
                for (auto ch : to_hms(lap).str())
                    _window.CUR_addch(y, x++, ch);
                ++y;
            }
        }

        _window.redraw();
    }

    void printDigit(int num, int x, int y) const
    {
        static char const* const s_masks[10] = {
            " -- "
            "|  |"
            "    "
            "|  |"
            " -- ",
            "    "
            "   |"
            "    "
            "   |"
            "    ",
            " -- "
            "   |"
            " -- "
            "|   "
            " -- ",
            " -- "
            "   |"
            " -- "
            "   |"
            " -- ",
            "    "
            "|  |"
            " -- "
            "   |"
            "    ",
            " -- "
            "|   "
            " -- "
            "   |"
            " -- ",
            " -- "
            "|   "
            " -- "
            "|  |"
            " -- ",
            " -- "
            "   |"
            "    "
            "   |"
            "    ",
            " -- "
            "|  |"
            " -- "
            "|  |"
            " -- ",
            " -- "
            "|  |"
            " -- "
            "   |"
            " -- ",
        };

        if (num < 0 || num > 9)
            throw std::runtime_error("Cannot assign Invalid digit must be: (0 < digit < 9)");

        for (auto l = s_masks[num]; *l; l += 4) {
            for (auto c = l; c < l+4; ++c)
                _window.CUR_addch(y, x++, *c);
            ++y; x-=4;
        }
    }
};

Behold, a thing of beauty. One should never underestimate the value of self-explanatory code (if only the constants here).

The Main Application

All that remains to be done is the demo application itself. It sets up some factories, and starts an input event loop to receive keyboard shortcuts. Handling them is pretty straightforward.

The stopwatch-specific operations ([l]ap and [!]reset) are the only mildly complicated ones because they will have to filter for any running tasks that might be stopwatch tasks.

Launching a task without selecting one or more views to connect up will result in a beep() and nothing happening.

Note how clearing the _tasks or _views automatically destroys the right subscriptions (due to the use of scoped_connection).

struct DemoApp : NCursesApplication {

    using TaskPtr = std::unique_ptr<TimerTask>;
    using ViewPtr = std::unique_ptr<TimerView>;

    using TaskFactory = std::function<TaskPtr()>;
    using ViewFactory = std::function<ViewPtr()>;

    using ViewFactoryRef = std::reference_wrapper<ViewFactory const>;

    int run() {
        _screen.useColors();
        _screen.centertext(lines - 4, "Select (multiple) views: [d]igital/[s]egmented");
        _screen.centertext(lines - 3, "Launch task: [r]andom [c]ountdown s[t]opwatch [?]any");
        _screen.centertext(lines - 2, "Control tasks: [l]ap [!]reset [k]ill");
        _screen.centertext(lines - 1, "Other: [z]ap all views [q]uit/Esc");
        ::timeout(10);

        // replace with set<> to disallow repeats
        std::multiset<ViewFactoryRef, CompareAddresses> selected_views;

        while (true) {
            TaskPtr added;

            switch (auto key = ::CUR_getch()) { // waits max 10ms due to timeout
                case 'r': added = makeRandomTimeTask(); break;
                case 'c': added = makeCountdownTask(); break;
                case 't': added = makeStopwatchTask(); break;
                case 'l': 
                  for (auto& t : _tasks) 
                      if (auto sw = dynamic_cast<StopwatchTask*>(t.get()))
                          sw->lap();
                  break;
                case '!': 
                  for (auto& t : _tasks) 
                      if (auto sw = dynamic_cast<StopwatchTask*>(t.get()))
                          sw->reset();
                  break;
                case '\x1b': case 'q': return 0;
                case 'k': _tasks.clear(); break;
                case 'z': _views.clear(); break;
                case 'd': selected_views.insert(makeDigitalView); break;
                case 's': selected_views.insert(makeSegmentView); break;
                default:
                    for (auto& d : _tasks) d->tick();
            }

            if (added) {
                if (selected_views.empty())
                    ::beep();
                else {
                    for (ViewFactory const& maker : selected_views) {
                        _views.push_back(maker());
                        _views.back()->subscribe(*added);
                    }
                    selected_views.clear();
                    _tasks.push_back(std::move(added));
                }
            }

            ::refresh();
        }
    }

The remainder is the setup for the DemoApp, which includes rather boring stuff like generating random view positions.

  private:
    std::mt19937 prng{ std::random_device{}() };

    NCursesPanel _screen;
    int const lines = _screen.lines();
    int const cols  = _screen.cols();

    int ranline() { return std::uniform_int_distribution<>(0, lines - 9)(prng); }; 
    int rancol()  { return std::uniform_int_distribution<>(0, cols  - 9)(prng); }; 

    TaskFactory 
        makeRandomTimeTask = [] { return TaskPtr(new RandomTimeTask); },
        makeCountdownTask  = [] { return TaskPtr(new CountdownTask(rand()%240 * 1s)); },
        makeStopwatchTask  = [] { return TaskPtr(new StopwatchTask()); },
        taskFactories[2] {
            makeRandomTimeTask,
            makeCountdownTask,
        };

    ViewFactory 
        makeDigitalView = [&] { return std::make_unique<DigitalView>(Bounds { ranline(), rancol(), 1, 13 }); },
        makeSegmentView = [&] { return std::make_unique<SegmentView>(Bounds { ranline(), rancol(), 7, 48 }); },
        viewFactories[2] = { makeDigitalView, makeSegmentView };

    std::vector<ViewPtr> _views;
    std::vector<TaskPtr> _tasks;

    struct CompareAddresses { // in case you wanted to make selected_views unique
        template <typename A, typename B>
        bool operator()(A const& a, B const& b) const { return std::addressof(a.get())<std::addressof(b.get()); };
    };
};

static DemoApp app;

You can see a "live" demo here:

enter image description here

\$\endgroup\$
  • \$\begingroup\$ Once again impressive implementation ! But there is only one catch that i didn't mention in the question - The stopwatch is not going to be a standalone application, it's going to be a part of a bigger program i'm working on, and second you implemented it with an observer/builder pattern and while you did make it simpler i wanted to practice the abstract factory pattern which is absent here, lastly i wanted to mention that i didn't use chrono from what is mentioned in the question (but from the example you gave i should really consider it) \$\endgroup\$ – globalturist Jul 4 '17 at 8:56
  • \$\begingroup\$ BTW about the bigger project i don't use Ncurses on porpuse because i intend on making a blog about the project and than when the project is done extend more blog posts dedicated to updating the full project to use Ncurses. \$\endgroup\$ – globalturist Jul 4 '17 at 8:59
  • \$\begingroup\$ Just a single question you said "(you can't really usefully do a countdown and a lap time simultaneously)" why would you ? I mean if we apply this to real life, you have a stopwatch it can do a countdown or count laps but not both at the same time (at least the common once). In short can you explain how i could make use of such a feature ? \$\endgroup\$ – globalturist Jul 4 '17 at 15:48
  • \$\begingroup\$ The abstract factory isn't absent. You just don't recognize it because I spelled it differently. std::function<> is exactly a 1-method virtual interface. Like I explain in the question, I made more abstract factories [kind of demonstrating the use]. I'm glad you noticed how Duration simplifies the operations. \$\endgroup\$ – sehe Jul 4 '17 at 20:38
  • \$\begingroup\$ About your "single question": that's the whole point. The fact that these things aren't compatible is the key reason for not conflating the implementations. In your question, the Stopwatch class does stopwatch timing, countdown, formatting, time conversions, rounding, display, program flow. In my design a "StopWatchDevice" would simply hold one "StopwatchTask|CountdownTask|IdleTask" at a time. And it would still not be responsible for program flow nor UI. \$\endgroup\$ – sehe Jul 4 '17 at 20:38
8
\$\begingroup\$

A few random notes.

I don't like the mix between "object oriented" and "procedural" here.

  • If a Digit is supposed to "have a" position, then in no way should it be incrementing the y of that position during print (the digit doesn't move!). And, obviously, at print time, the Digit should not have to be reminded of its position each time (the position is a property of the digit).

  • While we're at it, make a type

    struct Position { int x, y; };
    
  • Right now the SegmentedStopWatch has all the logic for printing each digit at a particular location, and it's all spread out across many helper functions. They mostly add noise. (I already mentioned how it's useless to store imperative state into Digits just-in-time to "fake" object-oriented digits that "have-a" location.)

  • Likewise, the code separated into these unclearly named subfunctions with magically named arguments:

    void SegmentedStopWatch::set(Digit digArr[], int startIndex, int unit)
    {
        if (unit < 10) digArr[startIndex - 1] = 0;
        else digArr[startIndex - 1] = unit / 10;
             digArr[startIndex]     = unit % 10;
    }
    
    void SegmentedStopWatch::setDigitsToCurrentTime()
    {
        setHours();
        setMinutes();
        setSeconds();
    }
    
    void SegmentedStopWatch::setSeconds()
    {
        set(m_stopWatchDigits, secondsIndexStart, m_seconds);
    }
    
    void SegmentedStopWatch::setMinutes()
    {
        set(m_stopWatchDigits, minutesIndexStart, m_minutes);
    }
    
    void SegmentedStopWatch::setHours()
    {
        set(m_stopWatchDigits, hoursIndexStart, m_hours);
    }
    

    Could be more readily expressed as:

    void SegmentedStopWatch::setCurrentTime()
    {
        auto set = [this](int startIndex, int unit) {
            m_digits[startIndex - 1] = (unit < 10)? 0 : unit/10;
            m_digits[startIndex]     = unit % 10;
        };
        set(5, m_seconds);
        set(3, m_minutes);
        set(1, m_hours);
    }
    

    This avoids all the confusion (why was set a member of the class? what does it do? etc.)

  • Although a bit more contrived, you can likewise make printDigit selfcontained:

    void Digit::printDigit() const
    {
        int segment = 0;
        auto dash  = [&] { return m_segmentMask.test(segment++)? '-':' '; };
        auto bar   = [&] { return m_segmentMask.test(segment++)? '|':' '; };
    
        Position cursor = m_pos;
        auto print  = [&](std::initializer_list<char> chars) {
            s_console.gotoxy(cursor);
            for (auto& ch : chars) s_console.putSymbol(ch);
            ++cursor.y; ;
        };
    
        print({' ', dash(), dash(), ' '});
        print({bar(), ' ', ' ', bar()});
        print({' ', dash(), dash(), ' '});
        print({bar(), ' ', ' ', bar()});
        print({' ', dash(), dash(), ' '});
    }
    
  • The segment bitmasks are "magic", but can be made a lot less "magical" by spelling them out as bits:

    static int const s_masks[] = {
        0b11'1100'1111, 0b00'1000'1000, 0b11'0111'1011, 0b11'1011'1011, 0b00'1011'1100, 
        0b11'1011'0111, 0b11'1111'0111, 0b00'1000'1011, 0b11'1111'1111, 0b11'1011'1111, 
    };
    
    Digit& Digit::operator=(int num)
    {
        m_segmentMask = (num >= 0 && num <= 9)? s_masks[num] : throw "Cannot assign Invalid digit must be: (0 < digit < 9)";
        return *this;
    }
    
  • All the comments seem very noisy

    /*|----PUBLIC_FUNCTIONS_END----|*/
    /*|---PRIVATE_FUNCTIONS_START---|*/
    }//end of if
    }//end of while
    

    There's a golden rule: if the code needs comments like that, you need to clean up the code. Those comments will only get out of sync.

  • Avoid 2-phase initialization. Instead of doing setStopWatchXY(x,y) after constructing, construct directly with

    explicit DigitalStopWatch(Position where) : StopWatch(where) {}
    
  • Avoid global instances (Console)

  • Use exception handling instead of randomly printing messages to std::cout

  • Use std::cerr for error reporting

  • Avoid polluting the global namespace with disputable utilities:

    enum
    {
        First,
        Second,
        Third,
        Fourth,
        Fifth,
        Sixth
    };
    

    At the very least make it function-local scope, or consider enum class.

DEMO TIME

What follows is a crude adaptation to standard linux (no ncurses for brevity). It ends up with a pretty elegant SegmentedStopWatch:

class SegmentedStopWatch final : public StopWatch
{
public:
    virtual void printTime() override;

    explicit SegmentedStopWatch(Position);

private:
    using Digits = std::array<Digit, 6>;
    std::array<Digit, 6>    m_digits;
    std::array<Position, 4> m_colons;

    void printColon(int x, int y);
    void setCurrentTime();
};

Most importantly, printTime() is as short as:

void SegmentedStopWatch::printTime() 
{
    setCurrentTime();
    for (auto& digit : m_digits) digit.printDigit();
    for (auto& colon : m_colons) s_console.putSymbol(colon, '.');
}

That's because, as mentioned above, the digits know where they are. The constructor actually initializes their positions, as well as the location of the dots for the colons:

SegmentedStopWatch::SegmentedStopWatch(Position where) : StopWatch(where)
{ 
    auto left = m_pos.x + 2*timePadding;
    auto xpos = [left](int index) { return left + (index * digitPadding) + (index/2 * 3); };

    int index = 0;
    for (auto& digit : m_digits)
        digit.setPosition({xpos(index++), m_pos.y});

    m_colons = { {
            { xpos(2) - 2, m_pos.y + 1 },
            { xpos(2) - 2, m_pos.y + 3 },
            { xpos(4) - 2, m_pos.y + 1 },
            { xpos(4) - 2, m_pos.y + 3 },
        } };
}

Full Listing

Live On Coliru

#ifndef CONSOLE_GRAPHICS_H
#define CONSOLE_GRAPHICS_H

#include<utility>
#include<array>
#include<iostream>

struct Position {
    int x, y;
};

template <int Rows = 15, int Columns = 80>
struct Console {
    Console() {
        std::cout.setf(std::ios::unitbuf);
        std::fill(begin(buffer.front()), end(buffer.back()), ' ');
    }

    void putSymbol(Position pos, const char symbol) {
        buffer.at(pos.y).at(pos.x) = symbol;
        gotoxy(pos);
    }

    void putSymbol(const char symbol) {
        putSymbol(curPos, symbol);
        curPos = { curPos.x + 1, curPos.y };
    }

    void gotoxy(Position pos) {
        curPos = { pos.x % Columns, pos.y % Rows };
    }

    void clrScr() {
        std::fill(begin(buffer.front()), end(buffer.back()), ' ');
        gotoxy({0,0});
        render();
    }

    void render() {
        for (auto& line : buffer) {
            std::cout << std::string(line.begin(), line.end()) << "\n";
        }
    }

  private:
    std::array<std::array<char, Columns>, Rows> buffer;
    Position curPos = { 0, 0 };
};

static Console<> s_console;

#endif 

#ifndef DIGITS_H
#define DIGITS_H
#include<algorithm>
#include<bitset>

class Digit
{
  public:
    explicit Digit(Position where = {0,0}) : m_pos(where)
    { }

    void setPosition(Position);
    Digit& operator=(int num);
    void printDigit() const;

  private:
    std::bitset<10> m_segmentMask;
    Position        m_pos;
};

#endif

void Digit::setPosition(Position where) {
    m_pos = where;
}

static int const s_masks[] = {
    0b11'1100'1111, 0b00'1000'1000, 0b11'0111'1011, 0b11'1011'1011, 0b00'1011'1100, 
    0b11'1011'0111, 0b11'1111'0111, 0b00'1000'1011, 0b11'1111'1111, 0b11'1011'1111, 
};

Digit& Digit::operator=(int num)
{
    m_segmentMask = (num >= 0 && num <= 9)
        ? s_masks[num] 
        : throw std::runtime_error("Cannot assign Invalid digit must be: (0 < digit < 9)");
    return *this;
}

void Digit::printDigit() const
{
    int segment = 0;
    auto dash  = [&] { return m_segmentMask.test(segment++)? '-':' '; };
    auto bar   = [&] { return m_segmentMask.test(segment++)? '|':' '; };

    Position cursor = m_pos;
    auto print  = [&](std::initializer_list<char> chars) {
        s_console.gotoxy(cursor);
        for (auto& ch : chars) s_console.putSymbol(ch);
        ++cursor.y; ;
    };

    print({' ', dash(), dash(), ' '});
    print({bar(), ' ', ' ', bar()});
    print({' ', dash(), dash(), ' '});
    print({bar(), ' ', ' ', bar()});
    print({' ', dash(), dash(), ' '});
}

#ifndef STOP_WATCH
#define STOP_WATCH
#include<vector>
#include<memory>
#include<ctime>

class StopWatch
{
public:
    virtual void printTime() = 0;
    void setPosition(Position where) { m_pos = where; }
    bool countDownFrom(int seconds);
    void updateTime();
    void reset();
    void start();
    void stop();
    void lap();

    const std::vector<int>& getLapTimes() const;
    int getElapsed() const;

    StopWatch(Position where = {0,0}) : m_pos(where) { }

    virtual ~StopWatch() = default;

protected:
    Position m_pos;
    int m_seconds;
    int m_minutes;
    int m_hours;

private:
    std::vector<int> m_lapTimes;
    bool             m_running{ false };
    int              m_elapsed{};
    int              m_beg;
    std::time_t      m_now;

    void converter(int seconds);
    void clearTime();
};

class DigitalStopWatch final : public StopWatch
{
public:
    virtual void printTime() override;

    explicit DigitalStopWatch(Position where) : StopWatch(where) {}
};

class SegmentedStopWatch final : public StopWatch
{
public:
    virtual void printTime() override;

    explicit SegmentedStopWatch(Position);

private:
    using Digits = std::array<Digit, 6>;
    std::array<Digit, 6>    m_digits;
    std::array<Position, 4> m_colons;

    void printColon(int x, int y);
    void setCurrentTime();
};

class Factory
{
public:
    virtual std::unique_ptr<StopWatch> createStopWatch(Position where = {0,0}) const = 0;
};

class DigitalStopWatchFactory final : public Factory
{
    virtual std::unique_ptr<StopWatch> createStopWatch(Position where) const override
    {
        return std::make_unique<DigitalStopWatch>(where);
    }
};

class SegmentedStopWatchFactory final : public Factory
{
    virtual std::unique_ptr<StopWatch> createStopWatch(Position where) const override
    {
        return std::make_unique<SegmentedStopWatch>(where);
    }
};
#endif   

#include<thread>
#include<iomanip>
#include<sstream>

namespace
{
    constexpr int maxTime      { 356400 };
    constexpr int digitPadding { 5 };
    constexpr int timePadding  { 2 };
}

bool StopWatch::countDownFrom(int seconds)
{
    if (seconds > maxTime) seconds = maxTime;
    while (seconds >= 0)
    {
        converter(seconds);
        printTime();
        s_console.render();
        if (seconds > 0)
        {
            std::this_thread::sleep_for(std::chrono::seconds(1));
        }
        --seconds;
    }
    return true;
}

void StopWatch::updateTime()
{
    long long curTimeInSec{ static_cast<long long>(std::time(&m_now)) - m_beg + m_elapsed };
    if (curTimeInSec > maxTime) curTimeInSec = 0;
    converter(curTimeInSec);
}

void StopWatch::reset()
{
    m_running = false;
    m_lapTimes.clear();
    m_lapTimes.shrink_to_fit();
    clearTime();
}

void StopWatch::start()
{
    if (!m_running)
    {
        m_beg = static_cast<long long>(std::time(&m_now));
        m_running = true;
    }
}

void StopWatch::stop()
{
    if (m_running)
    {
        m_elapsed += static_cast<long long>(std::time(&m_now)) - m_beg;
        m_running = false;
    }
}

void StopWatch::lap()
{
    if (m_running)
    {
        stop();
        m_lapTimes.emplace_back(m_elapsed);
        clearTime();
        start();
    }
}

const std::vector<int>& StopWatch::getLapTimes() const
{
    return m_lapTimes;
}

int StopWatch::getElapsed() const
{
    return m_elapsed;
}

void StopWatch::converter(int seconds)
{
    m_hours   = seconds / 3600;
    seconds   = seconds % 3600;
    m_minutes = seconds / 60;
    m_seconds = seconds % 60;
}

void StopWatch::clearTime()
{
    m_elapsed = 0;
    m_seconds = 0;
    m_minutes = 0;
    m_hours   = 0;
}

void DigitalStopWatch::printTime() 
{
    std::stringstream ss;
    ss << std::setfill('0') << std::setw(2) << m_hours << ':' << std::setw(2) << m_minutes << ':' << std::setw(2) << m_seconds;

    char c;
    s_console.gotoxy(m_pos);
    while (ss >> c) s_console.putSymbol(c);
}

SegmentedStopWatch::SegmentedStopWatch(Position where) : StopWatch(where)
{ 
    auto left = m_pos.x + 2*timePadding;
    auto xpos = [left](int index) { return left + (index * digitPadding) + (index/2 * 3); };

    int index = 0;
    for (auto& digit : m_digits)
        digit.setPosition({xpos(index++), m_pos.y});

    m_colons = { {
            { xpos(2) - 2, m_pos.y + 1 },
            { xpos(2) - 2, m_pos.y + 3 },
            { xpos(4) - 2, m_pos.y + 1 },
            { xpos(4) - 2, m_pos.y + 3 },
        } };
}

void SegmentedStopWatch::printTime() 
{
    setCurrentTime();
    for (auto& digit : m_digits) digit.printDigit();
    for (auto& colon : m_colons) s_console.putSymbol(colon, '.');
}

void SegmentedStopWatch::setCurrentTime()
{
    auto set = [this](int startIndex, int unit) {
        m_digits[startIndex - 1] = (unit < 10)? 0 : unit/10;
        m_digits[startIndex]     = unit % 10;
    };
    set(5, m_seconds);
    set(3, m_minutes);
    set(1, m_hours);
}

int main() {
    try {
        std::unique_ptr<Factory> segFact{ std::make_unique<SegmentedStopWatchFactory>() };
        std::unique_ptr<Factory> digFact{ std::make_unique<DigitalStopWatchFactory>() };

        std::unique_ptr<StopWatch> stoppers[2];
        stoppers[0] = segFact->createStopWatch({5, 5});
        stoppers[1] = digFact->createStopWatch();

        //to test the second stopper simply change stoppers[0] to stoppers[1]
        stoppers[0]->countDownFrom(12); //test countdown
    } catch(std::exception const& e) {
        std::cerr << "Exception: " << e.what() << "\n";
    }
}

Post Scriptum

These are random notes. Specifically, I didn't mention important things (like brace style, const-correctness, use of raw arrays etc.)

\$\endgroup\$
  • \$\begingroup\$ Thanks for the extremely detailed answer! It is elegant and portable as opposed to the variant i wrote. As the code i wrote is for practice i will carefully review your answer and write my variant of it (using your answer as a cheat sheet) \$\endgroup\$ – globalturist Jul 3 '17 at 9:13
  • \$\begingroup\$ you used Console.h functions and the variable "position" and didn't include it anywhere did you forget to include it or am i missing something ? \$\endgroup\$ – globalturist Jul 3 '17 at 10:38
  • \$\begingroup\$ After playing around with the code i found a few bugs: in StopWatch.cpp i had to remove s_console.render from the function countDownFrom and put it in SegmentedStopWatch's and DigitalStopWatch's printTime method from the following concerns: a) the name says printTime and it didn't print anything on its own b) the function will be regularly called if the regular stopwatch mode is active (start(), updateTime(), printTime(), stop()) and i didn't think it will be convenient to call render each time in addition to updateTime() and printTime() see use example above \$\endgroup\$ – globalturist Jul 3 '17 at 12:46
  • \$\begingroup\$ Additionally i had to add s_console.render() into Digits.cpp -> void printDigit() const because: a)the name implies it will print the digit and it doesn't so i added render() and the end of the function. b) if i would want to use it on its own in some other project i will have to allways call render() after printDigit() which can easily be forgotten \$\endgroup\$ – globalturist Jul 3 '17 at 12:49
  • 1
    \$\begingroup\$ All the things you mention as "bugs" were simply not features and cannot be portably solved. The main point of the Console mockup with a render() hook was to make it easy to change the underlying implementation, e.g. ncurses: paste.ubuntu.com/25011237 (ncurses at once gets rid of the "problems"). To recap: things out of scope for this answer: Console.h/cpp and source code organization (the classical header/source division looked ok and isn't very interesting in general). Or in other words: This site is for code review, not "solve my homework for me". \$\endgroup\$ – sehe Jul 3 '17 at 13:12

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.