# Refactoring

___

## Old C++98/11 code 

Please everyone to sit comfortably. I will show You a piece of code which I had to reactor a few years ago. This code will be simpler than those which I handled, but general ideas stayed, like: `void*` etc...
<!-- .element: class="fragment fade-in" -->
Let's start from Valgrind output about this piece of code:
<!-- .element: class="fragment fade-in" -->

```C++
==1667238== More than 10000000 total errors detected.  I'm not reporting any more.
==1667238== Final error counts will be inaccurate.  Go fix your program!
```
<!-- .element: class="fragment fade-in" -->
___

## General idea

We want to have a `RequestHandler` which will store requests in the queue and handle them on a separate thread. We have 4 requests:
* <!-- .element: class="fragment fade-in" --> Download sth, for instnace an image,
* <!-- .element: class="fragment fade-in" --> Upload sth,
* <!-- .element: class="fragment fade-in" --> Remove data from cache,
* <!-- .element: class="fragment fade-in" --> Clear all cached values.

We have an API to Server class that has one method handle, which has 4 overloads, each for one request.
<!-- .element: class="fragment fade-in" -->
___

## Requests

<div class="multicolumn">
<div class="col">

```C++
struct Credentail { int cert_; };

struct Download {
    enum ConnectionType { Telnet,
                          Ssh };

    std::string url_;
    Credentail credentail_;
    int maxMbps_;
    bool cache_;
    ConnectionType type_;
};
```
</div>
<div class = "col">

```C++
struct Upload {
    enum ConnectionType { Telnet,
                          Ssh };

    std::string url_;
    Image image_;
    Credentail credentail_;
    int maxMbps_;
    int size_;
    ConnectionType type_;
};
```
</div>
</div>

<div class="multicolumn">
<div class="col">

```C++
struct RemoveFromCache {
    std::string url_;
};
```
</div>
<div class = "col">

```C++
struct ClearCache {
};
```
</div>
</div>
___

## Server class

```C++
class Server {
public:
    void handle(const Download& request, void (*callback)(bool, Image));

    void handle(const Upload& request, void (*callback)(bool, Image));

    void handle(const RemoveFromCache& request, void (*callback)(bool, Image));

    void handle(const ClearCache& request, void (*callback)(bool, Image));

private:
    bool download(const Download& request, Image* image);

    bool upload(const Upload& request, const Image* image);

    std::vector<std::pair<Image, std::string>> cache_;
};
```
<!-- .slide: style="font-size: 0.9em" -->
___

## Server class - one of handle method

```C++
void handle(const Download& request, void (*callback)(bool, Image)) {
    Image image;

    for (const auto& pair : cache_) {
        if (pair.second == request.url_) {
            image = pair.first;
            callback(true, image);
            return;
        }
    }

    if (download(request, &image)) {
        if (request.cache_) {
            cache_.push_back(std::pair<Image, std::string>(image, request.url_));
        }

        callback(true, image);
        return;
    }

    callback(false, Image{});
}
```
<!-- .slide: style="font-size: 0.87em" -->
___

## Server class - download method

```C++
bool download(const Download& request, Image* image) {
    if (request.type_ == Download::Ssh && request.credentail_.cert_ != 123) {
        return false;
    }
    if (request.type_ == Download::Telnet && request.credentail_.cert_ != 231) {
        return false;
    }

    // Simulate some other error
    if (request.maxMbps_ % 2) {
        return false;
    }
    image->bitmap_ = {97, 98, 99, 100, 101, 102};
    return true;
}
```
<!-- .slide: style="font-size: 0.9em" -->
___

## Let's stop here for a while

* <!-- .element: class="fragment fade-in" --> What's wrong with this code?
* <!-- .element: class="fragment fade-in" --> How we can improve it?
* <!-- .element: class="fragment fade-in" --> Which modern C++ feature we should use here?

```C++
void handle(const Download& request, void (*callback)(bool, Image)) {
    Image image;
    for (const auto& pair : cache_) {
        if (pair.second == request.url_) {
            image = pair.first;
            callback(true, image);
            return;
        }
    }
    if (download(request, &image)) {
        if (request.cache_) {
            cache_.push_back(std::pair<Image, std::string>(image, request.url_));
        }
        callback(true, image);
        return;
    }
    callback(false, Image{});
}

bool download(const Download& request, Image* image) {
    if (request.type_ == Download::Ssh && request.credentail_.cert_ != 123) {
        return false;
    }
    if (request.type_ == Download::Telnet && request.credentail_.cert_ != 231) {
        return false;
    }
    // Simulate some other error
    if (request.maxMbps_ % 2) {
        return false;
    }
    image->bitmap_ = {97, 98, 99, 100, 101, 102};
    return true;
}
```
<!-- .element: class="fragment fade-in" -->
<!-- .slide: style="font-size: 0.6em" -->
___

