Question about return style

[Matt B]

I was just wondering if there is a "best" choice from the following
couple of ways of returning a value from a method:

1)
private HashAlgorithm GetSpecificHashAlgorithm(string hashString){
if (hashString == "MD5") {
return System.Security.Cryptography.MD5.Create();
} else {
return System.Security.Cryptography.SHA512.Create();
}
}

or

2)
private HashAlgorithm GetSpecificHashAlgorithm(string hashString){
HashAlgorithm hash;
if (hashString == "MD5") {
hash = System.Security.Cryptography.MD5.Create();
} else {
hash = System.Security.Cryptography.SHA512.Create();
}
return hash;
}

Is there a general opinion on which is better style?

Thanks,
Matt

 

[Peter Bromberg [C# MVP]]

Unless you have additional code that might want to use the local "hash"
variable, I don't think it makes much difference. However, you might want
to pay attention to what to do is somebody calls your method and passes
"hamburger" as the parameter...
Peter

 

[Jeroen Mostert]

I was just wondering if there is a "best" choice from the following
couple of ways of returning a value from a method:

1)
private HashAlgorithm GetSpecificHashAlgorithm(string hashString){
if (hashString == "MD5") {
return System.Security.Cryptography.MD5.Create();
} else {
return System.Security.Cryptography.SHA512.Create();
}
}

or

2)
private HashAlgorithm GetSpecificHashAlgorithm(string hashString){
HashAlgorithm hash;
if (hashString == "MD5") {
hash = System.Security.Cryptography.MD5.Create();
} else {
hash = System.Security.Cryptography.SHA512.Create();
}
return hash;
}

Is there a general opinion on which is better style?

No, but I'm pretty sure there'll be plenty of people to offer their own.
Single-exit versus multiple-exit is as old as there has been a return statement.

As far as I'm concerned, as long as you keep your functions small (which is
always a good idea), this is a bikeshed problem. http://www.bikeshed.com/

Focusing on style, you haven't even addressed the actual problems with your
functions (even granting that they're just meant to illustrate one issue):

- You don't check if the argument is null. Although these functions are
private, I wouldn't omit a simple null check even for private functions,
since it's cheap and very effective at catching problems early. If you were
not expecting "hashString" to ever be null, you would be surprised to
suddenly be working with a SHA512 hash you never thought you requested.

- You don't check if the argument is "SHA512". Anything that's not MD5 is
SHA512? That's not extensible. A dictionary lookup would be more flexible,
and would not require rewriting the function for new algorithms.

- You are duplicating functionality that's already available in the form of
HashAlgorithm.Create().

Finally, and less importantly, while you're free to name private members in
any way you like, it's probably a better idea to not start them with an
uppercase letter, so that it's immediately that it's not a public member (as
those *should* start with an uppercase letter). That's an opinion, though,
as you're free to name your private members however you like.

 

[sylvain.rodrigue]

Personally, I would use the last one for the following reasons :

- I often place a break point at the exit point of a fonction in order
to check the returned value. Having only one exit point is a (little)
time saver.
- Default value standout : you just have to look at the returned
variable declaration to know the value.
- You can easily change the returned value using the debugger.

Regards,
Sylvain.

 

[sylvain.rodrigue]

I was just wondering if there is a "best" choice from the following
couple of ways of returning a value from a  method:

1)
private HashAlgorithm GetSpecificHashAlgorithm(string hashString){
            if (hashString == "MD5") {
                return System.Security.Cryptography.MD5.Create();
            } else {
                return System.Security.Cryptography.SHA512..Create();
            }

}

or

2)
private HashAlgorithm GetSpecificHashAlgorithm(string hashString){
            HashAlgorithm hash;
            if (hashString == "MD5") {
                hash = System.Security.Cryptography.MD5.Create();
            } else {
                hash = System.Security.Cryptography.SHA512.Create();
            }
            return hash;

}

Is there a general opinion on which is better style?

Thanks,
Matt

Personnaly, I would use the second method for the following reasons :

1. I often place a break point at the exit point of a function in
order to check the returned value. Having only one return point is a
(little) time saver.
2. I often change the returned value of a function when I am
debugging. Try doing this using the first method
3. Default value standout since it can be assigned at the declaration
point.

