Peter Golde's Blog : bool return values

Tuesday, July 13, 2004 5:21 PM pgolde

bool return values

The “AddOrUpdate” and “FindOrAdd” methods both return a bool that indicates which operation ended up being beformed (Add vs Update, Find vs. Add). That leaves the question, which should be true and which should be false? How will people remember easily?

The solution I've settled on (for now), is that all the bool returning methods return “true” if the key was in the dictionary when the method call was made, “false” if the key was not in the dictionary. This applies to:

AddOrUpdate, FindOrAdd, TryGetValue, Remove, and ContainsKey.

Filed under: Peter Golde

Comment Notification

If you would like to receive an email when updates are made to this post, please register here

Subscribe to this post's comments using RSS

Comments

#

Tuesday, July 13, 2004 6:10 PM by Michael Ruck

Why not use an enum? This way it would even be extensible if required at some point.

#

Tuesday, July 13, 2004 6:13 PM by Jeremy Marsch

Would this be a situation where the intent would be more clear if the return value were an enum? Granted, there are only 2 states, and the bool is probably smaller (not sure on that -- is a bool really just a bit, or is it an int underneath...), but the resulting code would certainly be more readable.

#

Tuesday, July 13, 2004 6:23 PM by DarthPedro

I agree with Jeremy for readability's sake go with an enum instead of a bool return value.

#

Tuesday, July 13, 2004 7:40 PM by Cyrus Najmabadi

Peter: Why not use an enum?

#

Tuesday, July 13, 2004 8:37 PM by MartinJ

My question is shouldn't the names be consistent? You have AddOrUpdate and FindOrAdd. Shouldn't the "Add" be in the same place for both methods? Of course, I'm not looking at the full list of methods and such.

#

Wednesday, July 14, 2004 4:51 AM by adam

The method AddOrUpdate should return void. If you want to know which happened then call ContainsKey before-hand. Keep it simple don't add methods to do _everything_ that a user might possibly think of.

// if key already there, returns old value, else adds new value

public bool FindOrAdd(KeyType key, ref ValueType value);

This method just seems a mess to me. If anything the return should be ValueType? (or whatever the syntax is for nullable of type ValueType) and ValueType should not be ref. No on second thoughs this method is too bespoke.

From my point of view the base collection class should provide the core of what is required and not have methods that are simply an amagamation of other methods.

I am assuming that AddOrUpdate will do this,

if (ContainsKey(key))

Update(key, value);

else

Add(key, value);

which is in-fact the same as the indexer.

just my thoughts,

#

Wednesday, July 14, 2004 11:14 AM by Andrey Shchekin

I agree that enum would be a best solution for these methods. But to you really want to implement them this way ? Let's see, we have some parallel methods that differ only on their behavior when value does not exist:

true/false if not there if there

AddOrUpdate Add Update

FindOrAdd Add get_Value

TryGetValue -- get_Value

Remove -- Remove

ContainsKey -- --

The naming is not very obvious, I think. I would prefer something like the following (do not know if my idea is better, but, I think, it is more consistent):

public enum ItemAction // Well, it surely can have a better name

{

Found,

Added,

Updated

}

public class OrderedDictionary<KeyType, ValueType> : IDictionary<KeyType,ValueType>, IDictionary, ICloneable

{

...

// Removing keys

public void Remove(KeyType key); // the simplest removal, if you do not want to think too much

public bool TryRemove(KeyType key); // returns whether the actual removal was performed

public void Clear();

// Adding key/values or updating values for existing key

public void Add(KeyType key, ValueType value); // never updates, but adds only

public bool TryAdd(KeyType key, ValueType value); // adds only and returns whether the addition succeded

public void Update(KeyType key, ValueType value); // nevers adds, but updates existing key only

public bool TryUpdate(KeyType key, ValueType value); // nevers adds and returns whether the update succeded

public ItemAction AddOrUpdate(KeyType key, ValueType value); // adds or updates

// Getting values for the specified keys

public bool TryFetch(KeyType key, out ValueType value); // test for containment and retrieve in one operation

public ItemAction FetchOrAdd(KeyType key, ref ValueType value); // get or add

// The simplest way to access dictionary

public ValueType this[KeyType key] {get; set;} // adds or updates

// Combining with other collections

public void Merge(IDictionary<KeyType,ValueType> dictionary); // add/update everything in the dictionary

public void RemoveCollection(IEnumerable<KeyType> keys); // I would not call it RemoveCollection, because it too

// hard to understand (remove collection of what ?)

// but I do not have a better idea

// Testing for containment (just to have it)

public bool ContainsKey(KeyType key);

...

}