## Now will be only worse!

```C++
class RequestHandler {
public:
    enum RequestType { Download, Upload, Remove, Clear };

    void start(Server* server);

    void stop();

    void pushRequest(void* request, RequestType type, void (*callback)(bool, Image));

private:
    void run(Server* server);
    std::tuple<void*, RequestType, void (*)(bool, Image)> waitForRequest();

    bool stop_;
    std::queue<std::tuple<void*, RequestType, void (*)(bool, Image)>> requests_;
};
```
<!-- .slide: style="font-size: 0.9em" -->
___

## Implementations (1)

```C++
void start(Server* server) {
    std::thread(&RequestHandler::run, this, server).detach();
}

void stop() {
    stop_ = true;
}

void pushRequest(void* request, RequestType type, void (*callback)(bool, Image)) {
    requests_.push(std::tuple<void*, RequestType, void (*)(bool, Image)>(
        request, type, callback));
}
```
<!-- .slide: style="font-size: 0.9em" -->
___

## Implementations (2)

```C++
    void run(Server* server) {
        while (!stop_) {
            auto tuple = waitForRequest();
            switch (std::get<1>(tuple)) {
            case Download:
                server->handle(*((::Download*)(std::get<0>(tuple))), std::get<2>(tuple));
                break;
            case Upload:
                server->handle(*((::Upload*)(std::get<0>(tuple))), std::get<2>(tuple));
                break;
            case Remove:
                server->handle(*((RemoveFromCache*)(std::get<0>(tuple))), std::get<2>(tuple));
                break;
            case Clear:
                server->handle(*((ClearCache*)(std::get<0>(tuple))), std::get<2>(tuple));
                break;
            }
        }
    }

    std::tuple<void*, RequestType, void (*)(bool, Image)> waitForRequest() {
        while (requests_.empty() || !stop_) {
        }

        auto pair = requests_.front();
        requests_.pop();

        return pair;
    }
```
<!-- .slide: style="font-size: 0.67em" -->
___

## The funny part - it actually works!

```C++
int main() {
    Server server;
    RequestHandler handler;

    handler.start(&server);

    Download d{"sth.png", 123, 100, true, Download::Ssh};
    handler.pushRequest((void*)(&d),
                        RequestHandler::Download,
                        [](bool succes, Image image) {
                            if (succes) {
                                for (auto el : image.bitmap_) {
                                    std::cout << el << ' ';
                                }
                                std::cout << '\n';
                            } else {
                                std::cout << "FAILED!\n";
                            }
                        });

    std::this_thread::sleep_for(std::chrono::seconds(1));
    handler.stop();
}
```
<!-- .element: class="fragment fade-in" -->
<!-- .slide: style="font-size: 0.8em" -->
___

## Let's fix this ugly code!

For Requests we only need add `enum class` instead of `enum` the rest seems ok.

<div class="multicolumn">
<div class="col">

```C++
struct Credentail { int cert_; };

struct Download {
    enum class ConnectionType { Telnet,
                          Ssh };

    std::string url_;
    Credentail credentail_;
    int maxMbps_;
    bool cache_;
    ConnectionType type_;
};
```
</div>
<div class = "col">

```C++
struct Upload {
    enum class ConnectionType { Telnet,
                          Ssh };

    std::string url_;
    Image image_;
    Credentail credentail_;
    int maxMbps_;
    int size_;
    ConnectionType type_;
};
```
</div>
</div>

<div class="multicolumn">
<div class="col">

```C++
struct RemoveFromCache {
    std::string url_;
};
```
</div>
<div class = "col">

```C++
struct ClearCache {
};
```
</div>
</div>

___

## Alias and status code

The `bool` flag is the best option for describing an error
<!-- .element: class="fragment fade-in" -->
```C++
enum class StatusCode {
    Ok,
    WrongUrl,
    CanNotConnect,
    WrongCredential,
    MaximumSizeExceeded,
    MissingImage,
};
```
<!-- .element: class="fragment fade-in" -->
Let's add also an aliast for `callback`
<!-- .element: class="fragment fade-in" -->

```C++
using CallbackType = void (*)(StatusCode, Image);

void handle(const Download& request, CallbackType callback);
```
<!-- .element: class="fragment fade-in" -->
___

