Sign in to follow this  
c_olin

Elegant way of returning possibly non-existant values..

Recommended Posts

I'm using an std::map of boost:anys to store configuration variables for my game. The function prototypes should give an idea of what I am doing:
template <class T>
int variableExists(const string& variableName); // Returns -1 if the variable doesn't exist, 0 if it exists and is not of type T, 1 if it exists and is of type T.

template <class T>
void setVariable(const string& variableName, T value); // Sets the variable to the value.

template <class T>
T getVariable(const string& variableName); // Returns the value of the variable if it exists.


The problem is that in getVariable(), the given variable might not exist, or might not be of that type. For example:
setVariable<int>("test_int", 23);
int test = getVariable<int>("not_test_int"); // not_test_int does not exist, what to return?


I implemented a simple template class that contains a value of type T and a boolean stating whether or not the value is valid. This works, but I'm interested if there are more elegant ways of doing this, or if any other structures exist in the std or boost library that may help. Thanks

Share this post


Link to post
Share on other sites
I would combine your variableExists and getVariable into one function to get the variable.

// Returns -1 if the variable doesn't exist
// 0 if it exists and is not of type T
// 1 if it exists and is of type T.
// If 1 is returned, you may safely use the contents of 'value'.
// Otherwise, the contents are undefined.
template <class T>
int getVariable(const string& variableName, T & value);




Or, I forgot about this approach as well, if you want to switch things around:

// isValid stores the result of the operating.
// 0 if it exists and is not of type T
// 1 if it exists and is of type T.
// If isValid is 1, you may safely use the contents returned.
// Otherwise, the contents are undefined.
template <class T>
T getVariable(const string& variableName, bool & isValid);

Share this post


Link to post
Share on other sites
emplate <class T>
T getVariable(const string& variableName, T default_value) {
if (exists(variableName) {
return get(variableName)
} else {
return default_value;
}
}


However, this is an answer to a possibly incorrect question.

Assume you have server configuration file, with the following line:
port=2345

What all can go wrong? What do you expect the result to be.
- port entry is missing. Does it make sense to continue, or use a default value? Usually, no, throw an exception
- port value is unparsable. Assume a default value? Generally no, typos happen, and admin will be wondering why the server is running, but cannot be contacted. Again, throw exception.
- port value is unexpected (1234567). This is logical error, but not something configuration can determine by itself. Return, let caller work it out.

- Optionally, implement parsing as part of type, put default value in there, in case it is unable to parse, or logical error occurs, throw an exception.

IMHO, if an optional field is missing, provide default value.
If a required field is missing, throw an exception.



Also - there is boost::program_options, which solves all of the above problems already.

Share this post


Link to post
Share on other sites
Quote:
Original post by Drew_Benton
I would combine your variableExists and getVariable into one function to get the variable.
*** Source Snippet Removed ***


On the other hand... this method is quite slick. I will implement both and see how I like it.

Thanks for the replies.

Share this post


Link to post
Share on other sites

bool variableExists(const string& variableName);

template <class T>
bool variableMatchesType( const string &variableName );

template <class T>
void setVariable(const string& variableName, T value); // Sets the variable to the value.

template <class T>
T getVariable(const string& variableName); // Returns the value of the variable if it exists, exception if it doesn't.

// OR

template <class T>
*T getVariable(const string& variableName); // Returns the value of the variable if it exists, null if it doesn't.

// OR

template <class T>
&T getVariable(const string& variableName, const T& defaultValue); // Returns the value of the variable if it exists, default if it doesn't.




[edit: personally I'd err towards exceptions. You're not using C, do it right.]

Share this post


Link to post
Share on other sites
Quote:
Original post by Drew_Benton
Or, I forgot about this approach as well, if you want to switch things around:
*** Source Snippet Removed ***


I prefer the first one you mention, because I like the syntax of using it.


int test_value;
if (getVariable("test_value", &test_value)) {
cout << "test_value: " << test_value << endl;
} else {
// Throw exception or use default value.
}


Share this post


Link to post
Share on other sites
I've never really used exceptions in C++. Out of curiosity, is this what you guys have in mind when you suggest to use exceptions?


template <class T>
T getVariable(const string& variableName) throw(runtime_error) {
if (variableDoesNotExistOrIsWrongType)
throw(runtime_error("Wrong type or doesn't exist"));
else
return value;
}

int value;
try {
value = getVariable("test_int");
} catch (runtime_error& e) {
// Deal with error.
}

Share this post


Link to post
Share on other sites
Quote:
Original post by c_olin
I've never really used exceptions in C++. Out of curiosity, is this what you guys have in mind when you suggest to use exceptions?

*** Source Snippet Removed ***


Something like that, except you should probably derive your own type from std::runtime_error so that you can distinguish between other types of exceptions.

Share this post


Link to post
Share on other sites
Quote:
Original post by c_olin
I've never really used exceptions in C++. Out of curiosity, is this what you guys have in mind when you suggest to use exceptions?

*** Source Snippet Removed ***


I prefer RAII:
class Foo {
Foo(const Configuration & c)
: name(c.get("name", "no_name")
, address(c.get("address") // throws
, age(c.get("age") // throws
{
if (age < 18) throw std::exception("Too young");
if (age > 100) age = 100;
}
};

int main() {
try {
Configuration c("some_config.txt");
Foo foo(c); // either foo is created and in valid state

foo.do_something();
} catch ( /* something */ ) {
// error occured, application can't continue
// exit
}
}


The difference is - I let Foo decide what is valid or not, as well as how to handle unusual problems, all in constructor. If it succeeds, we're fine. If not, Foo is broken and can't be created.

This keeps rules in one place where they make sense. No point in creating a class, then letting user wonder what went wrong and how.

Share this post


Link to post
Share on other sites
Like explained above, exceptions, or assertions (release time assert), is the right way of doing this in C++. All you want is ensure the user calls the isvalid method before trying to get the value, otherwise the user has made a programming error.

Also, consider changing the signature of your method that gets the status. Why encode things in -1, 0, 1 return values etc? Aside from encoding this in an enum which is a good way of making obscure codes more readable, why not go one step further by creating two separate methods that return true/false instead: one called "isValid" that returns true if and only if it's the right type and it exists, and another method called "exists" that merely checks if the data exists?

Bunching together two logical queries into one and returning the answer in a code just makes the code harder to understand, makes coding new features slower, maintenance more expensive, and in this particular case performance will also suffer (e.g. if all you want to check for is existence, it will still perform the type equality check, and you also need to add a big if-else if-else or switch construct to handle the return codes at the call site, and finally any code that uses codes just begs to be extended at some later stage by another programmer who wants to add a new code for a new case, which leads to problems unless the first programmer realized that this extension would happen and added a proper assert(false) in the else statement/default case).

[Edited by - sb01234 on June 6, 2009 8:37:44 AM]

Share this post


Link to post
Share on other sites

Create an account or sign in to comment

You need to be a member in order to leave a comment

Create an account

Sign up for a new account in our community. It's easy!

Register a new account

Sign in

Already have an account? Sign in here.

Sign In Now

Sign in to follow this