4
\$\begingroup\$

I just have learned the Command Pattern carefully. I try to use this pattern to implement a simple editor.

Some code may need some improvements, say the instance of Invoker should be only one since it's the simple editor itself. I mean one editor could manage several documents in practice while in one document needs a separate instance of Invoker in the code below.

Here is the simple code snippet to achieve a naive editor.

#include <iostream>
#include <string>
#include <cstdint>
#include <algorithm>
#include <cstdint>
#include <memory>
#include <functional>
#include <deque>
#include <limits>
#include <sstream>

class Document  
{
public:
    enum class DocPos:uint8_t
    {
        DOC_END,
        DOC_BGN,
        DOC_MID,
    };

    int insert(size_t row, size_t col, const std::string str, size_t& ins_pos)
    {
        size_t pos = 0;
        ins_pos = std::string::npos;
        for(int i = 0; i<row-1; i++)
        {
            pos = content.find('\n', pos);  //for simpleness, just consider '\n' other than `\r\n` and etc.
            if(std::string::npos == pos)
            {
                return -1;
            }
        }
    
        auto itr = content.begin() + pos;
        for(int j=0; j<col-1; j++)
        {
            if(*itr++=='\n')
            {
                return -1;
            }
        }

        ins_pos = pos+col-1;
        content.insert(ins_pos, str);
        return 0;
    }

    int append(const std::string& str)
    {
        content += str;
        return 0;
    }

    int erase(std::size_t length, DocPos pos = DocPos::DOC_END, std::size_t offset=0)
    {
        switch(pos)
        {
            case DocPos::DOC_END:
                if(content.size() < length)
                {
                    return -1;
                }
                content.erase(content.end()-length, content.end());
                break;
            case DocPos::DOC_BGN:
                if(content.size() < length)
                {
                    return -1;
                }
                content.erase(content.begin(), content.begin()+offset);
                break;
            case DocPos::DOC_MID:
                if(std::distance(content.begin()+offset, content.end()) < length)
                {
                    return -1;
                }

                content.erase(content.begin()+offset, content.begin()+offset+length);
                break;
            default:
                return -2;
        }

        return 0;
    }

    friend std::ostream& operator<<(std::ostream& os, const Document& doc)
    {
        os << doc.content;
        return os;
    }
private:
    std::string content;
};

class ICommand
{
public:
    virtual int execute()=0;
    virtual int unexecute()=0;
    virtual ~ICommand() = default;
};

class Append final:  public ICommand
{
public:
    Append(std::shared_ptr<Document> doc_ptr, const std::string& str):m_doc_ptr(doc_ptr), m_str(str){};
    int execute() override
    {
        m_doc_ptr->append(m_str);
        return 0;
    }
    int unexecute() override
    {
        m_doc_ptr->erase(m_str.length(), Document::DocPos::DOC_END);
        return 0;
    }
    ~Append() 
    {
        //std::cout << "append resource is released, str=" << m_str << std::endl;
    }
private:
    std::shared_ptr<Document> m_doc_ptr;
    std::string m_str;
};

class Insert final: public ICommand
{
public:
    Insert(std::shared_ptr<Document> doc_ptr, size_t row, size_t col, const std::string str):m_doc_ptr(doc_ptr),m_row(row),m_col(col),m_str(str),m_ins_pos(std::string::npos){}
    int execute() override
    {
        return m_doc_ptr->insert(m_row, m_col, m_str, m_ins_pos);
    }

    int unexecute() override
    {
        if(std::string::npos == m_ins_pos)
        {
            return -1;
        }

        return m_doc_ptr->erase(m_str.length(), Document::DocPos::DOC_MID, m_ins_pos);
    }
private:
    std::shared_ptr<Document> m_doc_ptr;
    size_t m_row;
    size_t m_col;
    std::string m_str;
    size_t m_ins_pos;
};

class Invoker
{
public:
    Invoker() = default;

    int execute(std::shared_ptr<ICommand> cmd)
    {
        cmd->execute();
        m_history.push_back(cmd);

        m_redo_list.clear();

        return 0;
    }
    
