|
It could have been written before the db contained the validation or without knowledge of the previous. (I know I'm making assumptions and such. It's late and I'm ready to go to bed.)
I wasn't, now I am, then I won't be anymore.
|
|
|
|
|
The fact that an integer is being set to 1 or 0 rather than using a bool is a pretty good indicator that some of this code was copied from the database. Here is what my best guess is as to how this code evolved:
*Only single numbers were checked, so they were put in a database table.
*Next, certain ranges needed to be checked, and they were hard coded in the SP that checked single numbers.
*Somebody thought the SQL code looked ugly or needed to use the SP without ranges, so they moved that to the VB.Net.
*Somebody thought the VB.Net looked ugly, so they added ranges to the database table.
*At some point, there was some VB.Net copy/pasting so that hard coded logic stayed.
Result:
|
|
|
|
|
aspdotnetdev wrote: integer is being set to 1 or 0 rather than using a bool is a pretty good indicator that some of this code was copied from the database
I would have agreed with you until I say some javascript web page logic a while back that was hand coded to work with 1s and 0s instead of booleans. Now I just think there are some pretty thick junior devs out there being allowed to write code that gets to prod without review.
I wasn't, now I am, then I won't be anymore.
|
|
|
|
|
I have seen people use ints with 1s and 0s because they learned programming with a language that did not have a boolean type. I say that is what you get when you have someone whose degree is not in programming write the software
What is really scary is when folk with a real programming degree do the same thing!
Just because the code works, it doesn't mean that it is good code.
|
|
|
|
|
I'm currently ROFL-ing inside, guys!!! A true pearl.
Notice, that ANY int suits the condition )))
|
|
|
|
|
I must have really been tired last night not to catch that. I'm having the same laugh now.
I wasn't, now I am, then I won't be anymore.
|
|
|
|
|
Indeed, that was the real bug I fixed and one of the two things that was odd about the code (excluding the fact that it's VB.Net rather than C#, which is obviously a flaw too).
|
|
|
|
|
aspdotnetdev wrote: VB.Net rather than C#
Very obviously...
I wasn't, now I am, then I won't be anymore.
|
|
|
|
|
Heh heh.. messed up boolean conditions are the bane of so many.. I think boolean logic is one of the hardest things to read well, and is one reason why I unit test my code thoroughly. Misused AND and OR is so easy to miss on code inspection.. but good tests never lie.
|
|
|
|
|
With that kind of range checking, the coder is more suited to checking the range where the burgers are being cooked.
Chris Meech
I am Canadian. [heard in a local bar]
In theory there is no difference between theory and practice. In practice there is. [Yogi Berra]
|
|
|
|
|
Either that, or being brought to the range to be slaughtered. Which wouldn't be much different, because he'd end up at the burger place anyway.
|
|
|
|
|
Shhh!!! Don't give John any ideas!!!
I wasn't, now I am, then I won't be anymore.
|
|
|
|
|
I was digging around some code I wrote three years ago, looking for the cause of a bug some reported, when I found this. And no wonder - the full code is so full of holes that I'm surprised it works as expected.
private static bool CheckSpecialCases(List<List<int>> adjancies, Dictionary<int, Region> regions)
{
if (regions.Count < 2) return true;
if (regions.Count == 2)
{
int key1 = 0, key2 = 0, inc = 0;
foreach (KeyValuePair<int, Region> kvp in regions)
{
if (inc == 0)
{
inc++;
key1 = kvp.Key;
}
else
key2 = kvp.Key;
}
lock (listLock)
{
if (adjancies[key1].Contains(key2))
return true;
adjancies[key1].Add(key2);
adjancies[key2].Add(key1);
return true;
}
}
return false;
}
Hmm, where to start. How about that comment "True if it was a special case". Yes, because it is obvious what a special case is. Then the whole looping over a collection of two items, with a flag to set two items to specific values (instead of say, asking for the keys of the Dictionary). Oh, and the whole reason for the reported bug is that the indices of key1 and key2 were not checked for being in bounds somewhere prior in the code, which would have saved user trouble from this. Plus, the redundancy of using lists to specify connections, while matching the underlying implementation of data that I was working with, makes things unnecessarily complicated.
|
|
|
|
|
Harsh! I hope the guy or gal who wrote this can take a little constructive criticism.
|
|
|
|
|
jamie550 wrote: unnecessarily complicated
In OOP they call it "extendable".
Greetings - Jacek
|
|
|
|
|
A company hired me for cleaning up the PHP-code the CEO created. The whole code reads like a how-not-to of programming, so I decided to post some of the gems here. This is the first one (I preserved all the formatting):
MyCart::IsContentvalid() checks items in the cart against a database. Each call does 3 queries (per item!) of tables with around 500.000 entries. Note also the clever use of quotation marks.
if(MyCart::IsContentvalid()==1)
{
Helper::Redirect("confirmation.php");
}
if(MyCart::IsContentvalid()==2)
{
$this->setErrorMsg("$curlang[error_msg1]");
}
if(MyCart::IsContentvalid()==-12)
{
$this->setErrorMsg("These items can not be sold.");
}
if(MyCart::IsContentvalid()==15)
{
$this->setErrorMsg("Your account doesn't allow you to buy these items.");
}
else
{
$this->setErrorMsg("$lang[error_msg2]");
}
Oh, and the use of else is only for advanced programmers:
if ($result == 1)
{
}
if ($result != 1)
{
}
More will come soon!
|
|
|
|
|
I **really** like the second one.
|
|
|
|
|
Maybe it's just because its a Friday afternoon but I had to laugh at the following code I just found for removing the first 7 chars of a string....
s = s.Replace(s.Substring(0,7), "");
... See the funny side? or is it just me?
Life goes very fast. Tomorrow, today is already yesterday.
|
|
|
|
|
|
That's a particular funny way of doing it wrong yes
|
|
|
|
|
s = "badcoder you creazy badcode?"
s = s.Replace(s.Substring(0,7), "");
(it's 1 am here)
Greetings - Jacek
|
|
|
|
|
dont you mean...
Life goes very fast. Tomorrow, today is already yesterday.
|
|
|
|
|
bool hasWarnings = false;
if (!hasWarnings)
{
hasWarnings = true;
}
|
|
|
|
|
I just saw a piece of javascript today which effectively looked like this:
function check_somethingOrOther()
{
someVariable = false;
setTimeout("check_somethingOrOther()", 500);
} Now, I know that someVariable was obviously meant to do something, but this is the only place in the code that it's actually used. Optimising this code was really simple.
|
|
|
|
|
Yes, but what if it changes. You have to make sure it is really false!
|
|
|
|