Nah, the nested ifs aren't a problem, they are a replacement for multiple return statements per function, which is something i'm attempting to avoid.
Opinion needed: returning values without null checks everywhere
If the only way that client code can call with invalid keys, sanitize that input in a central place before passing it on to the rest of the program. Sounds like after that input has been checked, it would be a program fault if an invalid key was supplied, so then you can handle that with exceptions and return references to your data.
Sticking to pointers and checking null everywhere because there may have been an invalid key supplied at the command line seems wrong to me.
That's fair, but it basically boils down to assert-on-invalid. You really need the lambda support to provide sane fallback semantics.Wouldn't a boost::optional be a valid C++ equivalent even without C++11?
Without knowing what you're doing with the objects in your map, I can't say if this will be a valid resolution for you or not, but what I've done in some cases (for example, with a map of commands), is have one 'empty' object, that can be manipulated the same way any other object in the map can be, it just either does nothing, or does some default 'empty' behavior. For example, in the case of a map of commands, the empty command would simply send 'command not found' to the appropriate output. These empty objects don't need to be in the map itself. If you have some higher level container wrapping your map, that container can contain an instance of the 'empty' object, and return a pointer to it when find() fails to find the entry in the map.
This doesn't work if the object returned NEEDS to perform some nontrivial actions to be usable. Or, at least, it doesn't work as well. At that point the object is no longer empty. It may still be an acceptable resolution, it just becomes more of a default object as opposed to an empty object.
This would allow you to return references instead of pointers as well, as you are then always returning an actual object
If the only way that client code can call with invalid keys, sanitize that input in a central place before passing it on to the rest of the program. Sounds like after that input has been checked, it would be a program fault if an invalid key was supplied, so then you can handle that with exceptions and return references to your data.
Sticking to pointers and checking null everywhere because there may have been an invalid key supplied at the command line seems wrong to me.
Agreed.
So while you'll probably still need some way of checking that the colleciton contains the key->value mapping, you only need to do it once when the key is received from the user.
Speaking in terms of a generic collection object, a return value of NULL from your function value* getThing(const key&); ) could mean that the key is mapped onto NULL or that the key is not in the map. You've already mentioned that keys will not be mapped onto null values, so in this instance a return value of NULL can only mean that the key is not in the collection. However will that always be the case? Are you intending to re-use the collection? Also someone updating the code in the future may not immediatly appreciate that a return value of NULL can only be generated when the key isn't mapped. Therefore, while NULL checks would work, I think adding explicit 'exists' checks could make your code easier to read and make the collection class more re-usable.
Both of your suggestions look like fine choices.
Another option you might want to consider, (although I'd still go with the 'bool exists(const key&)' suggestion) is to have your collection return a default object when the key isn't stored. The object would be an instance of a default class, extending the class of the values stored in the collection, but stubbing out all procedures so that they do nothing and all functions so that they return neutral values. Then the rest of your code can process the default object as if a key-value map was found, removing the need for any 'exists' or 'is NULL' checks. (The NULL Object pattern, from Robert Martin's book 'Agile Softwaree Development: Principles, Patterns and Practices')
I've seen it done in game engines. It doesn't solve the problem.... have one 'empty' object ... then always returning an actual object
First it just migrates the NULL test. In some ways that is good as it consolidates the logic. But every function call you still end up doing a test for search failure and still end up taking an alternate action. That means that you don't actually save any cycles for testing for a NULL search result, you just handle it inside the function instead of outside the function, the count of NULL tests remains the same. In fact, it is often very slightly worse because now you are not just testing for failure, you are also potentially constructing a useless dummy game object.
Second, very often it just exchanges a test for NULL with a test for the special dummy object. I remember writing hundreds of times variants of "if(result.Type == ObjectType.FailureObject)". It is basically identical to testing against NULL, except it is a special dummy object just in case somebody forgets to check for a bad result. All the operations on a FailureObject just assert in dev and QA builds so the programmers can fix the bug, but we get a silent failure in the final build rather than a null-pointer crash.
It is just a different variant of the identical problem. In all cases you still need to test for the condition of a missing object and code defensively.
Objective-C has an interesting take on this problem. If you have a nil object reference, calling a method is a no-op, and calling a function returns a default value (i.e. zero, false, or nil). Good coding practice is *not* checking the object for nil, but instead just send the message and checking the return value instead. Unfortunately there's no simple equivalent construction in C++.
Using the Null Object pattern would be fine if the logic has to be the same no matter whether the object exists or not, but this is not the case. Non existance has to be logged, and the client code needs to exit without a crash (effectively informing the user via the trace log that the key entered was invalid), and not continue processing something that doesn't actually exist pretending everything is okay.
An idea popped into my head about using the Null Object as a logging device, so that method calls actually log the error, but the problem in that case is that client code thinks everything is okay, and continues to the next positive case, when it should have exited instead. And besides, the objects contain other data, and some objects do non-trivial operations, so it's not applicable in all cases.
EDIT:
I forgot to comment Aardvajk's suggestion about sanitizing the input. The input is sanitized in one location purely on a syntactic level, making sure that string arguments are strings, integers are really numbers, and the correct sub-command was invoked. When the data from the command arrives to our part of the code, it's all potentially fine until we check something is actually mapped to that key (the key is a hash string).
Creating a 'ThingExists' function, and checking its result is the equivalent of checking for a NULL result from the 'GetThing' function, at least in my experience. ThingExists could be implemented by calling GetThing and performing the NULL check there.
Creating a 'ThingExists' function, and checking its result is the equivalent of checking for a NULL result from the 'GetThing' function, at least in my experience. ThingExists could be implemented by calling GetThing and performing the NULL check there.
Not so good in practice.
Doing the query to check for existence is usually identical to the code to get the object. For example, ThingExists() could be trivially implemented as { return GetThing() != NULL; }.
Better to do the call only once rather than twice.
If you have to do two sets of queries, the first is existential the second is acquisition, it is usually better to write just a single acquisition query that gets the final result and then tests for failure.
A similar example in C#, don't do both "is" and "as" operations, just do the "as" and check for null.