    int undo()
    {
        if(m_history.empty())
        {
            return -1;
        }

        auto& undo = m_history.back();
        undo ->unexecute();      
        m_redo_list.push_back(std::move(undo));
        m_history.pop_back();

        return 0;
    }
    int redo()
    {
        if(m_redo_list.empty())
        {
            return -1;
        }

        auto& redo = m_redo_list.back();
        redo ->execute();      
        m_history.push_back(std::move(redo));
        m_redo_list.pop_back();

        return 0;
    }
private:
    std::deque<std::shared_ptr<ICommand>> m_history;
    std::deque<std::shared_ptr<ICommand>> m_redo_list;
};

int main()
{
    auto diary_ptr = std::make_shared<Document>();
    auto report_ptr = std::make_shared<Document>();

    auto diary_editor = std::unique_ptr<Invoker>(new Invoker);
    auto report_editor = std::unique_ptr<Invoker>(new Invoker);

    report_editor->execute(std::make_shared<Append>(report_ptr, "Work Report\n"));

    diary_editor->execute(std::make_shared<Append>(diary_ptr, "5/2/2024 "));
    diary_editor->execute(std::make_shared<Append>(diary_ptr, "thursday"));
    diary_editor->execute(std::make_shared<Append>(diary_ptr, "\n"));
    std::cout << *diary_ptr << std::endl;

    std::cout << "===insert" << std::endl;
    diary_editor->execute(std::make_shared<Insert>(diary_ptr, 1, 10, "rainy "));
    std::cout << *diary_ptr << std::endl;

    std::cout << "===undo" << std::endl;
    diary_editor->undo();
    std::cout << *diary_ptr << std::endl;

    std::cout << "===redo" << std::endl;
    diary_editor->redo();
    std::cout << *diary_ptr << std::endl;

    std::cout << "===append" << std::endl;
    diary_editor->execute(std::make_shared<Append>(diary_ptr, "A white doggy lies in the sunshine."));
    diary_editor->execute(std::make_shared<Append>(diary_ptr, "An old man worrys about the coming strom repoted by the weather report."));
    std::cout << *diary_ptr << std::endl;

    std::cout << "===undo" << std::endl;
    diary_editor->undo();
    std::cout << *diary_ptr << std::endl;

    std::cout << "===append" << std::endl;
    diary_editor->execute(std::make_shared<Append>(diary_ptr, "Some children run after a scaried cat."));
    std::cout << *diary_ptr << std::endl;

    std::cout << "===insert" << std::endl;
    diary_editor->execute(std::make_shared<Insert>(diary_ptr, 1, 10, " rainy"));
    std::cout << *diary_ptr << std::endl;

    report_editor->execute(std::make_shared<Append>(report_ptr, "1. implenments done\n"));
    report_editor->execute(std::make_shared<Append>(report_ptr, "2. unit tests done\n"));

    auto test_undo_redo = [](std::unique_ptr<Invoker>& editor, std::shared_ptr<Document>& doc_ptr, int times){
        std::stringstream org_ss;
        org_ss << *doc_ptr;


        for(int i=0; i<times; i++)
        {
            editor->undo();
        }

        for(int i=0; i<times; i++)
        {
            editor->redo();
        }

        std::stringstream cur_ss;
        cur_ss << *doc_ptr;

        return cur_ss.str() == org_ss.str();
    };

    if(!test_undo_redo(diary_editor, diary_ptr, 3))
    {
        std::cout << "not euqal" << std::endl;
    }

    if(!test_undo_redo(report_editor, report_ptr, 2))
    {
        std::cout << "not euqal" << std::endl;
    }

    std::cout << *report_ptr << std::endl;

    return 0;
}
\$\endgroup\$
0

2 Answers 2

6
+50
\$\begingroup\$

It's indeed a naive implementation

