Hi all.

Here at work, we have a class that stores non-POD objects by raw pointer, and has a get method that returns that raw pointer if the key was found (the storage is an std::map<key, value*> and the get method has a signature value* getThing(const key&); ). The objects are created elsewhere and simply stored in this class for ownership. A note to keep in mind is that c++ exceptions are disabled, and we're not using c++11 yet.

My issue with this is that everywhere the getThing method is called, we need to check for null, since if the value is not found, null is returned.

This annoys me to hell, since it bloats the code everywhere with null checks. I have a few ideas about changing it.

1. Add a 'bool exists(const key&)' method which does the obvious, and change the getThing to return a reference.

- The issue here is if someone calls getThing without checking if the value exists, then i don't know what to return? The .second value of the end iterator? It should cause a crash as soon as someone uses the reference, right?

- Since exceptions are disabled, i can't throw in case nothing was found, so i'm forced to always return something, and end.second seems like the only choice.

- Another problem with this is that the getThing method is const too, so i'm not sure if returning a plain reference will work (i remember it being a compilation error [in some cases]) or if the reference also has to be const, which makes it impossible to change the thing we get. A solution is to remove the const, but isn't nice.

2. Have the getThing method return a bool, and accept a double pointer or reference-to-pointer as an 'out' argument, so if the value is found, true is returned and the out argument is set to point to the found value.

- This seems ok and removes the need for a null check at the call site, but is again using pointers instead of references. I don't have anything against pointers, mind you, but i believe having references in the public interface of a class makes the code look cleaner since it's similar to the rest of it (using references everywhere instead of a mix of pointers and references, i find pointers fine for internals of specific classes).

I'd like to get others opinion on this, another perspective on how this could be done better and/or different. For all i know, maybe the current implementation is fine, and i just need to come to terms with it... Any suggestions are welcome.

Thanks!

Using std::map<key, value*> isn't always as convenient as you want to, because when you access it with operator[] you easily create bogus entries that will bite you later when you iterate the map. You can use find() instead, but the code won't be as pretty (but in general, code working with standard containers is not that pretty, you just have to cope with that fact).

About null values, they can be null for two reasons. Either (1) because of a bug in your code, and in that case use assert to make sure you catch the bug. Or (2) they actually represent the non-existence of something. If that's the case, using pointers and null checks is the most obvious way to do it.

Hi all.

Here at work, we have a class that stores non-POD objects by raw pointer, and has a get method that returns that raw pointer if the key was found (the storage is an std::map<key, value*> and the get method has a signature value* getThing(const key&); ). The objects are created elsewhere and simply stored in this class for ownership. A note to keep in mind is that c++ exceptions are disabled, and we're not using c++11 yet.

My issue with this is that everywhere the getThing method is called, we need to check for null, since if the value is not found, null is returned.

This annoys me to hell, since it bloats the code everywhere with null checks. I have a few ideas about changing it.

1. Add a 'bool exists(const key&)' method which does the obvious, and change the getThing to return a reference.

- The issue here is if someone calls getThing without checking if the value exists, then i don't know what to return? The .second value of the end iterator? It should cause a crash as soon as someone uses the reference, right?

- Since exceptions are disabled, i can't throw in case nothing was found, so i'm forced to always return something, and end.second seems like the only choice.

- Another problem with this is that the getThing method is const too, so i'm not sure if returning a plain reference will work (i remember it being a compilation error [in some cases]) or if the reference also has to be const, which makes it impossible to change the thing we get. A solution is to remove the const, but isn't nice.

2. Have the getThing method return a bool, and accept a double pointer or reference-to-pointer as an 'out' argument, so if the value is found, true is returned and the out argument is set to point to the found value.

- This seems ok and removes the need for a null check at the call site, but is again using pointers instead of references. I don't have anything against pointers, mind you, but i believe having references in the public interface of a class makes the code look cleaner since it's similar to the rest of it (using references everywhere instead of a mix of pointers and references, i find pointers fine for internals of specific classes).

I'd like to get others opinion on this, another perspective on how this could be done better and/or different. For all i know, maybe the current implementation is fine, and i just need to come to terms with it... Any suggestions are welcome.

Thanks!

I prefer getting null pointers back from a cache to be honest, it's not up to the cache to decide how you want to use that object its the calling codes responsibility, it might actually be able to deal with non existing objects. I am currently working on a codebase that asserts when this happens which is even more annoying, libs and upstream code shouldn't care about downstream usage patterns.

Current pattern seems fine to me, you could add an exists method but is not needed, returning a pointer kind of indicates whether a succes or fail has happened in one go, get a null pointer your operation failed.

The other thing you could do is, if all downstream code deals with default value and that value is the same everywhere, is to store default in the map and return a pointer to that when it fails to grab the object you want.

Returning NULL isn't so bad if the referred-to object may not exist. Either that or use boost::optional, but there's hardly a difference.

I try to design around that kind of situation whenever possible, which really comes down to asking questions like:

How does client code manage to get hold keys that don't exist in the map?

What does client code do when it receives a NULL and is there any commonality that may be generalised?

For example, if the client code is not meant to have keys that are not in the map then the NULL response indicates a problem. In which case move the NULL check inside the getThing() function with an assert (or whatever) - you want to die hard and fast.

Another example, if every client does the same thing when receiving a NULL, such as they all call loadThing(), then it's possible to move all that behind a higher-level abstraction, one that auto-loads things that aren't in the map.

The alternative used in languages with a richer type system (Scala, Rust, etc) is 'optional types'. In brief, a variable of type Optional<T> can either contain Some<T> or None(), and the language semantics force you to explicitly handle *both* cases.

This can be simulated in C++ (well, C++11 at least) by having an accessor function that takes a lambda to be invoked if the data exists. There is an interesting investigation in this direction over on github, which might be worth a look (particularly the semantics of maybe_if and otherwise).

Assertions are not an option, we don't have debug/release builds, there's just one build output which goes through 5 levels of testing (unit, component, function, system function, and manual exploratory test).

The keys either exist in the map or don't, together with their associated values. There cannot be a key mapped to NULL. The only thing a get does is do a .find() in the map, and returns null or it.second, depending on whether .find() returned end or not. The keys are supplied by command line, so it is entirely possible for someone to request a non existant key.

The only thing that is done if a null check fails is a trace log in the else block, which is done for each check in a 1-4 levels deep nested ifs that each check a different type, which leads to a lot of code duplication. But, this is another issue i'm working on.

What i was hoping for most is to somehow keep the client code entirely reference based, without having to deal with any pointers in it, but if that's not working, i guess i'll have to compromise.

Thanks for all your input!

If deeply nested ifs are the problem, one option is to use flat conditional null returns.