• Steve (unregistered)

    first++

  • Prime Mover (unregistered)

    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.

  • Bogolese (unregistered)

    Fizz buzz kill

  • Carsten (unregistered)

    Actually the code is so obviously nonsense that I wouldn't be surprised if the compiler optimizes it away

  • NoOne (unregistered)

    Actually, using the lax comparison removes the off-by-one: If dgView.Rows.Count == 1, using the strict operator would result incount == 0

  • NoOne (unregistered)

    Nah, nevermind what I said... Not enough coffee yet...

  • (nodebb)

    Buy a car, then reassemble the whole thing from scratch in your garage. Why? Because.

  • WTFGuy (unregistered)

    @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.

  • Latengorica (unregistered)

    This is easily corrected by subtracting 1 from 'count' after the loop.

    And easily made twice as correct by subsequently replacing '<=' with '<'.

  • Stephen (unregistered)

    And you just know that somewhere in the code base something now relies on the fact that this counts it wrong.

  • Prime Mover (unregistered) in reply to Latengorica

    Oh right yeah, I can do better than that:

    int count = 0;
for(int i = 0; i <= dgView.Rows.Count; i++)
{
    count++;
}
while (count > dgView.Rows.Count) {
    count--;
}

    Fixed it, boss.

  • Shiwa (unregistered)

    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

  • fanguad (unregistered)

    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.

  • Anonymous') OR 1=1; DROP TABLE wtf; -- (unregistered) in reply to Mr. TA

    Buy a car, then reassemble the whole thing from scratch in your garage. Why? Because.

    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

  • Brian Boorman (google) in reply to Mr. TA

    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.

  • (nodebb)

    This is right up there with the stupid way to return an element from an associative array:

    foreach ($array as $key => $value) {
  if ($key == $wanted) {
    $found = $value;
    break;
  }
}
  • Sole Purpose Of Visit (unregistered) in reply to Carsten

    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:

    1. Illegal.
    2. Wrong.
    3. Silly.
    4. You have transgressed the unwritten laws of our coding standards.
    5. Not even wrong.
    6. You are a total doofus.
    7. Disgusting.

    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.

  • The Incredible Holk (unregistered) in reply to Steve

    zreoth++

    Your comment has an off-by-one error. FTFY.

  • (nodebb)

    Add me to those who think there used to be some logic in the loop.

  • (nodebb)

    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

  • (nodebb) in reply to Loren Pechtel

    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!

  • Laie Techie (unregistered)

    Many database connections start with "1" instead of "0", so the <= is understandable, but then I should start at 1 instead of 0.

  • Tim (unregistered) in reply to Stephen

    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.

  • A guest (unregistered)

    ...but there is a bonus: variables "i" ancd "count" that count exactly the same thing.

  • Robin (unregistered)

    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.

  • dpm (unregistered)

    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.

  • WTFGuy (unregistered)

    @dpm True.

    if worried about that then

    int rowCount = dgView.Rows.Count;

    solves the problem of repeated object accesses and any hidden re-enumerations.

  • dpm (unregistered) in reply to WTFGuy

    int rowCount = dgView.Rows.Count;

    Yeah, we need that value stored in a fourth place.

  • Chris (unregistered)

    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).

  • 🤷 (unregistered) in reply to Loren Pechtel

    Since I am the submitter of the story I can safely say: No, there never was any logic in that loop.

  • 🤷 (unregistered) in reply to Auction_God

    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!

    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!

  • Hasseman (unregistered) in reply to Anonymous') OR 1=1; DROP TABLE wtf; --

    That is merely what Tesla does in Europe if I understood some articles correctly

  • Osric (unregistered) in reply to Prime Mover

    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; } }

  • Prime Mover (unregistered) 
    int count = 0;
for (int i = 0; i <= dgView.Rows.Count; i++)
{
    count++; 
}
int requiredCount = count-1;

count = 0;

if (count == requiredCount)
{
    return  count;
}
if (count + 1 == requiredCount)
{
    return count + 1;
}
if (count + 2 == requiredCount)
{
    return count + 2;
}
etc. 
:
:

// 20 should be enough for any requirements. If necessary add more tests to the end if you
// really feel you need to cater for ridiculously large tables.
if (count + 20 == requiredCount)
{
    return count + 20;
}
  • 🤷 (unregistered) in reply to Prime Mover

    Not bad, but could use some improvements. How about

    while(count<count+1)){
    if(count+1 == requiredCount)
        return count+1;
}

    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:

    int rowCount = SetRowCount();
if(rowCount != dgView.Rows.Count)
    rowCount = dgView.Rows.Count;
  • Dlareg (unregistered) in reply to Hasseman

    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.

  • Chris (unregistered) in reply to 🤷

    Bonus points for the infinite loop

  • Me (unregistered)

    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

  • 🤷 (unregistered) in reply to Chris

    It's a feature. When the winter nights get especially cold, you can use your CPU as an additional heat source.

  • (nodebb) in reply to Me

    Aha, so the original point of the code was to protect against Count being negative?

  • Rope (unregistered) in reply to NoOne

    Nope, 0<1 so the loop would go one round and count would be 1

  • Blurp (unregistered)

    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.

  • RussellWakely (google) in reply to Prime Mover

    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.

  • 🤷 (unregistered) in reply to RussellWakely

    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:

    if(true==false)
    cout >> "This should never happen" >> endl;
else
    cout >> "This should always happen" >> endl;

    And now I'll try the same with 4 spaces at the start of the code...

    if(true==false)
    cout >> "This should never happen" >> endl;
else
    cout >> "This should always happen" >> endl;
  • Craig (unregistered) in reply to dpm

    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.

  • pepoluan (unregistered) in reply to Prime Mover

    Ahahaha omigosh that's ... depressingly what's going on in lots of Enterprise code ... T.T

Leave a comment on “Count on Me”

Log In or post as a guest

Replying to comment #:

Log Out

« Return to Article