4
\$\begingroup\$

I am developing a simple Chained Flow (should I call it by this name?) that uses waterfall as main operation behavior.

Basically we will have a Flow and he will have some dialogs with functions to be executed. So, when you call start on Flow (passing the name of the dialog) some Waterfall objects will be created taking the pointers to the functions pre-stored and executing them. The functions to be executed can call its parent functions to execute some tasks (like calling the next function or reseting to the begin function).

Just take an example in Javascript (easy to understand) code:

var status = false; 

var array_of_functions = [
    function(parent) {
        print("first");

        parent.next();
    },
    function(parent) {
        print("second");

        if (!status) {
            status = true;
            // call itself again (not the whole array, reset() should do it)
            parent.restart();
        }
    }
];

doMagic(array_of_functions);

Output: first -> second -> second;

I'm concerned about the method that I used to make it. Should I try to rewrite in another way or is it good?

Probably the code is a little messy, but I can't find a better way to solve. And I am using templates to later add some more arguments to the functions that will be called. Taking that, I'm using lambda expressions too to avoid declaring the types in each function (passing a Course object).

The code takes only 2 files, excluding main.

main.cpp

(just a example, I will embed Flow into another Class later)

#include <iostream>

#include "Flow.h"

bool status = false;

void one(Course* course) {
    std::cout << "one" << std::endl;
    course->doNext();
}

void two(Course* course) {
    std::cout << "two" << std::endl;
    if (!status) {
        status = true;
        course->doRestart();
    } else {
        course->doReplace("another");
    }
}

void three(Course* course) {
    std::cout << "three" << std::endl;
    course->doNext();
}

int main() {
    std::vector<std::function<void(Course*)>> init = {one, two};
    std::vector<std::function<void(Course*)>> another = {three};
    Flow<void, Course*> flow;

    flow.setDialog("init", steps_x);
    flow.setDialog("another", steps_y);

    flow.doStart("init");
    return 1;
}

Flow.h

#ifndef FLOW_H
#define FLOW_H

#include <string>
#include <vector>
#include <functional>

#include "Waterfall.h"

template <typename rtype, typename ...fargs> class Waterfall;
template <typename rtype, typename ...fargs> class Flow {
    public:
        Flow() {}
        ~Flow() {}

        struct Node {
            std::string name;
            std::vector<std::function<rtype(fargs...)>> functions;
        };

        bool doTrigger(std::string name) {
            Node* node;
            bool found = false;

            for (Node& tmp : _dialogs) {
                if (tmp.name == name) {
                    node = &tmp;
                    found = true;
                    break;
                }
            }

            if (!found) return false;

            Waterfall<rtype, fargs...> waterfall(this, node->functions);
            waterfall.doTrigger();
            return true;
        }

        bool setDialog(std::string name, std::vector<std::function<rtype(fargs...)>> functions) {
            for (const Node& node : _dialogs) {
                if (node.name == name) {
                    return false;
                }
            }

            Node node = {name, functions};
            _dialogs.push_back(node);
            return true;
        }

    private:
        std::vector<Node> _dialogs;
};

#endif

Waterfall.h

#ifndef WATERFALL_H
#define WATERFALL_H

#include <vector>
#include <functional>
#include <iostream>

#include "Flow.h"

struct Course {
    std::function<void()> doNext;
    std::function<void()> doRestart;
    std::function<void()> doReset;
    std::function<void(std::string)> doReplace;
};