Regards,
Sylvain.

 

[Mythran]

I was just wondering if there is a "best" choice from the following
couple of ways of returning a value from a method:

1)
private HashAlgorithm GetSpecificHashAlgorithm(string hashString){
if (hashString == "MD5") {
return System.Security.Cryptography.MD5.Create();
} else {
return System.Security.Cryptography.SHA512.Create();
}
}

or

2)
private HashAlgorithm GetSpecificHashAlgorithm(string hashString){
HashAlgorithm hash;
if (hashString == "MD5") {
hash = System.Security.Cryptography.MD5.Create();
} else {
hash = System.Security.Cryptography.SHA512.Create();
}
return hash;
}

Is there a general opinion on which is better style?

Thanks,
Matt

Another way:

private HashAlgorithm GetSpecificHashAlgorithm(string hashString) {
if (hashString == "MD5") {
return System.Security.Cryptography.MD5.Create();
}
return System.Security.Cryptography.SHA512.Create();
}

What I would recommend, which furthers Peter's response, is to create an
enumeration of valid hash algorithm names and pass one of these values to
the method instead of a string. Then you can restrict the parameter values
to those listed in the enumeration.



HTH,
Mythran

 

[Jeroen Mostert]

Just set a breakpoint on the final brace of the method.

Unless all those different returns are so kind as to assign to a variable
first and return that, you can't actually check the return value when you do
that. AFAIK, they still haven't fixed that in VS 2008. A "$return"
pseudovariable shouldn't be that hard...

It's not really _that_ much harder to do otherwise.

Remember, we're talking about the case where the returned value isn't stored
in a local variable. The caller needn't be storing it in a variable either.
For unmanaged debugging, you can pluck the value from EAX, but I'm not aware
of any similar trick that works for managed debugging.

 

