Почему не работает (Chain of Responsibilty)?

Question

[egorggegor]

Ку
Реализовал паттерн цепочка обязанностей, но при тестировании выскакивает проблема с потоками, кто знает, как с этим разобраться?
(Проблема помечена в коде, тесты, на которых выявилась проблема в мэйне)

class Handler {
public:
    Handler(): next(nullptr) {};
    virtual ~Handler() {};
    virtual void HandleRequest(int data) {
        if (next) {
            this->next->HandleRequest(data);
        }
    }
    void SetNextHandler(Handler* next_handler) {  //Здесь вроде проблема
        if (next) {
            this->SetNextHandler(next_handler);
        } else {
            this->next = next_handler;
        }
    }
public:
    Handler* next;
};

class ConcreteHandler1 : public Handler {
public:
    void HandleRequest(int data) {
        //if can handle it do it
            //implementation of HadleRequest
        //else
        std::cout << "ConcreteHandler1" << std::endl;
        Handler::HandleRequest(data);
    }
};

class ConcreteHandler2 : public Handler {
public:
    void HandleRequest(int data) {
        //if can handle it do it
            //implementation of HadleRequest
        //else
        std::cout << "ConcreteHandler2" << std::endl;
        Handler::HandleRequest(data);
    }
};

int main(int argc, const char * argv[]) {
    Handler* handler = new Handler;
    int data;
    handler->SetNextHandler(new ConcreteHandler1());
    handler->SetNextHandler(new ConcreteHandler2());
    handler->HandleRequest(data);
    return 0;
}

Answer 1

[lorc]

Подозреваю, что должно быть вот так:

void SetNextHandler(Handler* next_handler) {  
        if (next) {
            next->SetNextHandler(next_handler); // <- next, не this
        } else {
            this->next = next_handler;
        }
    }

Иначе у вас бесконечная рекурсия получится.

Но вообще, это не самый оптимальный алгоритм, ибо сложность добавления нового хендлера - O(N), хотя можно было бы сделать O(1), если не важен порядок вызова.

А еще, можно бы завести виртуальную функцию, которая будет занимать только хендлингом. Это сделает код более надежным и позволит избежать Handler::HandleRequest(data); в конце каждого хендлера.

Comments

[egorggegor]

Спасибо за решение, я чего-то заглупил)

[egorggegor]

А сложность O(1) получается, если иметь указатель на последний элемент в этом "связном списке"?

[lorc]

egorggegor, да, можно и так. Вы все равно знаете что next_handler будет всегда последним, можно закешировать его в отдельном поле. Можно вывернуть список: передавать следующий хендлер в конструктор текущего:

Handler* handler = nullptr;
handler = new ConcreteHandler1(handler);
handler = new ConcreteHandler2(handler);
handler = new ConcreteHandler3(handler);
handler->HandleRequest(data);

А конструктор сделать как-то так:

Handler(Handler *next_handler = nullptr): next(next_handler) {};

Но вообще, я бы не стал выеживаться со связными списками, а сделал бы банальный vector<std::function<void, int>>. Потому что, рано или поздно вам захочется убрать один хендлер из середины. Я уж не говорю про попытку написать нормальный деструктор для этого чуда.