2

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?
5
  • 4
    What about throwing an exception in case trylock() returns null? Currently, it seems each of your functions is stopping its execution with a boolean failure return code whenever the inner trylock() has failed. This is precisely the situation where exceptions provide a less tedious alternative for.
    – Doc Brown
    Feb 15 at 6:21
  • 1
    ... however, I think it is pretty opinionated what counts as "major refactoring" (at least, it is hard to assess how large the impact on your codebase would be without knowing more about it).
    – Doc Brown
    Feb 15 at 8:15
  • @DocBrown So the getSession function is used in a lot of different places - basically our service needs a Session to do anything. And we have threads running in parallel trying to access different Sessions, get their locks, do some work etc. I don't really want to mess with the getSession function since the effect will be very large, also I have limited knowledge on thread safety so I'll be unsure if my new solution won't break anything that way. As far as exceptions go, we have to work around contention, so if a lock fails we cant just throw, we have some logic that allows us to recover.
    – Siddiqui
    Feb 15 at 17:49
  • This question might be moot if getSession either threw an exception if it cannot get a session, or simply did not return until a session was free. Basically, getSession should return a session or throw. And it should not return until a session is found, or some timeout period has elapsed. But I must admit my c++ knowledge is rusty. Feb 17 at 19:05
  • The real problem feels like it is caused by getSession returning a null pointer. Feb 17 at 19:07

2 Answers 2

3

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.

4
  • I wanted to write an answer with a macro that could be called as "AssertReturnFalse(ptr != nullptr, "SessionPtr is null, cannot continue"); So that can be used everywhere in your code, without having to write a new macro for every new situation. Either way, a macro like that increases the chance that checking actually is done.
    – gnasher729
    Feb 15 at 22:26
  • Haha, I kinda hate this too but it seems like one of the only options I have - will be marking it as the answer if no one has any other ideas
    – Siddiqui
    Feb 16 at 0:24
  • One way to make the macro more versatile is to accept a second argument that is a piece of code (optionally enclosed in curly braces) that is executed in case the pointer is null. That way, if some old code prefers { return false; } and some new code prefers { return something_else; } it can still be handled by the same macro. Note that adding multiple responsibilities to a macro can be confusing, so use with care.
    – rwong
    Feb 16 at 5:44
  • There is nothing in modern C++ that does it better, so no need to hate. :-) Incidentally, I think wrapping up conditional return is one of the areas where macros come in really handy. Had similar issues in some C# projects, and it was quite a pain there.
    – Martin Ba
    Feb 17 at 8:32
1

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?

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.