You Can Do It! (But that doesn't really mean that you should)
Last week an interesting discussion came up. During a code review with a co-worker, we couldn't agree on a code change. He wanted to change some code to do a certain thing, and my instant reaction was HELL NO!
In the course of trying to explain to him why it was not a good idea, I tried to find a discussion on best practices for it, but while I found several mentions of the possibility, I couldn't find anyone that had any recommendations. I really just wanted to know if my gut was telling me the right thing or not, so I decided to throw the discussion open to the internal codebetter mailing list. Once I had a general consensus, I figured whether I was right nor not, I'd have a few trustworthy opinions for my co-worker and I to read and discuss. This mailing list is a group mail that goes to every single blogger on codebetter, and it seemed a good way to get the feel for peoples answers.
If was an interesting discussion. As with all conversations, sometimes we got sidetracked, but even those sidetracks were a good read.
So now I have an easy post to make. Instead of writing it all, I only need to show the relevant parts of the discussion that we had, and then leave you to draw your own conclusions. I've done a little cleaning up of some typos and stuff, and certain peices of information have been destroyed in an attempt to protect the innocent, but the following is a prett faithful rendition of what went on. (All I really cut out was Jay's repeated encouragement to get this whole conversation blogged). What was cool is that it got almost all of use involved, which meant I got a pretty clear picture of a pretty large size chunk of people.
From: Geoff
Subject: Am I Wrong?
Before you just reply with 'yes', actually read my question first :)
A wierd one came up in a code review today.
There's this class, and it has about 3 public overloads of the same
method - there's no inheritance involved here, just pain old functions
with the same name and different parameter sets.
eg.
Public Function MyFunc(someParam as String) as MyCustomClass
Public Function MyFunc(someDifferentParam as Type, someParam as
String) as MyCustomClass
Public Function MyFunc(someThingElseEntirely as SomeOtherClass) as MyCustomClass
These are all exposed on the one class.
Without going into specifics, with one version of these overloads a
co-worker of mine is arguing that it should really be returning boolean. Now, yes,
this overload in question could return the boolean, but that's not
my problem.
My problem is if it should return a boolean.
It's entirely legal to have different overloads returning different
return types - I'm not saying you can't.
But to me it feels wrong. I think that API design should involve some
consistency - if you call a method named X, no matter how many
overloads there are, X will return Y. I'm willing to allow (and have
done) cases where X actually returns Z, but only so long as Y appears
somewhere in Z's inheritance chain, and normally this is when overriding
the method in a base class, not within the same class.
I can't really explain it more than that - it feels wrong to return
different types that aren't related. There's cases in the framework
where different overloads are either Subs (void functions for you
C#ers) or Functions, but all the functions have the same return type
in these ones (Add method on different specialised collections, for
example).
Does anyone have any opinions?
--Geoff
From: John Papa
I agree with you for the most part ... Although I don't think it's a
hard and fast rule either. In general, I like my overloads to either (a)
return the same datatype or (b) return void (Subs in VB.NET). If the
overload should return a boolean in your case, I would name the function
something different because to me, it does something different.
Just one man's opinion of course ... But I say stick to your guns.
From: Raymond Lewallen
For safety sake, I personally always return the same type from
overloaded function. Certainly intellisense makes it easy to know if
you're getting something different back, but by returning the same
type, it helps to avoid confusion. My opinion is its good practice to
be uniform in your return types. Later on, if you've written a bunch
of code to use a method that returns a string array, its pretty easy
to change to a different overloaded method of the same name and expect
the same type back. Otherwise, when you simply call a different
overloaded method of the same name, you're looking at rewritting a
bunch of code to handle a different return type, such as a datatable
instead, and that just doesn't feel good.
From: Geoff
Yes, I forgot to mention that that is definately something we agreed
on (myself and co-worker) - a different method name would make us both
feel happy :)
From: David Hayden
Egads. You are absolutely correct, Geoff.
The fact that you change the return type means it is no longer an overloaded
method in my opinion and would require you to rename the method to something
else. As we all know, overloaded methods have to do with varying parameters
and has nothing to do with return types.
Let's say you do this:
public string DoIt(string paramA)
public string DoIt(string paramA, string paramB)
public int DoIt(string paramA, string paramB, string ParamC)
now if you wanted to offer the 3rd method above without paramC:
public int DotIt(string paramA, string paramB)
you couldn't. It would clash with an already existing signature that
returns string. It would give you a compiler error, because changing only
the return type is not considered overloading a method. You couldn't offer
this signature to the client, which should tell you right there that
somebody screwed up.
From: Jeffrey Palermo
I agree with consistent return types, but I would look at another
issue first. I've encountered classes where three overloads were
provided in case they were needed. In reality, only one overload was
ever called, and the other two ended up being unused, untested code.
I'd look at that issue as well. In the code review, if all three
overloads aren't currently being used, I'd delete the unused code.
This situation doesn't occur if code is test-driven.
From: Geoff
I totally agree. Yes, all the methods are in use, and will be for some
time to come :) If I ever see something 'extensible' added that is
never called (and the project is not TDD), then it feels the wrath of
my delete-key-finger!
From: Sahil Malik
This restriction is in place only on constructors. Constructors can return
different types using the out keyword alongwith the return keyword, but they
MUST return atleast one instance of the type of the class itself. Any other
method can return whatever data-type it wishes to as long as the method
signatures are different. This might not appear obvious in a
*coff*crippled*coff* language such as VB.NET, but I believe the reasoning
behind this was to support better operator overloading support. For
instance, you can take a DateTime and subtract both another DateTime from
it, or another TimeSpan - something you couldn't do if multiple return types
wasn't supported.
I believe, this was not allowed in C++ however, but in C++ you could simply
return a pointer and get over the whole mess.
From: Steve Hebert
Seems like a code-smell.
I like John's comment that it should be named something different. I always keep the same return type because if I'm familiar with one signature on the class then I'll probably ignore intellisense and assume the same return type from the others. If my return type is different, it seems like a different behavior and therefore a different method name is necessary. Especially in the case where 2 functions have no return value and one function returns a bool , the compiler won't provide you with any indication that something is wrong when you discard the bool value.
From: Ranjan Sakalley
I think you are absolutely correct about the consistency. I think instead
of google, you can start looking at Enterprise library for such things, its
really a good place to find out how to structure a good API.
From: Peter van Ooijen
As a C# guy I didn't even know you could
overload the return type.
From: Peter van Ooijen
A rename has my vote :)
Just tried to overload the return type, and to my surprise C# does allow
that. As long as the other part of the signature is different as well. Bad !
But now I'm just repating what has been said allready.
From: Eric Wise
I always had a similar complaint about case sensitive languages. Anyone who declares var, Var, and VAR is just asking for confusion and killing readability.
From: Jeff Lynch
I also agree with everyone's comments. I use overloads in many methods but
as a rule, always return the same type.
From: Darrell Norton
Um, I agree!
Seriously, I agree that the method name should give an indication of the return type. Why would the function return a boolean? To prove that a class was created? Just return the class instead and I'll check for Nothing (or null, for you C# types).
I'm trying to think of when a function would return a boolean. Boolean functions sound like overdressed properties, like IsCool.
From: Ranjan Sakalley
I confess, I normally use boolean return values for ExecuteNonQueries, though I know "no exception" is the better way to know if there is a problem or not.
From: Jeffrey Palermo
I've used methods that return bool when I have several conditions, and
I want to refactor some of the &&'s and ||'s out into private methods
to simplify the method flow. In that case I've used methods because I
considered adding a property to the class wasn't appropriate. Perhaps
it all subjective anyway, since properties are overdressed methods.
:)
From: Sahil Malik
BTW, here's another thing to think alongwith Darrell's comment.
The VB Function CType returns you an instance of the given type. Then why does intellisense say that the return type is System.Type?
From: Raymond Lewallen
Because it returns the type of the same type that your provide as the
2nd parameter. You specify the derived class type that you want
returned. Just like any function that returns an instance of
anything, you could specify the return value as System.Object because
all objects are derived from it.
From: Sahil Malik
Yeah but it is not returning System.Type, it is returning System.Int32 or so on so forth.
From: Ben Reichelt
Jeffrey, I was thinking the same thing about removing the && and || into private methods that return bools, but they are usually private so they arent exposed to anyone using the api.
From: Raymond Lewallen
Jeffrey, I do the same thing when improving my cyclomatic
complexities: refactor those ANDs and ORs into functions that return
booleans.
From: Brendan Tompkins
My 2 cents is that when you find that you need a bunch of overloaded
methods, your design probably needs some work..
And I didn't know you could change the return type of an overload
either. I knew you couldn't JUST change the return type, but never
knew that changing the whole signature would allow you to compile.
In either case, changing the type is smelly, IMO
From: Eric Wise
The nice thing about boolean return values is that you don't have to actually catch the boolean if you don't want to. "True" or "False" on its own is a completely valid statement.
From: Jay Kimble
I'm amazed... I knew something that some of you guys didn't. I
knew that you could change the return type in an overloaded method. This needs to be blogged!
For those of you who skipped over it all (yeah, it's pretty long) I'll sum it up.
- When you overload a method, it's valid to change it's return type.
- The method signature must be different if you do change the return type. For the method signature to be different, only the parameters passed to the method are looked at.
- This is a pretty unobvious concept, as discovered by the fact that some of us didn't even know you could.
- It's considered good form to always have the same return type (the Framework has cases of using a constant return type or using a Sub (void), but we didn't discuss if this was acceptable).
- Sahil doesn't like VB very much.
- Jay was impressed that he knew that return types could be overloaded.
- Darrell thinks that functions should never return a boolean. I'm not sure I agree completely, but he's right for most of the cases I can think of.
While having this discussion on the list, my co-worker and I sorted out our differences. He's now in agreement with me (which actually isn't that important. What matters is that we agreed, not which side won).