Some Anti-Patterns At My Job



  • A few of my favorite anti-patterns I've seen at my job. Note that this is an (abbreviated) real function, and not even especially cherry-picked.

         
        //We're starting off with a bang! PascalCase arguments (C# convention is camelCase for arguments) and
        //RefNumber is misspelled.
        public static XmlDocument GetTrackingXMLWidget(string BookNumber, string RefNumbert)
        {
            //Now, for some reason we don't want to use the BookNumber string directly, so let's create a
            //new variable called tmpBookNumber, and put it in. Oh, and let's call ToString() on a string, just
            //just in case. We'll do this a lot. Always call ToString() on strings when assigning them to other
            //other strings.
            string tmpBookNumber = bookNumber.ToString().Trim();
            //Better call ToString() on it again before we check the length!
            if (tempBookNumber.ToString().Length == 0)
            {
                 XML += "<CombinedRates>";
                 XML += "<BestRates>";
                 XML += "<BestRate>";
                 XML += "<Errors>";    
                //Write some "XML" using fifty lines of string concatenation.
            }
         
            var myobject = new Vb6DllObject();
            myadorecordset = new ADODB.Recordset();
            myadorecordset = myobject.QuickBookingList(ref tmpBookNumber, ref tmpRefNumber, ref connectstring);
            if (myadorecordset == null || (myadorecordset.EOF && myadorecordset.BOF))
            {
                //Blank! The if is blank!!!!
            }
            else
            {
                //Actual logic is here. Why not just reverse the if statement???
                //Anyway, for some reason, we're going to start most of our variables with 'l', like it was Hungarian
                //Notation, except they always start with 'l'. Always. Longs? 'l'. But bools, string, and objects do too.
                lGotOne = true;
                //OK, these next two lines seem redundant, but maybe it's required by the ancient VB 6 dll it's calling,
                //so maybe I'll give him a pass on this one. On the other hand, he also wrote the original VB6 so.... anyway.
                myadorecordset.MoveLast();
                myadorecordset.MoveFirst();
                for (; !myadorecordset.EOF; myadorecordset.MoveNext())
                {
                    //I don't even... Amazingly, this is technically a valid for loop construct, but holy hell where did
                    //he come up with this? A while loop would make a lot more sense.
                    XML += "<Items>"
                    XML += myadorecorset.Fiels[0].ToSTring()
                    //a hundred more lines of string concatenation to continue building XML snipped.
                }
            }
        }
    

  • SockDev

    *smacks your cow-orker round the head with System.Xml.Linq.XDocument*



  • @Pharylon said:

    I don't even... Amazingly, this is technically a valid for loop construct, but holy hell where did he come up with this? A while loop would make a lot more sense.

    Actually, I only use while loops for situations where there may never be an end. A for loop makes sense if you know the data is limited. It's only a personal distinction, though.



  • Slightly off-topic:

    You can get syntax highlighting in code blocks if you delimit the code with opening and closing triple backticks (```) and put the language name directly after the opening triple backtick.

    Like this (view raw):

    var a = 1;
    foreach (var foo in bar) { }
    


  • @Pharylon said:

    Why not just reverse the if statement???

    I ask every job candidate to demonstrate De Morgan's laws. I don't care if they don't recognize the name, but if they can't reverse an if statement they don't get the job.



  • This post is deleted!


  • @Choonster said:

    You can get syntax highlighting in code blocks if you delimit the code with opening and closing triple backticks (```) and put the language name directly after the opening triple backtick.

    Thanks.


  • BINNED

    's OK, this is roughly what I remember being taught as a good way to get the address of the last element in a linked list (custom implementation, not a standard library) in C:

    for(last = list_begin; last->next != NULL; last = last->next);
    

    Filed under: INB4 I frelled the code up



  • @NedFodder said:

    I ask every job candidate to demonstrate De Morgan's laws. I don't care if they don't recognize the name, but if they can't reverse an if statement they don't get the job.

    You mean... put an exclamation mark in front of it?



  • You did not frell the code up, and it is perfectly cromulent.

    I would also (in the OP) move the myadorecordset.MoveFirst() to be in the first for subexpression, and consider that loop to be perfectly cromulent as well. This is what for was designed for, after all.



  • @Pharylon said:

    Anyway, for some reason, we're going to start most of our variables with 'l', like it was Hungarian
    //Notation, except they always start with 'l'. Always.

    It's one of the worst Hungarian abominations - a pre-prefix denoting scope. l = local, g = global and m = module.


  • BINNED

    @TwelveBaud said:

    You did not frell the code up, and it is perfectly cromulent.

    Yeah, well, it works, sure, but it's damned confusing to read, at least to me.



  • It doesn't help that the loop body does not exist, but that's been the textbook example of how to do foreach in a non-foreach language for the past 56 years.



  • @Jaime said:

    It's one of the worst Hungarian abominations - a pre-prefix denoting scope. l = local, g = global and m = module.

    Ahhhh... that makes sense! I mean, kind of.



  • @Pharylon said:

    You mean... put an exclamation mark in front of it?

    Yeah, then we ask them how they would get rid of the exclamation mark without changing the logic. Sorry, should've been more specific.


  • SockDev

    @RaceProUK said:

    *smacks your cow-orker round the head with System.Xml.Linq.XDocument

    *does the same with string.IsNullOrWhitespace()

    *and again with foreach, given it's designed for IEnumberable*


  • area_deu

    I see that ToString() anti-pattern quite often, as well. Without first checking for null strings, of course.

    Also:

    DateTime d = DateTime.Parse(datarow["some_datetime_column"].ToString());
    

    because casting is complicated and expensive. Or something.


  • BINNED

    @TwelveBaud said:

    It doesn't help that the loop body does not exist

    Right. Also, it's of unknown length, which just screams "put it in a while loop!" to me. Sure it's slightly more code but:

    last = list_begin;
    while(last->next != NULL)
        last = last->next;
    

    Is much more immediately understandable to me. The for variant makes me stop and examine it for far longer then necessary. Again, it might just be me, but construct abuse makes me flinch.


  • SockDev

    Aside from the null issue, aString.ToString() is likely to be optimised away as a no-op anyway



  • So this forum's discoformatting doesn't work the same way as StackOverflow's discoformatting? Discawesome.


Log in to reply
 

Looks like your connection to What the Daily WTF? was lost, please wait while we try to reconnect.