Dilbert





The Daily WTF

Error'd: Identification Without Authentication;

Mark M. wrote, "While I was reading the Feb 6th DailyWTF, Feedly chimed in with this helpful comment that really put it in context."

 

"I was looking for a wireless keyboard on Amazon but I forgot to mention that I wanted one that was hairless too," Jean-Pierre writes.

 

"Oh, yes, please! I'd love to let my phone make phone calls!" writes Jura K.

 

Oliver X. writes, "Technically, it's equivalent to UTC+1, right?"

 

"Shopping on Wayfair in preparation for our upcoming move when I discovered this untold treasure! ...or, should I say, undefined treasure?" Sarah S. writes.

 

"Thanks so much for the 'timely' alert, Ebay. I'll be sure to keep a close eye on this one," David wrote.

 

[Advertisement] Continuously monitor your servers for configuration changes, and report when there's configuration drift. Get started with Otter today!


CodeSOD: It's For DIVision;

We’ve discussed the evil of the for-case pattern in the past, but Russell F offers up a finding which is an entirely new riff on this terrible, terrible idea.

We’re going to do this is chunks, because it’s a lot of code.

protected void setDivColor()
{
    List<HtmlControl> Divs = new List<HtmlControl>();
    HtmlControl divTire1Det = divShopCart.FindControl("divTire1Det") as HtmlControl;
    if (divTire1Det.Visible)
    {
        Divs.Add(divTire1Det);
    }

    HtmlControl divTire2Det = divShopCart.FindControl("divTire2Det") as HtmlControl;
    if (divTire2Det.Visible)
    {
        Divs.Add(divTire2Det);
    }
    …

So, this method starts with a block of C# code which tries to find different elements in the HTML DOM, and if they’re currently visible, adds them to a list. How many elements is it finding? If I counted correctly (I didn’t count correctly), I see 13 total. Many of them are guarded by additional if statements:

    if (!bMAlignAdded)
    {
        HtmlControl divManufAlign_Add = divShopCart.FindControl("divManufAlign_Add") as HtmlControl;
        if (divManufAlign_Add.Visible)
        {
            Divs.Add(divManufAlign_Add);
        }
    }
    if (!bRoadHazardAdded)
    {
        HtmlControl divRoadHazard_Add = divShopCart.FindControl("divRoadHazard_Add") as HtmlControl;
        if (divRoadHazard_Add.Visible)
        {
            Divs.Add(divRoadHazard_Add);
        }
    }

That’s the first 80 lines of this method- variations on this pattern. And at the end of it, we have this shiny list of divs. The method is called setDivColor, so you know what we’ve got to do: change the UI presentation by flipping some CSS classes around in a for loop, but also, in a terrible way.

    int j = 0;
    foreach (HtmlControl dDiv in Divs)
    {
        if (dDiv.ClientID.Contains("divTire1Det"))
        {
            Divs[j].Attributes["class"] = (j % 2 == 0 ? "GreyBackground" : "WhiteBackground");
        }
        else if (dDiv.ClientID.Contains("divTire2Det"))
        {
            Divs[j].Attributes["class"] = (j % 2 == 0 ? "GreyBackground" : "WhiteBackground");
        }

        else if (dDiv.ClientID.Contains("divDownloadRebate"))
        {
            Divs[j].Attributes["class"] = (j % 2 == 0 ? "GreyBackground" : "WhiteBackground");
        }
        else if (dDiv.ClientID.Contains("divTire1WheelBal"))
        {
            Divs[j].Attributes["class"] = (j % 2 == 0 ? "GreyBackground" : "WhiteBackground");
        }
        else if (dDiv.ClientID.Contains("divTire2WheelBal"))
        {
            Divs[j].Attributes["class"] = (j % 2 == 0 ? "GreyBackground" : "WhiteBackground");
        }
        else if (dDiv.ClientID.Contains("divStudding"))
        {
            Divs[j].Attributes["class"] = (j % 2 == 0 ? "GreyBackground" : "WhiteBackground");
        }
        else if (dDiv.ClientID.Contains("divFWheelAlign"))
        {
            Divs[j].Attributes["class"] = (j % 2 == 0 ? "GreyBackground" : "WhiteBackground");
        }
        else if (dDiv.ClientID.Contains("divTireDisp"))
        {
            Divs[j].Attributes["class"] = (j % 2 == 0 ? "GreyBackground" : "WhiteBackground");
        }
        else if (dDiv.ClientID.Contains("divTireFee"))
        {
            Divs[j].Attributes["class"] = (j % 2 == 0 ? "GreyBackground" : "WhiteBackground");
        }
        else if (dDiv.ClientID.Contains("divSalesTax"))
        {
            Divs[j].Attributes["class"] = (j % 2 == 0 ? "GreyBackground" : "WhiteBackground");
        }
        else if (dDiv.ClientID.Contains("divManufAlign_Remove") && bMAlignAdded)
        {
            Divs[j].Attributes["class"] = (j % 2 == 0 ? "GreyBackground" : "WhiteBackground");
        }
        else if (dDiv.ClientID.Contains("divRoadHazard_Remove") && bRoadHazardAdded)
        {
            Divs[j].Attributes["class"] = (j % 2 == 0 ? "GreyBackground" : "WhiteBackground");
        }
        else if (dDiv.ClientID.Contains("divManufAlign_Add") && !bMAlignAdded)
        {
            Divs[j].Attributes["class"] = (j % 2 == 0 ? "GreyBackground" : "WhiteBackground");
        }
        else if (dDiv.ClientID.Contains("divRoadHazard_Add") && !bRoadHazardAdded)
        {
            Divs[j].Attributes["class"] = (j % 2 == 0 ? "GreyBackground" : "WhiteBackground");
        }
        else if (dDiv.ClientID.Contains("divDiscount") && !bRoadHazardAdded)
        {
            Divs[j].Attributes["class"] = (j % 2 == 0 ? "GreyBackground" : "WhiteBackground");
        }
        j++;
    }

We iterate across every element in the list, which we just selected those elements, mind you, and then on alternating rows, swap between GreyBackground and WhiteBackground, giving us a nice contrast between rows of divs. It’s not enough, however, to just do that, we need to have this bizarre addition of a chain of if/else ifs that have it behave like a for-switch, but none of those cases are actually necessary, since we’ve already built the list in the previous block.

Maybe I’ve been writing too much of my own bad code lately, but I think I understand what happened here. At one point, instead of using FindControl, this code just tried to get a list of all the children and iterated across it, and thus the for-switch section was born. But the DOM changed, or maybe that never worked, and thus the developer went back and added the top block where they FindControl each element they need, but never updated the for loop to simplify the logic.

As a bonus WTF, and I recognize that I risk starting a flamewar with this: but CSS classnames should never explicitly reference the UI effect they have (GreyBackground and WhiteBackground) and instead the logical classification of the element (form-row, form-row-alt, for example). There are CSS frameworks which disagree with me on this, but they are wrong.

Full code:

protected void setDivColor()
    {
        List<HtmlControl> Divs = new List<HtmlControl>();
        HtmlControl divTire1Det = divShopCart.FindControl("divTire1Det") as HtmlControl;
        if (divTire1Det.Visible)
        {
            Divs.Add(divTire1Det);
        }
 
        HtmlControl divTire2Det = divShopCart.FindControl("divTire2Det") as HtmlControl;
        if (divTire2Det.Visible)
        {
            Divs.Add(divTire2Det);
        }
        HtmlControl divTire1WheelBal = divShopCart.FindControl("divTire1WheelBal") as HtmlControl;
        if (divTire1WheelBal.Visible)
        {
            Divs.Add(divTire1WheelBal);
        }
        HtmlControl divTire2WheelBal = divShopCart.FindControl("divTire2WheelBal") as HtmlControl;
        if (divTire2WheelBal.Visible)
        {
            Divs.Add(divTire2WheelBal);
        }
        HtmlControl divStudding = divShopCart.FindControl("divStudding") as HtmlControl;
        if (divStudding.Visible)
        {
            Divs.Add(divStudding);
        }
        HtmlControl divFWheelAlign = divShopCart.FindControl("divFWheelAlign") as HtmlControl;
        if (divFWheelAlign.Visible)
        {
            Divs.Add(divFWheelAlign);
        }
        if (bMAlignAdded)
        {
            HtmlControl divManufAlign_Remove = divShopCart.FindControl("divManufAlign_Remove") as HtmlControl;
            if (divManufAlign_Remove.Visible)
            {
                Divs.Add(divManufAlign_Remove);
            }
        }
        if (bRoadHazardAdded)
        {
            HtmlControl divRoadHazard_Remove = divShopCart.FindControl("divRoadHazard_Remove") as HtmlControl;
            if (divRoadHazard_Remove.Visible)
            {
                Divs.Add(divRoadHazard_Remove);
            }
        }
        HtmlControl divTireDisp = divShopCart.FindControl("divTireDisp") as HtmlControl;
        if (divTireDisp.Visible)
        {
            Divs.Add(divTireDisp);
        }
        HtmlControl divTireFee = divShopCart.FindControl("divTireFee") as HtmlControl;
        if (divTireFee.Visible)
        {
            Divs.Add(divTireFee);
        }
        HtmlControl divSalesTax = divShopCart.FindControl("divSalesTax") as HtmlControl;
        if (divSalesTax.Visible)
        {
            Divs.Add(divSalesTax);
        }
        if (!bMAlignAdded)
        {
            HtmlControl divManufAlign_Add = divShopCart.FindControl("divManufAlign_Add") as HtmlControl;
            if (divManufAlign_Add.Visible)
            {
                Divs.Add(divManufAlign_Add);
            }
        }
        if (!bRoadHazardAdded)
        {
            HtmlControl divRoadHazard_Add = divShopCart.FindControl("divRoadHazard_Add") as HtmlControl;
            if (divRoadHazard_Add.Visible)
            {
                Divs.Add(divRoadHazard_Add);
            }
        }
        HtmlControl divDiscount = divShopCart.FindControl("divDiscount") as HtmlControl;
        if (divDiscount.Visible)
        {
            Divs.Add(divDiscount);
        }
 
        int j = 0;
        foreach (HtmlControl dDiv in Divs)
        {
            if (dDiv.ClientID.Contains("divTire1Det"))
            {
                Divs[j].Attributes["class"] = (j % 2 == 0 ? "GreyBackground" : "WhiteBackground");
            }
            else if (dDiv.ClientID.Contains("divTire2Det"))
            {
                Divs[j].Attributes["class"] = (j % 2 == 0 ? "GreyBackground" : "WhiteBackground");
            }
 
            else if (dDiv.ClientID.Contains("divDownloadRebate"))
            {
                Divs[j].Attributes["class"] = (j % 2 == 0 ? "GreyBackground" : "WhiteBackground");
            }
            else if (dDiv.ClientID.Contains("divTire1WheelBal"))
            {
                Divs[j].Attributes["class"] = (j % 2 == 0 ? "GreyBackground" : "WhiteBackground");
            }
            else if (dDiv.ClientID.Contains("divTire2WheelBal"))
            {
                Divs[j].Attributes["class"] = (j % 2 == 0 ? "GreyBackground" : "WhiteBackground");
            }
            else if (dDiv.ClientID.Contains("divStudding"))
            {
                Divs[j].Attributes["class"] = (j % 2 == 0 ? "GreyBackground" : "WhiteBackground");
            }
            else if (dDiv.ClientID.Contains("divFWheelAlign"))
            {
                Divs[j].Attributes["class"] = (j % 2 == 0 ? "GreyBackground" : "WhiteBackground");
            }
            else if (dDiv.ClientID.Contains("divTireDisp"))
            {
                Divs[j].Attributes["class"] = (j % 2 == 0 ? "GreyBackground" : "WhiteBackground");
            }
            else if (dDiv.ClientID.Contains("divTireFee"))
            {
                Divs[j].Attributes["class"] = (j % 2 == 0 ? "GreyBackground" : "WhiteBackground");
            }
            else if (dDiv.ClientID.Contains("divSalesTax"))
            {
                Divs[j].Attributes["class"] = (j % 2 == 0 ? "GreyBackground" : "WhiteBackground");
            }
            else if (dDiv.ClientID.Contains("divManufAlign_Remove") && bMAlignAdded)
            {
                Divs[j].Attributes["class"] = (j % 2 == 0 ? "GreyBackground" : "WhiteBackground");
            }
            else if (dDiv.ClientID.Contains("divRoadHazard_Remove") && bRoadHazardAdded)
            {
                Divs[j].Attributes["class"] = (j % 2 == 0 ? "GreyBackground" : "WhiteBackground");
            }
            else if (dDiv.ClientID.Contains("divManufAlign_Add") && !bMAlignAdded)
            {
                Divs[j].Attributes["class"] = (j % 2 == 0 ? "GreyBackground" : "WhiteBackground");
            }
            else if (dDiv.ClientID.Contains("divRoadHazard_Add") && !bRoadHazardAdded)
            {
                Divs[j].Attributes["class"] = (j % 2 == 0 ? "GreyBackground" : "WhiteBackground");
            }
            else if (dDiv.ClientID.Contains("divDiscount") && !bRoadHazardAdded)
            {
                Divs[j].Attributes["class"] = (j % 2 == 0 ? "GreyBackground" : "WhiteBackground");
            }
            j++;
        }
    }
[Advertisement] Ensure your software is built only once and then deployed consistently across environments, by packaging your applications and components. Learn how today!


Copy/Paste Culture;

Mark F had just gone to production on the first project at his new job: create a billables reconciliation report that an end-user had requested a few years ago. It was clearly not a high priority, which was exactly why it was the perfect items to assign a new programmer.

"Unfortunately," the end user reported, "it just doesn't seem to be working. It's running fine on test, but when I run it on the live site I'm getting a SELECT permission denied on the object fn_CalculateBusinessDays message. Any idea what that means?"

The problem was fairly obvious, and Mark knew exactly what the error meant. But the solution wasn't so obvious. Why did the GRANT script work fine in test, but not in production? How can he check to see what the GRANTS are in production? Is there someone specific he should ask to get permission to look himself? Does the DBA team use a sort of ticketing system maybe? Is this even the right approach? Who on his team could he even ask?

Fortunately, Mark had the perfect venue to ask these sorts of questions: the weekly one-on-one with his team lead, Jennifer. Although he had a few years of coding experience under his belt, he was brand new to The Enterprise and specifically, how large organizations worked. Jennifer definitely wasn't the most technical person he'd met, but she was super helpful in "getting unblocked" as he was learning to say.

"Huh", Jennifer answered in their meeting, "first off, why do you even need a function to calculate the business days between two dates?"

"This seems like something pretty common in our reports," Mark responded, "and this, if the logic ever changes, we only need to change it in one place."

Jennifer gave a mystified look and smiled, "Changes? I don't think the 7-day week is going to change anytime soon, nor is the fact that Saturday and Sunday are weekends."

"Well, umm," Mark definitely didn't expect that response. He was surprised to have to explain the basic principles of code reuse to his supposed mentor, "you see, this way we don't have to constantly rewrite the logic in all the places, so the code is a bit simpler."

"Why don't you just copy/paste the calculation code in your queries?" she rhetorically asked. "That seems like it'd be a lot simpler to me. And that's what I always do…. But if you really want to get the DBAs involved, your best contact is that dba-share email address. They are super-slow to project tickets, but everyone sees that box and they will quickly triage from there."

Needless to say, he didn't follow Jennifer's programming advice. She was spot on about how to work with the DBA team. That tip alone saved Mark weeks of frustration and escalation, and helped him network with a lot more people inside The Enterprise over the years.

##

Mark's inside connections helped, and he eventually found himself leading a team of his own. That meant a lot more responsibilities, but he found it was pretty gratifying to help others "get unblocked" in The Enterprise.

One day, while enjoying a short vacation on a beach far, far away from the office, Mark got a frantic call from one of his team members. An end-user was panicked about a billables reconciliation report that had been inaccurate for months. The auditors had discovered the discrepancies and needed answers right away.

"So far as I can tell," his mentee said, "this report is using a fn_ CalculateBusinessDays function, which does all sorts of calculations for holidays, but they already prorate those on the report."

The problem was fairly obvious, and Mark knew exactly what happened. Some must have changed the logic on that function to work for their needs. But changing it back would mean breaking someone else's report. And the whole idea of a function seemed strange, because that would mean taking a dependen--

The junior programmer interrupted his stream of thought.

"I think I should just add an argument to the function to not include holidays," he said. "That's really simple to do, and we can just edit our report to use that argument."

"Ehhh," Mark hesitated, "the logic is so simple. Why don't you just copy/paste the business day calculation? That's the simplest solution… that's what I do all the time."

[Advertisement] Otter - Provision your servers automatically without ever needing to log-in to a command prompt. Get started today!


Logs in the Cloud;

Carrol C just joined IniTech. They were hiring for someone who could help them tame their cloud costs. There’s a lot of money being spent on AWS. Management had bought in on the “it’s cheaper than on-prem”, and were starting to wonder why that promise wasn’t being fulfilled.

After a little settling in period, Carrol had his credentials for their AWS environment, and started digging around just to familiarize himself with the infrastructure. This environment had started as an on-prem system that got migrated to the cloud, so the infrastructure was mostly a collection of virtual-machines using attached virtual disks- EBS- for storing data.

And that was the first red flag. Each VM was using between 160–200GB of EBS. CPU usage was hovering at around 15%, and RAM was barely a blip, but the quantity of disk was getting a more than a little out of hand. Carrol scribbled a few notes on a post-it, and went down the hall to visit Merna’s cube. Merna was one of the engineers responsible for developing the application. “Hey, Merna, question for you: how big is our application code and deployable artifacts?”

“Oh, depends on which instance you’re talking about, but the biggest weighs in at about a gig,” Merna said. “But we have some additional stuff installed on our VM image, so the VM image itself is about 4GB.”

“And what does-” Carrol paused to check his note- “the WRPT–073 instance do?”

“Oh, that’s just a simple webserver. Just generates some reports. Barely operates at capacity on even a Micro instance.”

“So… why is the disk sized for 200GB?”

Merna looked at Carrol like he had just asked the dumbest possible question. “Logs, obviously,” she said.

“… you have 196ish gigs of logs?”

Merna nodded. “We might need them.”

“AWS has a log aggregator that doesn’t store the logs on EBS. You could just ship them there and use logrotate to trim your logs, and that would be way cheaper.”

Merna shook her head. “You say that, but- oh, hold on.” Merna’s email bleeped at her: instance WRPT–073 was getting close to its disk capacity threshold. She quickly pulled up the AWS console and added another 10GB to the disk, before turning back to Carrol. “A lot of those accessory services have highly variable costs, and it makes it hard to predict your spend. Using cloud VMs is a much more consistent option. But if you feel so strongly about it, you could submit a request and we’ll evaluate it for a future release.”

Carrol submitted a request, and also pinged his new boss. “I think I know a way we can manage costs.” For now, Merna and the other engineers just expand disks when they fill up. It remains to be seen if anything actually changes, but regardless, Carrol is prepared for an “interesting” time at IniTech.

[Advertisement] Otter - Provision your servers automatically without ever needing to log-in to a command prompt. Get started today!


CodeSOD: Legacy Documentation;

Vernice inherited a legacy web app. By "legacy" in this case, we mean "lots of jQuery." jQuery everywhere. Nested callbacks of HTTP requests, no separation of concerns at all, just an entire blob of spaghetti code that was left out on the counter and is now stuck together as a big blob of sauceless starch. And as for documentation? There isn't any. No technical documentation. No comments. The code didn't even pretend to be self-documenting.

For the past few months, Vernice has been tasked with adding features. This generally meant that she'd find the code she thought was responsible for that section of the app, change something, see nothing happen, realize she was looking at the wrong module, try that three more times, finally find the actual code that governed that behavior, but as it turns out it had downstream dependents which broke.

Adding a single textbox to a form was a week long process.

So imagine Vernice's joy, when she opened a file and saw neat, cleanly formatted code, a compact function, and her text editor showed her the telltale color of comments. There were comments! Had she finally found the "good" part of the application?

Then she read the code.

<script> $(function() { $('.notification-exp').bind('click', function() { var confirmation = confirm( 'Confirm expiration?' ); // confirm if(confirmation) return true; // don't confirm else return false; }); }); </script>

Finding this, after all of her struggles, Vernice writes: "I found this small snippet of self-documenting yet excessively documented code, I had strong feelings about this oasis and had to submit it."

[Advertisement] Ensure your software is built only once and then deployed consistently across environments, by packaging your applications and components. Learn how today!


Error'd: A Taste of Nil;

"This nil looks pretty tasty, but I think I’m allergic to it since I always feel sick when I see it in my debugger," Kevin T. writes.

 

"Well, better to see this on the display than the airplane, I suppose," writes Mark M.

 

Bartosz wrote, "I knew Bill Gates is a visionary, but I'd have never imagined that he came up with Single Sign-On to his products so many years before I was born!"

 

"Allstate's got some attractive rates on Undefined insurance," Ted C. wrote.

 

Philipp L. writes, "If Bangkok's MRT is planning an upgrade, I wonder whether they consider skipping Windows XP and going straight to Vista."

 

Michael R. wrote, "In a world where two digit identifiers aren't always a fit, McDonalds is stepping up its game."

 

[Advertisement] Ensure your software is built only once and then deployed consistently across environments, by packaging your applications and components. Learn how today!


CodeSOD: The Powerful Parent;

As we’ve explored recently, developers will often latch onto something they pick up in one language and carry it forward with them into others. Kerry still is working with the co-worker who has… odd ideas about how things should work. At is turns out, this developer is also a Highly Paid Consultant, which we just discussed yesterday.

The latest problem Kerry found was in a display grid. It lists off a bunch of information linked to the user’s account, and each entry on the grid has a little plus sign on it to display further details. What, exactly, appears on that grid is tied to your account. It’s also worth noting that this service has a concept of corporate accounts- a “parent” account can administer entries for all their child accounts.

When someone clicks that little plus sign, we need to fetch additional details, regardless of whether or not they’re a corporate parent account or not. So how exactly does the C# controller do this?

           long? acctId = null
            // if the user is a corporate dealer fetch claims without using an account Id
            if (User.IsInRole("Parent_User") ||
                User.IsInRole("Parent_User_2"))
            {
                acctId= null;
            }
            else
            {
                acctId= fetchAcctId();
            }
            Model model = _service.fetchDetails(itemId, acctId);

The first thing that’s worth noting is that we already have the account ID value- we needed it to display the grid. So now we’re fetching it again… unless you’re one of the parent user roles, which we make null. It was already null on the first line, but we make it null again, just for “clarity”. The comment tells us that this is intentional, though it doesn’t give us any sense as to why. Well, why do we do that? What does that fetchDetails method actually do?

        public DetailViewModel fetchDetails(long itemId, int? acctID)
        {

            DetailViewModel results = new DetailViewModel();

            try
            {
                using (var ctx = new DBContext())
                {
                    DetailViewInternal detailData = ctx.DetailViewInternals.SingleOrDefault(
                      x => x.itemID == itemId && (x.FacilityID == acctID || acctID == null)
                    );
                    results.CustomerFirstName = detailData.FirstName;
                    results.CustomerLastName = detailData.LastName;
                    ...snip out a lot of copying of info...
            }
            catch (Exception ex)
            {
                _log.Debug($"Error getting detail.. exception {ex.Message}");
            }
            return results;
        }

The key logic is the lambda that we use to filter: x => x.itemID == itemId && (x.FacilityID == acctID || acctID == null)

For “normal” users, their acctID must match the FacilityID of the data they’re trying to see. For “parent” users, it doesn’t, so we pass a null to represent that. We could, of course, pass a flag, which would make sense, but that’s clearly not what we’re doing here. Worse, this code is actually wrong.

Specifically, a “parent” account has a specific list of children- they actually have a set of acctIDs that are valid for them. With this approach, a maliciously crafted request could actually pass an itemID for a different company’s data (by guessing ID values, perhaps?) and fetch that data. The “parent” privileges give them permission to see anything if they’re clever.

What makes this worse is that the grid code already does this. Instead of reusing code, we reimplement the authorization logic incorrectly.

With that in mind, it’s barely worth pointing out that there’s no null check on the query result, so the results.CustomerFirstName = detailData.FirstName line can throw an exception, and our exception just gets logged in debug mode, without any of the stack trace information. I’m pointing it out anyway, because there’s not many things more annoying than a useless log message. This also means that the exception is swallowed, which means there’s no way for the UI to present any sort of error- it just displays an empty details box if there are any errors. Enjoy puzzling over that one, end users!

[Advertisement] ProGet can centralize your organization's software applications and components to provide uniform access to developers and servers. Check it out!