## Use references and STL algorithms (1)

```C++
void handle(const Download& request, CallbackType callback) {
    if (const auto it = findImage(request.url_); it != std::cend(cache_)) {
        callback(StatusCode::Ok, it->first);
        return;
    }

    Image img;
    const auto status = download(request, img);
    if (status == StatusCode::Ok) {
        if (request.cache_) {
            cache_.emplace_back(img, request.url_);
        }
    }

    callback(status, img);
}
```
<!-- .slide: style="font-size: 0.85em" -->
___

## Use references and STL algorithms (2)

```C++
std::vector<std::pair<Image, std::string>>::const_iterator findImage(const std::string& url) const {
    return std::find_if(cbegin(cache_), cend(cache_),
                        [url](const auto& pair) {
                            const auto& [_, thisUrl] = pair;
                            return thisUrl == url;
                        });
}

StatusCode download(const Download& request, Image& image) const {
    if (request.type_ == Download::ConnectionType::Ssh && request.credentail_.cert_ != 123) {
        return StatusCode::WrongCredential;
    }
    if (request.type_ == Download::ConnectionType::Telnet && request.credentail_.cert_ != 231) {
        return StatusCode::CanNotConnect;
    }

    // Simulate some other error
    if (request.maxMbps_ % 2) {
        return StatusCode::WrongUrl;
    }
    image.bitmap_ = {97, 98, 99, 100, 101, 102};
    return StatusCode::Ok;
}
```
<!-- .slide: style="font-size: 0.75em" -->

___

## Use references and STL algorithms (3)

```C++
void handle(const Upload& request, CallbackType callback) const {
    if (request.size_ > 100) {
        callback(StatusCode::MaximumSizeExceeded, Image{});
        return;
    }

    callback(upload(request), Image{});
}

void handle(const RemoveFromCache& request, CallbackType callback) {
    if (const auto it = findImage(request.url_); it != std::cend(cache_)) {
        cache_.erase(it);
        callback(StatusCode::Ok, Image{});
        return;
    }

    callback(StatusCode::MissingImage, Image{});
}
```
<!-- .slide: style="font-size: 0.85em" -->
___

## Let's fix <code>void*</code>

Could be better, but this work and is safe.

```C++
using RequestType = std::variant<Download, Upload, RemoveFromCache, ClearCache>;

void pushRequest(const RequestType& request, Server::CallbackType callback) {
    requests_.emplace(request, callback);
}

void run(Server* server) {
    const auto [request, callback] = waitForRequest();
    switch (request.index()) {
    case 0:
        server->handle(std::get<Download>(request), callback);
        break;
    case 1:
        server->handle(std::get<Upload>(request), callback);
        break;
    case 2:
        server->handle(std::get<RemoveFromCache>(request), callback);
        break;
    case 3:
        server->handle(std::get<ClearCache>(request), callback);
        break;
    }
}
```
<!-- .slide: style="font-size: 0.8em" -->
___

## Make it thread safe (1)

```C++
void start(Server* server) {
    std::thread(&RequestHandler::run, this, server).detach();
}

void stop() {
    stop_ = true;
    cv_.notify_one();
}

void pushRequest(const RequestType& request, Server::CallbackType callback) {
    {
        std::lock_guard lock(m_);
        requests_.emplace(request, callback);
    }
    cv_.notify_one();
}

using QueueType = std::pair<RequestType, Server::CallbackType>;

std::mutex m_;
std::condition_variable cv_;
std::atomic<bool> stop_{false};
std::queue<QueueType> requests_;
```
<!-- .slide: style="font-size: 0.8em" -->
___

## Make it thread safe (2)

```C++
std::optional<QueueType> waitForRequest() {
    std::unique_lock lk(m_);
    cv_.wait(lk, [&]() { return !requests_.empty() || stop_; });
    if (stop_) {
        return std::nullopt;
    }
    auto pair = requests_.front();
    requests_.pop();

    return pair;
}
```
___

## Usage actually not change

```C++
Server server;
RequestHandler handler;

handler.start(&server);

handler.pushRequest(Download{"sth.png", 123, 100, true, Download::ConnectionType::Ssh},
                    [](Server::StatusCode status, Image image) {
                        if (status == Server::StatusCode::Ok) {
                            for (auto el : image.bitmap_) {
                                std::cout << el << ' ';
                            }
                            std::cout << '\n';
                        } else {
                            std::cout << "FAILED!\n";
                        }
                    });

std::this_thread::sleep_for(std::chrono::milliseconds(10));
handler.stop();
```
<!-- .slide: style="font-size: 0.8em" -->
___