And about View - why not just create an interface and implement it by the OrderedDictionary itself ? I think that Reversed must return an interface that would allow me at least to reverse it. Without thinking about whether it is an original dictionary or a view. So I could write:

public void TestReverse(ISomeInterface dictionary, int step)

{

if(step &lt;= 10)

{

this.TestReverse(dictionary.Reversed, step + 1);

}

else

{

Console.WriteLine("We reversed the dictionary 10 times !");

}

}

#

Wednesday, July 14, 2004 11:44 AM by Ken

If you really like the dual purpose methods (I understand the perf concerns) but dislike the enum return solution (though clear, it does seem to make things feel larger and more complicated), perhaps the methods could be named such that they can be understood without further knowledge about the class. I propose the following scheme:

[A]Else[B](...)

Therefore, AddOrUpdate(...) would become

UpdateElseAdd(...)

Upon seeing that, I think most people would be able to figure out that it would return true if it was able to Update, else it would return false if it had to Add it.

The current naming scheme is not only nonobvious, it's inconsistant from method to method (at least in terms of ordering), offering more chances for confusion.

Personally, given the dual purpose methods and bool return, I'd prefer that the method return true if it did have to create a new entry in the dictionary, ie AddElseUpdate(...), but I can't really formulate a strong argument why that is, other than it makes more intuitive sense to me.

#

Wednesday, July 14, 2004 1:23 PM by Peter Golde

Wow -- lots of great feedback. I love to see it.

While an enum seems nice when you are just looking at the one example, I really think adding an enum for one method is a mistake.

First of all, every enum that is added shows up at the top level of types in the name space and makes it harder to find the types you are really looking for. This may seem like a silly reason, but if you look at usability studies and what users really do, it is clear that the fewer top-level types, the easier it is for users to find what they are really looking for. Second, adding the enum seems like papering over the real problem, which is that the method is too hard to understand at first. So I am highly resistant to adding enum(s) for these kinds of methods.

I think Adam is onto something -- should these methods exist at all? "AddOrUpdate" is the same as the indexer, but just returns whether it added or updated. It seems like the cases where you care about this are pretty rare. We should not be providing two ways to do essentially the same thing.

"FindOrAdd" seems a bit more useful, and less duplicative of other methods. But, it still is fairly rare compared to the other methods. If we do have this functionality, the name can clearly be improved. "GetValueElseAdd" is long, but perhaps the clearest and most consistent with existing names. Do we need this method at all?

The bool return value for TryGetValue doesn't seem confusing.

So, to summarize, my current thinking is:

a) Remove "AddOrUpdate", as it is too much a duplicate of the indexer.

b) Either remove "FindOrAdd", or rename it to "GetValueElseAdd". I would love to get more feedback on whether people think this functionality is useful enough to have a method for.

#

Wednesday, July 14, 2004 2:02 PM by Max

What about overloading Add? The default Add method adds or updates (this seems natural for me) a value whereas the overloaded version gives you finer grained control for the action to perform:

public class OrderedDictionary<KeyType, ValueType> : IDictionary<KeyType,ValueType>, IDictionary

{

...

public enum Action

{

AddOrUpdate,

UpdateOnly

AddOnly,

Whatever,

}

public void Add(KeyType key, ValueType value);

public void Add(KeyType key, ValueType value, Action action);

//same with TryAdd here...

}

Sure you now no longer have a return value indicating the action actually performed. But is it really that important? If so one could make two calls. One TryAdd(key, value, OrderedDictionary.Action.UpdateOnly) and if that fails another call to Add(key, value, OrderedDictionary.Action.AddOnly).

btw I prefer "Replace" over "Update"

#

Wednesday, July 14, 2004 3:20 PM by Gregory T. Loker

I don't think either of these 2 methods is necessary at all.

If you get in the habit of providing what are essentially combination methods, how do you justify which combinations you will include, and which you won't? I think this is a case where the person best suited to determining what convenience methods are required is the class consumer rather than the class developer(s).

Further, while I'm posing... I agree with Max's comment about using Replace instead of Update. Update (to me) would make more sense if you planned on actually modifying the current value instance, rather than actually replacing the value instance with a new one.

Finally:

public bool Remove(KeyType key);

I would rather see this as:

public ValueType Remove(KeyType key);

The ValueType returned here would clearly be either the value that was in the corresponding position, or null. My reasoning here is simply that I may need to do some custom cleanup with the value object even though I immediately want it out of the collection.

#

Wednesday, July 14, 2004 4:49 PM by Max

> If you get in the habit of providing what are essentially combination

> methods, how do you justify which combinations you will include,

