Count the WTFs: SQL Query



  • I think this simple block of code explains quite a lot into what is wrong with the special type of cargo-cult programming they use here:

    public void GetProducts()
    {
      StringBuilder strStr = new StringBuilder();
      strStr.Append(@"select *");
      strStr.Append(@" from Product, Producer");
      strStr.Append(@" where Product.ProducerId = Producer.ProducerId");
    //  strStr.Append(@" and Product.IsVisible = 1");
    
      using (SqlConnection con = GetSqlConnection())
      {
        using (SqlCommand com = new SqlCommand(con, strStr.ToString()))
        {
          con.Open();
    
          SqlDataReader cor = com.ExecuteDataReader();
          List<Product> r = new List<Product>();
          while (cor.Read())
          {
            r.Add(GetObject<Product>(cor));
            Products = r;
          }
        }
      }
    }
    

    Some anonymization was used, but local variable names have not been changed.



  • Meh. Not as bad as it could be.

    • StringBuilder for concatenating the same constant strings, instead of, you know, just using one BIG string
    • Non-generic List (but then this could be .NET 1.0)
    • Setting Products on every iteration of the loop, instead of at the end ("realtime" updates?)
    • Non-ANSI join syntax


  • @ekolis said:

    • StringBuilder for concatenating the same constant strings, instead of, you know, just using one BIG string
    • Non-generic List (but then this could be .NET 1.0)
    • Setting Products on every iteration of the loop, instead of at the end ("realtime" updates?)
    • Non-ANSI join syntax
    • Local variables "com", "con", and "cor", which look almost identical, to make sure you're really paying attention when reading this code
    • Selecting all rows from a table - I bet this will be followed by some filtering code that should have been done in the WHERE clause

  • Discourse touched me in a no-no place

    @random999 said:

    @ekolis said:

    • StringBuilder for concatenating the same constant strings, instead of, you know, just using one BIG string
    • Non-generic List (but then this could be .NET 1.0)
    • Setting Products on every iteration of the loop, instead of at the end ("realtime" updates?)
    • Non-ANSI join syntax
    • Local variables "com", "con", and "cor", which look almost identical, to make sure you're really paying attention when reading this code
    • Selecting all rows from a table - I bet this will be followed by some filtering code that should have been done in the WHERE clause
    Method called “GetProducts” that returns void. It's a public method too, so all the callers have to know about the convention of calling this and then actually retrieving the product list by other means. That's where the code smell really starts…


  • @dkf said:

    @random999 said:
    @ekolis said:

    • StringBuilder for concatenating the same constant strings, instead of, you know, just using one BIG string
    • Non-generic List (but then this could be .NET 1.0)
    • Setting Products on every iteration of the loop, instead of at the end ("realtime" updates?)
    • Non-ANSI join syntax
    • Local variables "com", "con", and "cor", which look almost identical, to make sure you're really paying attention when reading this code
    • Selecting all rows from a table - I bet this will be followed by some filtering code that should have been done in the WHERE clause
    Method called “GetProducts” that returns void. It's a public method too, so all the callers have to know about the convention of calling this and then actually retrieving the product list by other means. That's where the code smell really starts…
    Yeah, I was wondering about this. The WTF would be bigger if Products was a static non-syncronized variable.


  • Let's see...

    1) Using a StringBuilder + raw ADO.NET instead of an ORM or even some kind of data mapper (possibly excusable depending on how old the code is, but done poorly)

    2) Use of SELECT *

    3) Joining tables on the WHERE clause instead of using a join (possibly excusable, but not likely)

    4) No generics on the List

    5) Bad variable names (con/com are alright, but cor for a reader?  r for  list?)

    6) A void method that (presumably) populates a property on a class?  It should return an IList<Product> or similar (probably an ICollection if it is read-only)

    7) Product variable gets assigned on every iteration of the loop, which means if there aren't any products (unlikely since it's a blind select *, but you never know) the variable never gets set and could be a NullReference or other exception when it's accessed.

    8) Not filtering only visible products (line is commented out); possibly acceptable depending on usage.



  • @ekolis said:

    • Non-generic List (but then this could be .NET 1.0)

    That bit was my fault - I forgot to encode the HTML entities :/

    @ubersoldat said:

    Yeah, I was wondering about this. The WTF would be bigger if Products was a static non-syncronized variable.

    You're getting the spirit of things here. It is. And this method is called on its own thread, using Parallel.Invoke, and nobody ever checks if it finished. This is done while the splash screen is visible though, and probably finishes before the splash screen's timer, so we don't get any crashes - yet.

    You might think it's okay because Parallel.Invoke waits for all the methods to return. You'd be sadly mistaken. The code looks something like this (though I'm writing it from memory at the moment):

    new Thread(new Action(() => Parallel.Invoke(new Action[]] { new Action(GetProducts), new Action(...), ... }))).Start();
    

    @ObiWayneKenobi said:

    5) Bad variable names (con/com are alright, but cor for a reader?  r for  list?)

    cor = command result, r = result (from the current function). I think. Though some of the time I think they were picked to maximize confusion.

    @ObiWayneKenobi said:

    7) Product variable gets assigned on every iteration of the loop, which means if there aren't any products (unlikely since it's a blind select *, but you never know) the variable never gets set and could be a NullReference or other exception when it's accessed.


    If there aren't any products a crash is fine because the program is entirely unusable. But if, for example, the connection would take too long (i.e. more than about a second) due to some sort of network congestion... this would be one of the hundreds of failures that would be caused.

    @ObiWayneKenobi said:

    8) Not filtering only visible products (line is commented out); possibly acceptable depending on usage.

    It was commented out so that it could be filtered by code every time any list is populated. I suspect the reason it was commented out is that users from older versions may have these products already selected. So they are always loaded, even though they're highly unlikely to actually be needed.



  • @configurator said:

    And this method is called on its own thread, using Parallel.Invoke, and nobody ever checks if it finished. This is done while the splash screen is visible though, and probably finishes before the splash screen's timer, so we don't get any crashes - yet.

    Oh hey look, another person from Bridget99's team.


Log in to reply