Jump to content
  • Advertisement
Sign in to follow this  
Strewya

Opinion needed: returning values without null checks everywhere

This topic is 1678 days old which is more than the 365 day threshold we allow for new replies. Please post a new topic.

If you intended to correct an error in the post then please contact us.

Recommended Posts

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!

Share this post


Link to post
Share on other sites
Advertisement

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.

Share this post


Link to post
Share on other sites

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.

Edited by NightCreature83

Share this post


Link to post
Share on other sites

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.

Share this post


Link to post
Share on other sites

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).

Share this post


Link to post
Share on other sites

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!

Share this post


Link to post
Share on other sites
Sign in to follow this  

  • Advertisement
×

Important Information

By using GameDev.net, you agree to our community Guidelines, Terms of Use, and Privacy Policy.

GameDev.net is your game development community. Create an account for your GameDev Portfolio and participate in the largest developer community in the games industry.

Sign me up!