> and which you won't?

I don't think the problem is to justify the combinations. The problem is whether performance matters or not. If performance doesn't matter the whole discussion of TryAdd, TryRemove, AddOrReplace, TryToFly and so on would be meaningless. You could e.g. call ContainsKey prior to Add if you want an "insert but don't replace" behaviour.

> I would rather see this as:

> public ValueType Remove(KeyType key);

This could be a handy feature but it would introduce some trouble if the Dictionary contains value types. What should Remove return if there doesn't exist a value with the specified key? One could throw an exception in those cases but in cases where you are not interested in whether key was actually removed or never in the collection you have to "eat" an exception - not very good.

But again this is a question of performance. If performance doesn't matter you could call TryGetValue prior to Remove.

#

Wednesday, July 14, 2004 8:46 PM by Cyrus Najmabadi

Peter: "First of all, every enum that is added shows up at the top level of types in the name space and makes it harder to find the types you are really looking for. This may seem like a silly reason, but if you look at usability studies and what users really do, it is clear that the fewer top-level types, the easier it is for users to find what they are really looking for."

This is a silly reason :-)

The enum would neer be placed in the namespace. It's specific to your type and would exist as a nested type in this case.

i.e.:

class OrderedDictionary {

public enum /*insert good name here*/ { ... }

}

#

Wednesday, July 14, 2004 8:50 PM by Cyrus Najmabadi

Max: "This could be a handy feature but it would introduce some trouble if the Dictionary contains value types. What should Remove return if there doesn't exist a value with the specified key?"

Instead of having:

public V Remove(K key), you should have:

public IOptional<V> Remove(K key)

where IOptional<T> is defined as:

interface IOptional<T> {

T Value { get; }

bool HasValue { get; }

}

that way you can distinguish between having and not having a value.

Jay and I have blogged extensively on this in the past:

<a target="_new" href="http://blogs.msdn.com/jaybaz_ms">http://blogs.msdn.com/jaybaz_ms</a>

<a target="_new" href="http://blogs.msdn.com/cyrusn">http://blogs.msdn.com/cyrusn</a>

This eliminates all the duplication that comes up in the API with needs to return a value and a bool stating if that value is valid or not.

#

Friday, July 16, 2004 2:31 AM by Peter Golde

Max: I think I like "Replace" better than "Update" also.

#

Friday, July 16, 2004 5:54 AM by Michael Teper

Please, PLEASE, don't go the bool route. I cringe every time I see a method return a bool as an arbitrary measure of success.

#

Tuesday, November 09, 2004 2:09 AM by V. Richards

Most functions do not need more than one return value, but when you need more there’s no easy way around it. Java has this problem, functions are limited to one return value, and in my experience it is often complicated to get along with this limitation. In most cases you either write more functions than necessary or you need to create a additional classes that need even more constructors, accessor methods and documentation.

C++ and C# allow multiple return values using pointers and references. This solves the problem, but I think it feels wrong and especially the C++ syntax does not create very readable code. So I am going to show you an alternative, derived from Python’s tuples.

Disadvantages:

Neither declaration nor invocation syntax indicate whether ’success’ is really a return value. It may also be just an optimization for an input value (admittedly unlikely in this example) or may be both input and output value. Only the documentation and the implementation can help

You can not find out whether null is allowed for ’success’ without looking at the documentation or the implementation

The compiler won’t catch a bug if ’success’ is not initialized before returning in some code paths, because it does not know the purpose of ’success’

Disadvantages:

You still need to look at documentation/implementation to find out what success is good for

The compiler will still not notice when success has not been set before returning, and the check whether ’success’ is null adds another potential error

Two additional lines-of-code were needed in the implementation to make success optional.

Advantages:

References do not have the ‘null’ issue or other pointer problems

Disadvantages:

The invocation does not have any hint that the second argument will be modified. This can make code very hard to read if you do not know the functions, because any function may modify any argument

You still do not know whether ’success’ is a input or an output value

Default values are not possible, you always need to have a bool even if you do not look at it

The compiler won’t notice the bug when ’success’ is not initialized in some code paths, because it does not know the purpose of ’success’

Advantages:

It’s obvious in declaration and invocation that ’success’ is an output argument (in/out arguments use the keyword ‘ref’

The compiler can check whether ’success’ has been set by the function before returning

There are no pointer issues

Disadvantages:

Default arguments are not possible (a C# limitation)

You always need to declare the bool before invoking the function.

Name (required)
Your URL (optional)
Comments (required)

Please enter the answer to the math question here: (required)
Remember Me?

Search

Tags

Navigation

Archives