How to handle duplicate null checks

Question

I'm working with a legacy codebase that has a lot of functions with a nullcheck on the same object. Example:

std::vector<SessionNode*> * SessionManager::SessionMap;

Session * getSession(int SessionId)
{
  //Try to find the Session
  //And get the lock
  int index1 = getIndex1FromSessionId();
  int index2 = getIndex2FromSessionId();
  auto ptr = SessionMap[index1][index2]->trylock();
  return ptr;
}

Now I have a lot of functions calling this one function and they're all checking, logging and returning if ptr is nullptr.

bool SessionManager::validateMouseRequest(int SessionId)
{
   auto ptr = getSession(SessionId);
   if (ptr == nullptr)
   {
      //We even have different log lines for the same
      //nullcheck ...
      LOG(__func__, "SessionPtr for this request, cannot continue");
      return false;
   }

   //doSomethingElse
}

bool SessionManager::validateKeyboardRequest(int SessionId, ...)
{
   auto ptr = getSession(SessionId);
   if (ptr == nullptr)
   {
      LOG(__func__, "SessionPtr is null, cannot continue");
      return false;
   }
   
   //doSomethingElse
}

//How these functions are called:

//Request parent class with some
//default methods
class Request
{
};

class MouseRequest : Request
{ 
  private: 
     //Mouse Request event is
     //received from some other service 
     //This request is initialized and pushed into a
     //queue and createTask is called when the thread
     //gets to process this request
     MouseRequestData Data;
   
  //Creating a task object with 
  //this MouseRequest's Data
  Task * createTask()
  {
    //doingSomeWork
    bool validationSuccessful = validateMouseRequest(Data.request_id, ...);
    if (validationSuccessful)
    //doSomeWork
    else
      return nullptr;
  }
};

class KeyboardRequest : Request
{ 
  private:  
     KeyboardData Data;
   
  Task * createTask()
  {
    //doingSomeWork
    bool validationSuccessful = validateKeyboardRequest(Data.request_id, ...);
    if (validationSuccessful)
    //doSomeWork
    else
      return nullptr;
  }
};

Is there a way to avoid the duplication of this nullcheck without major refactoring?

My thoughts:

1. Overload this function and take the second argument as a functor. The calling function could then define the functor and the nullcheck executes first and is only in one place. But I feel like this is too messy and rigid: what if there's some work that I want to execute before my nullcheck? I would need another overloaded getSession().
2. Could std::optional help? Not sure.
3. Create another function that would wrap getSession() and check for null and log an error. But then I'd be in the same position that I started in: I'd just be checking the return of that function instead.
4. Wrap my pointer object inside a smartpointer since I don't want to upheave the basic foundations right now?

Answer 1

Modern C++ fans will probably hate me for this, but I think your best bet here is to go route #3 - just not with a function, but with an old-style preprocessor macro:

#define GetSessionOrReturn(SessionID) auto ptr = getSession((SessionId));\
   if (ptr == nullptr)\
   {\
      LOG(__func__, "SessionPtr is null, cannot continue"); \
      return false;\
   }

(If you really need the log message variable, make it a parameter).

Of course, you may see this as an ugly hack, and it may be debatable if that is worth the hassle, but it will definitely help you to reduce the code duplication, and you don't have to change the logic in your code that way.

Nethertheless you may consider to provide a new function getSessionEx which is going to replace getSession gradually over time. getSessionEx should throw an exception when it cannot get a lock, which may allow to reduce the error handling/logging at each of the intermediate levels of the calling sequence. That allows centralizing it at some upper level.

Answer 2

If you are willing to do some refactoring, one way is to return the session as a reference (instead of a pointer to it). This implicitly guarantees to each caller that it is valid, and removes the need to test for nullptr.
Inside the method, you would throw if it fails, catching this on a much higher level in the code. You wouldn’t have the info where it happened - but does it matter if the session was lost during mouse click or during mouse move?

This comes with the disadvantage of bringing the full session details inside the calling code, creating a dependency - but maybe that dependcy is already there anyway?