template <typename rtype, typename ...fargs> class Flow;
template <typename rtype, typename ...fargs> class Waterfall {
    public:
        Waterfall(Flow<rtype, fargs...>* flow, std::vector<std::function<rtype(fargs...)>>& functions) :
            _index(0),
            _flow(flow)
        {
            for (unsigned int i = 0; i < functions.size(); i++) {
                Step step;
                step.callback = &functions[i];
                _steps.push_back(step);
            }
        }

        ~Waterfall() {}

        Flow<rtype, fargs...>* getFlow() {return _flow;}

        struct Step {
            unsigned int count = 0;
            std::function<rtype(fargs...)>* callback;
        };

        bool doTrigger() {
            if (_index >= _steps.size()) return false;

            Step* step = &_steps[_index];
            unsigned int& count = step->count;

            Course course;

            course.doNext = [this]() {
                this->doNext();
            };

            course.doRestart = [this]() {
                this->doRestart();
            };

            course.doReset = [this]() {
                this->doReset();
            };

            course.doReplace = [this](std::string name) {
                this->getFlow()->doTrigger(name);
            };

            count++;
            (*(step->callback))(&course);
            return true;
        }

        bool doNext() {
            if (_index >= _steps.size()) return false;
            _index++;
            return doTrigger();
        }

        bool doRestart() {
            return doTrigger();
        }

        bool doReset() {
            _index = 0;
            return doTrigger();
        }

    private:
        unsigned int _index;
        Flow<rtype, fargs...>* _flow;
        std::vector<Step> _steps;
};

#endif

You can use this to compile:

#!/bin/bash

g++ -W -Wall -Werror -Wextra -pedantic -O2 \
    main.cpp \
    Flow.h \
    Waterfall.h \
-o main.exe

./main.exe

Thanks in advance.

\$\endgroup\$
3
\$\begingroup\$

Prefer passing a std::string_view instead of a std::string in parameters. This avoids needing to create (and allocate) std::string when they don't need to be. Std::string should still be used in Node to ensure the string remains valid.

Having said that using integer keys is better. Using an enum for the keys means that a typo in the key becomes a compile error instead of a bug and if you take care to make them sequential and start from 0 the lookup is a vector index instead of a linear search.

the template parameters for the function type is meaningless. You can only use std::function<void(Course*)> in it. Any other parameter list will result in a compilation error.

Your control flow through the Course object is deeply recursive when it doesn't need to be. This means that the user could create a stack overflow by jumping back and forth between menu items.

Instead you can keep the index and current node as a local and manipulate it, if you get rid of the Waterfall class this becomes a bit easier:

bool doStart(std::string_view name) {

    Node* currentNode = //find the corresponding Node;
    size_t functionIndex = 0;
    Course course;
    course.doNext = [&]() {
        functionIndex ++;
    };

    course.doRestart = [&]() {
        //nop
    };

    course.doReset = [&]() {
        functionIndex = 0;
    };

    course.doReplace = [&](std::string_view name) {
        currentNode = //find the corresponding Node;
        functionIndex = 0;
    };

    while(currentNode != nullptr && functionIndex < currentNode->funcs.length){
        currentNode->funcs[functionIndex](&course);
    }
}

However looking at that code it makes sense to make Node* currentNode, size_t functionIndex and a Flow* flow as fields of Course and make the functions normal instance functions:

struct Course {
    Node* currentNode = nullptr;
    size_t functionIndex = 0;
    Flow* flow;

    void doNext(){
            functionIndex ++;
    }
    void doRestart() {
        //nop
    }
    void doReset() {
        functionIndex = 0;
    }
    void doReplace(std::string_view name) {
        currentNode = flow->findNode(name);
        functionIndex = 0;
    }
};


bool doStart(std::string_view name) {

    Course course;
    course.currentNode = findNode(name);
    course.functionIndex = 0;
    course.flow = this;

    while(course.currentNode != nullptr && 
            course.functionIndex < course.currentNode->funcs.length){
        course.currentNode->funcs[course.functionIndex](&course);
    }
}
\$\endgroup\$
  • \$\begingroup\$ Thank you, this makes too much simple. \$\endgroup\$ – Radagast May 10 '19 at 18:04
  • \$\begingroup\$ I followed your resolution, was way better than mine. Just to complement, I added a counter to manage the times that the functions will be called, and if none functions is next or is up to replace, the counter drops to 0 and the flow just ends. \$\endgroup\$ – Radagast May 13 '19 at 1:27
  • \$\begingroup\$ Another obs that I found that probably can be useful to someone: if the code still remains Node and Course objects, than you must to separate both and passing by lambda to avoid nested classes. \$\endgroup\$ – Radagast May 13 '19 at 1:30

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.