CodeSOD: Nullable Booleans |
Austin's team received a bug report. When their C# application tried to load certain settings, it would crash. Now, since this was a pretty severe issue in their production software, impacting a fair number of customers, everyone on the team dove in.
It didn't take Austin long to spot the underlying problem, which isn't quite the WTF.
bool? setting;
public static bool GetSetting(){
return (bool)setting;
}
setting
is defined as nullable, so when attempting to cast to bool
, a null value will fail. Theoretically, this should have been caught by testing, but at least the fix was easy. Austin simply added a coalescing operator to the return line: return setting ?? false
.
While Austin sent out a pull request, a second pull request came in. This one was from Austin's boss, who was far more experienced- and who had been gifted the ability to approve their own pull requests. So this is what they approved, a change in where GetSetting
is called:
try{
checkbox.Checked = Foo.GetSetting();
}
catch{}
While Austin took a moment to understand the root cause of the issue, his boss took the straight line path: "this line throws an exception, so let's just make the exception go away".
After some convincing, the team agreed that the boss's commit should be rolled back and an actual proper null handling was a better choice. That didn't stop the boss from grumbling that it was an emergency, and it was more important to get it fixed fast than fixed right. Austin didn't point out that he submitted his pull request first.
Метки: CodeSOD |
CodeSOD: Observing the Observer |
In the endless quest to get asynchronous operations right, we're constantly changing approaches. From callbacks, to promises, to awaits, to observables. Observables are pretty straight forward: we observe something, like a socket or a HTTP request, and when something happens, we trigger some callbacks. In a lightly pseudocode version, it might look something like:
requestFactory.postFormRequest(url).subscribe(
resp => myResponseHandler(resp),
err => myErrorHandler(err),
() => myRequestCompleteHandler()
)
It's cleaner than pure callback hell, but conceptually similar. The key advantage to observables is that they work exactly the same way on objects that may emit multiple events, like a WebSocket. Each time a message arrives on the socket, we can simply re-execute the handlers for our subscription.
I lay out this background, because Lucas has a co-worker who doesn't quite get it. Because every time they need to make a request, they follow this pattern:
return new Observable(observer => {
this._request.new_postForm(this._app.apiUrl() + requestUrl + paramsUrl, formData).subscribe(
leaseFileRes => observer.next(leaseFileRes),
error => observer.error(error),
() => observer.complete()
);
});
This creates two observables. The first, created by this bit, this._request.new_postForm
, is a request factory. It creates a request, adds some error handling code- code which uses window.alert
to raise errors- and returns an observable.
In this code example, we create a second outer observable, which is what subscribes to the request- the observer.next
, observer.error
, and observer.complete
do those steps. So the outer observable is just a do-nothing observer.
Why? I'll let Lucas explain:
The reason for this is that, sometimes, it wants to return a particular field from the response, and not all of it, and it was then copy-pasted all over. Or maybe someone doesn't understand asynchronous requests. Probably both.
Метки: CodeSOD |
CodeSOD: Counting Answers |
Lacy's co-worker needed to collect groups of strings into "clusters"- literally arrays of arrays of strings. Of great importance was knowing how many groups were in each cluster.
Making this more complicated, there was an array list of clusters. Obviously, there's a code smell just in this data organization- ArrayList>
is not a healthy sounding type name. There's probably a better way to express that.
That said, the data structure is pretty easy to understand: I have an outer ArrayList
. Each item in the ArrayList
is another ArrayList
(called a cluster), and each one of those contains an array of strings (called an answer, in their application domain).
So, here's the question: for a set of clusters, what is the largest number of answers contained in any single cluster?
There's an obvious solution to this- it's a pretty basic max
problem. There's an obviously wrong solution. Guess which one Lacy found in the codebase?
int counter = 0;
int max_cluster_size = 0;
for (ArrayList cluster : clusters) {
for (String[] item : cluster) {
counter++;
}
if (counter > max_cluster_size) {
max_cluster_size = counter;
}
counter = 0;
}
Now, on one hand, this is simple- replace the inner for loop with a if (cluster.size() > max_cluster_size) max_cluster_size = cluster.size()
. And I'm sure there's some even more readable Java streams way to do this.
And with that in mind, this is barely a WTF, but what I find interesting here is that we can infer what actually happened. Because here's what I think: once upon a time, someone misunderstood the requirements (maybe the developer, maybe the person writing the requirements). At one time, they wanted to base this off of how many strings were in the answer. Something like:
for (String[] item : cluster) {
counter += item.length;
}
This was wrong, and so someone decided to make the minimal change to fix the code: they turned the in-place addition into an increment. Minimal changes, and it certainly works. But it lacks any sense of the context or purpose of the code. It's a completely understandable change, but it's also hinting at so many other bad choices, lurking just off camera, that we can't see.
Метки: CodeSOD |
CodeSOD: Uniquely Unique |
Giles's company has a hard time with doing things in the database.
In today's example, they attempt the very challenging task of generating unique IDs in a SQL Server database. Now, what you're about to see follows the basic pattern of "generate a random number and see if it's already been used", which is a fairly common anti-pattern, but it's managed to do this in some of the worst ways I've ever seen. And it can't even hide behind the defense of being written a long time ago- it's a handful of years old.
Comments to this C# code have been added by Giles, and no, there were no comments.
protected void AddBlankRowToDatabase()
{
//This - in effect - calls a stored procedure which has been carefully designed to work around the issue with SPs and
//SQL injection - i.e. in theory using a stored procedure could help prevent it from happening, which is obviously
//a problem, so this SP allows it once again.
//also note that this actually fetches *all* machines for the currently selected customer,
//so is basically calling "select * from ..." and creating a list of objects to represent them.
List allMachine = MachineManager.GetByCriteria("CustomerId=" + Convert.ToString(Session["ActiveCustomerId"]), "");
//having gone to all that trouble, let us see how many machines there are, and increment this since we are going to
//add one
int machineCount = allMachine.Count + 1;
//Create a new object representing a machine.
Machine machine = new Machine();
//we will definitely want random numbers, as that is the correct way to ensure uniqueness.
Random _rng = new Random();
//new numbers must start with 999 for no reason that is documented or known to anyone.
string paddedString, padding = "999";
//OK, random number please. We want it to be 3 chars, but instead of all that zero-padding nonsense, let's just
//ensure this by starting from 100.
int RandomId = _rng.Next(100, 999);
//append our random number to our "999"
paddedString = padding + RandomId;
RandomId = Convert.ToInt32(paddedString);
//now here's the big brain part; we don't know if the machine ID already exists, so we check if it does!
bool machineIdNotExists = true;
while (machineIdNotExists)
{
//Get any machine with that ID, again via this garbage SP system.
List machineExists = MachineManager.GetByCriteria("MachineId=" + RandomId, "");
if (machineExists.Count > 0)
{
//ah well, let's try *another* random 'number'
RandomId = _rng.Next(100, 999);
paddedString = padding + RandomId;
RandomId = Convert.ToInt32(paddedString);
machineIdNotExists = true;
}
else
{
//ok, so the machine does not not Exist, we have a new number!
machineIdNotExists = false;
}
}
//Good stuff, ensure we use the ID we found
machine.MachineId = RandomId;
//let's use the count of machines we arrived at earlier to set a temporary title for the machine....
//yes, this is the *only* use of the count, which we worked out by selecting an entire list
//of DB rows, constructing a bunch of objects and then *counting* them.
machine.Title = "New Machine " + machineCount;
////////
//Snip a bunch of other boring property setting
/////////
//Now we save the created machine.
//This internally takes the Machine Object and (manually - no EF or similar)
//pulls its properties into params for another stored proc, which adds the entry to the DB.
MachineManager.InsertMachine(machine);
//Now run through the entire grid of all machines, and save them all individually.
//Even though you haven't amended them.
SaveAllRecords();
//Reload the page, which will refresh the grid to show our new machine!
Response.Redirect("MachinesAdmin.aspx?RecordSaved=Yes");
}
SQL Injection by stored procedure, fetching entire sets from the database when you need a count. Forcing entire front-end page refreshes via redirect. Mysterious and very not random padding.
Every choice made here was a bad choice.
Метки: CodeSOD |
CodeSOD: Capital Irregularities |
Carolyn's company has a bunch of stringly-typed enums. That's already a problem, even in Python, but the other problem is that they need to display these in a human readable format. So, "ProductCategory"
needs to become "Product Category"
. Now, it's true that every one of these stringly typed enums follows the PascalCase convention. It's also true that the list of them is constantly growing.
So this is the method someone wrote for formatting:
def format_text(data):
field = data["field_name"]
if field == "ProductCategory":
field = "Product Category"
elif field == "ProductPrice":
field = "Product Price"
elif field == "ProductName":
field = "Product Name"
…
elif field == "UnknownField":
field = "Unknown Field"
return field
It's unclear how many fields there actually are from the submission, but suffice to say, it's a lot. The fact that "field_name" is a dictionary key hints at a deeper WTF, like an inner-platform style homebrew ORM or something similar, but I have no real evidence about how the code is actually used.
But looking at this code, it makes me wish there was some way to identify the regularities in the input strings, some sort of expression that could identify substrings that I could modify according to the regular pattern. Some sort of regular expression if you will.
Nah, something like that would probably make my code to cryptic to read, and probably just give me extra problems.
Метки: CodeSOD |
CodeSOD: Exceptionally TF |
Steve's predecessor knows: sometimes, stuff happens that makes you go "WTF". While Steve was reviewing some inherited PHP code, he discovered that this predecessor had built some code to handle those situations.
namespace WtfInc;
##use \Exception as Exception;
class WTFException extends \Exception
{
public function __construct($message = null, $code = null)
{
if (! $message) {
$message = "WTF!?";
} else {
$message = "WTF!? " . $message;
}
parent::__construct($message, $code);
}
}
Now, Steve tracked down all the places it was actually used, which was not zero, but was only one location:
if ($contents === false) {
throw new WTFException("Couldn't read the yaml");
}
This confirms what we all already knew: YAML is TRWTF.
Метки: CodeSOD |
CodeSOD: Fetching Transactions |
When companies reinvent their own accounting software, they usually start from the (reasonable) position of just mirroring basic accounting processes. You have transactions, for an amount, and then tagged with information about what the transaction actually represents. So, for example, if you wanted to find all the transactions which represent tax paid, you'd need to filter on some metadata and then sum up the amounts.
It quickly gets more complicated. In some organizations, that complexity keeps growing, as it turns out that each department uses slightly different codes, the rules change over time, this central accounting database gradually eats older databases which had wildly different rules. Before long, you end up with a database so krufty that it's a miracle SQL Server doesn't just up and quit.
That's the situation Giles W found himself in. What follows is an example query, which exists to answer the simple question: on 20th December, 2013, how much tax was paid across all transactions? For most database designs, that might be an expensive query, but hopefully a simple query. For this one… well… they may need to do some redesigning. Note the horizontal scrolling on today's code, there was no good place for me to add linebreaks for readability.
SELECT SUM(Tax_Paid) AS Tax_Paid FROM ( SELECT SUM(te.tax1_amount + te.tax2_amount) AS Tax_Paid FROM transactions t WITH (NOLOCK) LEFT JOIN transaction_ext te WITH (NOLOCK) on t.transaction_id = te.transaction_id and t.location_id = te.location_id LEFT JOIN sale_invoices si WITH (NOLOCK) on te.sale_invoice_no = si.invoice_no and te.location_id = si.location_id WHERE t.Start_Date BETWEEN '2013-12-20' AND '2013-12-21' AND t.Start_Time BETWEEN '2013-12-20 06:00:00' AND '2013-12-21 05:59:59' AND t.Reversed <> 1 AND t.location_id = 40123 AND t.Sublocation = 0 AND t.Cashier > '' AND t.transaction_type in ('NPROD','NCRADJ','PROD','CRADJ','NEW','TAXEXEM','COUPON','COUPONS','TAXFREE') UNION ALL SELECT SUM((te.tax1_amount + te.tax2_amount) * t.quantity) AS Tax_Paid FROM other_transactions t WITH (NOLOCK) LEFT JOIN ot_extention te WITH (NOLOCK) on t.transaction_id = te.transaction_id and t.location_id = te.location_id LEFT JOIN sale_invoices si WITH (NOLOCK) on te.sale_invoice_no = si.invoice_no and te.location_id = si.location_id WHERE t.Transaction_Date BETWEEN '2013-12-20' AND '2013-12-21' AND t.Transaction_Time BETWEEN '2013-12-20 06:00:00' AND '2013-12-21 05:59:59' AND t.location_id = 40123 AND t.Sublocation = 0 AND t.Cashier > '' AND t.transaction_type in ('SCOFFER','SCPRIV','C_SALE','COUPON','COUPONS','TAXEXEM','TAXFREE') UNION ALL SELECT SUM(te.tax1_amount + te.tax2_amount) AS Tax_Paid FROM transactions t WITH (NOLOCK) LEFT JOIN transaction_ext te WITH (NOLOCK) on t.transaction_id = te.transaction_id and t.location_id = te.location_id LEFT JOIN sale_invoices si WITH (NOLOCK) on te.sale_invoice_no = si.invoice_no and te.location_id = si.location_id LEFT JOIN tab_accounts ta WITH (NOLOCK) on te.sale_invoice_no = ta.sale_invoice_no and ta.location_id = ta.location_id WHERE t.Start_Date <= '2013-12-20' AND t.Start_Time <= '2013-12-20 06:00:00' AND t.Reversed <> 1 AND t.location_id = 40123 AND t.Sublocation = 0 AND t.Cashier > '' AND t.transaction_type in ('NPROD','NCRADJ','PROD','CRADJ','NEW','TAXEXEM','COUPON','COUPONS','TAXFREE') AND ta.payment_cancelled <> 1 AND te.group_sale_no in (SELECT sp.group_sale_no FROM Split_Payments sp WITH (NOLOCK) INNER JOIN Sale_Invoices si WITH (NOLOCK) on sp.location_id = si.location_id and sp.group_sale_no = si.invoice_no WHERE sp.Transaction_Time BETWEEN '2013-12-20 06:00:00' AND '2013-12-21 05:59:59' AND si.date_issued < '2013-12-20 06:00:00' AND sp.location_id = 40123 AND sp.Sublocation = 0 AND Transaction_Amount >= 0 GROUP BY sp.group_sale_no) UNION ALL SELECT SUM((te.tax1_amount + te.tax2_amount)* t.quantity) AS Tax_Paid FROM other_transactions t WITH (NOLOCK) LEFT JOIN ot_extention te WITH (NOLOCK) on t.transaction_id = te.transaction_id and t.location_id = te.location_id LEFT JOIN sale_invoices si WITH (NOLOCK) on te.sale_invoice_no = si.invoice_no and te.location_id = si.location_id LEFT JOIN tab_accounts ta WITH (NOLOCK) on te.sale_invoice_no = ta.sale_invoice_no and ta.location_id = ta.location_id WHERE t.Transaction_Date <= '2013-12-20' AND t.Transaction_Time <= '2013-12-20 06:00:00' AND t.location_id = 40123 AND t.sublocation = 0 AND t.Cashier > '' AND t.transaction_type in ('SCOFFER','SCPRIV','C_SALE','COUPON','COUPONS','TAXEXEM','TAXFREE') AND ta.payment_cancelled <> 1 AND te.group_sale_no in (SELECT sp.group_sale_no FROM Split_Payments sp WITH (NOLOCK) INNER JOIN Sale_Invoices si on sp.location_id = si.location_id and sp.group_sale_no = si.invoice_no WHERE sp.Transaction_Time BETWEEN '2013-12-20 06:00:00' AND '2013-12-21 05:59:59' AND si.date_issued < '2013-12-20 06:00:00' AND sp.location_id = 40123 AND sp.Sublocation = 0 AND Transaction_Amount >= 0 GROUP BY sp.group_sale_no) UNION ALL SELECT -1 * SUM(te.tax1_amount + te.tax2_amount) AS Tax_Paid FROM transactions t WITH (NOLOCK) LEFT JOIN transaction_ext te WITH (NOLOCK) on t.transaction_id = te.transaction_id and t.location_id = te.location_id LEFT JOIN sale_invoices si WITH (NOLOCK) on te.sale_invoice_no = si.invoice_no and te.location_id = si.location_id LEFT JOIN (SELECT transaction_amount=sum(transaction_amount), transaction_time=min(transaction_time), location_id, group_sale_no FROM Split_Payments GROUP BY location_id, group_sale_no) as sp on sp.group_sale_no = te.group_sale_no and sp.location_id = te.location_id WHERE t.Start_Date BETWEEN '2013-12-20' AND '2013-12-21' AND t.Start_Time BETWEEN '2013-12-20 06:00:00' AND '2013-12-21 05:59:59' AND t.Reversed <> 1 AND t.location_id = 40123 AND t.Sublocation = 0 AND t.Cashier > '' AND t.payment_type = 222 AND t.transaction_type in ('NPROD','NCRADJ','PROD','CRADJ','NEW','TAXEXEM','COUPON','COUPONS','TAXFREE') AND (sp.group_sale_no is NULL OR (sp.transaction_time > '2013-12-21 05:59:59' AND sp.transaction_amount >= 0)) UNION ALL SELECT -1 * SUM((te.tax1_amount + te.tax2_amount) * t.quantity) AS Tax_Paid FROM other_transactions t WITH (NOLOCK) LEFT JOIN ot_extention te WITH (NOLOCK) on t.transaction_id = te.transaction_id and t.location_id = te.location_id LEFT JOIN sale_invoices si WITH (NOLOCK) on te.sale_invoice_no = si.invoice_no and te.location_id = si.location_id LEFT JOIN (SELECT transaction_amount=sum(transaction_amount), transaction_time=min(transaction_time), location_id, group_sale_no FROM Split_Payments GROUP BY location_id, group_sale_no) as sp on sp.group_sale_no = te.group_sale_no and sp.location_id = te.location_id WHERE t.Transaction_Date BETWEEN '2013-12-20' AND '2013-12-21' AND t.Transaction_Time BETWEEN '2013-12-20 06:00:00' AND '2013-12-21 05:59:59' AND t.location_id = 40123 AND t.Sublocation = 0 AND t.Cashier > '' AND t.payment_type = 222 AND (t.transaction_type in ('SCOFFER','SCPRIV','C_SALE','COUPON','COUPONS','TAXEXEM','TAXFREE') OR t.transaction_type = 'DEPOSIT' and (te.comment IS NULL or te.comment = '')) AND (sp.group_sale_no is NULL OR (sp.transaction_time > '2013-12-21 05:59:59' AND sp.transaction_amount >= 0)) UNION ALL SELECT sum((te.tax1_amount + te.tax2_amount) * t.quantity) as Tax_Paid FROM other_transactions t WITH (NOLOCK) LEFT JOIN ot_extention te WITH (NOLOCK) on t.transaction_id = te.transaction_id and t.location_id = te.location_id INNER JOIN (SELECT distinct te.sale_invoice_no FROM other_transactions t WITH (NOLOCK) LEFT JOIN ot_extention te WITH (NOLOCK) on t.transaction_id = te.transaction_id and
t.location_id = te.location_id LEFT JOIN sale_invoices si WITH (NOLOCK) on te.sale_invoice_no = si.invoice_no and te.location_id = si.location_id LEFT JOIN (SELECT transaction_amount=sum(transaction_amount), transaction_time=min(transaction_time), location_id,
group_sale_no FROM Split_Payments GROUP BY location_id, group_sale_no) as sp on sp.group_sale_no = te.group_sale_no and sp.location_id = te.location_id WHERE t.Transaction_Date BETWEEN '2013-12-20' AND '2013-12-21' AND t.Transaction_Time BETWEEN '2013-12-20 06:00:00'
AND '2013-12-21 05:59:59' AND t.location_id = 40123 AND t.Sublocation = 0 AND t.Cashier > '' AND t.payment_type = 222 AND t.transaction_type = 'CANCTAB' AND (sp.group_sale_no is NULL OR (sp.transaction_time > '2013-12-21 05:59:59' AND sp.transaction_amount >= 0)) ) s ON
te.sale_invoice_no = s.sale_invoice_no WHERE t.Transaction_Date BETWEEN '2013-12-20' AND '2013-12-21' AND t.Transaction_Time BETWEEN '2013-12-20 06:00:00' AND '2013-12-21 05:59:59' AND t.transaction_type <> 'CANCTAB' AND t.location_id = 40123 AND t.Sublocation = 0 AND
t.Cashier > '' UNION ALL SELECT sum(te.tax1_amount + te.tax2_amount) as Tax_Paid FROM transactions t WITH (NOLOCK) LEFT JOIN transaction_ext te WITH (NOLOCK) on t.transaction_id = te.transaction_id and t.location_id = te.location_id INNER JOIN (SELECT distinct
te.sale_invoice_no FROM other_transactions t WITH (NOLOCK) LEFT JOIN ot_extention te WITH (NOLOCK) on t.transaction_id = te.transaction_id and t.location_id = te.location_id LEFT JOIN sale_invoices si WITH (NOLOCK) on te.sale_invoice_no = si.invoice_no and
te.location_id = si.location_id LEFT JOIN (SELECT transaction_amount=sum(transaction_amount), transaction_time=min(transaction_time), location_id, group_sale_no FROM Split_Payments GROUP BY location_id, group_sale_no) as sp on sp.group_sale_no = te.group_sale_no and
sp.location_id = te.location_id WHERE t.Transaction_Date BETWEEN '2013-12-20' AND '2013-12-21' AND t.Transaction_Time BETWEEN '2013-12-20 06:00:00' AND '2013-12-21 05:59:59' AND t.location_id = 40123 AND t.Sublocation = 0 AND t.Cashier > '' AND t.payment_type = 222 AND t.transaction_type = 'CANCTAB' AND (sp.group_sale_no is NULL OR (sp.transaction_time > '2013-12-21 05:59:59' AND sp.transaction_amount >= 0)) ) s ON te.sale_invoice_no = s.sale_invoice_no WHERE t.Start_Date BETWEEN '2013-12-20' AND '2013-12-21' AND t.Start_Time BETWEEN '2013-12-20 06:00:00' AND '2013-12-21 05:59:59' AND t.Reversed <> 1 AND t.location_id = 40123 AND t.Sublocation = 0 AND t.Cashier > '' UNION ALL SELECT SUM((te.tax1_amount + te.tax2_amount) * t.quantity) AS Tax_Paid FROM other_transactions t WITH (NOLOCK) LEFT JOIN ot_extention te WITH (NOLOCK) on t.transaction_id = te.transaction_id and t.location_id = te.location_id LEFT JOIN sale_invoices si WITH (NOLOCK) on te.sale_invoice_no = si.invoice_no and te.location_id = si.location_id WHERE t.Transaction_Date BETWEEN '2013-12-20' AND '2013-12-21' AND t.Transaction_Time BETWEEN '2013-12-20 06:00:00' AND '2013-12-21 05:59:59' AND t.location_id = 40123 AND t.Sublocation = 0 AND t.Cashier > '' AND t.transaction_type = 'REFUND' UNION ALL SELECT SUM((te.tax1_amount + te.tax2_amount) * t.quantity) AS Tax_Paid FROM other_transactions t WITH (NOLOCK) LEFT JOIN ot_extention te WITH (NOLOCK) on t.transaction_id = te.transaction_id and t.location_id = te.location_id LEFT JOIN sale_invoices si WITH (NOLOCK) on te.sale_invoice_no = si.invoice_no and te.location_id = si.location_id WHERE t.Transaction_Date BETWEEN '2013-12-20' AND '2013-12-21' AND t.Transaction_Time BETWEEN '2013-12-20 06:00:00' AND '2013-12-21 05:59:59' AND t.location_id = 40123 AND t.Sublocation = 0 AND t.Cashier > '' AND t.transaction_type = 'REVADJ' ) AS Taxes
"This," Giles writes, "is what you would see if you wanted to tinker with the query to account for some new transaction type. Presumably shortly before resigning."
Метки: CodeSOD |
CodeSOD: Annotated Private Members |
Shery sends us perfectly innocent looking Java class, marked up with some annotations. The goal of this class is to have an object which contains a list of names that is definitely not null. Let's see how we did.
@Data
@Builder
@NoArgsConstructor
@AllArgsConstructor
public class NamesHolder {
@NotNull
private List names;
}
So, the first four annotations are from the Lombok library. They're code generation tools that can generate most of your boiler plate for you.
@Data
is easy: it just generates getters and setters for every property. Note, these getters/setters don't do any validation, so this won't stop you from setting to null. It also creates a constructor with arguments for all properties.
Which, @AllArgsConstructor
also generates a constructor with arguments for all properties, so that's a curious choice.
Neither of these do anything to prevent the list from being initialized to null.
@NoArgsConstructor
does exactly what it implies, which is useful here, since that's a default constructor. It does not, however, initialize objects to non-null values.
Finally, @Builder
creates a static factory. This is common for classes with lots of properties which need to be initialized, e.g., ComplicatedClass instance = ComplicatedClass.builder().withProp(value).withOtherProp(value).build()
This is completely overkill for a class with only one property, and still hasn't done anything to prevent our list from being null.
But hey, there's that @NotNull
annotation on there. That must do something, right? Well, that isn't from the same library, that's from JavaX
, and it's meant to support serialization. The annotation is information for when serializing or deserializing data: a valid object can't be null.
The class in question here isn't getting serialized, it's just a lightweight container to pass some data between modules. So @NotNull
doesn't do anything in this case.
When you take into account all the autogenerated code in play here, while that list of names is declared private, it is very much public, and very much available to everybody to manipulate. There's no guarantee that it's not null, which was the entire purpose of this class.
Shery adds:
All the original programmer had to do is make sure our original list is not null, chuck it in a box marked 'NamesHolder' and send it on it's way. Instead they've made it as nullable as possible using 3rd party tools that they don't understand and aren't needed. Friends of mine recently contemplated giving up programming, fed up after having to deal with similar stuff for years, and may open a brewery instead. I may join them.
Метки: CodeSOD |
Error'd: Time is Time in Time and Your Time |
Shocked sharer Rob J. blurts "I feel like that voltage is a tad high."
On the Heisenbahn you either know where you're going, or how much it costs, but not both. Personenzug Christian K. chose the wrong train, as he now knows neither place nor price. "Seems that I should get a ticket from %1$@ to %2$@. I wonder how much that’ll be."
The anonymous OP titled this "Changing trains the German way". To appreciate it, it helps to know that Umsteigszeit is how one says "connection time" auf Deutsch. Zero minutes seems plenty generous if you're trying to connect to a train that left seven minutes before you arrived! Maybe someone more familiar with German rail schedules can explain.
Aussie Paul J. complains "Apparently Red can't be my favourite colour, and I can't have a cat called Ada..." It's a right dictatorship downunder.
Sad Bret finds himself unpopular with chatbots lately. He reckons "I guess the Customer Service Bot is having a smoke and doesn't want to deal."
Geniesse das Wochenende.
https://thedailywtf.com/articles/time-is-time-in-time-and-your-time
Метки: Error'd |
CodeSOD: Counting References |
If you're working in a research field, references matter- specifically, the citations made by your paper and the citations eventually made against yours. But when programming, references can be hard.
Dorothy is a scientist, and understands that code itself is a valuable artifact- it's not enough to just to get the solution, but the code itself needs to be maintanable and readable. So when her peers get into trouble, they frequently come to Dorothy to figure out why.
This Java code is one such example:
// convert image stack to correct architecture with scaling
if (impType != "GRAY32") {
ImageProcessor ip;
for (int i = 1; i <= testImage.getStackSize(); i++) {
ip = testImage.getStack().getProcessor(i);
if (impType == "GRAY16")
ip = ip.convertToShortProcessor(true);
else
ip = ip.convertToByte(true);
}
}
Without knowing the details, we can already see that there's some stringly-typed data. Java has had enums for a long time, and that'd be a much better way to manage these variations.
But that's boring style nonsense. testImage
is an ImagePlus
object, which contains a set of ImageProcessor
objects. An ImageProcessor
wraps an actual image, and has concrete subclasses for various kinds of ImageProcessors.
This code wants to iterate across all the ImageProcessor
s in an ImagePlus
and convert them to a new type. Here's the problem with this approach: the convertTo
methods don't modify the object in place: they return a new instance.
The developer has gotten a little lost in their references and the result is that they never update the object they're trying to update: they create a local copy and modify the local copy.
It's a pretty basic mistake, but it's the sort of thing that ends up eating a lot of time, because as you can imagine, nobody actually documents which methods work in place and which return copies, and as you can see they don't even consistently name methods which do the same thing- convertToShortProcessor
and convertToByte
.
The real WTF though, is that for loop. Array types with 1-based indexes? That's just unforgivable.
Метки: CodeSOD |
CodeSOD: Never Don't Stop Not Doing This |
It's not nothing to never write confusing English. And it doesn't never influence the code that we write. Don't fail to look at this anti-pattern from today's un-named submitter.
If Not port Is Nothing Then
portUnAvailable = False
End If
If the port isn't nothing, then the port isn't unavailable. That's… not untrue. But it is surprisingly confusing when you're reading it in a larger block of code. And, of course, this would be simpler to express as a boolean operation:
portUnAvailable = port is Nothing
Then again, without context, that might be wrong- perhaps portUnAvailable
gets set elsewhere and then changes only if Not port Is Nothing
. But let's assume that's not how this works, because that hints at a bigger WTF.
Do never don't avoid this pattern in your own code or writing.
https://thedailywtf.com/articles/never-don-t-stop-not-doing-this
Метки: CodeSOD |
A Slice of Spam |
In addition to being a developer, Beatrix W manages a few small email servers, which means she sometimes needs to evaluate the kinds of messages arriving and their origins, especially when they're suspicious. One such suspicious message arrived, complete with a few malicious links, and some hints of possibly being the start of a spear-phishing attack.
That was concerning, and as it turns out, the email came through a vendor who specializes in sending marketing emails- but the requested sort (or at least the sort where you got confused about which box to uncheck at checkout and accidentially signed yourself up for a newsletter). So Beatrix tracked down the contact form on the company website.
She filled out the form with a description of the issue. The form had a handy-dandy "Attachments" field, and the instructions said, "Attach the suspicious email with its full email headers." So, she copy/pasted the suspicious email, headers included, into a text file, and attached the text file. She reviewed her work, confirmed that the attachment had uploaded successfully, and then pushed "Send". A copy of her submission arrived in her inbox, attachment and all, so she filed it away and forgot about it.
Two weeks later, the vendor replied.
We were unable to complete your investigation of unwanted email because we did not have enough information. In order for us to address issues you may be experiencing with users of our services sending you unwanted, unsolicited, or otherwise problematic emails, it will be necessary for you to send us the full content of this message including the full headers.
Beatrix paused reading right there, and pulled up her email, and confirmed, yes, she had attached the email, complete with its headers. She went back to the vendor's email reply and continued reading:
Please note that due to security concerns we will not open attachments under any circumstance. You must provide any necessary information in plaintext in the body of your report.
At least they care about their security, if not yours. Though it does raise the question: why does their contact form have an attachments button if you shouldn't use it?
Метки: Feature Articles |
CodeSOD: Confessions of a Deep Copy |
While JavaScript (and TypeScript) may have conquered the world, those languages have… unusual conventions relative to some lower level languages you might encounter. I recently was having a debate with a C-guru, who lamented all the abstractions and promised that well written C-code would forever be faster, more compact, and easier to read that the equivalent code in a higher level language.
That may or may not be true (it's not), but I understand his point of view.
Lily comes from a background where she's used to writing lower level code, mostly C or C++ style languages. So when she tried to adapt to typescript, the idea that everything was a reference was inconvenient. An example of a thing which flummoxed her:
let inner = [1,2,3,4,5];
let outer = [];
outer.push(inner);
inner.length = 0; //oops, outer[0] is now an empty array
Since outer
contains a reference to inner
, changing inner
means that outer
also gets changed. That wasn't the behavior that Lily wanted, and she was new to JavaScript. So she did what any of us might do in that situation: she searched MDN for some sort of Array.prototype.deepClone()
method. She didn't find one, and was out of things to Google, so she just did her best and invented her own "deep clone" method for parsing some code.
private splitTokenByPhysicalEOL(): Array {
const ret: Array = [];
const cur: Token[] = [];
for (const token of this.tokenList) {
cur.push(token);
if (token.type === 'EOL') {
ret.push(JSON.parse(JSON.stringify(cur))); // ???
cur.length = 0;
}
}
if (cur.length) ret.push(cur);
return ret;
}
This code is part of a parser. Lily wanted to return an array of lines, where each line was an array of tokens for the parser to use. At the end of a line, she wanted to take the tokens parsed so far, put them into the result set, and then clear out cur
to restart the next line.
Now, since cur
was declared const
, she couldn't do cur = []
, which would create a new object and a new reference, so she did the cur.length = 0
trick to empty the array. But doing that modifies the array in place, which means all references to the array, including the ones in ret
, also get cleared.
Which brings us to this line: ret.push(JSON.parse(JSON.stringify(cur))); // ???
, which was Lily's attempt at a deep copy. Not a terrible attempt, honestly, given the things she didn't know and JavaScript's… idiosyncrasies. Turn the array into a JSON string, parse the JSON string. Voila, a copy! The comment is a helpful note from her mentor on the project, who suggested that there were better ways to deep copy in JavaScript, or avoid needing to copy at all by just spawning new references as needed.
Метки: CodeSOD |
Error'd: Horned Megafauna |
While we don't know the precise taxonomy of the fabled dilemma, this week's submissions include a few wild examples.
Jon has captured a classic sample of the anticancelling cancel, reporting "While bulk-deleting files from an S3 bucket, Amazon dangled a carrot in front of me only to cruelly whip it away at the last moment."
"To opt, or not to opt," muses Michael R, asking "Do I have to select both to be double sure?"
Flight risk Daniel M. is ambiguously relieved to know "My original flight got delayed, but at least I was able to be put on ?ResourceBundle:myBookings.table.status.DS? for an earlier one."
Meanwhile Mark mutters about these seemingly mundane mathematical errors "Hey Pluralsight, my goal is to have zero goals."
But perhaps they're not so mundane after all? Hugenkiss chortles "I can't wait to see the charge for $NaN show up on my credit card statement. If I have the credit card on autopay from my checking account, will my checking account also go NaN?" I certainly wouldn't be so eagerly anticipatory if it were my checking account!
Метки: Error'd |
Keeping Things Simple |
Sandra from Initrovent, previously featured here on the site, has finally left that job, finding herself at InitAg instead. InitAg is a small agricultural tech startup, a little rough around the edges for a data science company, but overall functional. With one notable exception: The customer portal.
The customer portal is a mid-sized Symfony application, built up over the course of 5 years by an externally contracted one-man shop (let's call him Bjorn) who jealously guarded the source code up until the point he decided to drop InitAg as a client in late 2021. Upon informing them he was dropping them, he took a few weeks to clean up some of the code before giving it to InitAg. The Symfony part was mostly all right, with a few small WTFs but nothing too surprising. However, part of the portal's task was the slicing of large (>1GB) image files into smaller chunks for display in a browser, and a few other time-intensive tasks. Which brings us to today's topic: queuing.
There's lots of ways to do queuing right. If you're working in the modern day, you're probably going to use RabbitMQ or another AMQP message broker. If it's 1995, probably the Linux system queue. But Bjorn was a fan of "keeping things simple". What mechanism did he choose for the queue? A collection of "task files" (with no defined structure) in a shared directory, and a bash script on an every-5-minutes cron job polling that directory for the next task, which would then trigger other bash scripts that actually performed the long-running operation. Some of these scripts were many hundreds of lines long, and without any form of documentation or comments to explain why they were shuffling files around or performing other operations.
Of course, that's not all. It turns out that a large number of the scripts were unused (tasks that had been superseded or were no longer needed), but only Bjorn knew which ones were and weren't active.
Bjorn's reasoning for all of this? "I like to keep things simple."
It also didn't help that Bjorn had terrible git hygiene and seemingly didn't know about .gitignore, so the directory with all these scripts in it contained about 7,000 of these task files.
Thankfully, this WTF has a happy ending: Sandra was eventually able to kill off the home-built queuing mechanism and the vast majority of the scripts, and replace the whole mess with a nice RabbitMQ consumer. Still, you gotta love the guys who like to "keep things simple" by avoiding proven, robust solutions in favor of homebrew nonsense.
Метки: Feature Articles |
Optimized Database Access Patterns for Dummies |
Initech sold some expensive devices which supported location tracking. Their customers wanted to track the location of these devices, with full history of all previous locations, down to five minute increments.
When Eurydice F joined the team, she understood that it would be a lot of data to manage. She was an experienced DBA, and had all sorts of ideas about how you might partition the database to make that scale of data manageable, and the ways you would index it to make access and retrieval efficient.
What she found instead was a table called DEVICELOGDAY
. It had three fields. The first was a date- only a date, no time information, the second was an integer field simply called deviceId
, and the third was a CLOB
field called data
.
There was also a temporary table, a transaction-local table, simply called DEVICELOG
. This had more reasonable fields- a timestamp, a device ID, a lat/lon pair.
The secret of the dataflow existed in two stored procedures, PACKLOGDAY
and UNPACKLOGDAY
. A previous developer had seen "we need to store a row for every device, every five minutes" as an unbearable transaction burden. So instead, they decided to store one row per device, per day. That was DEVICELOGDAY
. Every five minutes, a device would report its location and put that location in the DEVICELOG
temporary table. Then PACKLOGDAY
would be called, with the device ID as a parameter.
PACKLOGDAY
would take all rows in DEVICELOG
with that device ID (which, as its a transaction-local temp table, would be the only rows in that table), and append them to the data
column in DEVICELOGDAY
for that device and that day, as a character-separated-values entry, e.g.:
13:41:57|42.6559038|-73.8060233
13:42:02|41.9262175|-74.0180634
One row, per device, by day. If you wanted to convert the pipe-separated fields back into rows and columns, there was a helpful UNPACKLOGDAY
stored procedure which would populate a temporary table, based on the combination of device ID and a date range.
This solution was taken because of performance concerns, and the solution proved the importance of those performance concerns: it was slow as a dead turtle at the end of a marathon. "But," stated the developers who had already been broken by this code, "imagine how slow it'd be if we hadn't optimized?"
Eurydice has this to add:
As a bonus WTF: we developed and edited stored procedures directly in the production database using Oracle SQL Developer
The database itself was their source control.
https://thedailywtf.com/articles/optimized-database-access-patterns-for-dummies
Метки: Feature Articles |
The Squawk Card |
In 1981, Mark was hired at a company that produced minicomputers widely used in retail establishments and small/medium businesses. On the first day, Roger gave him a tour of the plant and introduced him to his new coworkers. After shaking hands and parting ways with Walt, the Manufacturing QA manager, Roger beckoned Mark to lean in close with an impish smirk.
"See, with each system we sell, we send out a response card that the installing CE or customer is supposed to fill out, telling us about any problems with the installation," Roger said. "One time, a 'squawk card' came back from a customer with the comment, Two dead rats in power supply. You know what Walt sent back? Must be shipping damage. They were alive when they left here."
Mark laughed, but then also wondered if maybe he was being pranked as the FNG. "Did that really happen? If so, how is he still working here?"
"It did happen. Follow me!"
Roger led him to the hallway just outside the cafeteria, over to a lit trophy case. He pointed inside at a plaque with an image of the squawk card mounted on it, response and all.
"There were some people who were pissed off, but, Walt's worked here for ages and has a ton of friends at Corporate," Roger explained. "He was fine! And he's a legend around here to this day."
To Mark, it seemed a good omen for the kind of company and culture he'd signed onto. As the years passed, he got to tell the story to several new hires himself.
Through multiple corporate spinoffs, mergers, and acquisitions, the plaque in the trophy case endured. It was still there when Mark finally parted ways with the company in 2009, and we can only hope that this corporate legend still endures.
Метки: Feature Articles |
Classic WTF: Scratch One Inevitability |
Since this weekend was a holiday, and today is Tax Day in the US, we're reaching back into the archives for this classic that fits the theme. Original -- Remy
Before Curtis even got to sit down at his desk, he was accosted by a frenzied, sweating junior developer. "OhmygodCurtis," he began. Curtis extended his hand in a "calm the hell down" gesture and allowed him to continue. "A whole bunch of our stores had no data posted last night and I'm not sure why orwhat to doabout it or whoIshouldtalktoand-" Curtis gestured again, to which the developer handed him a thin stack of papers. After a deep breath, the developer continued. "It's a list of the stores that didn't post last night."
The stores in question were part of what we'll call Hewitt & Liberty Block – a reasonably large tax preparation company serving a handful of states with over 2,000 retail locations. Each of the locations was set up to post tax data and sales records to the central computer at the main facility. According to Curtis's list of stores that hadn't posted any data, it was nearly 1/4. It was going to be a long day.
OK, Curtis thought to himself, what would be the simplest possible explanation... some of the stores were closed! Except that he called a few and they were open as usual and had sales that should've posted the previous night. OK, so what's the second simplest possible explanation... the listener stopped working at some point during the night! Paging through the log file, however, there were no signs of the service failing. Third simplest possible explanation... I'm in my own personal hell. So far the best theory Curtis had.
The stores that had posted data seemed to have nothing differentiating them from the stores that hadn't posted data – regardless of how long the location had been there, how close it was to another location that had posted data successfully – there was nothing.
The change control staff repeatedly insisted that while yes, there had been some changes the previous night, they were all properly documented and had gone through the appropriate process. Specifically, a minor firewall change, a database was moved to a different server, and a few stale DNS entries for servers that no longer existed were removed.
Curtis made a few more calls to make sure that the staff was able to get online where necessary, and strangely, all of the stores that hadn't posted any data were able to access the web, while the ones that did post data weren't. Curtis grilled the managers at several of the locations, asking if anyone was there to inspect the equipment, who the ISP was, what connection type they used, which gradually made it clearer – all of the locations on his list used broadband.
So he'd found the common thread, but still, what the hell? Did the software have some kind of built-in mechanism to verify the upload speed and deny it if it was too fast? No, because it wasn't a problem yesterday. Could the "small firewall change" have affected this? Well, no, because most of the locations posted their data correctly, and all stores used the same port. Perhaps the stale DNS entries that got deleted?
In the change logging system, Curtis found the list of DNS entries that had been removed. With a quick search, he found a reference to one of the entries in a utility function. In pseudocode:
function ConnectionTypes DialUpOrBroadband() { var pingResult = util.ping(HQ_SERVER_NAME); if (pingResult == "Ping request could not find host...") { activateModem(); // activates the modem and connects to the internet return ConnectionTypes.Dialup; } else if (pingResult == "Request timed out") { return ConnectionTypes.Broadband; } throw someException; }
And it's not just a coincidence that there's no check for "Reply from x.x.x.x...", as this function was, crazily, written to check against a server that had never even existed. It would simply fail.
Of course, management decided that this was the system admins' fault, for deleting the stale DNS entry. They should've known better!
https://thedailywtf.com/articles/classic-wtf-scratch-one-inevitability
Метки: Feature Articles |
Error'd: Trauma Bonding |
During this, America's national season of shared trauma, our regular contributor Mr. Bargle shares a bit of levity which may lighten your mood ever so slightly.
Argle Bargle explains
"My girlfriend is Thai. She speaks English, but not on an advanced level, so when we chat, I typically translate to Thai. I learned long ago to reverse the translation to make sure the translation says what I meant. My girlfriend took a leave of absence from her research position to be an entrepreneur. April 13-15 is the holiday of Songkran: Thailand's New Year celebration. She said she would too busy to chat with me until the 15th. I mentioned that the 15th is America's tax day. As a precaution, I translated it and then reversed. The attached image is clearly not an error, but it's a helluva WTF. On the left is LOL... tax day in the USA in Thai. On the right is.... OMG."Modernist Allie C. wonders "This is from my daily USPS delivery digest. How does an API deliver a package - maybe some new compression format?" Carrier pigeons, Allie. Lots and lots of carrier pigeons.
Ren'e remarks "Sure, there are checkboxes to turn a feature off; but a checkbox with a bare No label is somewhat counterintuitive."
Confused Bart confesses "I have many questions. Was the problem report I was trying to generate generated? And who or what generated the error message reporting on the generation of the error report?"
Gamer Phil B. catches Fortnite out "Talking about an early release ..."
Метки: Error'd |