2
\$\begingroup\$

Sorry for my poor English. I want to decouple receiving data and processing data, this is the demo code:

#include<memory>
#include<iostream>
#include<functional>

using message_handler=std::function<void(const std::string&)>;

class Session: public std::enable_shared_from_this<Session>
{
class PacketProc
{
public:
    void SetOnMsgProc(const message_handler &&on_msg_func)
    {
        this->inner_on_msg_func = on_msg_func;
    }

    void ProcRcved(const std::string &str)
    {
        if(inner_on_msg_func)
        {
            inner_on_msg_func(str);
        }
    }

private:
    message_handler inner_on_msg_func; 
};

public:
    ~Session()
    {
        std::cout << "~Session()" << std::endl;
    }

    void Init(void)
    {
        std::weak_ptr<Session> weak=shared_from_this();
        packet_proc.SetOnMsgProc([weak](const std::string & str){
            auto shared = weak.lock();
            if(shared && shared->outter_on_msg_func)
            {
                shared->outter_on_msg_func(str);
            }
        });

        outter_on_msg_func= [](const std::string & str)
        {
            std::cout << "recieved:" << str << std::endl;
        };
    }

    void Proc(void)
    {
        std::string str = read();
        packet_proc.ProcRcved(str);
    }

    std::string read(void)
    {
        return std::string("this is a test");
    }

private:
    PacketProc packet_proc;
    message_handler outter_on_msg_func;
};

int main()
{
    std::shared_ptr<Session> pSession= std::make_shared<Session>();
    pSession->Init();
    pSession->Proc();
}

Any suggestion and advice is welcome.

\$\endgroup\$
2
  • 2
    \$\begingroup\$ Please do not edit the question, especially the code after an answer has been posted. Everyone needs to be able to see what the reviewer was referring to. What to do after the question has been answered. \$\endgroup\$
    – pacmaninbw
    Jul 4 '21 at 13:33
  • \$\begingroup\$ Sorry, I am new to code review. \$\endgroup\$
    – John
    Jul 4 '21 at 13:36
2
\$\begingroup\$

The declaration for the class PacketProc should be indented, right now the code is hard to read.

class Session : public std::enable_shared_from_this<Session>
{
    class PacketProc
    {
    public:
        void SetOnMsgProc(const message_handler&& on_msg_func)
        {
            this->inner_on_msg_func = on_msg_func;
        }

        void ProcRcved(const std::string& str)
        {
            if (inner_on_msg_func)
            {
                inner_on_msg_func(str);
            }
        }

    private:
        message_handler inner_on_msg_func;
    };

public:
    ~Session()
    {
        std::cout << "~Session()" << std::endl;
    }

    ...
};

If the class PacketProc is supposed to be reusable then it should be declared outside the class Session, right now it can only be accessed through the Session class.

To follow best practices, operators such as = should have blank spaces on both sides:

using message_handler = std::function<void(const std::string&)>;

That would also improve the readability of the code.

Using the this pointer really shouldn't be necessary in this code:

                void SetOnMsgProc(const message_handler&& on_msg_func)
                {
                    inner_on_msg_func = on_msg_func;
                }

Inside a class it generally isn't necessary to use the this pointer, it is the default. The only time this isn't true is is if an identifier has 2 possible uses within the same scope of the code, such as if inner_on_msg_func was defined as a global in the Session class and a local in the PacketProc class.

\$\endgroup\$
3
  • \$\begingroup\$ "Using the this pointer really shouldn't be necessary in this code."? I can't catch up. Could you please explain that in more detail? \$\endgroup\$
    – John
    Jul 4 '21 at 13:24
  • \$\begingroup\$ @John The answer is updated. \$\endgroup\$
    – pacmaninbw
    Jul 4 '21 at 13:32
  • \$\begingroup\$ The lambda captures a weak pointer (for Session) in Session::SetOnMsgProc(). Do you think it is redundant? Is there any potential problem that I should be aware of? \$\endgroup\$
    – John
    Aug 22 '21 at 4:22

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.