## Valgird is almost satisfied

It is suspicious only for a pointer to a server that was created in another thread. It marks it as possibly lost, but we know that we didn't do any allocation, because we created it on the stack, and pass only the address.

```C++
==1678528== LEAK SUMMARY:
==1678528==    definitely lost: 0 bytes in 0 blocks
==1678528==    indirectly lost: 0 bytes in 0 blocks
==1678528==      possibly lost: 288 bytes in 1 blocks
==1678528==    still reachable: 0 bytes in 0 blocks
==1678528==         suppressed: 0 bytes in 0 blocks
==1678528== 
==1678528== ERROR SUMMARY: 1 errors from 1 contexts (suppressed: 0 from 0)
```
___

## Further improvements

We can have 2 callbacks. One returns a downloaded image and error code, and the second returns only an error code. We can change the name of function `handle` to `download`, `upload` etc.. because inside `switch` we know which function we call because we know the type of request (there is no polymorphism). So we can avoid unnecessary arguments for image, especially this is important when classes don't have default C'tor. We should only make it as a template to allow the download other things than `Images.` We can also use `std::visit` method to avoid switch case.

___

## Better solution - use OOP

This particular example works, but what happens when we extend the program to handle also other types like Videos and audio? We need to extend server interface to handle different types. 

```C++
    void handle(const DownloadVideo& request, void (*callback)(bool, Video));
    void handle(const UploadVideo& request, void (*callback)(bool, Video));

    void handle(const DownloadAudio& request, void (*callback)(bool, Audio));
    void handle(const UploadAudio& request, void (*callback)(bool, Audio));

    void handle(const DownloadCertificates& request, void (*callback)(bool, Certificates));
    void handle(const UploadCertificates& request, void (*callback)(bool, Certificates));
```
<!-- .element: class="fragment fade-in" -->

Server class grow and grow, and now have a lot of resposibilities and depends form implementations of `Image`, `Video`, `Audio` and `Certificates` this is bad, really bad. We should folow SOLID principles.
<!-- .element: class="fragment fade-in" -->
<!-- .slide: style="font-size: 0.8em" -->
___

## Command pattern

Let's create an abstraction and break the dependency between `Server` and `Request`. `Command` object will have only one method `operator()` that we can call. This operator will perform one action like: `Download`, `Upload`, `RemoveFormCache`, `ClearCache`.

```C++
class Command {
public:
    enum class StatusCode {
        Ok, WrongUrl, CanNotConnect, WrongCredential, MaximumSizeExceeded, MissingImage
    };

    // Rule of 5!
    virtual ~Command() = default;
    Command(const Command&) = default;
    Command(Command&&) = default;
    Command& operator=(const Command&) = default;
    Command& operator=(Command&&) = default;

    virtual void operator()() const = 0;
};
```
<!-- .slide: style="font-size: 0.87em" -->
<!-- .element: class="fragment fade-in" -->

___

## Delegate pattern

Because `Command` needs to know only how to deal with Files like `Image`, `Video` or `Audio`. We need to create a `Delegate` that delegates the rest of the actions to the `Server` class.

```C++
class Command {
    class Delegate {
    public:
        virtual ~Delegate() = default;
        virtual StatusCode removeFromCache(const std::string& url) = 0;
        virtual void clearCache() = 0;
        virtual void addToCache(const std::vector<uint8_t>& data, const std::string& url) = 0;
        virtual StatusCode download(std::vector<uint8_t>& data, const DownloadRequest& request) const = 0;
        virtual StatusCode upload(const std::vector<uint8_t>& data, const UploadRequest& request) const = 0;
    };

    explicit Command(Delegate* delegate)
        : delegate_(delegate) {}
    // ...
protected:
    Delegate* delegate_;
};
```
<!-- .slide: style="font-size: 0.77em" -->
<!-- .element: class="fragment fade-in" -->
___

## Server class

Now server-class depends on abstraction which is `Delegate` and only performs actions with connecting with network and caching values. we can also create another class that will cache values.  Class `Command` depends on abstraction, because we don't need to know which class will implement delegate methods.

```C++
class Server : public Command::Delegate {
public:
    ~Server() override = default;

    Command::StatusCode removeFromCache(const std::string& url) override;

    void clearCache() override;

    void addToCache(const std::vector<uint8_t>& data, const std::string& url) override;

    Command::StatusCode download(std::vector<uint8_t>& data, const DownloadRequest& request) const override;

    Command::StatusCode upload(const std::vector<uint8_t>& data, const UploadRequest& request) const override;

private:
    std::vector<std::pair<std::vector<uint8_t>, std::string>>::const_iterator findFile(const std::string& url) const;

    std::vector<std::pair<std::vector<uint8_t>, std::string>> cache_;
};
```
<!-- .slide: style="font-size: 0.71em" -->
<!-- .element: class="fragment fade-in" -->

