- Feature Articles
- CodeSOD
-
Error'd
- Most Recent Articles
- Stop Poking Me!
- Operation Erred Successfully
- A Dark Turn
- Nothing Doing
- Home By Another Way
- Coast Star
- Forsooth
- Epic
- Forums
-
Other Articles
- Random Article
- Other Series
- Alex's Soapbox
- Announcements
- Best of…
- Best of Email
- Best of the Sidebar
- Bring Your Own Code
- Coded Smorgasbord
- Mandatory Fun Day
- Off Topic
- Representative Line
- News Roundup
- Editor's Soapbox
- Software on the Rocks
- Souvenir Potpourri
- Sponsor Post
- Tales from the Interview
- The Daily WTF: Live
- Virtudyne
Admin
first++
Admin
Possible this started life as a method to count the number of columns which matched some criterion, so inside the loop there was an if statement.
Then the requirements changed and they needed to include all columns, so they just removed the condition.
That doesn't excuse the fact that the count is off by one, of course, unless this actually subtly wants to include e.g. the area beyond the last column.
Admin
Fizz buzz kill
Admin
Actually the code is so obviously nonsense that I wouldn't be surprised if the compiler optimizes it away
Admin
Actually, using the lax comparison removes the off-by-one: If
dgView.Rows.Count == 1
, using the strict operator would result incount == 0
Admin
Nah, nevermind what I said... Not enough coffee yet...
Admin
Buy a car, then reassemble the whole thing from scratch in your garage. Why? Because.
Admin
@Prime Mover s/column/row/
Otherwise I agree this makes a mindless sort of sense as a hunk of copypasta ; start with a code hunk that counts rows subject to some if statement condition (written pre-LINQ of course), then copypasta it into this situation deleting the now unneeded if.
Effort required: small. Thought involved: zero.
For way too many devs, that's a tradeoff they'll make every chance they get.
Admin
This is easily corrected by subtracting 1 from 'count' after the loop.
And easily made twice as correct by subsequently replacing '<=' with '<'.
Admin
And you just know that somewhere in the code base something now relies on the fact that this counts it wrong.
Admin
Oh right yeah, I can do better than that:
Fixed it, boss.
Admin
Too much uncertainty in this algorithm… They should have: – initialized an array of all possible values – iterate on [0...Rows.Count[ to mark values that aren’t the count – iterate on ]Rows.Count...int.MaxValue] to also mark them as irrelevant – iterate over the whole array to search the remaining count candidates – check the count of the count candidates is 1 (avoid silly bugs on the comparison operator!) – return the one and only count, or FILE_NOT_FOUND if something bad happened
Admin
Let's also appreciate that not only are they doing unnecessary counting, they're also doing twice as much unnecessary counting. Both i and count are incremented, so at the end of the method, you have 3 variables with the count, with various degrees of off-by-oneness.
Admin
You joke, but that's actually what some companies did in the past. Tarriffs on light trucks were higher than on passenger vehicles in the US, so they would strip the vehicles, import them as passenger vehicles for the lower tarriffs, then reassemble them so that they would be classified as "locally manufactured."
https://en.wikipedia.org/wiki/Chicken_tax
Admin
Buy a car, then ~re~assemble the whole thing from scratch in your garage. Why? Because it's awesome fun.
FTFY.
https://www.dfkitcar.com/
Addendum 2020-12-14 10:54: Argh. TRWTF is that this comments board doesn't accept valid markdown.
Addendum 2020-12-14 10:55: Strikeout the "re". Just assemble.
Admin
This is right up there with the stupid way to return an element from an associative array:
Admin
That would be nice, wouldn't it? (Apart from leaving the off-by-one in place.)
And it would work! Except for the vast majority of cases, where the grid view (or at least its Count member function) is in a separate assembly. In which case there's no way for the compiler to figure out the necessary constraints.
I'm more and more inclined to believe that compiler optimisation is barking up the wrong tree. What we actually want, inspired from a WTF from last week, is a set of lint-like warnings:
We could even make these part of a flag enum. There's nothing to say that you can't be a total doofus, but "not even wrong" still applies.
Admin
zreoth++
Your comment has an off-by-one error. FTFY.
Admin
Add me to those who think there used to be some logic in the loop.
Admin
Even with the logic in the loop, you did not had to fill a 'count'-named variable. Maybe a variance of a 'current'-named variable. @Loren
Admin
Count me out of your group. If there had been logic, it would have crashed trying to access the array on the last iteration of the loop. The existence of the off-by-one error kills your theory!
Admin
Many database connections start with "1" instead of "0", so the <= is understandable, but then I should start at 1 instead of 0.
Admin
I suspect the off-by-one error is exactly the reason this function still exists. Someone tried to replace it with a simple count but it broke something further down the road so they just put it back how they found it.
Admin
...but there is a bonus: variables "i" ancd "count" that count exactly the same thing.
Admin
There are 2 hard things in computer science - caching, off by one errors, and just setting a value instead of using a pointless incrementing loop.
Admin
Coming in late, but I haven't seen anyone mention that (to the best of my knowledge) the ".Count" is calculated fresh each time it is accessed, which could conceivably result in a performance hit.
Admin
@dpm True.
if worried about that then
solves the problem of repeated object accesses and any hidden re-enumerations.
Admin
Yeah, we need that value stored in a fourth place.
Admin
Perhaps they heard about "deferred execution", and were afraid they wouldn't get the correct Count unless they looped over the whole thing (which this doesn't actually do anyway).
Admin
Since I am the submitter of the story I can safely say: No, there never was any logic in that loop.
Admin
Correct. The programmer (I use that term in a very loose sense here) often ran into "off-by one errors". His solution? Put a try-catch in the loop, but leave the catch empty. GENIUS!
Admin
That is merely what Tesla does in Europe if I understood some articles correctly
Admin
I will go one better
int count = 0; for(int i = 0; i <= dgView.Rows.Count; i++) { count++; } int requiredCount = count-1;
// I will assume without checking that this is valid in C#. while (--count > dgView.Rows.Count) { if (count==requiredCount) { break; } }
Admin
Admin
Not bad, but could use some improvements. How about
This would also make sure that we recieve the wrong count, but we don't know how wrong it will be, until we introduce a check in the calling method:
Admin
Actually yes. There is factory in NL. Where they receive engines and assembled rest of the cars. Place engine connect engine. Tadah made in Europe.
Admin
Bonus points for the infinite loop
Admin
Actually, the compiler DOES optimize this out pretty well:
https://godbolt.org/z/hcjcnY
Basically becomes a simple "return 0 if negative, else add one".
However, if we make the type a signed char... it gets REALLY insane:
https://godbolt.org/z/74nxsx
Admin
It's a feature. When the winter nights get especially cold, you can use your CPU as an additional heat source.
Admin
Aha, so the original point of the code was to protect against
Count
being negative?Admin
Nope, 0<1 so the loop would go one round and count would be 1
Admin
Also this will return the wrong amount of rows if you are using headertemplates in the grid, gridviews sees the header as a row as well.
Admin
I don't think up to 20 is sufficient at all.
int count = 0; for (int i = 0; i <= dgView.Rows.Count; i++) { count++; } int requiredCount = count-1;
Map<String,String> countValidationMap = new ConcurrentSkipListMap<String,String>(); while (Integer i = 0; i < Integer.MAX_VALUE; i ++) { countValidationMap.put(i.toString, i.toString()); }
int cachedCorrectCountValue = 0; while (count < Integer.MAX_VALUE) { for (Map.Entry<String,String> entry : countValidationMap.entrySet()) { if (count == Integer.parse(entry.getKey()) || count == Integer.parse(entry.getValue() && count == requiredCount) { // we found our result, but need to escape this while loop count = Integer.MAX_VALUE; } }
count = cachedCorrectCountValue;
I didn't test this yet, but I am certain it is both robust and scaleable
Addendum 2020-12-17 05:24: Dammit, how on earth do I format code here?
Not that I need to, the formatting above is more than good enough for this example, I feel.
Admin
You can use 3 backticks at the start and the end, or I think 4 spaces at the start of the code works too.
Example with three backticks:
And now I'll try the same with 4 spaces at the start of the code...
Admin
Re re-calculating each time through, you can't tell without seeing the source for Count, but the fact that it's a property would tend to imply that it's pre-computed and you get a value back each time. It could be re-calculated, but that would be bad manners on a property.
Admin
Ahahaha omigosh that's ... depressingly what's going on in lots of Enterprise code ... T.T