• King (unregistered)

    So, where is the FileNotFound return?

  • switch(poster) (unregistered)

    I can see how you get to most of this, but switch on the same variable (I'm assuming a lot of anonymisation happened here) inside a default doesn't make sense even if you are adding the incremental feature.

  • Ulli (unregistered)

    I think the author wanted to teach the compiler how it should deal with a switch statement to produce optimized code: Test against 10, build a look-up-table for cases 21..29 and finally deal with 49+50

  • (nodebb) in reply to switch(poster)

    The version control history would be ... illuminating, I think.

  • RLB (unregistered)

    The only way I can explain this is that, once upon a time (probably back when computers ran on wooden electrons), each or some of those cases did something else as well - logging, for instance - and over time, the "something else" became unnecessary and was eventually cut. Probably in stages. And at each point, the programmer of the day thought "well, I could simplify this, but what if we later want to do something else again?".

  • Simon Clarkstone (unregistered)

    Perhaps several programmers looked at it and decided they were too hung over to be sure what the existing logic did to simplify it, and just kept adding to it. (And I guess it was not suitable for unit-testing as that would be great for nailing down which way round all the trues and falses are before refactoring.)

  • (nodebb)

    I have a lot of such code at work. Thinks like functions expecting to be called as

    CALL getTypeAndIndex(counter, type, index) CALL doThing(type, index, ...)

    or sometimes

    type = getType(index) CALL doThing(type, index, ...)

    I want to change it, but I cannot, because we don't have tests -- and the code structure makes manually testing an effort of several weeks without even being able to guarantee completeness.

    Addendum 2020-10-05 08:58: I really need documentation on formatting commands in the comments...

    Addendum 2020-10-05 09:00: There also needs to be a way to delete comments, in case a comment accidentally is too easily personally identifiable by colleagues. (Not applicable here, but the "Addendum" mechanism surprised me.)

    Addendum 2020-10-05 09:13: Another anti-pattern, I can't refactor: A large if-else-switch tree, where for all cases I have reproduced all it does is select a task, and for this task call "init", "execute" and "cleanup" routines.

  • Naomi (unregistered)

    I really need documentation on formatting commands in the comments...

    Pst, it's just Markdown (but, yes, it would be great to have that actually documented somewhere near the comments box - along with maybe a preview feature, Remy!)

  • Scott (unregistered)

    How come nobody's bitching about the hard-coded values?

  • (nodebb) 
    switch (cigarette_brand)
{
case 'Tareyton':
return FIGHT();
default:
return cigarett_brand = 'Tareyton';
}
  • (nodebb) in reply to RLB

    The only way I can explain this is that, once upon a time (probably back when computers ran on wooden electrons), each or some of those cases did something else as well - logging, for instance - and over time, the "something else" became unnecessary and was eventually cut. Probably in stages. And at each point, the programmer of the day thought "well, I could simplify this, but what if we later want to do something else again?".

    That ^^^ happens a lot more than I'd like to admit, but usually the something happens again or becomes a different something condition. What annoys me is when I see this type of useless function, which is totally inexcusable:

     function dataValidate(data) {
      if data != null { return valid; }
      else { return valid; }
      }
  • giammin (unregistered)

    I know is less performant but I would replace it with:

    => new HashSet<int>{10, 21, 24, ...}.Contains(effectID);

    looking at that switch hurts

  • Foo AKA Fooo (unregistered) in reply to giammin

    Wait, you'd create a new HashSet every time this check is done? I guess "less performant" is one way to put it.

    If you used a static (or whatever that's called in C-octothorpe) one, alright. But otherwise, all I can say is, kids these days ...

  • (nodebb)

    Did nobody notice this aspect?

        if (effectID != 10)
	{
                ...
	}
	return true;

    The return is indented like it belongs to the if, but it doesn't.

  • Old Man Croner (unregistered)

    array.Contains, and read active effects from env.

  • dan (unregistered) in reply to switch(poster)

    It probably started out as two flags, and at some point effectIDex was merged into effectID.

  • (nodebb)

    The biggest flaw is using integer effect IDs instead of enums (or constants as a stopgap half-measure).

  • 516052 (unregistered) in reply to nyanpasu64

    No. The biggest flaw in this code is that it has so many flaws we can actually argue over which is the biggest flaw.

  • (nodebb) in reply to Old Man Croner

    Something like this

    return (new string[]{"10", "21", "24", "27", "28", "29", "49", "50"}).Contains(effectId);
  • WTFGuy (unregistered)

    @giammin ref

    I know is less performant but I would replace it with:

    => new HashSet<int>{10, 21, 24, ...}.Contains(effectID);

    looking at that switch hurts

    Confusing fixed hard-coded control flow with data hurts a lot worse. Talk about separations of concern.

    As Old Man Croner almost says, if the effect list is configurable then it'll have to be encoded in some data structure to be queried dynamically at decision time. But if not you're just producing overterse conceptual mud.

    Back to the original code ...

    As Scot said, obviously this whole thing is one giant ball o' magic numbers. Every one of those effectIDs ought to be a descriptively named constant or enum value or similar. Somehow it's mentally easier to create garbage spaghetti (or garbage nesting mixed if/switch constructs) when the whole statement tree has a "computer gobbledygook" feel to it. Convert half of that to comprehensible sorta-English and the shitty structure jump out of the screen and screams "Fix Me!"

  • ADBjester (unregistered)

    TRWTF is that he modified the source of a third party library without establishing his own fork. The next time some poor developer updates the NuGet, it's just going to revert....

  • doubtingposter (unregistered) in reply to switch(poster)

    It doesn't make sense incrementally, but it DOES make sense if you include previous refactoring. I suspect the sub-switch used to be a separate function call that got inlined.

  • (nodebb)

    In C# 9.0 this could be:

    return effectID is 10 or 21 or 24 or 27 or 28 or 29 or 49 or 50;
  • Some Ed (unregistered) in reply to switch(poster)

    I've seen that happen, actually. For the case I saw, there was a change in the middle where there was another variable to reference, and so the second switch made sense. Then someone noted that the codes for the second variable were all distinct from the codes for the first variable, and all occurred for a single value of the second variable.

    So the variables were merged. The person who did the code change for the merge just adjusted the attribute name for the inner switch statement. I did the review and told him to fix it the right way. He told the boss, and got another person to review who was more comfortable with giving a rubber stamp.

Leave a comment on “Switched Requirements”

Log In or post as a guest

Replying to comment #:

Log Out

« Return to Article