___

## Easy to extend

Now we can create whatever command we want to.

```C++
class UploadImageCommand : public Command {
public:
    using CallbackType = void (*)(StatusCode);

    ~UploadImageCommand() override = default;

    UploadImageCommand(Delegate* delegate, const Image& image, CallbackType callback, const UploadRequest& request)
        : Command(delegate), image_(image), callback_(callback), request_(request) {}

    void operator()() const override {
        callback_(delegate_->upload(image_.bitmap_, request_));
    };

private:
    Image image_;
    CallbackType callback_;
    UploadRequest request_;
};
```
<!-- .slide: style="font-size: 0.73em" -->
<!-- .element: class="fragment fade-in" -->

___

## Download Image command

We can deal with different parameters, and callbacks.

```C++
class DownloadImageCommand : public Command {
public:
    using CallbackType = void (*)(StatusCode, Image);

    ~DownloadImageCommand() override = default;

    DownloadImageCommand(Delegate* delegate, CallbackType callback, const DownloadRequest& request)
        : Command(delegate), callback_(callback), request_(request) {}

    void operator()() const override {
        std::vector<uint8_t> data;
        if (auto status = delegate_->download(data, request_); status == Command::StatusCode::Ok) {
            // Do some conversion on vector
            if (request_.cache_) {
                delegate_->addToCache(data, request_.url_);
            }
            Image image{data};
            callback_(status, std::move(image));
        } else {
            callback_(status, Image{});
        }
    };

private:
    CallbackType callback_;
    DownloadRequest request_;
};
```
<!-- .slide: style="font-size: 0.7em" -->
<!-- .element: class="fragment fade-in" -->
___

## Request handler

Request handlers also break their dependency on the server. We don't need to know about `Server` class anymore. We just push the command on queue and call `operator()`.

```C++
class RequestHandler {
public:
    void start() {
        std::thread(&RequestHandler::run, this).detach();
    }

    void stop() {
        stop_ = true;
        cv_.notify_one();
    }

    void pushRequest(std::unique_ptr<Command> command) {
        {
            std::lock_guard lock(m_);
            requests_.push(std::move(command));
        }
        cv_.notify_one();
    }

private:
    void run() {
        while (!stop_) {
            const auto request = waitForRequest();
            if (!request) {
                return;
            }

            (*request)();
        }
    }

    std::unique_ptr<Command> waitForRequest() {
        std::unique_lock lk(m_);
        cv_.wait(lk, [&]() { return !requests_.empty() || stop_; });
        if (stop_) {
            return nullptr;
        }

        std::unique_ptr<Command> request = std::move(requests_.front());
        requests_.pop();

        return request;
    }

    std::mutex m_;
    std::condition_variable cv_;
    std::atomic<bool> stop_{false};
    std::queue<std::unique_ptr<Command>> requests_;
};
```
<!-- .slide: style="font-size: 0.7em" -->
<!-- .element: class="fragment fade-in" -->
___

## Usage

```C++
int main() {
    Server server;
    RequestHandler handler;

    handler.start();
    handler.pushRequest(std::make_unique<DownloadImageCommand>(
        &server,
        [](Command::StatusCode status, Image img) {
            if (status == Command::StatusCode::Ok) {
                std::copy(cbegin(img.bitmap_), cend(img.bitmap_), 
                          std::ostream_iterator<uint8_t>(std::cout, " "));
                std::cout << '\n';
            } else {
                std::cout << "Sth went wrong!\n";
            }
        },
        DownloadRequest{"Sth123", Credentail{123}, 200, true, DownloadRequest::ConnectionType::Ssh}));

    std::this_thread::sleep_for(std::chrono::milliseconds(100));
    handler.stop();
}
```
<!-- .slide: style="font-size: 0.8em" -->
___

## Valgrind

Now we have easy to extend code, which also satisfies Valgrind.

```C++
==1790927== 
==1790927== HEAP SUMMARY:
==1790927==     in use at exit: 0 bytes in 0 blocks
==1790927==   total heap usage: 11 allocs, 11 frees, 74,770 bytes allocated
==1790927== 
==1790927== All heap blocks were freed -- no leaks are possible
==1790927== 
==1790927== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0)
```