Decoupling receiving data and processing data by passing callback and shared_ptr

Question

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.

• pacmaninbw: 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.
• John: Sorry, I am new to code review.

Answer 1

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.

• John: "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?
• pacmaninbw: @John The answer is updated.
• John: 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?