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

  • FoxDev

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


  • FoxDev

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


  • FoxDev

    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.


  • Discourse touched me in a no-no place

    @Medinoc said:

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

    Sometimes this forum's Discoformatting doesn't even work the same way as this forum's Discoformatting.



  • @RaceProUK said:

    does the same with string.IsNullOrWhitespace()

    That was only added in .NET 4.0 - hopefully this simply predates that...


  • area_deu

    It isn't.

    public class Program {
    	public static void Main() {
    		string s = "testme";
    		string s2 = s.ToString();
    	}
    }
    

    MSIL:

        IL_0000:  nop
        IL_0001:  ldstr      "testme"
        IL_0006:  stloc.0
        IL_0007:  ldloc.0
        IL_0008:  callvirt   instance string [mscorlib]System.Object::ToString()
        IL_000d:  stloc.1
        IL_000e:  ret
    

  • FoxDev

    That's... retarded.

    Maybe the runtime's JITter optimises it instead...



  • @RaceProUK said:

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

    Actually, amazingly enough, ADODB.Recordset isn't enumerable, as it's an interface from "classic" asp (yes, it's a built-in Interface without an "I" in front of the name).

    @atimson said:

    That was only added in .NET 4.0 - hopefully this simply predates that...

    Most of it is. But it's all pretty much .NET 2.0 and later, and that includes string.IsNullOrEmpty().


  • area_deu

    @Pharylon said:

    ADODB.Recordset isn't enumerable

    Exactly. It's basically SqlDataReader's granddad.

    it's an interface from "classic" asp
    Please provide a trigger warning before mentioning _that_.


  • @ChrisH said:

    It isn't.

    Huh. string is sealed, and I'm pretty sure string.ToString() is just return this;, so it should be able to figure that one out...



  • I do see your point, but I don't see it as abuse. "For all of the things, until you're on the last one, do nothing. To go to the next thing, last = last->next." To me this is structurally no different from the array form of "For all of the things, until you're on the last one, do nothing. To go to the next thing, index++."



  • @ChrisH said:

    It isn't.

    Huh. I thought it might be optimized away in Release mode, but it isn't there either:

    .method private hidebysig static void  Main(string[] args) cil managed
    {
      .entrypoint
      // Code size       12 (0xc)
      .maxstack  8
      IL_0000:  ldstr      "testme"
      IL_0005:  callvirt   instance string [mscorlib]System.Object::ToString()
      IL_000a:  pop
      IL_000b:  ret
    } // end of method Program::Main
    
    

  • area_deu

    @Maciejasjmj said:

    Huh. string is sealed, and I'm pretty sure string.ToString() is just return this;

    It is:

    // Returns this string.
    public override String ToString() {
        Contract.Ensures(Contract.Result<String>() != null);
        Contract.EndContractBlock();
        return this;
    }
    



  • FoxDev

    @ChrisH said:

    ``` C#
    // Returns this string.
    public override String ToString() {
    Contract.Ensures(Contract.Result<String>() != null);
    Contract.EndContractBlock();
    return this;
    }

    Ah, that contract stuff will be why it's not simply inlined.


  • @RaceProUK said:

    Ah, that contract stuff will be why it's not simply inlined.

    You can absolutely inline it with all that contract stuff. I think you mean "elided".


  • FoxDev

    Yes, that



  • Actually, you had the reason all along:

    @RaceProUK said:

    Aside from the null issue

    The contract stuff is the null check.


  • Notification Spam Recipient

    @ChrisH said:

    I see that ToString() anti-pattern quite often,

    if((""+ string).equalsIgnoreCase("null")){
    // do stuff
    }

    *edit I have no idea who or what i'm quoteing but I'm too lazy to change it or use spellcheck!


  • Discourse touched me in a no-no place

    @Pharylon said:

    //Actual logic is here. Why not just reverse the if statement???

    My company's web application is full of that. I fix it when I notice it.

    I also broke people's brains with DeMorgan's theorem:

    if myrecordset.eof and myrecordset.bof then
    else
       ' logic goes here
    end if
    
    if not myrecordset.eof or not myrecordset.bof then
        ' logic goes here now, bitches!
    end if
    

  • Discourse touched me in a no-no place

    @Pharylon said:

    //Anyway, for some reason, we're going to start most of our variables with 'l',

    My guess? l is for local variable. My company does that too, except usually they use w_ for no reason anyone could ever explain, and s_ for shared variables, which are kind-of like globals, except they're not actually global.



  • @NedFodder said:

    The contract stuff is the null check.

    ...which is still pointless if the method is called via callvirt, for obvious reasons.



  • I was about to point that out, but it's still dumb. Yours is worse, though.

    Our company just uses the Resharper _thing-style for members, which I still think is silly, but whatever. I prefer just using a different color, and not having enough members and locals to get them confused easily.



  • @Pharylon said:

    XmlDocument

    What's amazing is that whoever wrote this is obviously aware of the XmlDocument class, and yet somehow still does string concat to generate XML. It's mind-blowing. Even when he knows about all the correct tools to use, he's not using them.

    I know you anonymized this, but I hope lGotOne is in class-scope (instead of you just accidentally removing the declaration), because that'd really add to the WTF factor here.



  • @Maciejasjmj said:

    obvious

    Not to me, I don't know jack about IL.

    Does the compiler have enough information to elide the contract stuff inside the ToString method?



  • Well, virtual call guarantees that this won't be null - otherwise you eat a NullReferenceException before you get a chance to start executing.



  • @Onyx said:

    '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);
    ```</blockquote>
    
    Unless your custom implementation of a linked list already had a pointer to the last element somewhere, that *is* the best way to get the address of the last element.
    
    The real questions are: why do you need the last element? how often do you need it, and would it be worth caching the pointer? would you be better off storing the list in reverse order, or in a different type of data structure?
    
    edit: oh, I see you've posted that, for syntactic raisins, you prefer the `while` version. They're equally good; they accomplish exactly the same thing.


  • @RaceProUK said:

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

    Except it can't be, because, as you yourself mentioned, optimizing it away would change the behavior. (Remove the possible null exception.) So no, using .ToString() on a string can't and won't be optimized away. C# ain't that broken that optimization would change behavior.

    The new stuff coming down the pipeline, with non-null reference types, could potentially lead to a situation where a non-null String can optimize-away a .ToString() call on it. But that ain't production ready yet.





  • @RaceProUK said:

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

    aString.ToString() should return "The programmer is an idiot."

    Assuming aString is a String; if it's an Object that happens to be a String, calling ToString() on it isn't idiotic, of course. And ` doesn't change the background color inside a <small> block; yeah, discoursistency!


  • Discourse touched me in a no-no place

    @ChrisH said:

    public class Program {
    public static void Main() {
    string s = "testme";
    string s2 = s.ToString();
    }
    }

    Interestingly(?), if you turn on optimizations, you get this slightly improved version:

      IL_0000:  ldstr      "testme"
      IL_0005:  callvirt   instance string [mscorlib]System.Object::ToString()
      IL_000a:  pop
      IL_000b:  ret
    

    Oh boy, I skipped some stack accesses!

    also :hanzo:


  • area_pol

    @Pharylon said:

    //Blank! The if is blank!!!!

    At Samsung somebody is forcing mandatory "else" with all "ifs", so a lot of code look like this:

    if (somethig)
    {
        // logic
    }
    else
    {
    }
    

    That way you never know if someone forgot to add something, or accidentally removed, or is a placeholder of some sorts, or whatever.


  • Discourse touched me in a no-no place

    @NeighborhoodButcher said:

    At Samsung

    I found the problem.


Log in to reply