Strings: the perfect way to compare numbers.
-
You have no idea how much I would like to accommodate you on this one, but I'm not ready to retire quite yet and I don't think I could stop at just one, so I can't make it look like an accident.
I wasn't, now I am, then I won't be anymore.
You know you want to... :laugh: 5!
Real men don't use instructions. They are only the manufacturers opinion on how to put the thing together. Digital man: "You are, in short, an idiot with the IQ of an ant and the intellectual capacity of a hose pipe."
-
I'm not saying exactly where this came from (to protect the guilty), but it was a question on this very site. (I have also changed the field and variable names) A code fragment you may appreciate:
float result = 0F;
SqlCommand cmd = new SqlCommand("Select SUM(myField) From myTable Where myOtherField = 'Value'", con);
if (cmd.ExecuteScalar().ToString() != "0" && cmd.ExecuteScalar().ToString() != "")
result= float.Parse(cmd.ExecuteScalar().ToString().Trim());Three lines of code; How many don't-do-its can you spot?
Two extra database accesses
Three unnecessary int-to-string conversions
One unnecessary Trim operation (with the output guaranteed to equal the input)
One unnecessary Parse operation
Six unnecessary string creationsReal men don't use instructions. They are only the manufacturers opinion on how to put the thing together. Digital man: "You are, in short, an idiot with the IQ of an ant and the intellectual capacity of a hose pipe."
...so how would you write that bit of code?
OriginalGriff wrote:
Three unnecessary int-to-string conversions
There are 0 int-to-string conversions in the code you posted
OriginalGriff wrote:
One unnecessary Parse operation
Need to get it to a float somehow, and parse will only take a string. You could use ...
(float)Convert.ToDouble(cmd.ExecuteScalar());
...but not sure how good that is for performance, or perhaps take the chance of a straight cast (but could be risky)...
float result = (float)cmd.ExecuteScalar();
EDIT: :mad: What deserves a vote of 1? the fact that somebody failed to note that ExecuteScalar return an object not an int? or the fact that people don't like other people not being in complete agreement?
Illogical thoughts make me ill
-
...so how would you write that bit of code?
OriginalGriff wrote:
Three unnecessary int-to-string conversions
There are 0 int-to-string conversions in the code you posted
OriginalGriff wrote:
One unnecessary Parse operation
Need to get it to a float somehow, and parse will only take a string. You could use ...
(float)Convert.ToDouble(cmd.ExecuteScalar());
...but not sure how good that is for performance, or perhaps take the chance of a straight cast (but could be risky)...
float result = (float)cmd.ExecuteScalar();
EDIT: :mad: What deserves a vote of 1? the fact that somebody failed to note that ExecuteScalar return an object not an int? or the fact that people don't like other people not being in complete agreement?
Illogical thoughts make me ill
More like this, I didn't do any tests, so I'm sure it can still be improved
var cmd = new SqlCommand("Select SUM(myField) From myTable Where myOtherField = 'Value'", con);
var resultString = cmd.ExecuteScalar().ToString();var result = (!string.IsNullOrEmpty(result) & !result.Equals("0"))
? (float)System.Convert.ToDouble(resultString)
: 0F; -
More like this, I didn't do any tests, so I'm sure it can still be improved
var cmd = new SqlCommand("Select SUM(myField) From myTable Where myOtherField = 'Value'", con);
var resultString = cmd.ExecuteScalar().ToString();var result = (!string.IsNullOrEmpty(result) & !result.Equals("0"))
? (float)System.Convert.ToDouble(resultString)
: 0F;get rid of the var's and then we can talk :laugh: certainly an improvement thou you would be wasting your time checking for result.Equals("0") to end up setting it to 0 anyway, also Convert.ToDouble() will return 0 for empty string anyway (meant that for a null object). Problem is if the result is non-numeric. That's why I would lean more to a (try)parse
SqlCommand cmd = new SqlCommand("...", con);
float result = 0;
float.TryParse(cmd.ExecuteScalar().ToString(), out result);
return result;I am not saying this is the best best way, but it covers null, empty and non-numeric values EDIT: as I have correctly been corrected, I should test for null before using ToString() a shameful mistake...
SqlCommand cmd = new SqlCommand("...", con);
float result = 0;
float.TryParse(cmd.ExecuteScalar() AS string, out result);
return result;GETTING THERE...
SqlCommand cmd = new SqlCommand("...", con);
float result = 0;
object cmdResult = cmd.ExecuteScalar();
if(cmdResult != null)
float.TryParse(cmdResult.ToString(), out result);
return result;...of course _Erik_ has the better (and smaller) amount of code
Don't vote my posts down just because you don't understand them - if you lack the superior intelligence that I possess then simply walk away
modified on Wednesday, March 2, 2011 7:10 AM
-
get rid of the var's and then we can talk :laugh: certainly an improvement thou you would be wasting your time checking for result.Equals("0") to end up setting it to 0 anyway, also Convert.ToDouble() will return 0 for empty string anyway (meant that for a null object). Problem is if the result is non-numeric. That's why I would lean more to a (try)parse
SqlCommand cmd = new SqlCommand("...", con);
float result = 0;
float.TryParse(cmd.ExecuteScalar().ToString(), out result);
return result;I am not saying this is the best best way, but it covers null, empty and non-numeric values EDIT: as I have correctly been corrected, I should test for null before using ToString() a shameful mistake...
SqlCommand cmd = new SqlCommand("...", con);
float result = 0;
float.TryParse(cmd.ExecuteScalar() AS string, out result);
return result;GETTING THERE...
SqlCommand cmd = new SqlCommand("...", con);
float result = 0;
object cmdResult = cmd.ExecuteScalar();
if(cmdResult != null)
float.TryParse(cmdResult.ToString(), out result);
return result;...of course _Erik_ has the better (and smaller) amount of code
Don't vote my posts down just because you don't understand them - if you lack the superior intelligence that I possess then simply walk away
modified on Wednesday, March 2, 2011 7:10 AM
I was copying the logic of the OP, like I said, I hadn't tested anything. But reducing the DB access from 3 to 1, will make a HUGE performance gain. ps. 'var' is gold. I use it everywhere since my variable names are clear, and I refactor a lot. And not having to change types saves a lot of time.
-
I was copying the logic of the OP, like I said, I hadn't tested anything. But reducing the DB access from 3 to 1, will make a HUGE performance gain. ps. 'var' is gold. I use it everywhere since my variable names are clear, and I refactor a lot. And not having to change types saves a lot of time.
Yep I agree reducing the DB access is definitely the best performance optimisation.
GibbleCH wrote:
I use it everywhere since my variable names are clear
That's fair enough, I just prefer having the type declared at the start as it makes easier scanning IMO
Don't vote my posts down just because you don't understand them - if you lack the superior intelligence that I possess then simply walk away
-
...so how would you write that bit of code?
OriginalGriff wrote:
Three unnecessary int-to-string conversions
There are 0 int-to-string conversions in the code you posted
OriginalGriff wrote:
One unnecessary Parse operation
Need to get it to a float somehow, and parse will only take a string. You could use ...
(float)Convert.ToDouble(cmd.ExecuteScalar());
...but not sure how good that is for performance, or perhaps take the chance of a straight cast (but could be risky)...
float result = (float)cmd.ExecuteScalar();
EDIT: :mad: What deserves a vote of 1? the fact that somebody failed to note that ExecuteScalar return an object not an int? or the fact that people don't like other people not being in complete agreement?
Illogical thoughts make me ill
musefan wrote:
...so how would you write that bit of code?
musefan wrote:
Need to get it to a float somehow
int? result = (int?)cmd.ExecuteScalar();
return (float)result.GetValueOrDefault();There is no need for esoteric conversions, Parsing or ToString at all. Actually, there is no need for an if in the original code posted, and if ExecuteScalar returns null, the original code would throw a null reference exception.
musefan wrote:
What deserves a vote of 1?
I don't know, I did not downvote you. Edit: Where you see "int?" I put "float?" before, and that was a mistake.
modified on Wednesday, March 2, 2011 6:18 AM
-
get rid of the var's and then we can talk :laugh: certainly an improvement thou you would be wasting your time checking for result.Equals("0") to end up setting it to 0 anyway, also Convert.ToDouble() will return 0 for empty string anyway (meant that for a null object). Problem is if the result is non-numeric. That's why I would lean more to a (try)parse
SqlCommand cmd = new SqlCommand("...", con);
float result = 0;
float.TryParse(cmd.ExecuteScalar().ToString(), out result);
return result;I am not saying this is the best best way, but it covers null, empty and non-numeric values EDIT: as I have correctly been corrected, I should test for null before using ToString() a shameful mistake...
SqlCommand cmd = new SqlCommand("...", con);
float result = 0;
float.TryParse(cmd.ExecuteScalar() AS string, out result);
return result;GETTING THERE...
SqlCommand cmd = new SqlCommand("...", con);
float result = 0;
object cmdResult = cmd.ExecuteScalar();
if(cmdResult != null)
float.TryParse(cmdResult.ToString(), out result);
return result;...of course _Erik_ has the better (and smaller) amount of code
Don't vote my posts down just because you don't understand them - if you lack the superior intelligence that I possess then simply walk away
modified on Wednesday, March 2, 2011 7:10 AM
-
musefan wrote:
...so how would you write that bit of code?
musefan wrote:
Need to get it to a float somehow
int? result = (int?)cmd.ExecuteScalar();
return (float)result.GetValueOrDefault();There is no need for esoteric conversions, Parsing or ToString at all. Actually, there is no need for an if in the original code posted, and if ExecuteScalar returns null, the original code would throw a null reference exception.
musefan wrote:
What deserves a vote of 1?
I don't know, I did not downvote you. Edit: Where you see "int?" I put "float?" before, and that was a mistake.
modified on Wednesday, March 2, 2011 6:18 AM
I think you where better of with float, what if SUM returns a double value? Plus int to float can be implicitly cast I get what you are saying thou. I am not sure on the exact ins and outs of the performances of casting to nullable and using GetValueOrDefault() over an if null statement but I doubt it would be much either way so preference will get the job done
Don't vote my posts down just because you don't understand them - if you lack the superior intelligence that I possess then simply walk away
-
musefan wrote:
float.TryParse(cmd.ExecuteScalar().ToString(), out result);
That will throw a null reference exception if ExecuteScalar returns null.
-
I think you where better of with float, what if SUM returns a double value? Plus int to float can be implicitly cast I get what you are saying thou. I am not sure on the exact ins and outs of the performances of casting to nullable and using GetValueOrDefault() over an if null statement but I doubt it would be much either way so preference will get the job done
Don't vote my posts down just because you don't understand them - if you lack the superior intelligence that I possess then simply walk away
Yup, you are right, I was better with "float?". I did not read the query (as usual, I am soooo absentminded). The nullable type to use must be the one which fits with the one defined into the database, I mean, if the query is defined to return a float you cannot use "double?", becouse the unboxing operation would fail. Anyway, GetValueOrDefault is a constant complexity operation, so in these cases I usually choose the solution which gives me a cleaner implementation.
-
:doh: just testing ;P ...perhaps you should put it in a new hall of shame post
Don't vote my posts down just because you don't understand them - if you lack the superior intelligence that I possess then simply walk away
Oh, no... We all make this kind of mistakes from time to time. However, the way you have corrected it would not work either. In this case ExecuteScalar would return null or a boxed value type, so checking its result with "as string" would always return null. I you don't use a nullabe type here you would have to use an object and check for null before trying to make any conversion, I mean:
object obj = cmd.ExecuteScalar();
if (obj != null)
// Now you can make the conversionI my opinion, nullable types are much cleaner for this.
-
Yup, you are right, I was better with "float?". I did not read the query (as usual, I am soooo absentminded). The nullable type to use must be the one which fits with the one defined into the database, I mean, if the query is defined to return a float you cannot use "double?", becouse the unboxing operation would fail. Anyway, GetValueOrDefault is a constant complexity operation, so in these cases I usually choose the solution which gives me a cleaner implementation.
_Erik_ wrote:
The nullable type to use must be the one which fits with the one defined into the database
I think a nullable float would still do the trick, even if the DB defined the value as decimal or numeric or int or float then the explicit case to float would still work, and as that is your retuning value anyway you need to cast to float at some point
Don't vote my posts down just because you don't understand them - if you lack the superior intelligence that I possess then simply walk away
-
_Erik_ wrote:
The nullable type to use must be the one which fits with the one defined into the database
I think a nullable float would still do the trick, even if the DB defined the value as decimal or numeric or int or float then the explicit case to float would still work, and as that is your retuning value anyway you need to cast to float at some point
Don't vote my posts down just because you don't understand them - if you lack the superior intelligence that I possess then simply walk away
No, that is not right. Just try this:
object obj = 1;
float? f = (float?)obj;This would throw an exception. The value type boxed into the object in this case is an int, so you need an (int) or (int?) casting to unbox it. Any other type would throw the same exception. Remember we first have to unbox the value type, and the compatibility rules among numeric types are not applied for boxing-unboxing operations.
-
No, that is not right. Just try this:
object obj = 1;
float? f = (float?)obj;This would throw an exception. The value type boxed into the object in this case is an int, so you need an (int) or (int?) casting to unbox it. Any other type would throw the same exception. Remember we first have to unbox the value type, and the compatibility rules among numeric types are not applied for boxing-unboxing operations.
