Giles's company has a hard time with doing things in the database.

In today's example, they attempt the very challenging task of generating unique IDs in a SQL Server database. Now, what you're about to see follows the basic pattern of "generate a random number and see if it's already been used", which is a fairly common anti-pattern, but it's managed to do this in some of the worst ways I've ever seen. And it can't even hide behind the defense of being written a long time ago- it's a handful of years old.

Comments to this C# code have been added by Giles, and no, there were no comments.

protected void AddBlankRowToDatabase()
{
    //This - in effect - calls a stored procedure which has been carefully designed to work around the issue with SPs and
    //SQL injection - i.e. in theory using a stored procedure could help prevent it from happening, which is obviously
    //a problem, so this SP allows it once again. 
    //also note that this actually fetches *all* machines for the currently selected customer,
    //so is basically calling "select * from ..." and creating a list of objects to represent them.        
    List<Machine> allMachine = MachineManager.GetByCriteria("CustomerId=" + Convert.ToString(Session["ActiveCustomerId"]), "");        
    
    //having gone to all that trouble, let us see how many machines there are, and increment this since we are going to 
    //add one
    int machineCount = allMachine.Count + 1;
    
    //Create a new object representing a machine.
    Machine machine = new Machine();
    
    //we will definitely want random numbers, as that is the correct way to ensure uniqueness.
    Random _rng = new Random();
    
    //new numbers must start with 999 for no reason that is documented or known to anyone.
    string paddedString, padding = "999";
    
    //OK, random number please.  We want it to be 3 chars, but instead of all that zero-padding nonsense, let's just
    //ensure this by starting from 100.
    int RandomId = _rng.Next(100, 999);
    
    //append our random number to our "999"
    paddedString = padding + RandomId;
    RandomId = Convert.ToInt32(paddedString);
    
    //now here's the big brain part;  we don't know if the machine ID already exists, so we check if it does!        
    bool machineIdNotExists = true;
    while (machineIdNotExists)
    {
        //Get any machine with that ID, again via this garbage SP system.
        List<Machine> machineExists = MachineManager.GetByCriteria("MachineId=" + RandomId, "");
                    
        if (machineExists.Count > 0)
        {
            //ah well, let's try *another* random 'number'
            RandomId = _rng.Next(100, 999);
            paddedString = padding + RandomId;
            RandomId = Convert.ToInt32(paddedString);
            machineIdNotExists = true;
        }
        else
        {
            //ok, so the machine does not not Exist, we have a new number!
            machineIdNotExists = false;
        }
    }
    
    //Good stuff, ensure we use the ID we found
    machine.MachineId = RandomId;
    
    //let's use the count of machines we arrived at earlier to set a temporary title for the machine....
    //yes, this is the *only* use of the count, which we worked out by selecting an entire list
    //of DB rows, constructing a bunch of objects and then *counting* them.
    machine.Title = "New Machine " + machineCount;
    
    ////////
    //Snip a bunch of other boring property setting
    /////////

    //Now we save the created machine.
    //This internally takes the Machine Object and  (manually - no EF or similar)
    //pulls its properties into params for another stored proc, which adds the entry to the DB.
    MachineManager.InsertMachine(machine);
    
    //Now run through the entire grid of all machines, and save them all individually.
    //Even though you haven't amended them.
    SaveAllRecords();  

    //Reload the page, which will refresh the grid to show our new machine!
    Response.Redirect("MachinesAdmin.aspx?RecordSaved=Yes");
}

SQL Injection by stored procedure, fetching entire sets from the database when you need a count. Forcing entire front-end page refreshes via redirect. Mysterious and very not random padding.

Every choice made here was a bad choice.

[Advertisement] Utilize BuildMaster to release your software with confidence, at the pace your business demands. Download today!