[Jon Skeet [C# MVP]]

I was just wondering if there is a "best" choice from the following
couple of ways of returning a value from a method:

[...]
Is there a general opinion on which is better style?

I doubt you'll find a consensus. My preference is for #2, because
it allows me to keep any common end-of-method processing all in one
place. If I write all my methods like that from the outset, then if I
have to add something like that later, it's already set up for that.

How often do you find yourself having to do that in a way which isn't
easily covered with try/finally? I find it's pretty rare - whereas I've
seen *lots* of code which strictly adheres to "thou shalt have a single
exit point" and is painfully complicated because of it.

Personally I almost always write in a style where as soon as you know
the result of the method and you've done everything you need to, just
return. I find it keeps any one individual flow simpler.

As you say though, there's certainly not going to be consensus

 

[Jeroen Mostert]

See below.


Personally, I would just step back out to the caller. The value is
either going to be copied into a variable, or passed to another method.
Either way, it'll wind up in some sort of named variable that you can
then modify. Things get a bit more complicated if the return value is
used in an expression, granted. Which bring me to...

It also doesn't work if you have no symbols for the caller. Actually, not
having source would probably be enough to make this unworkable. If debugging
the callers is an option (because you also own that code ), sure, go for
that, but it's not actually easier, just a possible workaround.

Admittedly I haven't explored advanced debugging techniques since moving
over to managed code, but given that the code that's executing is in
fact native x86, I think it's very likely there's a mode in the debugger
you can switch to that gives you access to the data at a lower level,
including register access. Maybe this is what you mean by "unmanaged"
debugging?

Actually, you *have* (read-only) register access even under managed
debugging (odd as this may seem), and EAX will actually contain the return
value when you break on the final brace of a function. However, unless your
function is returning a primitive type like int you will not be able to
inspect, let alone modify the value, because it's an address, and as far as
managed code is concerned, there's no such thing as an address to a managed
object.

You *should* be able to get it done when you turn on mixed-mode debugging
(where the debugger can debug managed and unmanaged code simultaneously).
This is what you get when you turn on the "enable unmanaged code debugging"
box in the project properties. You can now load SOS and use !DumpObject on
your address to get to your actual object (in a far less pleasing format
than what the debugger can give you, but still). Modifying it is another
chapter altogether.

However, mixed-mode debugging is slower and less flexible than pure managed
debugging (edit and continue ceases to work and there are some other more
esoteric side effects) so it should preferably be used only when you're
actually debugging an application that uses both managed and unmanaged
components. Even then I usually prefer WinDbg with SOS -- if you really need
to debug at this level, there's little point settling for less. It is not
what one would call convenient, though.

The moral of the story is that they really ought to have added a $return
pseudoregister/expression/whatever. Regular developers cannot be expected to
jump through these hoops just to have a foolproof way of accessing the
function's return value *before* you go off into some caller code. It's a
simple enough thing to want. We do get $exception for catch blocks that are
too lazy to declare a variable (and that should be *much* rarer than returns
without a variable), so why not?

In all my years of programming, I've yet to run into a situation that
required me to rewrite my code to accomodate whatever debugging I wanted
to do. Maybe those scenarios exist in the managed environment. But it
would surprise me if they do.

Oh, if you find out how to do this easily in managed mode (inspect and
modify the return value without rewriting the function or going up to the
callers), do let me know. I've not run into the issue often enough to really
try, but it's an actual issue.

 

[Ignacio Machin ( .NET/ C# MVP )]

How often do you find yourself having to do that in a way which isn't
easily covered with try/finally? I find it's pretty rare - whereas I've
seen *lots* of code which strictly adheres to "thou shalt have a single
exit point" and is painfully complicated because of it.

Been there

Personally I almost always write in a style where as soon as you know
the result of the method and you've done everything you need to, just
return. I find it keeps any one individual flow simpler.

I totally agree, as soon as you finished your processing you should
end it. It's simple and elegant IMO.

 

[Jon Skeet [C# MVP]]

I admit, I'm not entirely sure what you mean. I tend to avoid try/finally
unless there's an actual exception I anticipate occurring. Yes, it can be
used even without an exception, but for me, the presence of a "try"
statement sends a strong signal that an exception is anticipated.

That would be the case for try/catch, but for try/finally I simply
regard it as "do this whatever happens".

If C# allowed for a "finally" statement/block by itself, I might be more
likely to use that as my approach. But given that I can't use "finally"
with a "try", and the extra level of indentation for the entire method
block in that case, and given the "there's an exception" implication of
"try", that's just not an idiom I'm likely to use just for the sake of
cleanup.

How would a finally block by itself work? You need to give some scope

In truth, most methods are simple enough that this question just doesn't
even come up. But inasmuch as it does come up, and inasmuch as sometimes
it comes up outside the context of thrown exceptions, I find reasons to
keep a single return statement.

I'm sure there are some cases where it's useful. I just find they're
relatively rare

Any convention can certainly be abused. Foolish consistencies, and all
that.
Indeed.


Different situations call for different techniques. But as a general
rule, if you've got a method where there's a question of "any one
individual flow", I do find scenarios where I find it preferable to try to
keep as close to "just one individual flow" as much as possible.
Obviously conditionals always add different code paths, but I like to keep
those paths as short as possible and return to the main flow of the method
as soon as possible. Often this means consolidating shared code into one
place.

I find that often there are "early outs" which mean that most of the
method doesn't need to operate at all: if a parameter is null, or an
empty string, or an empty list etc. In those cases, where the result is
known without looking at the complex logic later on, why not get out as
quickly as possible? That's the kind of example I keep seeing (or used
to, anyway) where "single return point" has been painful.

Another classic example is looking for something in a loop. I find this
a nice, obvious way of expressing the logic:

Person FindByName(string name)
{
foreach (Person p in people)
{
if (p.Name==name)
{
return p;
}
}
return null;
}

.... and this less obvious:

Person FindByName(string name)
{
Person found = null;
foreach (Person p in people)
{
if (p.Name==name)
{
found = p;
break;
}
}
return found;
}

Reasons:
1) When we've found the person, what do we instinctively want to do?
Return it. Not break out of the loop so we can later return the person
we've just found.

2) When is null (the default value) relevant? When we've just exhausted
all the possibilities - not before we start looking.

Of course, this may be a case where you'd use the first version anyway,
but it's a good example (IMO) of where multiple returns help.

But every method is different. As I said, even I have been known to write
more than one "return" statement in a method.

 

[Matt B]

Thanks for all the replies guys. While my code samples didn't do the
actual code justice, I learned quite a bit from all your responses. I
actually started out thinking that I should be always following
example #2, but I'm not so sure anymore. In any case, by making me re-
think a few things you all have helped to make me a better programmer
today. Thanks!

Matt

 

[Arne Vajhøj]

I was just wondering if there is a "best" choice from the following
couple of ways of returning a value from a method:

1)
private HashAlgorithm GetSpecificHashAlgorithm(string hashString){
if (hashString == "MD5") {
return System.Security.Cryptography.MD5.Create();
} else {
return System.Security.Cryptography.SHA512.Create();
}
}

or

2)
private HashAlgorithm GetSpecificHashAlgorithm(string hashString){
HashAlgorithm hash;
if (hashString == "MD5") {
hash = System.Security.Cryptography.MD5.Create();
} else {
hash = System.Security.Cryptography.SHA512.Create();
}
return hash;
}

Is there a general opinion on which is better style?

Ignoring the issues with this specific code then I would go for #1.

I don't there is a problem with multiple returns. It saves the time of
checking the rest of the code when reading.

Arne

 

[Jon Skeet [C# MVP]]

(*) I think it goes without saying, but maybe it's worth me pointing out
that when I discuss general code layout conventions like this, I am in
fact speaking relatively generally. Many of my habits or opinions come
from years of C/C++ coding, and there's a lot in C# that dramatically
reduces the frequency with which a particular idiom might be needed. But
IMHO, there's no reason to toss the tool from the toolbox just because it
doesn't get used as much as all the others.

Well, that's an interesting point. Personally I find it worth re-
evaluating all
my habits and patterns when approaching a different language.

As you say, this particular pattern is less useful in C# than in C. If
you need
to do a bunch of resource deallocations at the end of a method in C,
for
instance, it really does help to know there's only one place the
method can end.
In C# that's very rarely a factor.

Another example is the pattern of writing "if (0==x)" rather than "if
(x==0)" in
C, in order to avoid problems due to accidentally writing a single =
sign
instead of a double. Again, this just isn't an issue in C#. I think
many of us
find the "constant first" style less readable than the "variable
first" style,
so to use "constant first" in C# is just holding on to C idioms for no
reason.

I agree that it's still important to be aware of the options
available, but
sometimes they become so rarely useful that it's no longer worth
keeping them as
what you might consider your general coding style. (Generic "you"
here, of course.)

Jon

 

[Jon Skeet [C# MVP]]

By the way, note that it's not true that "this just isn't an issue in
C#". C#'s rules mitigate the situation a lot, but you can still assign a
boolean in an if() statement without an error.

True - but how often do you actually compare with a boolean constant in
the first place? I always write

if (condition)
or
if (!condition)

instead of

if (condition == true)
or
if (condition == false)

anyway. I'd consider the top options to be the idiomatic C# style.
Allow me to amend my claim to "this just isn't an issue in idiomatic
C#"

(You do get a warning, but
again...that's something that's been in at least the one C/C++ compiler
I've used for a very long time).

I didn't even know it was a warning in C#, to be honest

[...]
I agree that it's still important to be aware of the options available,
but
sometimes they become so rarely useful that it's no longer worth keeping
them as
what you might consider your general coding style. (Generic "you"
here, of course.)

Well, I guess that depends on your definition of "general coding style".
As I said, I don't have a consistent "I always do it one way or the other"
attitude for this specific question. It's more about anticipating
possible future uses, and weighing the relative aesthetics between the
approaches.

Consistency can be a good thing, but it shouldn't be clinged to when there
are more dominant issues. For different people, what defines "more
dominant issues" may vary (and in matters such as these, they practically
always will).

Sure. I guess I'm just saying that in C# trying to keep to a single
return point (when other courses of action are naturally available) is
something which is so rarely explicitly useful that those situations in
which it *is* useful usually point themselves out anyway.