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:
- 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()
. - Could
std::optional
help? Not sure. - 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. - Wrap my pointer object inside a smartpointer since I don't want to upheave the basic foundations right now?
trylock()
returns null? Currently, it seems each of your functions is stopping its execution with a boolean failure return code whenever the innertrylock()
has failed. This is precisely the situation where exceptions provide a less tedious alternative for.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.getSession
returning a null pointer.