CodeSOD: The Label Printer |
If you create a UI object in code, you have to go through all that pesky, annoying work of initalizing that object so it displays correctly. I mean, who wants to write VB.NET code which looks like this:
Label = New Label
Label.Size = New Size(710, 300)
Label.TextLB = "Operation:"
someForm.Controls.Add(Label)
Here, we've created a label object which overrides a handful of default properties, and then gets added to the screen. You'll have to do that for every control! Obviously, you need a method.
No, not a method called something like OperationLabel() which creates and adds the Operation: label to the form. You have to think more generic than that. We want a method which allows us to add any label, with any configuration, to the form.
Benjamin's co-worker has an idea exactly about how to accomplish that. Instead of that ugly block of code above, you can write this gorgeous code:
CreateRectLabel(Label, "Label", String.Empty, New Point(290, 5), someForm, Color.Red, Color.White, Color.White, 710, 300, Color.White, "SomeText", String.Empty, New Font(MainFontName, 48, FontStyle.Bold), New Font(MainFontName, 16, FontStyle.Bold), New Font(MainFontName, 28, FontStyle.Bold), TextEffects.Floating, TextEffects.Floating, TextEffects.Floating, Lang.InstructionErrors.PushInRedButton, TextEffects.Floating, New Font(MainFontName, 34, FontStyle.Bold), Color.White, Color.White, TextEffects.None, "", Nothing, Color.White, TextEffects.None, "", New Font(MainFontName, 12, FontStyle.Regular), Color.White, TextEffects.None, "", New Font(MainFontName, 12, FontStyle.Regular))
Now you have to directly set every one of those parameters, no exceptions! You'll always have to be perfectly explicit about what settings you're using.
Here is the implementation of that CreateRectLabel method:
Public Sub CreateRectLabel(ByRef LabelObj As Label, ByVal LabelName As String, ByVal TextLT As String, ByVal LabelLocation As Point, ByRef PanelContainer As Object, ByVal LabelColor As Color, ByVal TextColorLT As Color, ByVal TextColorCaption As Color, ByVal LabelWidth As Integer, ByVal LabelHeight As Integer, ByVal TextColorLB As Color, ByVal TextLB As String, ByVal TextCaption As String, ByVal FontLT As Font, ByVal FontCaption As Font, ByVal FontLB As Font, ByVal TextEffectLT As TextEffects, ByVal TextEffectCaption As TextEffects, ByVal TextEffectLB As TextEffects, ByVal TextLM As String, ByVal TextEffectLM As TextEffects, ByVal FontLM As Font, ByVal TextColorLM As Color, ByVal TextColorRM As Color, ByVal TextEffectRM As TextEffects, ByVal TextRM As String, ByVal FontRM As Font, ByVal TextColorCB As Color, ByVal TextEffectCB As TextEffects, ByVal TextCB As String, ByVal FontCB As Font, ByVal TextColorCT As Color, ByVal TextEffectCT As TextEffects, ByVal TextCT As String, ByVal FontCT As Font, Optional ByVal LabelSpecialEffects As SpecialEffects = SpecialEffects.None)
LabelObj = New Label
LabelObj.Name = LabelName
LabelObj.Size = New Size(LabelWidth, LabelHeight)
LabelObj.Location = LabelLocation
LabelObj.Shape = Shapes.Rectangle
LabelObj.ShadowMode = ShadowModes.Blurred
LabelObj.ColorSurfaceNormal.Render3DType = ColorRenderType.Best3D
LabelObj.ColorSurfaceNormal.BackColor = LabelColor
LabelObj.ColorSurfaceNormal.BackColor2 = LabelColor
LabelObj.ColorSurfaceNormal.GradientType = ColorGradientType.Classic
LabelObj.ColorSurfaceNormal.GradientFactor = 4
LabelObj.Surface = Surfaces.HardPillow
LabelObj.SmoothEdges = Smooths.High
LabelObj.SpecialEffect = LabelSpecialEffects
' Left Top Text
LabelObj.TextDescrLT.ColorNormal = TextColorLT
LabelObj.TextDescrLT.SpecialEffect = TextEffectLT
LabelObj.TextDescrLT.Text = TextLT
LabelObj.FontLeftTop = FontLT
' Center Top Text
LabelObj.TextDescrCT.ColorNormal = TextColorCT
LabelObj.TextDescrCT.SpecialEffect = TextEffectCT
LabelObj.TextDescrCT.Text = TextCT
LabelObj.FontCenterTop = FontCT
' Caption Text
LabelObj.ForeColor = TextColorCaption
LabelObj.FontCaption = FontCaption
LabelObj.TextDescrCaption.SpecialEffect = TextEffectCaption
LabelObj.Text = TextCaption
' Left Middle Text
LabelObj.TextDescrLM.ColorNormal = TextColorLM
LabelObj.TextDescrLM.SpecialEffect = TextEffectLM
LabelObj.TextDescrLM.Text = TextLM
LabelObj.FontLeftMiddle = FontLM
' Left Bottom Text
LabelObj.TextDescrLB.ColorNormal = TextColorLB
LabelObj.TextDescrLB.SpecialEffect = TextEffectLB
LabelObj.TextDescrLB.Text = TextLB
LabelObj.FontLeftBottom = FontLB
' Center Bottom Text
LabelObj.TextDescrCB.ColorNormal = TextColorCB
LabelObj.TextDescrCB.SpecialEffect = TextEffectCB
LabelObj.TextDescrCB.Text = TextCB
LabelObj.FontCenterBottom = FontCB
' Right Middle Text
LabelObj.TextDescrRM.ColorNormal = TextColorRM
LabelObj.TextDescrRM.SpecialEffect = TextEffectRM
LabelObj.TextDescrRM.Text = TextRM
LabelObj.FontRightMiddle = FontRM
LabelObj.Visible = True
PanelContainer.Controls.Add(LabelObj)
End Sub
No, there are no overloads. Yes, they quite clearly understood what optional paremeters are. No, they didn't want to use them for anything else but the LabelSpecialEffects parameter.
In any case, when you want to change a setting on a label, enjoy counting parameters to figure out where exactly you should make the change.
[Advertisement]
ProGet can centralize your organization's software applications and components to provide uniform access to developers and servers. Check it out!
|
Метки: CodeSOD |
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!
https://thedailywtf.com/articles/identification-without-authentication
|
Метки: Error'd |
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 Divs = new List();
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 Divs = new List();
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++;
}
} |
Метки: CodeSOD |
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!
|
Метки: Feature Articles |
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!
|
Метки: Feature Articles |
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.
|
Метки: CodeSOD |
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!
|
Метки: Error'd |
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!
|
Метки: CodeSOD |
Hop Scotch |
IniTech’s fashion division, IniGarment, launched their new accounting solution by hiring “best in class” highly-paid-consultants (HPCs). The system launched, collapsed under the weight of the first week of use, hardware was thrown at the problem in an absolute crisis panic, and the end result was that they had a perfectly serviceable accounting package that was overbudget and supported by expensive HPCs.
It wasn’t sustainable. So out the HPCs went, and in came a pile of salaried employees. Jeff was one of those. His first six weeks at IniGarment were spent trying to understand the nest of stored procedures, quick hacks, and ugly choices. One of the first puzzles Jeff decided to look into was an invoice uploading step.
They used a turn-key eCommerce package to handle sales, and the accounting system needed to generate invoices and upload them to the eCommerce package. This relatively simple task took 30 minutes to generate and upload a single invoice, and that was on a good day. On bad days, it could take nearly an hour. So Jeff set out to figure out why.
In the middle of a 700+ line stored procedure, Jeff found the query which generated the report. The whole process required big piles of temporary tables (which, instead of being true temp tables, were created/dropped with each execution), and ugly group-bys and subqueries. Still, even with all that, it was just a SQL query, right?
INSERT INTO ApiInvoiceItemReports
(
BrandCode,
InvoiceNumber,
InvoiceLineNumber,
OrderNumber,
ProductNumber,
ProductName,
SeasonCode,
SeasonDescription,
DivisionCode,
DivisionDescription,
ColorCode,
ColorDescription,
GenderCode,
GenderDescription,
DimensionCode,
SizeScaleCode,
ProductCategoryCode,
ProductCategoryDescription,
InvoicedQuantity,
ProductInvoicedPrice,
StatusCode,
ObjectState
)
SELECT
NULL AS BrandCode,
dbo.TRIM(T.invno) AS InvoiceNumber,
MIN(T.[lineno]) AS InvoiceLineNumber,
dbo.TRIM(I.OrderNumber) AS OrderNumber,
dbo.TRIM(LEFT(T.ItemNo, 12)) AS ProductNumber,
MAX(dbo.TRIM(S.Title)) AS ProductName,
MAX(dbo.TRIM(T.season)) AS SeasonCode,
MAX(dbo.TRIM(T.season)) AS SeasonDescription,
MAX(compno) AS DivisionCode,
MAX(compno) AS DivisionDescription,
substring(dbo.trim(T.ItemNo),13,
CASE WHEN charindex('-',dbo.trim(T.ItemNo)) > 0 THEN charindex('-',dbo.trim(T.ItemNo))-13 ELSE 3 END
) AS ColorCode,
MAX(ISNULL(dbo.TRIM(C.descrip), '')) AS ColorDescription,
MAX(dbo.TRIM(Gender.GenderCode)) AS GenderCode,
MAX(dbo.TRIM(Gender.GenderCode)) AS GenderDescription,
NULL AS DimensionCode,
MAX(sizrange) AS SizeScaleCode,
MAX(dbo.TRIM(Category.CategoryCode)) AS ProductCategoryCode,
MAX(dbo.TRIM(Category.CategoryCode)) AS ProductCategoryDescription,
SUM(qtyord) AS InvoicedQuantity,
MAX(unitpr) AS ProductInvoicedPrice,
'S' AS StatusCode,
1 AS ObjectState
FROM Arytrans T
INNER JOIN #InvoiceReportsData I ON I.InvoiceNumber = T.invno
INNER JOIN aastock S ON S.ItemNo = T.ItemNo
LEFT JOIN aacolor C ON C.code = substring(dbo.trim(T.ItemNo), 13, 5)
LEFT JOIN
(
(SELECT ItemNo, Code AS GenderCode, NULL AS CategoryCode FROM rsoption WHERE [Key] = 'GENDER') AS Gender
FULL OUTER JOIN
(SELECT ItemNo, NULL AS GenderCode, Code AS CategoryCode FROM rsoption WHERE [Key] = 'PRODUCTCATEGORY') AS Category
ON Gender.ItemNo = Category.ItemNo
)
ON
Gender.ItemNo = T.ITEMNO OR Category.ItemNo = T.ITEMNO
WHERE T.compno = 9
AND T.ItemNo IS NOT NULL AND T.ItemNo <> ''
AND T.Custstyle IS NOT NULL AND T.custstyle <> ''
AND T.WebOrderid IS NOT NULL AND T.WebOrderid <> ''
AND T.WebStyle IS NOT NULL AND T.WebStyle <> ''
GROUP BY
T.invno, I.OrderNumber, T.itemnoNow, it’s not terribly surprising that this query didn’t perform well, but Jeff was surprised where the performance bottleneck was. Specifically, it was this inner join: INNER JOIN aastock S ON S.ItemNo = T.ItemNo, and the related field access: MAX(dbo.TRIM(S.Title)) AS ProductName.
Looking at the execution plan, Jeff discovered that the combination of the group-bys, and the MAX access meant that this was triggering a full tablescan of aastock for each result row. That was certainly going to hurt performance, but aastock wasn’t that large a table- 165,000 rows- so that didn’t quite explain what was going on.
But aastock wasn’t actually a table. It was a view. Defined as:
CREATE view [LOCALSQL].VENDOR.[dbo].[aastock] as
select *
from [REMOTESQL].ERP.[dbo].aastockThis was getting close to explaining the massive performance hit. aastock was a view to a table in a remote database. So each one of those full table scans needed a network hop. But wait, how was the remote table defined? Turns out, it’s a view as well:
create view [REMOTESQL].ERP.[dbo].[AASTOCK] as
select * from [REMOTESQL].ERP.[dbo].[syn_AASTOCK];But wait, what’s syn_AASTOCK? That sounds like a synonym. What does it point at?
CREATE SYNONYM [REMOTESQL].ERP.[dbo].[syn_AASTOCK]
FOR [ERP_stage1].[dbo].[AASTOCK]… it’s a reference to yet another remote server, their ERP system. As it turns out, the HPCs couldn’t figure out how to avoid deadlocks with their mirrored replication, so their “fix” was to create synonyms which point to an ERP replica, and then swap which replica the synonym pointed at depending on the hour, cycling through ERP_stage1, ERP_stage2, ERP_replA, etc.
The result is that this query did a full tablescan for each row which included two network hops. No one knew that it was doing this, and it certainly wasn’t documented anywhere. Once Jeff understood the path, he could refactor the entire stored procedure to clean up the access path using common table expressions and a cross apply to restrict the way functions got applied. With that, the query went from many minutes of execution to a handful of seconds.
Jeff briefly contemplated looking into the issues with deadlocks in replication, but decided that wasn’t the next problem to solve when there were other queries that could be tweaked to increase performance without opening that box of horrors.
[Advertisement]
Continuously monitor your servers for configuration changes, and report when there's configuration drift. Get started with Otter today!
|
Метки: Feature Articles |
CodeSOD: Install Your Package |
I use Python a lot at work, and if you're doing anything vaguely data oriented, you want to use NumPy. I gave a talk about how much I love NumPy. It's one of the things that I automatically include in every requriements.txt because it's so goddamn useful.
Lanny supports a product which uses NumPy, which is why he was surprised to find this block:
#################### Import Section of the code #############################
try:
import numpy as np
except Exception as e:
print(e,"\nPlease Install the package")
#################### Import Section ends here ################################
Now, Python is perfectly happy to let you put code inline in modules. This allows for useful idioms- like, for example, I often do import blocks like this in the entry point script of a product:
if mode == "debugging":
from debugging_main import main
elif mode == "server":
from server_main import main
elif mode == "client":
from client_main import main
main()
It's a powerful tool that lets me deploy the same core product in different modes depending on how I'm using it. It's also useful when you don't know exactly which version of a dependency should be used- some Python packages have binary versions, but those are platform specific, so you may have some pure Python wrapper that implements a matching API.
So, for example, you might do:
try:
import some_binary_dependency as my_lib
except:
import some_python_wrapper as my_lib
This practice lets us fail gracefully: when the exceptional situation happens, we fail into a state where the program can continue, perhaps with some limitations. Which is not what happens in the example Lanny found. Here, the program warns you that you should have NumPy installed- well, no, it tells you to "Please Install the package", and isn't terribly specific about which one- and then crashes out when it tries to use the package. This is one of the odder cases of "just swallow the exception and keep going". As Lanny points out, "NumPy is used in nearly every line of the following code, so it's not like this is getting partial functionality."
Frankly, though, that's not even TRWTF to me. By convention, all Python imports go at the top of the script. At a glance, it lets you see what is imported. You don't need those comments to mark out the section. Those comments are just visual noise, they're not making the code any clearer.
And also, if you're the sort that cares about such things, they're not the same length. It's like a painting hung not quite straight on the wall. Everything just feels lopsided and wrong.
[Advertisement]
Otter - Provision your servers automatically without ever needing to log-in to a command prompt. Get started today!
|
Метки: CodeSOD |
Representative Line: Without Directions |
Adam S sends us a representative line which represents a mystery. It's a simple enough Java statement:
private static final Integer[] DIRECTIONS = { 0, 1, 2, 3, 5, 6, 7, 8 };
There is no documentation, no commentary, no explanation of what, exactly, this is supposed to represent. Why did the developer choose to use Integer and not the int primitive type? Why is this just a sequence of numbers in order? Why is 4 missing from the sequence?
The only clue we have is to look at how this particular constant is used, but that is, sadly, no extra help: ``DIRECTIONS.length```. It leaves us with another question: why not just have a constant integer value?
The fact that there are eight entries is tantalizingly reasonable: North, North-East, East, etc. Except we include 0 and 8, but exclude 4, which doesn't make sense. Did the original developer want to suggest that there should be 8 possible directions, and didn't understand how counting works? Then they just deleted one number to make the numbers add up? Why 4 though? Was their fourth birthday a profound disappointment? Do they just consider 4 unlucky? It is the only number in the list which is the square of a prime, so maybe there's a deep numerological significance to their choice?
In the end, we must simply admit: we have no idea what this developer was doing. We are in good company, though, for it seems likely that this developer also had no idea what they were doing.
[Advertisement]
BuildMaster allows you to create a self-service release management platform that allows different teams to manage their applications. Explore how!
|
Метки: Representative Line |
Error'd: Con(text)ual Errors |
"A football coach needs at least a two line footer to succeed," writes Ergin S.
"I was looking for a specific Capitec Bank branch, but instead I found a bunch of HTML escape codes," Sam T. wrote.
Steve M. writes, "Chromium Edge installer appears to have been localised for Elvish or possibly Klingon."
Joel B. wrote, "Apparently unicode is hard, even when you're a multi-billion dollar company."
"We did not find this book ISBN, but maybe you are interested in other books on None, None, None, None, and None then?" writes Jake W.
"Maybe one of the premium features is the ability to count files?" Chris wrote.
[Advertisement]
ProGet supports your applications, Docker containers, and third-party packages, allowing you to enforce quality standards across all components. Download and see how!
|
Метки: Error'd |
Coded Smorgasbord: On the Hard Problems |
As the saying goes, there are two hard problems in computer science: naming things, cache expiration, and off-by-one errors. This little snipped of anonymously supplied PHP code highlights the first one of those challenges:
// if no error commit
if ($is_fail) {
$binder->commit();
}$is_fail is obviously inaccurately named, or maybe it’s more accurate than we think. There’s obviously a failure someplace around here.
Speaking of hard problems, defending against nulls is an important thing a programmer must do. Jim J found this… unique defense against nulls in some C# code.
Waybill.AgentAgreementId = null;
Waybill.AgentAgreementId = 1; //in case if AgentAgreementId == nullWell, at least we know it won’t be null. It seems like maybe there was an easier way to enforce that.
Nothing helps you solve weird bugs better than some decent logging. Annis supports a product that logs really aggressively. So much so, that it would fill up the disk. Was the fix to add log rotation? No, of course not. They just deleted the log files. What’s interesting, and made me blink when I saw it, is how they delete the log files.
cat /dev/null > json.logThis… isn’t actually wrong. Or maybe it isn’t. If this file is hardlinked elsewhere, simply rming it is only going to remove one of the links. The total storage it takes up is still allocated to the other links. Writing the contents of /dev/null to the file will ensure that the file takes up zero bytes, regardless of the links.
Is that a good idea? Honestly, I can’t be sure. Are hardlinks even a problem for this file? Is the filesystem they’re using going to play nice with this approach?
Regardless, I’ve learned a new way to empty a file.
[Advertisement]
BuildMaster allows you to create a self-service release management platform that allows different teams to manage their applications. Explore how!
|
Метки: Coded Smorgasbord |
CodeSOD: Going Down to the Object Store |
Odette’s company uses a popular video conferencing solution for browser use. In the base library, there’s a handy-dandy class called ObjectStorage, which implements an in-memory key/value store entirely in TypeScript/JavaScript.
“Wait,” you ask, “isn’t a JavaScript in-memory, key/value store just… an object? A map, if you’re being pedantic?”
Yes, of course, but have you thought about ways to make JavaScript’s maps/objects worse?
export class ObjectStorage {
private _storage: {
[k: string]: any;
} = {};
private _counter = 0;
public set(key: string, value: any): void {
this._storage[key] = value;
}
public push(value: any): string {
const key = this._getNextUniqueKey();
this._storage[key] = value;
return key;
}
public get(key: string): any {
return this._storage[key];
}
public get keys(): string[] {
return Object.keys(this._storage);
}
public get values(): any[] {
return Object.values(this._storage);
}
private _getNextUniqueKey() {
if (Object.keys(this._storage).includes(this._counter.toString())) {
this._counter++;
return this._getNextUniqueKey();
}
return (this._counter++).toString();
}
}So, yes, this does just wrap access to object fields in a set of methods: set, push, and get, with a handy-dandy little autonumber ID generator, _getNextUniqueKey. It’s not making anything significantly worse, but it’s hard to imagine how this makes anything better.
The clear flow, though it’s clearly not documented in the code, is that you store an object using push. push will return the key of the object, so get ready to remember that key someplace else in your application- maybe you should use an ObjectStorage to store the keys for your other ObjectStorage.
And how does that key get generated? We just increment a counter and convert it to a string. We’re at least smart enough to check that that key isn’t already in use, which at least avoids some disasters.
Of course, there’s no checks on set so it’s trivially easy to accidentally stomp on an existing key. This means that the calling code has to be very intentional about whether or not this is an insert or an update.
Which is to say: the whole push and set abstraction really doesn’t gain us anything. Yes, an autogenerated key is possible. But that’s strictly client side- if this is shipping objects to anyone else, there’s no way to avoid key collisions, so this ID can’t drive any API calls.
And that’s the core WTF: what is even the point of this object? It’s inconvenient and opaque methods wrapped around the same basic JavaScript object mangling that’s your standard process for web development.
[Advertisement]
Otter - Provision your servers automatically without ever needing to log-in to a command prompt. Get started today!
https://thedailywtf.com/articles/going-down-to-the-object-store
|
Метки: CodeSOD |
A Short REST |
I’m starting to wonder if we need a “Representative API” category. There are just so many ways in which a developer can mess up basic tasks, like mapping a RESTful API to their backend.
Today’s anonymous submitter was working with a REST API provided by a “world leading parcel” company. Everything started well. The documentation was thorough, contained links to example projects, and it came with a Swagger API doc. Based on that documentation and based on the Swagger doc, they went ahead and tried to CURL the API for a specific endpoint:
curl -X POST https://[redacted]/rest/returnsIt returned a 500 error, with no description of the error which occurred.
They tried playing around with the body content that gets sent. They went back to the documentation, and discovered an inconsistency- in the docs, it suggested that the url should be rest/v1/returns, so they tried that. That didn’t work. Another section of the docs disagreed, and suggested a slightly different URL pattern. That also didn’t work. They tried using Swagger’s code generation functionality to build a client right from that doc, and tried again. That also didn’t work.
After many hours of staring at information-free HTTP 500 errors, our anonymous submitter went ahead and tried running one of the sample projects. It worked.
It was a short program, so it wasn’t exactly hard to trace through it, line by line, and see if there were any obvious differences. There weren’t. Our submitter’s attempts send the same request to https://[redacted]/rest/returns failed, but when the test program sent its requests to https://[redacted]/rest/returns/ everything worked just fine.
A pair of shell commands proved it quite easily:
curl -X POST https://[redacted]/rest/returns # 500 error
curl -X POST https://[redacted]/rest/returns/ # 201 resultAnd that’s when they spotted it. The trailing slash. The service was configured so that returns and returns/ were actually separate routes. returns wasn’t a valid route, so cue the 500 error. Every single piece of documentation ignored that, and the only place where the URL was accurate was in the test application. Which, at least there was a working test application.
Our submitter adds: “God, I hate web development.”
[Advertisement]
Utilize BuildMaster to release your software with confidence, at the pace your business demands. Download today!
|
Метки: Feature Articles |
CodeSOD: An Accident |
There's a very specific brand of bad code that I see from time to time, which I think of as "Oh, this poor person was permanently damaged by exposure to C." They're not writing C, but there's something about their "accent" which tells you: they learned C and didn't recover from the experience. Every reference variable can be treated like a pointer if you're insistent enough.
There are other, similarly brain-breaking languages. COBOL. PL/SQL. VBA. Programmers learn the quirks of these languages, fail to understand them, and then start writing weirdly formal, structured code in languages that aren't supposed to work that way.
Kerry has a co-worker like that. They're doing C#, and they include weird little ticks that are a clear sign of some kind of other langugae trauma. My guess is that it's a mix of PL/SQL and Visual Basic (not .NET), based on my experience with other people who have suffered through learning PL/SQL and VB as their first languages.
We're going to take a look at some. In the interests of anonymization, I'm not going to include all of the code, which is 632 lines of tedium in one method, but I'm going to include the important highlights. Like, for starters, the message signature:
public long? saveClaim(CreateClaimViewModel startClaim, out long? IncidentNumber, out int ClaimSameIncidentCount, string claimOrigination)
As you might gather, the purpose of this method is to take an insurance claim, inputted in the UI, and save it into the database. As part of that operation, three fields will have their value set: the claim ID (the return value of long?), the IncidentNumber, and the ClaimSameIncidentCount, all of which are also fields on the claim's model object.
Why doesn't this return the claim model type, instead of using a return value and two out parameters to return that information? Because if you come from one of those traumatized backgrounds, you might think output parameters is the most natural way to pass data around.
The next weird thing you'll notice is:
string ProcessStep = "";
long claimId = 0;
try
{
ProcessStep = "Step 1";
int? modelId;
int? makeId;
fetchVehicleMakeModelIds(startClaim.VehicleMake, startClaim.VehicleModel, out makeId, out modelId);
ProcessStep = "Step 2";
int? customerId = fetchCustomer(startClaim .CustFirstName, startClaim.CustLastName);
int? claimOriginationId = fetchClaimOriginationId(claimOrigination);
// skip about 600 lines…
claimId = claim.ClaimID;
ProcessStep = "Step 21";
_log.Debug($"Claim {claim.ClaimID} successfully saved");
ProcessStep is a variable that gets set at the start of each functional block. There are, in total, 21 "process step" blocks. Or, as a normal programmer might think of them, things that should each be their own procedure.
For the longest time, I thought this ProcessStep variable was just an overglorified comment, but as it turns out, when you scroll all the way down to the one, giant exception handler at the bottom, you see that it gets used:
catch (Exception ex)
{
_log.Debug($"Error during save claim Process Step = {ProcessStep} in exception {ex.Message}");
_log.Fatal("ruh roh", ex);
…
}
That's… almost clever? I mean, it lets you easily know which processing step failed, which, if you're not using procedures, can be hard to track. Actually smart would be to not write a 632 line function that needs to be divided into "process steps", but this shows a kind of cunning, nonetheless.
Starting in Process Step 5, this code opens a database context. From there, we mix-and-match, sometimes doing LINQ style queries:
Customer customer = null;
var q1 = from t1 in ctx.Customers
where t1.FirstName == startClaim.CustFirstName &&
t1.LastName == startClaim.CustLastName &&
t1.Phone == startClaim.CustPhone
select t1;
and sometimes doing what our developer is clearly more comfortable with, calling a procedure with output parameters:
var incidentNumberId = new SqlParameter
{
ParameterName = "IDName",
Value = "IncidentNumber"
};
var nextId = new SqlParameter
{
ParameterName = "NextID",
SqlDbType = SqlDbType.Int,
Direction = ParameterDirection.Output
};
ctx.Database.ExecuteSqlCommand("spGetNextIdentifier @IDName, @NextID out", incidentNumberId, nextId);
var incidentNumber = (int)nextId.Value;
IncidentNumber = incidentNumber; // output value
That's not wrong to do, per se; an important feature of a decent ORM is that it lets the developer talk straight to the database when you need to. But mixing and matching approaches in the same method is just awkward and confusing.
Since we are using an ORM, we also know that we have some flexibility in how we actually trigger our persistence and interact with transactions. We can, for example, build the entire update in memory and then send it over to the database. Or, if you care about locking, you can write basically the same code but tell the ORM to lock the records while you're working. Or all variety of middle ground.
You'd think, for example, that we'd want to perform this entire update as a single transaction. You'd think that, but each process step that updates a row looks something like:
// insert customer record
customer = new Customer
{
FirstName = startClaim.CustFirstName, // what came from the API may have been overwritten from RHM
LastName = startClaim.CustLastName,
…
};
ctx.Customers.Add(customer);
ctx.SaveChanges();
That call to SaveChanges does what it says on the tin: it saves your changes to the database. This has a few disadvantages: you're increasing the number of database round trips, you're basically ensuring the ORM doesn't get any real chance to optimize how it batches up your queries, you're not getting any of the advantages of transactions so if a statement fails, you'll end up with an inconsistent state in the database.
On the plus side, if a later step does fail, you've only lost a small part of your work thus far.
Many of those 632 lines of code are object constructions. They mostly are all just big blocks of Property = Value, OtherProperty = OtherValue, but the block that initalizes a contract introduces a few bonus ternaries in there:
contract = new Contract
{
ContractNumber = startClaim.StampNumber,
ProductID = (int)startClaim.ProductID,
ProgramID = (int)startClaim.ProgramID,
…
CustomerID = customer.CustomerID,
Status = (startClaim.ContractStatus == null || startClaim.ContractStatus == 0) ? 1 : startClaim.ContractStatus, // default to eligible QRP 1/24/2020
ContractType = (startClaim.ContractType == null || startClaim.ContractType == 0) ? 4 : startClaim.ContractType, // default to Inboarded QRP 1/24/2020
PurchaseDate = startClaim.ContractPurchaseDate,
MilesAtPurchase = startClaim.MilesAtPurchase,
TreadAtPurchase = startClaim.TreadAtPurchase,
InvoiceNumber = startClaim.ContractInvoiceNumber == null ? startClaim.OriginalInvoiceNumber : startClaim.ContractInvoiceNumber,
…
};
Again, this isn't wrong, but in the context of a gigantic uber-method, it's a nasty trick. Also, the InvoiceNumber ternary is a null check, but as most of this code uses nullable values, most of the rest of the code is using the ?? coalescing operator. It's odd that they forgot on just this one step.
Oh, there is one thing that's wrong, though you wouldn't know it from looking at this code. startClaim.ProductID and startClaim.ProgramID are both defined as int?- nullable ints. The cast is an attempt to make them actual ints. Which is fine, unless they're actually null. Can they be? Maybe not! But someone clearly doesn't fully understand what nullable types are for. Maybe they're just lacking confidence- they don't want an int, but an int?
Well, let's go back to the exception handling, shall we? I elided a bit before, just because I wanted to highlight the one case where ProcessStep was used.
catch (DbEntityValidationException dbex)
{
foreach (DbEntityValidationResult validationError in dbex.EntityValidationErrors)
{
_log.Debug("Entity: " + validationError.Entry.Entity.GetType());
foreach (DbValidationError error in validationError.ValidationErrors)
{
_log.DebugFormat("Property: {0} Error: {1}", error.PropertyName, error.ErrorMessage);
}
}
_log.Debug($"Error during save claim Process Step = {ProcessStep} in exception {dbex.Message}");
throw new SaveClaimException("unable to save claim, see log for errors");
}
catch (Exception ex)
{
_log.Debug($"Error during save claim Process Step = {ProcessStep} in exception {ex.Message}");
_log.Fatal("ruh roh", ex);
Exception myEx = ex.InnerException;
while (myEx != null)
{
_log.Debug($"Inner Exception = {ex.InnerException.Message}");
myEx = myEx.InnerException;
}
throw new SaveClaimException("unable to save claim, see log for errors");
}
As you can see, there are actually two catch blocks. They both, basically do the same thing: log the exception and convert them both into a SaveClaimException, with no inner exception. This means that the calling code has no idea what the actual failure is- the only thing anyone can do is go back to the log. Whether the user entered invalid data or the database is down, it's anybody's guess. Whether the process failed at processing step 5 or processing step 17, also just a guess, unless you go back to the log. And since we're saving changes at every step, we have no idea what the internal database state might look like, without looking at the log.
Management is puzzled. They know that every time they assign this developer to a project, the project quickly spirals into chaos. The other developers constantly complain that this person writes terrible, unmaintainable code, but to management's eyes, that's just a judgement call. There must be some other reason.
[Advertisement]
BuildMaster allows you to create a self-service release management platform that allows different teams to manage their applications. Explore how!
|
Метки: CodeSOD |
Error'd: Little to Nothing at All |
"I wonder, what does a null pharmacist eat for lunch? BaNaNas?" writes Barry M.
Jeff K. wrote, "I like your optimism, but no."
"I don't know who thought "You can't change your password unless we tell you to" was a good idea, although it doesn't surprise me that it was somebody at Oracle," Lee G. writes.
Roland wrote, "I am sure Grappa del Padre tastes good but, half a million of euros per square meter still seems expensive to me."
"I thought I was helping save the environment by bringing my own water bottle. Now I'm not so sure," Adam R. writes.
"I wonder if the first part of the exam is to come up with a correct title," Forrest writes.
[Advertisement]
Otter - Provision your servers automatically without ever needing to log-in to a command prompt. Get started today!
|
Метки: Error'd |
CodeSOD: The Intern's Gadget |
Pierre is a contractor who supports a variety of companies helping them manage data-science and analytics applications. Code, in these contexts, is written to answer specific questions, driven more by mathematics than program design. That’s a polite way of saying that the vast majority of the code is unreadable nonsense that no one can explain.
One of Pierre’s clients has decided to migrate to that fancy new thing they’ve been hearing about, “the cloud”, and thus they have to move all of their applications to that cloud. This suite of applications involved a website with a pile of “gadgets” that users could string together to do their own data analysis. And one of those gadgets was quickly thrown together by an intern circa 2007.
Now, no one expects an intern to write good code on their own. They need guidance. They need experience. They absolutely don’t need to be thrown into the deep end to whip up a reusable “gadget” that nobody thinks will ever see production.
So, within that context, the intern did their best. While working for the company, the intern had learned a little bit about GNU Octave, which is what drove most of their analytics. The intern also knew some PHP, from dabbling in web development. Since this particular gadget needed to store some data, the intern brought in the other tool they knew: MySQL.
Thus was born the Intern’s Gadget, which works roughly like this:
start.txt and end.txtThe generate file contains one function, called simply fn, which is several thousand lines long. Among other things, it contains a chain of nested while loops like:
while P(c)>tol | i(1)<=fl(1) | -l(1)+(i(1)+1)*ll(1)-lf(i(1)+1)-log(i(1)-l(1))>tol,
while P(c)>tol | i(2)<=fl(2),
while P(c)>tol | i(3)<=fl(3),
while P(c)>tol | i(4)<=fl(4),
while P(c)>tol | i(5)<=fl(5),
while P(c)>tol | i(6)<=fl(6),
while P(c)>tol | i(7)<=fl(7),
while P(c)>tol | i(8)<=fl(8),
while P(c)>tol | i(9)<=fl(9),
while P(c)>tol | i(10)<=fl(10),
while P(c)>tol | i(11)<=fl(11),
c=c+1;
i(11)=i(11)+1;
k=sum(i);
P(c)=sum(log(XY-k+1:XY))+(XY-k)*lp0+sum(i.*lp)-sum(lf(i+1));
W(c)=sum(i.*w);
end
c=c+1;
i(10)=i(10)+1;
i(11:11)=0;
k=sum(i);
P(c)=sum(log(XY-k+1:XY))+(XY-k)*lp0+sum(i.*lp)-sum(lf(i+1));
W(c)=sum(i.*w);
end
c=c+1;
i(9)=i(9)+1;
i(10:11)=0;
k=sum(i);
P(c)=sum(log(XY-k+1:XY))+(XY-k)*lp0+sum(i.*lp)-sum(lf(i+1));
W(c)=sum(i.*w);
end
c=c+1;
i(8)=i(8)+1;
i(9:11)=0;
k=sum(i);
P(c)=sum(log(XY-k+1:XY))+(XY-k)*lp0+sum(i.*lp)-sum(lf(i+1));
W(c)=sum(i.*w);
end
c=c+1;
i(7)=i(7)+1;
i(8:11)=0;
k=sum(i);
P(c)=sum(log(XY-k+1:XY))+(XY-k)*lp0+sum(i.*lp)-sum(lf(i+1));
W(c)=sum(i.*w);
end
c=c+1;
i(6)=i(6)+1;
i(7:11)=0;
k=sum(i);
P(c)=sum(log(XY-k+1:XY))+(XY-k)*lp0+sum(i.*lp)-sum(lf(i+1));
W(c)=sum(i.*w);
end
c=c+1;
i(5)=i(5)+1;
i(6:11)=0;
k=sum(i);
P(c)=sum(log(XY-k+1:XY))+(XY-k)*lp0+sum(i.*lp)-sum(lf(i+1));
W(c)=sum(i.*w);
end
c=c+1;
i(4)=i(4)+1;
i(5:11)=0;
k=sum(i);
P(c)=sum(log(XY-k+1:XY))+(XY-k)*lp0+sum(i.*lp)-sum(lf(i+1));
W(c)=sum(i.*w);
end
c=c+1;
i(3)=i(3)+1;
i(4:11)=0;
k=sum(i);
P(c)=sum(log(XY-k+1:XY))+(XY-k)*lp0+sum(i.*lp)-sum(lf(i+1));
W(c)=sum(i.*w);
end
c=c+1;
i(2)=i(2)+1;
i(3:11)=0;
k=sum(i);
P(c)=sum(log(XY-k+1:XY))+(XY-k)*lp0+sum(i.*lp)-sum(lf(i+1));
W(c)=sum(i.*w);
end
c=c+1;
i(1)=i(1)+1;
i(2:10)=0;
k=sum(i);
P(c)=sum(log(XY-k+1:XY))+(XY-k)*lp0+sum(i.*lp)-sum(lf(i+1));
W(c)=sum(i.*w);
endThere’s no documentation, not in the PHP. Not in the Octave wrapper. Certainly not in the generated code. The one letter variable and function names are pretty standard, all the way through, in both the PHP and the Octave. The intern is long gone, and nobody has touched the code in that time. No one is entirely sure exactly what the gadget does, but users use it and have expectations about how it should work. So when Pierre was tasked with migrating this to the cloud, he did the best he could: they spun up a server, installed Octave and PHP, copied over the scripts, setup the Cron jobs, and pushed the button.
The code was written in 2007, and hadn’t broken once in that time. And once the Cron job fired… it kept right on working. As Pierre says: “It hasn’t broken in 13 years so there’s no need to fix it now. Not that we could.”
The real WTF, then, isn’t the Rube Goldbergian process. It isn’t the cloud transition. It isn’t even handing an important business task to an intern. The real WTF is a statement of awe, because our anonymous intern delivered a piece of software which has been running, error free, for 13 years and delivers real value to the end users, so much so that it had to get translated to an entirely new environment. And it just keeps humming along.
There are people who have spent their entire career in this industry that can’t claim to have accomplished what this intern did, so congratulations, anonymous intern.
[Advertisement]
ProGet supports your applications, Docker containers, and third-party packages, allowing you to enforce quality standards across all components. Download and see how!
|
Метки: CodeSOD |
CodeSOD: Exceptoinal Spelling |
Object-oriented languages often let you implement your own exception classes to hook into their structured exception handling. This is, usually, a good thing: you create your own custom types for your various errors, which makes various exception states more clear. PebkacException is more useful than just Exception, and you can build catch blocks specific to your application/API needs.
But there’s a dark side to this feature. People want to hook functionality into their exception objects. Instead of just using them as a signal to announce a failure state, they slap features into them, like exceptions which automatically self log.
Muriel discovered this PHP SelfLoggingException object when trying to throw an exception caused the application to crash with a stack overflow.
class SelfLoggingException extends Exception {
protected $cause = null;
public function __construct($message, $cause)
{
if (!is_null($cause)) {
$this->cause = $cause;
}
parent::__construct($message, 0, null);
$logger = LoggerProvider::get('exceptoin');
$logger->warn($message);
}
public function get_cause()
{
if ($this->cause) {
return $this->cause;
}
return $this->getPrevious();
}
public function get_root_cause()
{
$root = $this;
$root_found = false;
while (!$root_found) {
if ($root instanceof SelfLoggingException) {
if ($root->get_cause()) {
$root = $root->get_cause();
} else {
$root_found = true;
}
} else {
if ($root->getPrevious()) {
$root = $root->getPrevious();
} else {
$root_found = true;
}
}
}
}
}There’s a lot going wrong here. For starters, the Exception base class accepts a $previous parameter in their constructor, which is the intended way to build a chain of exceptions. Instead of just, y’know, using that built in functionality, this class re-implements it with a $cause property. This has the result of making get_root_cause extra complicated, since now it has to be smart enough to walk two possible chains- back up a series of $causes, or getPrevious calls.
The real WTF, though, is logging from inside of an exception object. Logging is how you handle an exception. By putting behavior into an exception object, you’re reducing the utility of your catch blocks- what exactly does it mean to catch a SelfLoggingException? You’ll need to wander up the cause chain to understand what went wrong, meaning your catch block doesn’t get to do the job. Essentially, everything this class does just makes exception handling worse.
There’s another reason to not put behavior in your exception objects, though, which is illustrated in this code. Note this line: $logger = LoggerProvider::get('exceptoin');, which has exceptoinal spelling in it. There is no exceptoin LoggerProvider configured, so this get fails. Any attempt to construct a SelfLoggingException would cause a stack overflow as accessing the logger failed.
Since then, Muriel has “fixed” this class, which is to say she’s corrected the typo. The SelfLoggingException no longer causes a crash. It is still in use.
[Advertisement]
ProGet supports your applications, Docker containers, and third-party packages, allowing you to enforce quality standards across all components. Download and see how!
|
Метки: CodeSOD |
CodeSOD: Microsoft's English Pluralization Service |
Despite founding The Daily WTF more than fifteen years ago, I still find myself astonished and perplexed by the curious perversions in information technology that you all send in. These days, I spend most of my time doing "CEO of Inedo stuff", which means I don't get to code that much. And when I do, it's usually working with the beautiful, completely WTF- and bug-free code that our that our world-class engineers create.
I mention this, because when I come across TDWTF-worthy code on my own, in the wild, it's a very special occasion. And today, I'm excited to share with you one of the worst pieces of code I've seen in a very long time: EnglishPluralizationServices.cs
Anyone even remotely familiar with the English language knows that pluralization is hard, and not exactly something that should be generalized in library... let alone Microsoft's most strategic programming asset of the past two decades. And yet, despite that:
internal class EnglishPluralizationService : PluralizationService, ICustomPluralizationMapping
{
private BidirectionalDictionary _userDictionary;
private StringBidirectionalDictionary _irregularPluralsPluralizationService;
private StringBidirectionalDictionary _assimilatedClassicalInflectionPluralizationService;
private StringBidirectionalDictionary _oSuffixPluralizationService;
private StringBidirectionalDictionary _classicalInflectionPluralizationService;
private StringBidirectionalDictionary _irregularVerbPluralizationService;
private StringBidirectionalDictionary _wordsEndingWithSePluralizationService;
private StringBidirectionalDictionary _wordsEndingWithSisPluralizationService;
private StringBidirectionalDictionary _wordsEndingWithSusPluralizationService;
private StringBidirectionalDictionary _wordsEndingWithInxAnxYnxPluralizationService;
private List _knownSingluarWords;
private List _knownPluralWords;
private string[] _uninflectiveSuffixList =
new string[] { "fish", "ois", "sheep", "deer", "pos", "itis", "ism" };
private string[] _uninflectiveWordList = new string[] {
"jackanapes", "species", "corps", "mackerel", "swine", "debris", "measles",
"trout", "diabetes", "mews", "tuna", "djinn", "mumps", "whiting", "eland",
"news", "wildebeest", "elk", "pincers", "police", "hair", "ice", "chaos",
"milk", "cotton", "pneumonoultramicroscopicsilicovolcanoconiosis",
"information", "aircraft", "scabies", "traffic", "corn", "millet", "rice",
"hay", "----", "tobacco", "cabbage", "okra", "broccoli", "asparagus",
"lettuce", "beef", "pork", "venison", "mutton", "cattle", "offspring",
"molasses", "shambles", "shingles" }; I'm not sure what's more baffling. Is it the fact that someone knew enough about language constructs to use words like uninflective or assimilatedClassicalInflection, yet didn't know enough to realize that automatic pluralization is impossible? Or perhaps, the fact that the same engineer thought that an Entity Framework user might not only have a database table named jackanapes or wildebeest, but would care about proper, automatic pluralization?
This code should never have been written, clearly. But as bad of an idea this all is, its implementation is just plain wrong. It's been a while since I've studied my sixth-grade vocabulary words, but I'm pretty sure that most of these are not irregular plurals at all:
private Dictionary _irregularPluralsDictionary =
new Dictionary()
{
{"brother", "brothers"}, {"child", "children"},
{"cow", "cows"}, {"ephemeris", "ephemerides"}, {"genie", "genies"},
{"money", "moneys"}, {"mongoose", "mongooses"}, {"mythos", "mythoi"},
{"octopus", "octopuses"}, {"ox", "oxen"}, {"soliloquy", "soliloquies"},
{"trilby", "trilbys"}, {"crisis", "crises"}, {"synopsis","synopses"},
{"rose", "roses"}, {"gas","gases"}, {"bus", "buses"},
{"axis", "axes"},{"memo", "memos"}, {"casino","casinos"},
{"silo", "silos"},{"stereo", "stereos"}, {"studio","studios"},
{"lens", "lenses"}, {"alias","aliases"},
{"pie","pies"}, {"corpus","corpora"},
{"viscus", "viscera"},{"hippopotamus", "hippopotami"}, {"trace", "traces"},
{"person", "people"}, {"chili", "chilies"}, {"analysis", "analyses"},
{"basis", "bases"}, {"neurosis", "neuroses"}, {"oasis", "oases"},
{"synthesis", "syntheses"}, {"thesis", "theses"}, {"change", "changes"},
{"lie", "lies"}, {"calorie", "calories"}, {"freebie", "freebies"}, {"case", "cases"},
{"house", "houses"}, {"valve", "valves"}, {"cloth", "clothes"}, {"tie", "ties"},
{"movie", "movies"}, {"bonus", "bonuses"}, {"specimen", "specimens"}
};In fact, the "just add an s to make it plural" rule is perhaps the ultimate starting point for pluralization. Words like "pie" and "cow" are not irregular at all, because you just add an "s" to make them "pies" and "cows". And this brings us to our next questions.
Where exactly did this list of not-actually-irregular plurals come from? It was obviously copy/pasted from somewhere, right? An English textbook? Like, perhaps the the teachers' edition? You know, where there's like a handout with pictures of a cow, then a bunch of cows, and a line below it where you write the word? Did the engineer… just use that page?
Up next on our tour is actual the actual pluralization logic, which is handled by InternalPluralize method. It's not nearly as simple as using those dictionaries. Consider this snippet from within that method:
// handle the word that do not inflect in the plural form
if (IsUninflective(suffixWord))
{
return prefixWord + suffixWord;
}Quick aside: please take a moment to appreciate the irony of improperly capitalization in the comment. "the word that do not inflect". Does this mean the author is not a native English speaker? Was this thing outsourced to Kerbleckistan? Does that even make sense to do for… English? Or was this just a typo?
I digress. Let's keep digging into IsUninflective.
// handle irregular inflections for common suffixes, e.g. "mouse" -> "mice"
if (PluralizationServiceUtil.TryInflectOnSuffixInWord(suffixWord,
new List() { "louse", "mouse" },
(s) => s.Remove(s.Length - 4, 4) + "ice", this.Culture, out newSuffixWord))
{
return prefixWord + newSuffixWord;
}
if (PluralizationServiceUtil.TryInflectOnSuffixInWord(suffixWord,
new List() { "tooth" },
(s) => s.Remove(s.Length - 4, 4) + "eeth", this.Culture, out newSuffixWord))
{
return prefixWord + newSuffixWord;
}
if (PluralizationServiceUtil.TryInflectOnSuffixInWord(suffixWord,
new List() { "goose" },
(s) => s.Remove(s.Length - 4, 4) + "eese", this.Culture, out newSuffixWord))
{
return prefixWord + newSuffixWord;
}
if (PluralizationServiceUtil.TryInflectOnSuffixInWord(suffixWord,
new List() { "foot" },
(s) => s.Remove(s.Length - 3, 3) + "eet", this.Culture, out newSuffixWord))
{
return prefixWord + newSuffixWord;
} It seems that in addition to not knowing English, the author of this class that's used within the .NET framework doesn't really know C# very well? And certainly not how string comparison works in .NET. On the bright side, is that a test for "ese"? Did... our engineer figure out that there are some basic pluralization patterns you can apply? Maybe there's hope after all!
private bool IsUninflective(string word)
{
EDesignUtil.CheckArgumentNull(word, "word");
if (PluralizationServiceUtil.DoesWordContainSuffix(word, _uninflectiveSuffixList, this.Culture)
|| (!word.ToLower(this.Culture).Equals(word) && word.EndsWith("ese", false, this.Culture))
|| this._uninflectiveWordList.Contains(word.ToLowerInvariant()))
{
return true;
}
else
{
return false;
}
} On the plus side, this "common" suffixes logic ensures that both titmouse and snaggletooth are properly pluralized.
[Advertisement]
BuildMaster allows you to create a self-service release management platform that allows different teams to manage their applications. Explore how!
https://thedailywtf.com/articles/microsoft-s-english-pluralization-service
|
Метки: CodeSOD |