Using a single std::string to store the contents of a document is a very naive design for a text editor. While it might be good enough for small documents, the problem is that the time most operations will take now linearly scale with the size of the document, so for large documents everything becomes very slow. A slightly less naive way to store the document would be to store the lines of the document in a std::vector<std::string>, as that at least makes navigation very fast, and edits within a line will also be fast (assuming you don't have extremely long lines). But there are data structures designed to make almost all operations fast, regardless of the number of lines or the length of the lines. Have a look at rope and gap buffers, although these are not the only ones.

Make executing commands simpler

Notice how in main() you have to execute commands by first creating a new shared object holding the command. That pattern is repeated over and over. You can avoid that by having execute create the shared object for you. Also, you always provide a pointer to the document to the command. Why not let the invoker handle that as well? Here is how that could look like:

class Invoker {
public:
    Invoker(std::shared_ptr<Document> document_ptr): m_document_ptr(document_ptr) {}

    template<typename T, typename... Args>
    int execute(Args&&... args) {
        auto cmd = std::make_shared<T>(m_document_ptr, std::forward<Args>(args)...);
        cmd->execute();
        …
    }
    …
private:
    std::shared_ptr<Document> m_document_ptr;
    …
};

int main() {
    auto diary_ptr = std::make_shared<Document>();
    …
    auto diary_editor = Invoker(diary_ptr);
    …
    diary_editor.execute<Append>("5/2/2024 ");
    diary_editor.execute<Append>("thursday");
    diary_editor.execute<Append>("\n");
    diary_editor.execute<Insert>(1, 10, "rainy ");
    …
}

Note that if you store a pointer to the document inside Invoker, you actually don't need it anymore in the classes derived from ICommand; instead Invoker can pass it when calling execute() and unexecute().

Naming things

While in the book, the command pattern has an "invoker", that doesn't mean you should name the class that implements an invoker "Invoker". Your Invoker is actually the object that is editing a document, so DocumentEditor would be a much better name for it. Similarly, intead of ICommand, IDocumentEditCommand would be better. To avoid having Document in every name, consider creating a namespace for everything document related, or possibly just move all the rest of the class definitions into the definition of Document itself.

Be consistent with your terminology. You use unexecute() in ICommand and its derived classes, but in Invoker you have undo() and redo(). Either go for execute()/unexecute()/reexecute(), or for do()/undo()/redo().

Avoid shadowing symbols within a function. For example, in undo(), don't create a variable named undo. In this case, I would rename the local variable undo to command.

Don't put the type into the name of a variable. For example, instead of m_doc_ptr, just write m_doc. That it is a pointer is already clear from its type. Even worse is when you only use the type as the name, like in m_str. I would write m_text or m_contents here. And even worse is when the type part is misleading: m_redo_list is not a std::list but a std::deque.

Error handling

You made some effort to return error codes from functions, but it just seems like a token gesture. The return values are ignored everywhere. Also, what do the error codes mean? What is -1 and what is -2?

Ideally, the errors you find should be reported back to the caller in such a way that the caller cannot ignore the error, and that they can determine what went wrong in an easy to understand way. If you can use C++23, I would recommend using std::expected<> as the return type, combined with C++17's [[nodiscard]] attribute and using std::error_code with a custom error class. If you cannot use that yet, I recommend you throw exceptions instead, using your own exception classes derived from std::runtime_error.

Use of smart pointers

You are making very generous use of smart pointers, but there is a cost to them. You don't always need them, and if you do, always check if you can use std::unique_ptr instead of std::shared_ptr, since the latter is more expensive than the former.

For Documents, it's indeed important to ensure that you have a valid object when performing actions on them. You could just not use pointers at all, and just document that any reference to a document you pass to the commands or to the invoker must always be to a valid object. This is not unreasonable. Your method of passing std::shared_ptr<Document> around goes the opposite way, and ensures anything that uses it will always have a pointer to a valid object, but now there is a lot of locking and reference counting done behind the scenes. Another solution is instead to have one class take ownership of a Document, and only allow actions on that document through itself. For example, your invoker could just be made to own the document:

class DocumentEditor {
public:
    DocumentEditor() = default;

    template<typename T, typename... Args>
    int execute(Args&&... args) {
        auto cmd = std::make_shared<T>(std::forward<Args>(args)...);
        cmd->execute(m_document);
        …
    }
    …
private:
    Document m_document;
    …
}

class ICommand
{
public:
    virtual int execute(Document& document) = 0;
    virtual int unexecute(Document& document) = 0;
    virtual ~ICommand() = default;
};

For the commands, it's unlikely that you actually want to share a pointer to a command object, you just to hand a command object over to the invoker, which then takes ownership and stores it in either m_history or m_redo_list. So std::unique_ptr should be enough, you just might need to std::move() it from time to time:

class DocumentEditor {
    …
    template<typename T, typename... Args>
    int execute(Args&&... args) {
        m_history.emplace_back(new T(std::forward<Args>(args)...));
        m_history.back()->execute(m_document);
        …
    }

    int undo() {
        …
        m_history.back()->unexecute();
        m_redo_list.push_back(std::move(m_history.back());
        m_history.pop_back();
        …
    }
    …
private:
    Document m_document;
    std::deque<std::unique_ptr<ICommand>> m_history;
    std::deque<std::unique_ptr<ICommand>> m_redo_list;
}

For diary_editor and report_editor, there was never a reason to use smart pointers for them.

Minor issues

  • Use std::size_t for all indices, counts and sizes. This will fix all of the compiler warnings you get when you enable them (which you should).
  • Use std::make_unique<>() if you can use C++14 or later.
  • Use '\n' instead of std::endl.
\$\endgroup\$
1
  • \$\begingroup\$ Thanks to @G. G. Sliepen's detailed guidelance. I have implenmented most the guidelance, except the runtime execption which will be done later. \$\endgroup\$
    – John
    Commented May 5 at 8:01
0
\$\begingroup\$

Thanks to @G. G. Sliepen's detailed guidelance. I have implenmented most the guidelance, except the runtime execption which will be done later.

For the readers who visit this post later, here is the updated version:

#include <iostream>
#include <string>
#include <cstdint>
#include <algorithm>
#include <cstdint>
#include <memory>
#include <functional>
#include <deque>
#include <limits>
#include <sstream>

namespace Document
{
    class Doc  
    {
    public:
        enum class DocPos:uint8_t
        {
            DOC_END,
            DOC_BGN,
            DOC_MID,
        };

        Doc(const std::string& name):m_name(name){};

        int insert(std::size_t row, std::size_t col, const std::string str, std::size_t& ins_pos)
        {
            std::size_t pos = 0;
            ins_pos = std::string::npos;
            for(int i = 0; i<row-1; i++)
            {
                pos = m_content.find('\n', pos);  //for simpleness, just consider '\n' other than `\r\n` and etc.
                if(std::string::npos == pos)
                {
                    return -1;
                }
            }
        
            auto itr = m_content.begin() + pos;
            for(int j=0; j<col-1; j++)
            {
                if(*itr++=='\n')
                {
                    return -1;
                }
            }

            ins_pos = pos+col-1;
            m_content.insert(ins_pos, str);
            return 0;
        }

        int append(const std::string& str)
        {
            m_content += str;
            return 0;
        }

        int erase(std::size_t length, DocPos pos = DocPos::DOC_END, std::size_t offset=0)
        {
            switch(pos)
            {
                case DocPos::DOC_END:
                    if(m_content.size() < length)
                    {
                        return -1;
                    }
                    m_content.erase(m_content.end()-length, m_content.end());
                    break;
                case DocPos::DOC_BGN:
                    if(m_content.size() < length)
                    {
                        return -1;
                    }
                    m_content.erase(m_content.begin(), m_content.begin()+offset);
                    break;
                case DocPos::DOC_MID:
                    if(std::distance(m_content.begin()+offset, m_content.end()) < length)
                    {
                        return -1;
                    }

                    m_content.erase(m_content.begin()+offset, m_content.begin()+offset+length);
                    break;
                default:
                    return -2;
            }

            return 0;
        }

        friend std::ostream& operator<<(std::ostream& os, const Doc& doc)
        {
            os << doc.m_content;
            return os;
        }
    private:
        std::string m_content;
        std::string m_name;
    };

    class IEditCommand
    {
    public:
        virtual int execute(Doc&)=0;
        virtual int unexecute(Doc&)=0;
        virtual ~IEditCommand() = default;
    };

    class Append final:  public IEditCommand
    {
    public:
        Append(const std::string& str): m_text(str){};
        int execute(Doc& doc) override
        {
            doc.append(m_text);
            return 0;
        }
        int unexecute(Doc& doc) override
        {
            doc.erase(m_text.length(), Doc::DocPos::DOC_END);
            return 0;
        }
        ~Append() 
        {
            //std::cout << "append resource is released, str=" << m_text << std::endl;
        }
    private:
        std::string m_text;
    };

    class Insert final: public IEditCommand
    {
    public:
        Insert(std::size_t row, std::size_t col, const std::string str):m_row(row),m_col(col),m_text(str),m_ins_pos(std::string::npos){}
        int execute(Doc& doc) override
        {
            return doc.insert(m_row, m_col, m_text, m_ins_pos);
        }

        int unexecute(Doc& doc) override
        {
            if(std::string::npos == m_ins_pos)
            {
                return -1;
            }

            return doc.erase(m_text.length(), Doc::DocPos::DOC_MID, m_ins_pos);
        }
    private:
        std::size_t m_row;
        std::size_t m_col;
        std::string m_text;
        std::size_t m_ins_pos;
    };

    class Editor
    {
    public:
        Editor(std::unique_ptr<Doc> doc_ptr) : m_doc(std::move(doc_ptr)) {};

        template<typename T, typename... Args>
        int execute(Args&&... args)
        {
            m_history.emplace_back(new T(std::forward<Args>(args)...));
            m_history.back()->execute(*m_doc);

            m_reexecute.clear();

            return 0;
        }
        
        int unexecute()
        {
            if(m_history.empty())
            {
                return -1;
            }

            auto& unexecute = m_history.back();
            unexecute ->unexecute(*m_doc);      
            m_reexecute.push_back(std::move(unexecute));
            m_history.pop_back();

            return 0;
        }
        int reexecute()
        {
            if(m_reexecute.empty())
            {
                return -1;
            }

            auto& reexecute = m_reexecute.back();
            reexecute ->execute(*m_doc);      
            m_history.push_back(std::move(reexecute));
            m_reexecute.pop_back();

            return 0;
        }

        const std::unique_ptr<Doc>& get_content_ptr() const
        {
            return m_doc;
        }

    private:
        std::deque<std::unique_ptr<IEditCommand>> m_history;
        std::deque<std::unique_ptr<IEditCommand>> m_reexecute;
        std::unique_ptr<Doc> m_doc;
    };
}


int main()
{
    using namespace Document;

    auto diary_ptr = std::unique_ptr<Doc>(new Doc("diay_05_02_2024"));
    auto report_ptr = std::unique_ptr<Doc>(new Doc("work report"));

    Editor diary_editor(std::move(diary_ptr));
    Editor report_editor(std::move(report_ptr));

    report_editor.execute<Append>("Work Report\n");

    diary_editor.execute<Append>("5/2/2024 ");
    diary_editor.execute<Append>("thursday");
    diary_editor.execute<Append>("\n");
    std::cout << *diary_editor.get_content_ptr() << std::endl;

    std::cout << "===insert" << std::endl;
    diary_editor.execute<Insert>(1, 10, "rainy ");
    std::cout << *diary_editor.get_content_ptr() << std::endl;

    std::cout << "===unexecute" << std::endl;
    diary_editor.unexecute();
    std::cout << *diary_editor.get_content_ptr() << std::endl;

    std::cout << "===reexecute" << std::endl;
    diary_editor.reexecute();
    std::cout << *diary_editor.get_content_ptr() << std::endl;

    std::cout << "===append" << std::endl;
    diary_editor.execute<Append>("A white doggy lies in the sunshine.");
    diary_editor.execute<Append>("An old man worrys about the coming strom repoted by the weather report.");
    std::cout << *diary_editor.get_content_ptr() << std::endl;

    std::cout << "===unexecute" << std::endl;
    diary_editor.unexecute();
    std::cout << *diary_editor.get_content_ptr() << std::endl;

    std::cout << "===append" << std::endl;
    diary_editor.execute<Append>("Some children run after a scaried cat.");
    std::cout << *diary_editor.get_content_ptr() << std::endl;

    std::cout << "===insert" << std::endl;
    diary_editor.execute<Insert>(1, 10, " rainy");
    std::cout << *diary_editor.get_content_ptr() << std::endl;

    report_editor.execute<Append>("1. implenments done\n");
    report_editor.execute<Append>("2. unit tests done\n");

    auto test_unexecute_reexecute = [](Editor& editor, int times){
        std::stringstream org_ss;
        org_ss << *editor.get_content_ptr();


        for(int i=0; i<times; i++)
        {
            editor.unexecute();
        }

        for(int i=0; i<times; i++)
        {
            editor.reexecute();
        }

        std::stringstream cur_ss;
        cur_ss <<  *editor.get_content_ptr();

        return cur_ss.str() == org_ss.str();
    };

    if(!test_unexecute_reexecute(diary_editor, 3))
    {
        std::cout << "not euqal" << std::endl;
    }

    if(!test_unexecute_reexecute(report_editor, 2))
    {
        std::cout << "not euqal" << std::endl;
    }

    std::cout << *report_editor.get_content_ptr() << std::endl;
    return 0;
}
\$\endgroup\$
3

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.