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 GetSpecificHash Algorithm(strin g hashString){
if (hashString == "MD5") {
return System.Security .Cryptography.M D5.Create();
} else {
return System.Security .Cryptography.S HA512.Create();
}
}

or

2)
private HashAlgorithm GetSpecificHash Algorithm(strin g hashString){
HashAlgorithm hash;
if (hashString == "MD5") {
hash = System.Security .Cryptography.M D5.Create();
} else {
hash = System.Security .Cryptography.S HA512.Create();
}
return hash;
}

Is there a general opinion on which is better style?

Thanks,
Matt

[Peter Bromberg [C# MVP]]

Re: Question about return style

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
"Matt B" <rounderweget@g mail.comwrote in message
news:299a30df-618a-4e61-b500-fb695c9bd2ca@s2 1g2000prm.googl egroups.com...

>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 GetSpecificHash Algorithm(strin g hashString){
if (hashString == "MD5") {
return System.Security .Cryptography.M D5.Create();
} else {
return System.Security .Cryptography.S HA512.Create();
}
}
>
or
>
2)
private HashAlgorithm GetSpecificHash Algorithm(strin g hashString){
HashAlgorithm hash;
if (hashString == "MD5") {
hash = System.Security .Cryptography.M D5.Create();
} else {
hash = System.Security .Cryptography.S HA512.Create();
}
return hash;
}
>
Is there a general opinion on which is better style?
>
Thanks,
Matt

[Jeroen Mostert]

Re: Question about return style

Matt B wrote:

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 GetSpecificHash Algorithm(strin g hashString){
if (hashString == "MD5") {
return System.Security .Cryptography.M D5.Create();
} else {
return System.Security .Cryptography.S HA512.Create();
}
}
>
or
>
2)
private HashAlgorithm GetSpecificHash Algorithm(strin g hashString){
HashAlgorithm hash;
if (hashString == "MD5") {
hash = System.Security .Cryptography.M D5.Create();
} else {
hash = System.Security .Cryptography.S HA512.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.C reate().

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.

--
J.

http://symbolsprose.blogspot.com

[Peter Duniho]

Re: Question about return style

On Wed, 28 May 2008 10:57:15 -0700, Matt B <rounderweget@g mail.comwrote:

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.

But even with that preference, I have been known to put a "return"
statement in the middle of a method. :)

Pete

[Mythran]

Re: Question about return style



"Matt B" <rounderweget@g mail.comwrote in message
news:299a30df-618a-4e61-b500-fb695c9bd2ca@s2 1g2000prm.googl egroups.com...

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 GetSpecificHash Algorithm(strin g hashString){
if (hashString == "MD5") {
return System.Security .Cryptography.M D5.Create();
} else {
return System.Security .Cryptography.S HA512.Create();
}
}
>
or
>
2)
private HashAlgorithm GetSpecificHash Algorithm(strin g hashString){
HashAlgorithm hash;
if (hashString == "MD5") {
hash = System.Security .Cryptography.M D5.Create();
} else {
hash = System.Security .Cryptography.S HA512.Create();
}
return hash;
}
>
Is there a general opinion on which is better style?
>
Thanks,
Matt

Another way:

private HashAlgorithm GetSpecificHash Algorithm(strin g hashString) {
if (hashString == "MD5") {
return System.Security .Cryptography.M D5.Create();
}
return System.Security .Cryptography.S HA512.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

[Jon Skeet [C# MVP]]

Re: Question about return style

Peter Duniho <NpOeStPeAdM@nn owslpianmk.comw rote:

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

--
Jon Skeet - <skeet@pobox.co m>
Web site: http://www.pobox.com/~skeet
Blog: http://www.msmvps.com/jon.skeet
C# in Depth: http://csharpindepth.com

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

Re: Question about return style

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.

[Peter Duniho]

Re: Question about return style

On Wed, 28 May 2008 12:52:22 -0700, Jon Skeet [C# MVP] <skeet@pobox.co m>
wrote:

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

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.

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

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

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.

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.

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

Pete

[Jon Skeet [C# MVP]]

Re: Question about return style

Peter Duniho <NpOeStPeAdM@nn owslpianmk.comw rote:

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 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
to it: if I exit <thisblock, do <thiswork.

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

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.

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

Indeed.

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.

>
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(stri ng name)
{
foreach (Person p in people)
{
if (p.Name==name)
{
return p;
}
}
return null;
}

.... and this less obvious:

Person FindByName(stri ng 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. :)

:)

--
Jon Skeet - <skeet@pobox.co m>
Web site: http://www.pobox.com/~skeet
Blog: http://www.msmvps.com/jon.skeet
C# in Depth: http://csharpindepth.com

[Matt B]

Re: Question about return style

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

[Peter Duniho]

Re: Question about return style

On Wed, 28 May 2008 14:52:14 -0700, Jon Skeet [C# MVP] <skeet@pobox.co m>
wrote:

[...]
How would a finally block by itself work? You need to give some scope
to it: if I exit <thisblock, do <thiswork.

Well, I'm no grammarist. :) But, I'd specify it sort of like the "yield"
statement is used. That is, if it's present at all, it has some effect on
how the entire method is interpreted by the compiler. Though, in this
case the rules could be simpler: a "finally" clause by itself is required
to be at the top-most scope within a method and must be at the end. If
those rules are met, a "finally" clause without supporting "try" is legal
and simply means to execute all of the code within the clause before
returning to the caller.

I could equivocate on the second requirement (must be at the end); I like
it from a readability point of view, but inasmuch as the first rule gets
most of the readability one needs, and inasmuch as I could see how someone
else would prefer to declare all their "cleanup" code at the beginning,
along with whatever locals and whatnot might actually need to be cleaned
up, I wouldn't argue strenuously for that rule.

I don't really mind that this syntax doesn't exist. The number of times
that it would be applied would be relatively low IMHO. And I like all of
the alternatives just fine (while I wouldn't use try/finally just for this
purpose myself, if someone else wants to that's fine by me). And of
course the "using" statement already accomplishes much of the sort of
cleanup that would have existed in a different environment (*). But it
could be done, and I think it'd be reasonable readable and useful, at
least in some situations.

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

[...]
Another classic example is looking for something in a loop. I find this
a nice, obvious way of expressing the logic:
>
Person FindByName(stri ng name)
{
foreach (Person p in people)
{
if (p.Name==name)
{
return p;
}
}
return null;
}
>
[...]
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.

Yes, I would typically use the first version. Maybe I understated my
willingness to use multiple returns. It's just one of many different
coding patterns I use, depending on the exact circumstance.

Pete

[=?ISO-8859-1?Q?Arne_Vajh=F8j?=]

Re: Question about return style

Matt B wrote:

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 GetSpecificHash Algorithm(strin g hashString){
if (hashString == "MD5") {
return System.Security .Cryptography.M D5.Create();
} else {
return System.Security .Cryptography.S HA512.Create();
}
}
>
or
>
2)
private HashAlgorithm GetSpecificHash Algorithm(strin g hashString){
HashAlgorithm hash;
if (hashString == "MD5") {
hash = System.Security .Cryptography.M D5.Create();
} else {
hash = System.Security .Cryptography.S HA512.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]]

Re: Question about return style

Peter Duniho <NpOeStPeAdM@nn owslpianmk.comw rote:

<snip stuff which is mostly a matter of discussion>

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.

--
Jon Skeet - <skeet@pobox.co m>
Web site: http://www.pobox.com/~skeet
Blog: http://www.msmvps.com/jon.skeet
C# in Depth: http://csharpindepth.com