Code Optimize
-
There is a valid reason for using the
ResultFlagform. It's called debugging. How do you find out what went wrong when theExecuteNonQueryreturns nonzero? Don't you think the value returned might give you a clue? PeterSoftware rusts. Simon Stephenson, ca 1994.
Agreed with Peter, Beside this just reduce the code, but I dont see any performance improvement. also in some programming language comparison does not return only true and false but sometime it also return -1(VBA)
-
That
if(something)
return true;
else return false;... is far too prevalent. Its cousin,
if(something)
return a;
else return b;... is at least understandable as some people have an allergic reaction to even simple ternaries (I have no idea why, they are a perfectly valid part of the language and have been since C). Interesting to see someone else who likes to do
if(0 != ...)
... as well.
Ternaries ... you're right, as long as the expressions in the ternaries are simple, they are readable. But they can be simple initially, and become monstrous as the code evolves. Which may be why many people avoid ternaries - the same as with braces around blocks consisting of a single statement. Although I don't buy either (no ternaries and braces around single statements) - you write the code as is fit initially, and reformat/refactor as needed when you change it. There's another horror format: boolean b; ... if (b == true) return x; else return y; A variant is b being a boolean function.
-
Agreed with Peter, Beside this just reduce the code, but I dont see any performance improvement. also in some programming language comparison does not return only true and false but sometime it also return -1(VBA)
Nope, not right. Any decent debugger/IDE has a facility for evaluating expressions (IIRC VBA has one too - something called the immediate window, I think). Depending on the language, your compiler might not optimize away the additional variable which you use. Besides, more compact code is always a bonus. And even in VBA, for this particular case, what you care about is != 0, which even VBA evaluates correctly, since -1 and 1 are both true, only 0 being false.
-
Ternaries ... you're right, as long as the expressions in the ternaries are simple, they are readable. But they can be simple initially, and become monstrous as the code evolves. Which may be why many people avoid ternaries - the same as with braces around blocks consisting of a single statement. Although I don't buy either (no ternaries and braces around single statements) - you write the code as is fit initially, and reformat/refactor as needed when you change it. There's another horror format: boolean b; ... if (b == true) return x; else return y; A variant is b being a boolean function.
Florin Jurcovici wrote:
Although I don't buy either (no ternaries and braces around single statements) - you write the code as is fit initially, and reformat/refactor as needed when you change it.
Yes, exactly. And a simple
return statement ? a : b
... is not too hard to read, for sure. Someone here is really passive-aggressive anti-ternary, judging by the downvote my other post got :~ Heh, that pattern is even worse.
-
That
if(something)
return true;
else return false;... is far too prevalent. Its cousin,
if(something)
return a;
else return b;... is at least understandable as some people have an allergic reaction to even simple ternaries (I have no idea why, they are a perfectly valid part of the language and have been since C). Interesting to see someone else who likes to do
if(0 != ...)
... as well.
BobJanova wrote:
Interesting to see someone else who likes to do
if(0 != ...)
... as well.
This is called Yoda condition.
-
Today I found this code, from DAL class
public Boolean Execute_NoN_Query(string Sqlstring)
{
int ResultFlag = 0;
ResultFlag = MSSqlHelper.SqlHelper.ExecuteNonQuery(SqlServerConnection.Cn, CommandType.Text, Sqlstring);if (ResultFlag != 0) return true; else return false; }My Code is ....
public Boolean Execute_NoN_Query(string Sqlstring)
{
return (0 != MSSqlHelper.SqlHelper.ExecuteNonQuery(SqlServerConnection.Cn, CommandType.Text, Sqlstring));
}my Tip/Tricks[^] | "Rajesh-Puli" now "Rajesh-Anuhya"
:thumbsdown:I disagree with the point the OP is trying to make. The original code is easier to read:confused: and
debug:(( . The second code has me counting nested parenthesis, :doh: and commas, it takes longer to read and understand when you're not familiar with the code, and it's harder to spot a factual error. X| I reckon that when you're reading the same code over and over :^) , the shorter route works,:cool: but in a professional environment,:suss: readable code is better than clever code.:thumbsup: Except maybe when performance is critical, :) and your compiler is not awesome, ;P like in some embedded systems, or some video games. :|"Computer Science is no more about computers than astronomy is about telescopes." - Edsger Dijkstra
-
Agreed with Peter, Beside this just reduce the code, but I dont see any performance improvement. also in some programming language comparison does not return only true and false but sometime it also return -1(VBA)
If examining the result while debugging is your main concern, you can still write
int ResultFlag = MSSqlHelper.SqlHelper.ExecuteNonQuery(SqlServerConnection.Cn, CommandType.Text, Sqlstring);
return ResultFlag != 0;
I've seen worse though...
const bool valTrue = true;
const bool valFalse = false;bool doTheWork()
{
...more impressive code...if ( result == valTrue ) return true;
else return false;
}That coder was preparing himself for the time that valTrue became false or something!?
-
:thumbsdown:I disagree with the point the OP is trying to make. The original code is easier to read:confused: and
debug:(( . The second code has me counting nested parenthesis, :doh: and commas, it takes longer to read and understand when you're not familiar with the code, and it's harder to spot a factual error. X| I reckon that when you're reading the same code over and over :^) , the shorter route works,:cool: but in a professional environment,:suss: readable code is better than clever code.:thumbsup: Except maybe when performance is critical, :) and your compiler is not awesome, ;P like in some embedded systems, or some video games. :|"Computer Science is no more about computers than astronomy is about telescopes." - Edsger Dijkstra
Good, every one saying about the readability and Debugging, However this is a Small code, which is called by many classes. my point is why should i declare a extra variable "resultflag", where this method called 5000+ times in every 10 seconds.
my Tip/Tricks[^] | "Rajesh-Puli" now "Rajesh-Anuhya"
-
:thumbsdown:I disagree with the point the OP is trying to make. The original code is easier to read:confused: and
debug:(( . The second code has me counting nested parenthesis, :doh: and commas, it takes longer to read and understand when you're not familiar with the code, and it's harder to spot a factual error. X| I reckon that when you're reading the same code over and over :^) , the shorter route works,:cool: but in a professional environment,:suss: readable code is better than clever code.:thumbsup: Except maybe when performance is critical, :) and your compiler is not awesome, ;P like in some embedded systems, or some video games. :|"Computer Science is no more about computers than astronomy is about telescopes." - Edsger Dijkstra
One line is more readable than four when three of the four are wasted space. That's because you get three more lines of other code on your screen which help to show the class context (unless the whole class fits on one screen, but that's unusual). There's only one operation in that method which is complex enough to require reading, and putting pointless extra lines makes it less readable. And I really hope you were intentionally making a point about spurious material with all those emoticons and extraneous formatting, because it makes your post much harder to read.
-
One line is more readable than four when three of the four are wasted space. That's because you get three more lines of other code on your screen which help to show the class context (unless the whole class fits on one screen, but that's unusual). There's only one operation in that method which is complex enough to require reading, and putting pointless extra lines makes it less readable. And I really hope you were intentionally making a point about spurious material with all those emoticons and extraneous formatting, because it makes your post much harder to read.
Quote:
And I really hope you were intentionally making a point about spurious material with all those emoticons and extraneous formatting, because it makes your post much harder to read.
punch :thumbsup: +5
my Tip/Tricks[^] | "Rajesh-Puli" now "Rajesh-Anuhya"
-
There is a valid reason for using the
ResultFlagform. It's called debugging. How do you find out what went wrong when theExecuteNonQueryreturns nonzero? Don't you think the value returned might give you a clue? PeterSoftware rusts. Simon Stephenson, ca 1994.
Agree. There is another VERY important reason why you should NOT change working production code unless you really must; Don't change tried and tested code! Ok. this code sample you show us is a bit clumsy but it works, right? So why change it? IMHO a good programmer must learnt NOT to change production code unless it's really really needed.
-
Agree. There is another VERY important reason why you should NOT change working production code unless you really must; Don't change tried and tested code! Ok. this code sample you show us is a bit clumsy but it works, right? So why change it? IMHO a good programmer must learnt NOT to change production code unless it's really really needed.
I definitely disagree with you there. Cleaning up code results in a (marginally, for any particular instance, but it builds up) nicer codebase to work in and that results in better productivity for everyone on the team. If it's 'tried and tested' then you can check that the tests still pass and therefore be sure you haven't broken anything with your cleanup.
-
Good, every one saying about the readability and Debugging, However this is a Small code, which is called by many classes. my point is why should i declare a extra variable "resultflag", where this method called 5000+ times in every 10 seconds.
my Tip/Tricks[^] | "Rajesh-Puli" now "Rajesh-Anuhya"
I shouldn't worry about it, if you're accessing a data layer any saving from avoiding allocating 32-bits on the stack is negligible. Premature optimisation is the root of all evil. The only actual "improvement" you've made is that the code occupies less space on screen. All the "improvements" you claim will be performed by a decent optimising compiler on release code anyway. Design for readability/maintainability, then optimise as you have evidence it is worthwhile.
-
I definitely disagree with you there. Cleaning up code results in a (marginally, for any particular instance, but it builds up) nicer codebase to work in and that results in better productivity for everyone on the team. If it's 'tried and tested' then you can check that the tests still pass and therefore be sure you haven't broken anything with your cleanup.
I agree - Cleaning up the codebase is very tempting, it's much nicer to work on "clean" code. But my point is this: Cleaning up production code just for the sake of making it look "nice" is very dangerous because (depending on code size of course) you WILL create new bugs in doing so.
-
I agree - Cleaning up the codebase is very tempting, it's much nicer to work on "clean" code. But my point is this: Cleaning up production code just for the sake of making it look "nice" is very dangerous because (depending on code size of course) you WILL create new bugs in doing so.
-
I agree - Cleaning up the codebase is very tempting, it's much nicer to work on "clean" code. But my point is this: Cleaning up production code just for the sake of making it look "nice" is very dangerous because (depending on code size of course) you WILL create new bugs in doing so.
Snorri wrote:
But my point is this: Cleaning up production code just for the sake of making it look "nice" is very dangerous because you SOMETIMES will create new bugs in doing so.
FTFY. :-) And sometimes cleaning up code will fix latent bugs that no one has run into yet, or possibly they've hit them and just haven't reported them, or possibly they've hit them and thought that was normal behavior. Creating new bugs will be mitigated with unit tests; which of course one always has before refactoring.
-
Good, every one saying about the readability and Debugging, However this is a Small code, which is called by many classes. my point is why should i declare a extra variable "resultflag", where this method called 5000+ times in every 10 seconds.
my Tip/Tricks[^] | "Rajesh-Puli" now "Rajesh-Anuhya"
Declaring an extra variable doesn't affect your performance at all. Compilers can (and will) easily do the code reduction you did (note what you did is not an optimization). On the other, having that extra variable does aid in readability and debugging. The only rewrite I would is (if I absolutely have to change something for no reason other than style):
return ResultFlag != 0;
-
Good, every one saying about the readability and Debugging, However this is a Small code, which is called by many classes. my point is why should i declare a extra variable "resultflag", where this method called 5000+ times in every 10 seconds.
my Tip/Tricks[^] | "Rajesh-Puli" now "Rajesh-Anuhya"
Rajesh Anuhya wrote:
However this is a Small code, which is called by many classes.
my point is why should i declare a extra variable "resultflag", where this method called 5000+ times in every 10 secondsI can only suppose that that is a general statement that has nothing to do with the code presented. First the code presented suggests it is doing a database call. You will not be able to even measure the performance gain that you are claiming because of that. The impact of the database call will completely overwhelm the the measurement of what you are claiming. I would suspect that even the variability of network traffic itself would reduce your measurement to the noise level. Second if your goal is to improve the performance of the that code then you must reduce the number of calls to the database. For example by using a memory cache. That would have a significant impact. Finally the code is using a return value that is going to end up on the stack regardless of whether it is explicitly stated or implicitly stated. I wouldn't be suprised if the emitted code is almost basically the same between the two versions.
-
Florin Jurcovici wrote:
Although I don't buy either (no ternaries and braces around single statements) - you write the code as is fit initially, and reformat/refactor as needed when you change it.
Yes, exactly. And a simple
return statement ? a : b
... is not too hard to read, for sure. Someone here is really passive-aggressive anti-ternary, judging by the downvote my other post got :~ Heh, that pattern is even worse.
Probably gets paid by lines of code.
-
Today I found this code, from DAL class
public Boolean Execute_NoN_Query(string Sqlstring)
{
int ResultFlag = 0;
ResultFlag = MSSqlHelper.SqlHelper.ExecuteNonQuery(SqlServerConnection.Cn, CommandType.Text, Sqlstring);if (ResultFlag != 0) return true; else return false; }My Code is ....
public Boolean Execute_NoN_Query(string Sqlstring)
{
return (0 != MSSqlHelper.SqlHelper.ExecuteNonQuery(SqlServerConnection.Cn, CommandType.Text, Sqlstring));
}my Tip/Tricks[^] | "Rajesh-Puli" now "Rajesh-Anuhya"
I actually came up with your result before I read further and saw your result. Then I thought, No, you(me) are wrong. Sure as shooting, as soon as I code it that way, when the return is 5 call this routine, when less than 0 call another routine otherwise call a third routine, then return true when the result isn't 0. If I make these 5 statements into 2, they'll never ask to do that. :laugh: