Magic of if...else...programming
-
Oddly enough, I just got rid of some code that looked a bit like that...
bool InitRoutine()
{
bool failed=false;ValType val; HRESULT ans = GetValue1(val); if (ans==S\_OK) { globalVal1 = val; } else { failed = failed || true; } ans = GetValue2(val); if (ans==S\_OK) { globalVal2 = val; } else { failed = failed || true; } ans = GetValue3(val); if (ans==S\_OK) { globalVal3 = val; } else { failed = failed || true; } ans = GetValue4(val); if (ans==S\_OK) { globalVal4 = val; } else { failed = failed || true; } return !failed; } -
What's so bad about that? I'd do it like the following but I've seen far worse than that code:
// Only proceed id if 'dt' is valid and contains at least one row.
if (dt == NULL)
return false;
if (dt.Rows.Count == 0)
return false;
return (t.Rows[0]["Number"].ToString() == "1"); -
This code reminds me one developer working under me, according to her every if has to have else, you can't use if alone. :-)
-
Just a question: What if there is no value integer or otherwise in t.Rows[0]["Number"]? Wouldn't this result into a crash.
Learn from the mistakes of others, you may not live long enough to make them all yourself.
Thats why I wrote the following after the code block:
The last part depends on the data the table contains. But if its clear that the column is filled with integers then this should be more efficient.
Probably I should have added: ... and otherwise the code will explode. :-D
-
Just a question: What if there is no value integer or otherwise in t.Rows[0]["Number"]? Wouldn't this result into a crash.
Learn from the mistakes of others, you may not live long enough to make them all yourself.
-
How would you think of this?
return dt != null && dt.Rows.Count > 0 && (int)t.Rows[0]["Number"] == 1;
The last part depends on the data the table contains. But if its clear that the column is filled with integers then this should be more efficient.
Nice (and compact) solution !! Still depends on the precedence priorities of the language / the optimization of the underlaying compiler .. More safe and maintainable code: ------------------------------------ bool res = false; if (null == dt) else if (null == dt.Rows) else if (dt.Rows.Count < 0) else res = (1 == (int)dt.Rows[0]["Number"]); return res; ------------------------------------ Rules to be applied : (1) : prevent against '=' instead of '==' : always put constants first (2) : always control potential nulls even if seems useless versus construction rules (ex null == dt.Rows) (3) : provide debugging / tracing points in case of future problems (4) : write readable code (5) : single return output point Shears and happy new year.
modified on Saturday, January 10, 2009 5:03 AM
-
Nice (and compact) solution !! Still depends on the precedence priorities of the language / the optimization of the underlaying compiler .. More safe and maintainable code: ------------------------------------ bool res = false; if (null == dt) else if (null == dt.Rows) else if (dt.Rows.Count < 0) else res = (1 == (int)dt.Rows[0]["Number"]); return res; ------------------------------------ Rules to be applied : (1) : prevent against '=' instead of '==' : always put constants first (2) : always control potential nulls even if seems useless versus construction rules (ex null == dt.Rows) (3) : provide debugging / tracing points in case of future problems (4) : write readable code (5) : single return output point Shears and happy new year.
modified on Saturday, January 10, 2009 5:03 AM
qualitychecker wrote:
Still depends on the precedence priorities of the language
Uh, yeah, so? Are we going to get into that again?
qualitychecker wrote:
always put constants first
If you can remember to do that, you're smart enough not to make that mistake in the first place.
qualitychecker wrote:
provide debugging / tracing points in case of future problems
No thanks.
-
qualitychecker wrote:
Still depends on the precedence priorities of the language
Uh, yeah, so? Are we going to get into that again?
qualitychecker wrote:
always put constants first
If you can remember to do that, you're smart enough not to make that mistake in the first place.
qualitychecker wrote:
provide debugging / tracing points in case of future problems
No thanks.
PIEBALDconsult wrote:
If you can remember to do that, you're smart enough not to make that mistake in the first place.
I agree, although one might argue that on the rare occasion one might forget to apply this strange habit, the statement could still be correct (i.e. one could accidentally forget to drop one of the = signs). :-D
Luc Pattyn [Forum Guidelines] [My Articles]
Love, happiness and fewer bugs for 2009!
-
The following code was found in someone's web template. Hope you enjoy the best logic.
if(dt != null)
{
if(dt.Rows.Count != 0)
{
if(dt.Rows[0]["Number"].ToString() == "1")
{
return true;
}
else
{
return false;
}
}
else
{
return false;
}
}
else
{
return false;
}There's nothing "horrible" in this code. Industry is all about making something that works and that's easily understandable by your co-workers. Sure, this is going over the top, and I'd do something that was suggested above, like:
if (dt == NULL)
return false;
if (dt.Rows.Count == 0)
return false;return (t.Rows[0]["Number"].ToString() == "1");
But, seriously, who cares? It's readable and it will run as fast as
return dt != null && dt.Rows.Count > 0 && (int)t.Rows[0]["Number"] == 1; -
Nah. This is much more difficult to debug, how would you set a breakpoint anywhere inside such a complex expression?
Luc Pattyn [Forum Guidelines] [My Articles]
Love, happiness and fewer bugs for 2009!
Agreed.
Steve
-
Nice (and compact) solution !! Still depends on the precedence priorities of the language / the optimization of the underlaying compiler .. More safe and maintainable code: ------------------------------------ bool res = false; if (null == dt) else if (null == dt.Rows) else if (dt.Rows.Count < 0) else res = (1 == (int)dt.Rows[0]["Number"]); return res; ------------------------------------ Rules to be applied : (1) : prevent against '=' instead of '==' : always put constants first (2) : always control potential nulls even if seems useless versus construction rules (ex null == dt.Rows) (3) : provide debugging / tracing points in case of future problems (4) : write readable code (5) : single return output point Shears and happy new year.
modified on Saturday, January 10, 2009 5:03 AM
qualitychecker wrote:
Rules to be applied : (1) : prevent against '=' instead of '==' : always put constants first (2) : always control potential nulls even if seems useless versus construction rules (ex null == dt.Rows) (3) : provide debugging / tracing points in case of future problems (4) : write readable code (5) : single return output point
You cannot satisfy rule (4) AND all the others :)
-- "My software never has bugs. It just develops random features."
-
Nice (and compact) solution !! Still depends on the precedence priorities of the language / the optimization of the underlaying compiler .. More safe and maintainable code: ------------------------------------ bool res = false; if (null == dt) else if (null == dt.Rows) else if (dt.Rows.Count < 0) else res = (1 == (int)dt.Rows[0]["Number"]); return res; ------------------------------------ Rules to be applied : (1) : prevent against '=' instead of '==' : always put constants first (2) : always control potential nulls even if seems useless versus construction rules (ex null == dt.Rows) (3) : provide debugging / tracing points in case of future problems (4) : write readable code (5) : single return output point Shears and happy new year.
modified on Saturday, January 10, 2009 5:03 AM
qualitychecker wrote:
(1) : prevent against '=' instead of '==' : always put constants first
I used to do this a bit when I was writting C, but I don't think there is any benefit with C#. Try doing a single = in a in a if statement and the compiler will complain. So can anybody see a benefit of doing (null == x)? Actually, I've just checked, it doesn't always complain, although I think it does sometimes complain.
modified on Monday, January 12, 2009 9:22 AM
-
qualitychecker wrote:
(1) : prevent against '=' instead of '==' : always put constants first
I used to do this a bit when I was writting C, but I don't think there is any benefit with C#. Try doing a single = in a in a if statement and the compiler will complain. So can anybody see a benefit of doing (null == x)? Actually, I've just checked, it doesn't always complain, although I think it does sometimes complain.
modified on Monday, January 12, 2009 9:22 AM
Member 4487083 wrote:
the compiler will complain
Except when using booleans.
-
That's not cheap though, it's way more expensive in the long run if they aren't competent. More fixing bugs, and longer development time. And what the heck kind of code would they put in the else if you don't technically need an else?
-
Oddly enough, I just got rid of some code that looked a bit like that...
bool InitRoutine()
{
bool failed=false;ValType val; HRESULT ans = GetValue1(val); if (ans==S\_OK) { globalVal1 = val; } else { failed = failed || true; } ans = GetValue2(val); if (ans==S\_OK) { globalVal2 = val; } else { failed = failed || true; } ans = GetValue3(val); if (ans==S\_OK) { globalVal3 = val; } else { failed = failed || true; } ans = GetValue4(val); if (ans==S\_OK) { globalVal4 = val; } else { failed = failed || true; } return !failed; }Beautiful... failed = failed || true;
-
Nice (and compact) solution !! Still depends on the precedence priorities of the language / the optimization of the underlaying compiler .. More safe and maintainable code: ------------------------------------ bool res = false; if (null == dt) else if (null == dt.Rows) else if (dt.Rows.Count < 0) else res = (1 == (int)dt.Rows[0]["Number"]); return res; ------------------------------------ Rules to be applied : (1) : prevent against '=' instead of '==' : always put constants first (2) : always control potential nulls even if seems useless versus construction rules (ex null == dt.Rows) (3) : provide debugging / tracing points in case of future problems (4) : write readable code (5) : single return output point Shears and happy new year.
modified on Saturday, January 10, 2009 5:03 AM
qualitychecker wrote:
More safe and maintainable code: ------------------------------------ bool res = false; if (null == dt) else if (null == dt.Rows) else if (dt.Rows.Count < 0) else res = (1 == (int)dt.Rows[0]["Number"]); return res; ------------------------------------
Oops - hate to say this, but if dt.Rows.Count == 0 - i.e. the table is empty - then that code will throw an array-out-of-bounds exception. You meant
else if (dt.Rows.Count <= 0), right? -- T -
The following code was found in someone's web template. Hope you enjoy the best logic.
if(dt != null)
{
if(dt.Rows.Count != 0)
{
if(dt.Rows[0]["Number"].ToString() == "1")
{
return true;
}
else
{
return false;
}
}
else
{
return false;
}
}
else
{
return false;
}Other than what has been already pointed out..."code trimming" for lesser cycles, I also suggest not to use the string literals as such. They break the clients when modified. static readonly string colName = "Number"; static readonly string colVal = "1"; -------------------------- if(dt != null && dt.Rows.Count > 0) { return dt.Rows[0][colName].ToString().Equals(colVal); } else return false;
-
Other than what has been already pointed out..."code trimming" for lesser cycles, I also suggest not to use the string literals as such. They break the clients when modified. static readonly string colName = "Number"; static readonly string colVal = "1"; -------------------------- if(dt != null && dt.Rows.Count > 0) { return dt.Rows[0][colName].ToString().Equals(colVal); } else return false;
vnike wrote:
They break the clients when modified.
Only if the client is a different assembly. I would imagine that column names and indexes would be private to a class, so I don't see any issue with using const there.
Regards Senthil [MVP - Visual C#] _____________________________ My Home Page |My Blog | My Articles | My Flickr | WinMacro
-
Beautiful... failed = failed || true;
Short circuit evaluation, man! You don't want to be setting failed to true if it is already! :)