|
|
Of course - as long as you use them as I say!
|
|
|
|
|
I'm sure there are legitimate uses for "outs" but the place most C# devs have to use them (and the only time that they ever seem to crop up in my code) is for various TryParse() methods, which violate the SRP by both validating and parsing.
To my mind, they really should have been implemented as IsParseable() and Parse().
98.4% of statistics are made up on the spot.
|
|
|
|
|
TryParse returns a bool, because sometimes, you need to know if it parsed correctly or not, but you also need the value that is parsed. So, how "else" would you do that? You could use a Tuple but I actually hate tuples and use them very sparingly.
|
|
|
|
|
I'd kind of hope that we always want to test the return value!
Whilst I don't see it as the crime of the century, TryParse() does do two things (hence its need for an "out" as well as a return value).
As I suggested, it should, strictly speaking, be split into two methods that do one thing each e.g.
bool IsParseable(string)
T Parse(string)
as opposed to:
bool TryParse(string, out T)
The return value does not relate to the entire purpose of the method and aside from not following SOLID principles, this also has an effect on code readability. For a start we have a method with a verbal name and a boolean value - "bool DoSomething()" is inherently misleading, a bool should answer a question not describe an action.
Also, the fact that we tend to be looking for a negative in the return value means that we tend to wind up with a rather inverted logic.
I would suggest that:
if (int.IsParseable(someString))
someProperty = int.Parse(someString);
else
would be much more readable than:
if (!int.TryParse(someString, out someProperty))
98.4% of statistics are made up on the spot.
|
|
|
|
|
if (int.IsParseable(someString))
someProperty = int.Parse(someString);
This could lead an interesting discussion, whether this code can be optimized by JIT-compiler to make a parsing only once. But this is C#, who cares?
|
|
|
|
|
PeejayAdams wrote: if (int.IsParseable(someString))
someProperty = int.Parse(someString);
Not very efficient IMHO.
If one is not going to use TryParse then I would use a tuple via an extension method.
|
|
|
|
|
I'd agree - the IsParsable test has to parse the whole date in order to decide it's valid, so you are repeating the code and throwing away the value you wanted. The only solution to that without tuples (which didn't exist in C# when TryParse was written) was Convert.ToDateTime with a try...catch block or extending DateTime to include a "IsBadDate" option, neither of which appeal to me at all. 
Bad command or file name. Bad, bad command! Sit! Stay! Staaaay...
AntiTwitter: @DalekDave is now a follower!
|
|
|
|
|
As pointed out elsewhere, the string does not actually have to be parsed twice as it can be validated with a regular expression.
I agree try ... catches would be hideous as would extending data types to have an isBad flag.
Maybe another option (not available back in the day, either) would be to use nullable types and return null when invalid but that, too, would make for some pretty ugly code with all the casting that would be going on.
I'm not seriously suggesting that they change TryParse, just pointing out that it's a rather ugly construct and it breaks a few rules.
98.4% of statistics are made up on the spot.
|
|
|
|
|
You cannot validate a date properly with a regex: is 29-02-2012 valid? How about 29-02-2013? Or 29-02-1900? How about 29-02-2000?
The answers are: Yes, No, Yes, No - but for different reasons. It's a leap year if the year is divisible by 4, but ... century ends aren't leap years, unless they are a milenium end as well. Try writing a Regex that copes flexibly with that!
You can check a date format with a regex, yes - but actual date validation is required for DateTime parsing, and you can only do that when you have converted the whole date!
Bad command or file name. Bad, bad command! Sit! Stay! Staaaay...
AntiTwitter: @DalekDave is now a follower!
|
|
|
|
|
DateTimes are always going to be a hugely complicated thing and locales have to be taken into account however you approach it - "01/11/2018" in some parts of the world means "11/01/2018" so it isn't simply a case of asking whether "01/11/2018" is a valid string. It couldn't be done in a single regex for that reason - it would require a number of regexes to cover various formats/locales.
It would be a lot of code, granted, but I rather suspect that the existing parsing routine is somewhat complex (and no, I certainly wouldn't volunteer to write it!)
98.4% of statistics are made up on the spot.
|
|
|
|
|
Since many times you can't tell if something is parsable until you actually parse it, splitting TryParse into IsParsable and Parse calls can actually double the amount of work it takes to parse.
|
|
|
|
|
You're being illogical here.
PeejayAdams wrote: if (int.IsParseable(someString))
is not the same as
PeejayAdams wrote: if (!int.TryParse(someString, out someProperty))
take off the negation (!) and it would be. equivalent.
So your argument about needing a ! is wrong and illogical.
#SupportHeForShe
Government can give you nothing but what it takes from somebody else. A government big enough to give you everything you want is big enough to take everything you've got, including your freedom.-Ezra Taft Benson
You must accept 1 of 2 basic premises: Either we are alone in the universe or we are not alone. Either way, the implications are staggering!-Wernher von Braun
|
|
|
|
|
PeejayAdams wrote: To my mind, they really should have been implemented as IsParseable() and Parse()
Which requires you to perform the parsing task twice.
Software Zen: delete this;
|
|
|
|
|
Not necessarily, the validation method could use a regex.
98.4% of statistics are made up on the spot.
|
|
|
|
|
And still not be 100% certain the actual Parse will work. The only way to be certain is to actually Parse the object.
|
|
|
|
|
PeejayAdams wrote: the only time that they ever seem to crop up in my code
So you've never used Dictionary<TKey,TValue>.TryGetValue[^]?
"These people looked deep within my soul and assigned me a number based on the order in which I joined."
- Homer
|
|
|
|
|
I always have a GetValueOrDefault() that calls TryGetValue and ignores the bool.
Most of the time the TValue is a class and I will never pass null, so checking for null will do the job.
|
|
|
|
|
This[^] is a great article imo on the fundamental flaw of SRP we've all seen via arguments at code reviews. It's much more productive to think in terms of cohesion and coupling. In this example validation and parsing are very cohesive which others have pointed out.
All you'd be doing by splitting the method is forcing boilerplate client code (if(can()) then do();) and forcing your own do() method to throw an exception in case of failure where before that wasn't necessary since a validate was in the pipeline already. Considering parsing is an operation that can fail quite often, throwing exceptions isn't an optimal design due to their cost.
|
|
|
|
|
Splitting it in two methods would be a performance hit.
TryParse will need to evaluate the entire string (and maybe actually parse it) to then return true or false, then Parse will do everything again.
I say actually parse it because for ints (and many other types, actually) the string length and characters might be valid, yet the value overflows and the easiest way for a TryParse to detect it is to actually do the full parsing process.
That being said, most TryParse methods today would be best implemented by using a Nullable<t> (or simply T?). Unfortunately, this will only work for structs (but it would also be valid for classes if a parse is never expected to return null).
|
|
|
|
|
What we have here is a case of pragmatism vs. principle.
TryParse is "wrong" in many ways - violation of SRP, use of exceptions as a method of flow control and so forth - but the alternatives are not too hot, either.
Whilst I maintain, contrary to what a lot have suggested, that conversion values could be validated without actually parsing, to do so would involve some mile-long regular expressions (if we take integer as the most easily parsed type, even a ranged regex for that would be quite a hefty beast).
The consensus seems to be that MS have taken the right approach here by taking the pragmatic route and I tend to agree with that but I can't help wondering how many of us would flag this up in a code review.
It would also be interesting to know how those who voted that outs should never be used would approach the problem.
98.4% of statistics are made up on the spot.
|
|
|
|
|
TryParse doesn't use exceptions as a method of flow control. Parse does. TryParse returns a boolean (the exception is only for null values, if I'm not wrong).
About validating with regular expressions, it would be actually much slower than doing the actual TryParse, and that means pre-validating to do a parse will be slower than the entire operation.
In the end, what happens is that now we have more alternatives than out parameters. For ints, for example, using int? (that is, Nullable<int>) would do the job pretty well. I never really tried to check the performance difference to see if there's any, but it looks nicer than the out parameter.
|
|
|
|
|
PeejayAdams wrote: TryParse is "wrong" in many ways - violation of SRP, use of exceptions as a method of flow control and so forth
Your comments may be driven by a misunderstanding of how TryParse works under the covers. The "Try" in the name doesn't refer to the "try" statement for catching exceptions. The method parses as it goes and if it comes to a character it can't parse it returns false, so you could argue (tenuously I admit) that TryParse doesn't validate, it only parses. The alternatives are to split the methods out which means doing the validation aspect twice. Most people would agree that breaking SRP (if it does) for such a trivial and oft-used method is a great price to pay for the performance gained. You shouldn't adhere to principals so strongly that they have negative effects elsewhere. Yes SOLID is good, but don't throw the baby out with the bathwater.
|
|
|
|
|
PeejayAdams wrote: which violate the SRP by both validating and parsing.
Yes, and CQS - Command Query Separation, which I find makes for cleaner and easier to understand code.
Kevin
|
|
|
|
|
General 
News 
Suggestion 
Question 
Bug 
Answer 
Joke 
Praise 
Rant 
Admin 
Submit an article or tip
