 |
 | Home | Contact Us | FAQ | Search & Site Map | Link to Us |
 | Sign In | Join | Other 45 Sites in Network |
.NET Forum / Languages / C# / May 2008Question about return style
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 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 >I was just wondering if there is a "best" choice from the following > couple of ways of returning a value from a method: [quoted text clipped - 25 lines] > Thanks, > Matt > I was just wondering if there is a "best" choice from the following > couple of ways of returning a value from a method: [quoted text clipped - 22 lines] > > 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.  SignatureJ. http://symbolsprose.blogspot.com 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. > 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. Just set a breakpoint on the final brace of the method. :) > - Default value standout : you just have to look at the returned > variable declaration to know the value. Not in the example give, which has no default assignment (instead, all code paths "definitely" assign the variable). > - You can easily change the returned value using the debugger. It's not really _that_ much harder to do otherwise. Absent any other information, I prefer a single "return" statement as well. But not for any of those reasons. :) Pete >> Personally, I would use the last one for the following reasons : >> [quoted text clipped - 3 lines] > > 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... >> - You can easily change the returned value using the debugger. > > 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. SignatureJ. http://symbolsprose.blogspot.com >>> - 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) [quoted text clipped - 6 lines] > value when you do that. AFAIK, they still haven't fixed that in VS 2008. > A "$return" pseudovariable shouldn't be that hard... See below. :) >>> - You can easily change the returned value using the debugger. >> It's not really _that_ much harder to do otherwise. [quoted text clipped - 4 lines] > EAX, but I'm not aware of any similar trick that works for managed > debugging. 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... 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? 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. :) Pete >>>> - 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 [quoted text clipped - 23 lines] > 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. SignatureJ. http://symbolsprose.blogspot.com > 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 > > I was just wondering if there is a "best" choice from the following > > couple of ways of returning a value from a method: [quoted text clipped - 6 lines] > 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 :) SignatureJon Skeet - <skeet@pobox.com> Web site: http://www.pobox.com/~skeet Blog: http://www.msmvps.com/jon.skeet C# in Depth: http://csharpindepth.com > 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. >> 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 [quoted text clipped - 3 lines] > 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 > >> 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 [quoted text clipped - 8 lines] > 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 <this> block, do <this> work. > 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 [quoted text clipped - 8 lines] > 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. :) :) SignatureJon Skeet - <skeet@pobox.com> Web site: http://www.pobox.com/~skeet Blog: http://www.msmvps.com/jon.skeet C# in Depth: http://csharpindepth.com 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 > [...] > How would a finally block by itself work? You need to give some scope > to it: if I exit <this> block, do <this> work. 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 [quoted text clipped - 15 lines] > 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 On May 28, 11:30 pm, "Peter Duniho" <NpOeStPe...@nnowslpianmk.com> wrote: > (*) 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 [quoted text clipped - 3 lines] > 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 > [...] > Well, that's an interesting point. Personally I find it worth > re-evaluating all > my habits and patterns when approaching a different language. For me, I find the opposite to be true. While I agree that habits learned elsewhere can sometimes be counter-productive, I've _rarely_ found this to be an issue in practice. But at the same time, I find that trying to essentially start over can be disorienting and time-consuming. Time-consuming I can live with if it's really important, but here not only do I not personally find it that important, being able to shift into a new language, especially when I am still coding in other langauges/environments, without becoming disoriented is very helpful. YMMV. > 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. I never said it was common. Just that it happens. Again, between garbage collection and IDisposable, it's much more rare in C# than it would have been before. But I don't see a need to completely abandon a potentially useful idiom just because in most cases it's not applicable. > 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#. It's not really an issue in C/C++ either, at least it hasn't been for a very long time. I don't recall whether this was a Microsoft feature or a general C/C++ specification feature, but I've used the "assignment in if()" warning for over a decade (in conjunction with "warnings as errors", which has always been a habit), for this very reason. And yes, I agree the "constant first" syntax is less readable. 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. (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 agree that it's still important to be aware of the options available, [quoted text clipped - 3 lines] > 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). Pete <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, [quoted text clipped - 14 lines] > 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. SignatureJon Skeet - <skeet@pobox.com> Web site: http://www.pobox.com/~skeet Blog: http://www.msmvps.com/jon.skeet C# in Depth: http://csharpindepth.com > I was just wondering if there is a "best" choice from the following > couple of ways of returning a value from a method: [quoted text clipped - 27 lines] > 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. > I was just wondering if there is a "best" choice from the following > couple of ways of returning a value from a method: [quoted text clipped - 25 lines] > 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 > I was just wondering if there is a "best" choice from the following > couple of ways of returning a value from a method: [quoted text clipped - 22 lines] > > 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 Free MagazinesGet these publications absolutely FREE for up to 12 months. There are no hidden fees and no obligation. Simply choose a title, complete the application form and submit it. Read more ...
|
Reply to this Message
|
 Sign In
 Join
 My Latest Posts My Monitored Threads My Blog My Photo Gallery My Profile My Homepage Start New Thread Enable EMail Alerts Rate this Thread
|
©2008 Advenet LLC Privacy Policy - Terms of Use
This website includes both content owned or controlled by Advenet as well as content owned or controlled by third parties.