You are here

The Daily WTF

Subscribe to The Daily WTF feed
Curious Perversions in Information Technology
Updated: 3 hours 29 min ago

CodeSOD: Delete This

12 hours 59 min ago

About three years ago, Consuela inherited a giant .NET project. It was… not good. To communicate how “not good” it was, Consuela had a lot of possible submissions. Sending the worst code might be the obvious choice, but it wouldn’t give a good sense of just how bad the whole thing was, so they opted instead to find something that could roughly be called the “median” quality.

This is a stored procedure that is roughly about the median sample of the overall code. Half of it is better, but half of it gets much, much worse.

CREATE proc [dbo].[usermgt_DeleteUser] ( @ssoid uniqueidentifier ) AS begin declare @username nvarchar(64) select @username = Username from Users where SSOID = @ssoid if (not exists(select * from ssodata where ssoid = @ssoid)) begin insert into ssodata (SSOID, UserName, email, givenName, sn) values (@ssoid, @username, 'Email@email.email', 'Firstname', 'Lastname') delete from ssodata where ssoid = @ssoid end else begin RAISERROR ('This user still exists in sso', 10, 1) end

Let’s talk a little bit about names. As you can see, they’re using an “internal” schema naming convention- usermgt clearly is defining a role for a whole class of stored procedures. Already, that’s annoying, but what does this procedure promise to do? DeleteUser.

But what exactly does it do?

Well, first, it checks to see if the user exists. If the user does exist… it raises an error? That’s an odd choice for deleting. But what does it do if the user doesn’t exist?

It creates a user with that ID, then deletes it.

Not only is this method terribly misnamed, it also seems to be utterly useless. At best, I think they’re trying to route around some trigger nonsense, where certain things happen ON INSERT and then different things happen ON DELETE. That’d be a WTF on its own, but that’s possibly giving this more credit than it deserves, because that assumes there’s a reason why the code is this way.

Consuela adds a promise, which hopefully means some follow-ups:

If you had access to the complete codebase, you would not EVER run out of new material for codesod. It’s basically a huge collection of “How Not To” on all possible layers, from single lines of code up to the complete architecture itself.

hljs.initHighlightingOnLoad(); [Advertisement] ProGet’s got you covered with security and access controls on your NuGet feeds. Learn more.
Categories: Technology

CodeSOD: Extended Time

Tue, 2020-10-20 07:30

The C# "extension method" feature lets you implement static methods which "magically" act like they're instance methods. It's a neat feature which the .NET Framework uses extensively. It's also a great way to implement some convenience functions.

Brandt found some "convenience" functions which were exploiting this feature.

public static bool IsLessThen<T>(this T a, T b) where T : IComparable<T> => a.CompareTo(b) < 0; public static bool IsGreaterThen<T>(this T a, T b) where T : IComparable<T> => a.CompareTo(b) > 0; public static bool And(this bool a, bool b) => a && b; public static bool Or(this bool a, bool b) => a || b; public static bool IsBetweensOrEqual<T>(this T a, T b, T c) where T : IComparable<T> => a.IsGreaterThen(b).Or(a.Equals(b)).And( a.IsLessThen(c).Or(a.Equals(c)) );

Here, we observe someone who maybe heard the term "functional programming" and decided to wedge it into their programming style however it would fit. We replace common operators and expressions with extension method versions. Instead of the cryptic a || b, we can now write a.Or(b), which is… better?

I almost don't hate the IsLessThan/IsGreaterThan methods, as that is (arguably) more readable. But wait, I have to correct myself: IsLessThen and IsGreaterThen. So they almost got something that was (arguably) more readable, but with a minor typo just made it all more confusing.

All this, though, to solve their actual problem: the TimeSpan data type doesn't have a "between" comparator, and at one point in their code- one point- they need to perform that check. It's also worth noting that C# supports operator overloading, and TimeSpan does have an overload for all your basic comparison operators, so they could have just done that.

Brandt adds:

While you can argue that there is no out of the box 'Between' functionality, the colleague who programmed this ignored an already existing extension method that was in the same file, that offered exactly this functionality in a better way.

hljs.initHighlightingOnLoad(); [Advertisement] Utilize BuildMaster to release your software with confidence, at the pace your business demands. Download today!
Categories: Technology

CodeSOD: Don't Not Be Negative

Mon, 2020-10-19 07:30

One of my favorite illusions is the progress bar. Even the worst, most inaccurate progress bar will make an application feel faster. The simple feedback which promises "something is happening" alters the users' sense of time.

So, let's say you're implementing a JavaScript progress bar. You need to decide if you are "in progress" or not. So you need to check: if there is a progress value, and the progress value is less than 100, you're still in progress.

Mehdi's co-worker decided to implement that check as… the opposite.

const isInProgress = progress => !(!progress || (progress && progress > 100))

This is one of those lines of code where you can just see the developer's process, encoded in each choice made. "Okay, we're not in progress if progress doesn't have a value: !(!progress). Or we're not in progress if progress has a value and that value is over 100."

There's nothing explicitly wrong with this code. It's just the most awkward, backwards possible way to express that check. I suspect that part of its tortured logic arises from the fact that the developer wanted to return false if the value was null or undefined, and this was the way they figured out to do that.

Of course, a more straightforward way to write that might be (progress) => (progress && progress <= 100) || false. This will have "unexpected" behavior if the progress value is negative, but then again, so will the original code.

In the end, this is just a story of a double negative. I definitely won't say you should never not use a double negative. Don't not avoid them.

hljs.initHighlightingOnLoad(); [Advertisement] Utilize BuildMaster to release your software with confidence, at the pace your business demands. Download today!
Categories: Technology

Error'd: Try a Different Address

Fri, 2020-10-16 07:30

"Aldi doesn't deliver to a ...computer memory address!? Ok, that sounds fair," wrote Oisin.

 

"When you order pictures from this photography studio, you find that what sets them apart from other studios is group photography and not web site design," Stephen D. wrote.

 

John H. writes, "To be honest, I'm a little bit surprised. I figured for sure that it would have been a 50/50 split between meat and booze."

 

"I was just trying to perform a trivial merge using GVim, but then ṕ̵̡̛̃̍̊͊̌̉h̸̢̦̭̟̿̉̔̔͛̋̓̑̉́͌͊̒̕͜'̸̛͎̹̦͔̳͎̹͎̥̻̺̦̳͈̞̃̿̐́́͑̒͛̀̋̑̕͠n̸̨̛͉̥̻̘̣̞̉́͂̌͋́̾͗͠g̵̣̫̯̈̈́̕l̷͍̳͚̻̺̀̊͆̂͒͐̀̔́͘͜͝͝͝ừ̷̯̬̜̱̭̳̞͇̠̄̎͗͝į̴̗̺͓̣̘͚̯̳͕̠̗̮̄͛͌̍̈́͌͊͌̓̈́͌̌͝ ̵̼̼̥͍̙̋̇͂͋͐͐͂m̶̥̭͍͕͍̑̏g̸̡͖̜̜̭͈͖͇̦͍͉͚̎͜ͅl̵̡͉̜̀̂̎́͐̑̑͑̍̚ŵ̴̡͚̝̣̬̭̥̙͎̻̽͝'̸̼̩̑́̄͆̾͆̿́̕ṅ̶̛̯͍̰͒́̂̎ǎ̶͇̬̲͕͍̻̞̫̻͕͈͔̍͌͆͌̈̓͛̀̌̿̚͝͠f̸͎͖̰̖̫̪͎̄̈̓̏́̐̄̈̒́̈̾̔̕ḩ̶̠̺̯̦̪͓̜͙̬͎̳̭͊̀̌͂͆́̾̑̾͝ ̶̣̘̟͊̈́͊͗͒͋̊͊̀͛̉͝Ģ̶̛̙̜̗̖̼̺͓͐̀͐͒V̸̢̙̰̟̙͚̗͖̆̈̇̓͘͠͝͝i̶̛̹̳̍͂́͝͝͝m̶̛͈͈̹̯͕̗͐̂͊̇̃̃͌̌̓̄̔̆͘ ̸̢̛̣͓̪͚̘͚̰͖͐́͜R̸͍͈͚̻͕̗̻͉͙͆͌͐͒̓͒̐̓̑̊͒͝'̴̧͚͔̫̼̺͔͎̖͈̞͙͆͆l̷͍̠̄͂̆̒͊̔ẏ̶̢̮͓̄͆́̉̈́̑͗̉͠ͅè̴̡̧̝͙̖̹͓̤̼̻͓̬͚̰̅̋́̑̾͘͜h̷̳͇̖͔̤͇̦̹̮̐̏͛̊͐͒͐ ̷̧͚̔̉̍͆͌̓͗̓̐̐͘w̸̤̝̪͎͇̩̤̳͒̏̒̎̈́̉͗̈́̚̚͝m̴̼̦̩͉̜̳͓͔̟̭͇̜̰̬̋̎͗͆̒̀̍̍̇̔͋̕͝͝ͅê̸̢̡̢̝͓̟̭̞͍̞͈̖̠̲̬̒̐̎̿̇̍́̂͒̏̉͘͠r̸̬̉̈́͆̌͆̈̉ǧ̴̢̙͚͓͇͖͔̩̣͕̞̚e̸̡̢̩̠͙͖̺̥͉̦̟̩͐͘'̵̡̢̧̟͇̭̲̳͕̻̜͇̘̬̙̈̀̓́̄̒̔̒̕͘͠n̶̢̛͎̹̮̻̼̳̜͖̲̂̇̅̾̄̐̊̇̓̒̒̍͘a̸̟̞̗̻͕̘̳͔̿̌͑̌̎̈̊̓͊̊͋͜g̷̢̮̟̠͖̞̤͖̘̻̞̀̄̓͌̾́̉̏͑͐͜l̵̪̫̝̐̃ ̸̮̤̱͇͂̔̂̀̾̀̚f̵̥͌̂̒̐̚h̷̡̤̮͇̥̖̼̙́̌̕ͅţ̴̛̞̦̩͚̝̦̮͎͕̹̖̰̀̋̋͐̍̅̀͛̕̕̚͘͝͠ȁ̸̖̝͈͎̤͇̽͛́̄͆͒̃̓̏͐͊͒̔̌g̸̜̠͉̝̱̳͔̭̦͇̱̘̺͋ͅͅn̶̦̥͈̻͍̠̂̔̊́̑͆̉̈́͝," wrote Ivan.

 

Robert M. writes, "Look, considering how 2020 is going, 'Invalid Date' could be really any day coming up. Listen - I just want to know, is this before or after Christmas, because like it or not, I still have to do holiday shopping."

 

[Advertisement] Keep the plebs out of prod. Restrict NuGet feed privileges with ProGet. Learn more.
Categories: Technology

CodeSOD: A New Generation

Thu, 2020-10-15 07:30

Mapping between types can create some interesting challenges. Michal has one of those scenarios. The code comes to us heavily anonymized, but let’s see what we can do to understand the problem and the solution.

There is a type called ItemA. ItemA is a model object on one side of a boundary, and the consuming code doesn’t get to touch ItemA objects directly, it instead consumes one of two different types: ItemB, or SimpleItemB.

The key difference between ItemB and SimpleItemB is that they have different validation rules. It’s entirely possible that an instance of ItemA may be a valid SimpleItemB and an invalid ItemB. If an ItemA contains exactly five required fields importantDataPieces, and everything else is null, it should turn into a SimpleItemB. Otherwise, it should turn into an ItemB.

Michal adds: “Also noteworthy is the fact that ItemA is a class generated from XML schemas.”

The Java class was generated, but not the conversion method.

public ItemB doConvert(ItemA itemA) { final ItemA emptyItemA = new ItemA(); emptyItemA.setId(itemA.getId()); emptyItemA.setIndex(itemA.getIndex()); emptyItemA.setOp(itemA.getOp()); final String importantDataPiece1 = itemA.importantDataPiece1(); final String importantDataPiece2 = itemA.importantDataPiece2(); final String importantDataPiece3 = itemA.importantDataPiece3(); final String importantDataPiece4 = itemA.importantDataPiece4(); final String importantDataPiece5 = itemA.importantDataPiece5(); itemA.withImportantDataPiece1(null) .withImportantDataPiece2(null) .withImportantDataPiece3(null) .withImportantDataPiece4(null) .withImportantDataPiece5(null); final boolean isSimpleItem = itemA.equals(emptyItemA) && importantDataPiece1 != null && importantDataPiece2 != null && importantDataPiece3 != null && importantDataPiece4 != null; itemA.withimportantDataPiece1(importantDataPiece1) .withImportantDataPiece2(importantDataPiece2) .withImportantDataPiece3(importantDataPiece3) .withImportantDataPiece4(importantDataPiece4) .withImportantDataPiece5(importantDataPiece5); if (isSimpleItem) { return simpleItemConverter.convert(itemA); } else { return itemConverter.convert(itemA); } }

We start by making a new instance of ItemA, emptyItemA, and copy a few values over to it. Then we clear out the five required fields (after caching the values in local variables). We rely on .equals, generated off that XML schema, to see if this newly created item is the same as our recently cleared out input item. If they are, and none of the required fields are null, we know this will be a SimpleItemB. We’ll put the required fields back into the input object, and then call the appropriate conversion methods.

Let’s restate the goal of this method, to understand how ugly it is: if an object has five required values and nothing else, it’s SimpleItemB, otherwise it’s a regular ItemB. The way this developer decided to perform this check wasn’t by examining the fields (which, in their defense, are being generated, so you might need reflection to do the inspection), but by this unusual choice of equality test. Create an empty object, copy a few ID related elements into it, and your default constructor should handle nulling out all the things which should be null, right?

Or, as Michal sums it up:

The intention of the above snippet appears to be checking whether itemA contains all fields mandatory for SimpleItemB and none of the other ones. Why the original author started by copying some fields to his ‘template item’ but switched to the ‘rip the innards of the original object, check if the ravaged carcass is equal to the barebones template, and then stuff the guts back in and pretend nothing ever happened’ approach halfway through? I hope I never find out what it’s like to be in a mental state when any part of this approach seems like a good idea.

Ugly, yes, but still, this code worked… until it didn’t. Specifically, a new nullable Boolean field was added to ItemA which was used by ItemB, but had no impact on SimpleItemB. This should have continued to work, except that the original developer defaulted the new field to false in the constructor, but didn’t update the doConvert method, so equals started deciding that our input item and our “empty” copy no longer matched. Downstream code started getting invalid ItemB objects when it should have been getting valid SimpleItemB objects, which triggered many hours of debugging to try and understand why this small change had such cascading effects.

Michal refactored this code, but was not the first person to touch it recently:

A cherry on top is the fact that importantDataPiece5 came about a few years after the original implementation. Someone saw this code, contributed to the monstrosity, and happily kept on trucking.

hljs.initHighlightingOnLoad(); [Advertisement] Utilize BuildMaster to release your software with confidence, at the pace your business demands. Download today!
Categories: Technology

CodeSOD: Nothing But Garbage

Wed, 2020-10-14 07:30

Janell found herself on a project where most of her team were offshore developers she only interacted with via email, and a tech lead who had not ever programmed on the .NET Framework, but did some VB6 “back in the day”, and thus “knew VB”.

The team dynamic rapidly became a scenario where the tech lead issued an edict, the offshore team blindly applied it, and then Janell was left staring at the code wondering how to move forward with this.

These decrees weren’t all bad. For example: “Avoid repeated code by re-factoring into methods,” isn’t the worst advice. It’s not complete advice- there’s a lot of gotchas in that- but it fits on a bumper-sticker and generally leads to better code.

There were other rules that… well:

To improve the performance of the garbage collector, all variables must be set to nothing (null) at the end of each method.

Any time someone says something like “to improve the performance of the garbage collector,” you know you’re probably in for a bad time. This is no exception. Now, in old versions of VB, there’s a lot of “stuff” about whether or not you need to do this. It was considered a standard practice in a lot of places, though the real WTF was clearly VB in this case.

In .NET, this is absolutely unnecessary, and there are much better approaches if you need to do some sort of cleanup, like implementing the IDisposable interface. But, since this was the rule, the offshore team followed the rule. And, since we have repeated code, the offshore team followed that rule too.

Thus:

Public Sub SetToNothing(ByVal object As Object) object = Nothing End Sub

If there is a platonic ideal of bad code, this is very close to that. This method attempts to solve a non-problem, regarding garbage collection. It also attempts to be “DRY”, by replacing one repeated line with… a different repeated line. And, most important: it doesn’t do what it claims.

The key here is that the parameter is ByVal. The copy of the reference in this method is set to nothing, but the original in the calling code is left unchanged in this example.

Oh, but remember when I said, “they were attempting to be DRY”? I lied. I’ll let Janell explain:

I found this gem in one of the developers classes. It turned out that he had copy and pasted it into every class he had worked on for good measure.

hljs.initHighlightingOnLoad(); [Advertisement] Keep the plebs out of prod. Restrict NuGet feed privileges with ProGet. Learn more.
Categories: Technology

Slow Load

Tue, 2020-10-13 07:30

LED traffic light on red

After years spent supporting an enterprisey desktop application with a huge codebase full of WTFs, Sammy thought he had seen all there was to be seen. He was about to find out how endlessly deep the bottom of the WTF barrel truly was.

During development, Sammy frequently had to restart said application: a surprisingly onerous process, as it took about 30 seconds each and every time to return to a usable state. Eventually, a mix of curiosity and annoyance spurred him into examining just why it took so long to start.

He began by profiling the performance. When the application first initialized, it performed 10 seconds of heavy processing. Then the CPU load dropped to 0% for a full 16 seconds. After that, it increased, pegging out one of the eight cores on Sammy's machine for 4 seconds. Finally, the application was ready to accept user input. Sammy knew that, for at least some of the time, the application was calling out to and waiting for a response from a server. That angle would have to be investigated as well.

Further digging and hair-pulling led Sammy to a buried bit of code, a Very Old Mechanism for configuring the visibility of items on the main menu. While some menu items were hard-coded, others were dynamically added by extension modules. The application administrator had the ability to hide or show any of them by switching checkboxes in a separate window.

When the application first started up, it retrieved the user's latest configuration from the server, applied it to their main menu, then sent the resulting configuration back to the server. The server, in turn, called DELETE * FROM MENU_CFG; INSERT INTO MENU_CFG (…); INSERT INTO MENU_CFG (…); …

Sammy didn't know what was the worst thing about all this. Was it the fact that the call to the server was performed synchronously for no reason? Or, that after multiple DELETE / INSERT cycles, the table of a mere 400 rows weighed more than 16 MB? When the users all came in to work at 9:00 AM and started up the application at roughly the same time, their concurrent transactions caused quite the bottleneck—and none of it was necessary. The Very Old Mechanism had been replaced with role-based configuration years earlier.

All Sammy could do was write up a change request to put in JIRA. Speaking of bottlenecks ...

[Advertisement] ProGet’s got you covered with security and access controls on your NuGet feeds. Learn more.
Categories: Technology

CodeSOD: Intergral to Dating

Mon, 2020-10-12 07:30

Date representations are one of those long-term problems for programmers, which all ties into the problem that "dates are hard". Nowadays, we have all these fancy date-time types that worry about things like time zones and leap years, and all of that stuff for us. Pretty much everything, at some level, relies on the Unix Epoch. But there is a time before that was the standard.

In the mainframe era, not all the numeric representations worked the same way that we're used to, and it was common to simply define the number of digits you wanted to store. So, if you wanted to store a date, you could define an 8-digit field, and store the date as 20201012: Octobel 10th, 2020.

This is a pretty great date format, for that era. Relatively compact (and yes, the whole Y2K thing means that you might have defined that as a six digit field), inherently sortable, and it's not too bad to slice it back up into date parts, when you need it. And like anything else which was a good idea a long time ago, you still see it lurking around today. Which does become a problem when you inherit code written by people who didn't understand why things worked that way.

Virginia N inherited some C# code which meets that criteria. And the awkward date handling isn't even the WTF. There's a lot to unpack in this particular sample, so let's start with… the unpack function.

.comment { border: none; } public static DateTime UnpackDateC(DateTime dateI, long sDate) { if (sDate.ToString().Length != 8) return dateI; try { return new DateTime(Convert.ToInt16(sDate.ToString().Substring(0, 4)), Convert.ToInt16(sDate.ToString().Substring(4, 2)), Convert.ToInt16(sDate.ToString().Substring(6, 2))); } catch { return dateI; } }

sDate is our integer date: 20201012. Instead of converting it to a string once, and then validating and slicing it, we call ToString four times. We also reconvert each date part back into an integer so we can pass them to DateTime, and of course, DateTime.ParseExact is just sitting there in the documentation, shaking its head at all of this.

The really weird choice to me, though, is that we pass in dateI, which appears to be the "fallback" date value. That… worries me.

Well, let's take a peek deep in the body of a method called GetMC, because that's where this unpack function is called.

while (oDataReader.Read()) { //... DataRow oRow = oDataTable.NewRow(); if (oDataReader["DEB"] != DBNull.Value) { DateTime dt = DateTime.Today; dt = UnpackDateC(dt, Convert.ToInt64(oDataReader["DEB"])); } else { oRow["DEB"] = DBNull.Value; } //... }

It's hard to know for absolute certain, based on the code provided, but I don't think UnpackDateC is actually doing anything. We can see that the default dateI value is DateTime.Today. So perhaps the desired behavior is that every invalid/unknown date is today? Seems problematic, but maybe that jives with the requriements.

But note the logic. If the database value is null, we store a null in oRow["DEB"]- our output data. If it isn't null, we unpack the date and store it in… dt. Also, if you trace the type conversions, we convert an integer in the database into an integer in our program (which it already would have been) so that we can convert that integer into a string so that we can split the string and convert each portion into integers so we can convert it into a date.

How do I know that the field is an integer in the database? Well, I don't know for sure, but let's look at the query which drives that loop.

public static void GetMC(string sConnectionString, ref DataTable dtToReturn, string sOrgafi, string valid, string exe, int iOperateur, string sColTri, bool Asc, bool DBLink, string alias) // iOperateur 0, 1, 2 { sSql = " select * from (select ENTLCOD as ENTAFI, MARKYEAR as EXE, nvl(to_char(MARKNUM),'')||'-'||nvl(MARKNUMLOT,'') as COD, to_char(MARKNUM) as NUM, MARKOBJ1 as COM, MARKSTARTDATE as DEB, MARKENDDATE as FIN, MARKNUMLOT as NUMLOT, MARKVALIDDATE, TIERNUM as FOUR, MARTBASTIT as TYP " + " from SOMEEXTERNALVIEW" + (DBLink ? alias + " " : " ") + " WHERE 1=1"; if (valid != null && valid.Length > 0) sSql += " and (MARKVALIDDATE >= " + valid + " or MARKVALIDDATE=0 or MARKVALIDDATE is null)"; if (exe != null && exe.Length > 0) sSql += " and TRIM( MARKYEAR ) ='" + exe.Trim() + "' "; sSql += " ) where 1=1"; //...

We can see that MARKSTARTDATE is the database field we call DEB. We can also see some conditional string concatenation to build our query, so hello possible SQL injection attacks. Now, I don't know that MARKSTARTDATE is an integer, but I can see that a similar field, MARKVALIDDATE is. Note the lack of quotes in the query string: "…(MARKVALIDDATE >= " + valid + " or MARKVALIDDATE=0 or MARKVALIDDATE is null)"

So MARKVALIDDATE is numeric in the database, which is great because the variable valid is passed in as a string, so we're just all over the place with types.

The structure of this query also adds on an extra layer of unnecessary complexity, as for some reason, we wrap the actual query up as a subquery, but the outer query is just SELECT * FROM (subquery) WHERE 1=1, so there is literally no reason to do that.

To finish this off, let's look at where GetMC is actually invoked, a method called CallWSM.

private void CallWSM(ref DataTable oDataTable, string sCode, string sNom, string sFourn, int iOperateur) // iOperateur 0, 1, 2 { try { m_bError = false; string sColTri = m_Grid_SortRequest.FieldName; SortOperator oDirection = m_Grid_SortRequest.SortDirection; m_sAnnee = ctlRecherche.GetValueFilterItem(1); string svalid = ""; string sdt = ctlRecherche.GetValueDateItem(0); if (sdt.Length > 0) { DateTime dtvalid = DateTime.Parse(sdt); long ldt = dtvalid.Year * 10000 + dtvalid.Month * 100 + dtvalid.Day; svalid = ldt.ToString(); } m_AppLogic.GetMC(sColTri, oDirection, m_sAdresseWS, ref oDataTable, svalid, m_sAnnee, iOperateur, PageCurrent, m_PageSize); } catch (WebException ex) { if (ex.Status == WebExceptionStatus.Timeout) { frmMessageBox.Show(ML.GetLibelle(4137), CONST.AppTITLE, MessageBoxButtons.OK, MessageBoxIcon.Error); m_bError = true; } else { frmMessageBox.Show(ex.Message); m_bError = true; } } catch (Exception ex) { frmMessageBox.Show(ex.Message); m_bError = true; } }

Now, I'm reading between the lines a bit, and maybe making some assumptions that I shouldn't be, but this method is called CallWSM, and one of the parameters we pass to GetMC is stored in a variable called m_sAdresseWS, and GetMC can apparently throw a WebException.

Are… are we building a query and then passing it off to a web service to execute? And then wrapping the response in a data reader? Because that would be terrible. But if we're not, does that mean that we're also calling a web service in some of the code Virginia didn't supply? Query the DB and call the web service in the same method? Or are we catching an exception that just could never happen, and all the WS stuff has nothing to do with web services?

Any one of those options would be a WTF.

Virginia adds, "I had the job to make a small change in the call. ...I'm used to a good amount of Daily WTF-erry in our code." After reading through the code though, Virginia had some second thoughts about changing the code. "At this point I decided not the change anything, because it hurts my head."

You and me both.

$WORD [Advertisement] BuildMaster allows you to create a self-service release management platform that allows different teams to manage their applications. Explore how!
Categories: Technology

Error'd: Math is HARD!

Fri, 2020-10-09 07:30

"Boy oh boy! You can't beat that free shipping for the low, low price of $4!" Zenith wrote.

 

Brendan wrote, "So, as of September 24th, my rent is both changing and not changing, in the future, but also in the past?"

 

"Does waiting for null people take forever or no time at all?" Pascal writes.

 

"Whoever at McDonalds is responsible for calculating their cheeseburger deals needs to lay off of the special sauce," Valts S. wrote.

 

"So, assuming I were to buy 0.9 of a figure, what's the missing 10%?" writes Zenith.

 

[Advertisement] BuildMaster allows you to create a self-service release management platform that allows different teams to manage their applications. Explore how!
Categories: Technology

Translation by Column

Thu, 2020-10-08 07:30

Content management systems are… an interesting domain in software development. On one hand, they’re one of the most basic types of CRUD applications, at least at their core. On the other, it’s the sort of problem domain where you can get 90% of what you need really easily, but the remaining 10% are the cases that are going to make you pull your hair out.

Which is why pretty much every large CMS project supports some kind of plugin architecture, and usually some sort of marketplace to curate those plugins. That kind of curation is hard, writing plugins tends to be more about “getting the feature I need” and less about “releasing a reliable product”, and thus the available plugins for most CMSes tend to be of wildly varying quality.

Paul recently installed a plugin for Joomla, and started receiving this error:

Row size too large. The maximum row size for the used table type, not counting BLOBs, is 8126. This includes storage overhead, check the manual. You have to change some columns to TEXT or BLOBs

Paul, like a good developer, checked with the plugin documentation to see if he could fix that error. The documentation had this to say:

you may have too many languages installed

This left Paul scratching his head. Sure, his CMS had eight different language packs installed, but how, exactly, was that “too many”? It wasn’t until he checked into the database schema that he understood what was going on:

A screencap of the database schema, which shows that EVERY text field has a column created for EVERY language, e.g. 'alias_ru', 'alias_fr', 'alias_en'

This plugin’s approach to handling multiple languages was simply to take every text field it needed to track and add a column to the table for every language. The result was a table with 231 columns, most of them language-specific duplicates.

Now, there are a few possible reasons why this is this way. It may simply be whoever wrote the plugin didn’t know or care about setting up a meaningful lookup table. Maybe they were thinking about performance, and thought that “well, if I denormalize I can get more data with a single query”. Maybe they weren’t thinking at all. Or maybe there’s some quirk about creating your own tables as a Joomla plugin that the developer didn’t want to create more tables than absolutely necessary.

Regardless, it’s definitely one of the worst schemas you could come up with to handle localization.

Paul adds: “I vote the developer for Best Database Designer of 2020.”

[Advertisement] ProGet’s got you covered with security and access controls on your NuGet feeds. Learn more.
Categories: Technology

CodeSOD: A Long Time to Master Lambdas

Wed, 2020-10-07 07:30

At an old job, I did a significant amount of VB.Net work. I didn’t hate VB.Net. Sure, the syntax was clunky, but autocomplete mostly solved that, and it was more OR
less feature-matched to C# (and, as someone who needed to handle XML, the fact that VB.Net had XML literals was handy).

Every major feature in C# had a VB.Net equivalent, including lambdas. And hey, lambdas are great! What a wonderful way to express a filter condition.

Well, Eric O sends us this filter lambda. Originally sent to us as a single line, I’m adding line breaks for readability, because I care about this more than the original developer did.

Function(row, index) index <> 0 AND (row(0).ToString().Equals("DIV10106") OR row(0).ToString().Equals("326570") OR row(0).ToString().Equals("301100") OR row(0).ToString().Equals("305622") OR row(0).ToString().Equals("305623") OR row(0).ToString().Equals("317017") OR row(0).ToString().Equals("323487") OR row(0).ToString().Equals("323488") OR row(0).ToString().Equals("324044") OR row(0).ToString().Equals("317016") OR row(0).ToString().Equals("316875") OR row(0).ToString().Equals("323976") OR row(0).ToString().Equals("324813") OR row(0).ToString().Equals("147000") OR row(0).ToString().Equals("326984") OR row(0).ToString().Equals("326634") OR row(0).ToString().Equals("306039") OR row(0).ToString().Equals("307021") OR row(0).ToString().Equals("307050") OR row(0).ToString().Equals("307603") OR row(0).ToString().Equals("307604") OR row(0).ToString().Equals("307632") OR row(0).ToString().Equals("307704") OR row(0).ToString().Equals("308184") OR row(0).ToString().Equals("308531") OR row(0).ToString().Equals("309930") OR row(0).ToString().Equals("104253") OR row(0).ToString().Equals("104532") OR row(0).ToString().Equals("104794") OR row(0).ToString().Equals("104943") OR row(0).ToString().Equals("105123") OR row(0).ToString().Equals("105755") OR row(0).ToString().Equals("106075") OR row(0).ToString().Equals("108062") OR row(0).ToString().Equals("108417") OR row(0).ToString().Equals("108616") OR row(0).ToString().Equals("108625") OR row(0).ToString().Equals("108689") OR row(0).ToString().Equals("108851") OR row(0).ToString().Equals("108997") OR row(0).ToString().Equals("109358") OR row(0).ToString().Equals("109551") OR row(0).ToString().Equals("110081") OR row(0).ToString().Equals("111501") OR row(0).ToString().Equals("111987") OR row(0).ToString().Equals("112136") OR row(0).ToString().Equals("11229") OR row(0).ToString().Equals("112261") OR row(0).ToString().Equals("113127") OR row(0).ToString().Equals("113266") OR row(0).ToString().Equals("114981") OR row(0).ToString().Equals("116527") OR row(0).ToString().Equals("121139") OR row(0).ToString().Equals("121469") OR row(0).ToString().Equals("142449") OR row(0).ToString().Equals("144034") OR row(0).ToString().Equals("144693") OR row(0).ToString().Equals("144900") OR row(0).ToString().Equals("150089") OR row(0).ToString().Equals("194340") OR row(0).ToString().Equals("214950") OR row(0).ToString().Equals("215321") OR row(0).ToString().Equals("215908") OR row(0).ToString().Equals("216531") OR row(0).ToString().Equals("217151") OR row(0).ToString().Equals("220710") OR row(0).ToString().Equals("221265") OR row(0).ToString().Equals("221387") OR row(0).ToString().Equals("300011") OR row(0).ToString().Equals("300013") OR row(0).ToString().Equals("300020") OR row(0).ToString().Equals("300022") OR row(0).ToString().Equals("300024") OR row(0).ToString().Equals("300026") OR row(0).ToString().Equals("300027") OR row(0).ToString().Equals("300050") OR row(0).ToString().Equals("300059") OR row(0).ToString().Equals("300060") OR row(0).ToString().Equals("300059") OR row(0).ToString().Equals("300125") OR row(0).ToString().Equals("300139") OR row(0).ToString().Equals("300275") OR row(0).ToString().Equals("300330") OR row(0).ToString().Equals("300342") OR row(0).ToString().Equals("300349") OR row(0).ToString().Equals("300355") OR row(0).ToString().Equals("300363") OR row(0).ToString().Equals("300413") OR row(0).ToString().Equals("301359") OR row(0).ToString().Equals("302131") OR row(0).ToString().Equals("302595") OR row(0).ToString().Equals("302621") OR row(0).ToString().Equals("302649") OR row(0).ToString().Equals("302909") OR row(0).ToString().Equals("302955") OR row(0).ToString().Equals("302986") OR row(0).ToString().Equals("303096") OR row(0).ToString().Equals("303249") OR row(0).ToString().Equals("303753") OR row(0).ToString().Equals("304010") OR row(0).ToString().Equals("304016") OR row(0).ToString().Equals("304047") OR row(0).ToString().Equals("304566") OR row(0).ToString().Equals("305347") OR row(0).ToString().Equals("305486") OR row(0).ToString().Equals("305487") OR row(0).ToString().Equals("305489") OR row(0).ToString().Equals("305526") OR row(0).ToString().Equals("305568") OR row(0).ToString().Equals("305769") OR row(0).ToString().Equals("305773") OR row(0).ToString().Equals("305824") OR row(0).ToString().Equals("305998") OR row(0).ToString().Equals("306039") OR row(0).ToString().Equals("307021") OR row(0).ToString().Equals("307050") OR row(0).ToString().Equals("307603") OR row(0).ToString().Equals("307604") OR row(0).ToString().Equals("307632") OR row(0).ToString().Equals("307704") OR row(0).ToString().Equals("308184") OR row(0).ToString().Equals("308531") OR row(0).ToString().Equals("309930") OR row(0).ToString().Equals("322228") OR row(0).ToString().Equals("121081") OR row(0).ToString().Equals("321879") OR row(0).ToString().Equals("327391") OR row(0).ToString().Equals("328933") OR row(0).ToString().Equals("325038")) AND DateTime.ParseExact(row(2).ToString(), "dd.MM.yyyy", System.Globalization.CultureInfo.InvariantCulture).CompareTo(DateTime.Today.AddDays(-14)) <= 0

That is 5,090 characters of lambda right there, clearly copy/pasted with modifications on each line. The original developer, at no point, paused to think a moment about whether or not this was the way to achieve their goal?

If you’re wondering about those numeric values, I’ll let Eric explain:

The magic numbers are all customer object references, except the first one, which I have no idea what is.

hljs.initHighlightingOnLoad(); [Advertisement] ProGet’s got you covered with security and access controls on your NuGet feeds. Learn more.
Categories: Technology

News Roundup: Excellent Data Gathering

Tue, 2020-10-06 07:30

In a global health crisis, like say, a pandemic, accurate and complete data about its spread is a "must have". Which is why, in the UK, there's a great deal of head-scratching over how the government "lost" thousands of cases.

Oops.

Normally, we don't dig too deeply into current events on this site, but the circumstances here are too "WTF" to allow them to pass without comment.

From the BBC, we know that this system was used to notify individuals if they have tested positive for COVID-19, and notify their close contacts that they have been exposed. That last bit is important. Disease spread can quickly turn exponential, and even though COVID-19 has a low probability of causing fatalities, the law of large numbers means that a lot of people will die anyway on that exponential curve. If you can track exposure, get exposed individuals tested and isolated before they spread the disease, you can significantly cut down its spread.

People are rightfully pretty upset about this mistake. Fortunately, the BBC has a followup article discussing the investigation, where an analyst explores what actually happened, and as it turns out, we're looking at an abuse of everyone's favorite data analytics tool: Microsoft Excel.

The companies administering the tests compile their data into plain text which appear to be CSV files. No real surprise there. Each test created multiple rows within the CSV file. Then, the people working for Public Health England imported that data into Excel… as .xls files.

.xls is the old Excel format, dating back into far-off years, and retained for backwards compatibility. While modern .xlsx files can support a little over a million rows, the much older format caps out at 65,636.

So: these clerks imported the CSV file, hit "save as…" and made a .xls, and ended up truncating the results. With the fact that these input datasets had multiple rows per tests, "in practice it meant that each template was limited to about 1,400 cases."

Again, "oops".

I've discussed how much users really want to do everything in Excel, and this is clearly what happened here. The users had one tool, Excel, and it looked like a hammer to them. Arcane technical details like how many rows different versions of Excel may or may not support aren't things it's fair to expect your average data entry clerk to know.

On another level, this is a clear failing of the IT services. Excel was not the right tool for this job, but in the middle of a pandemic, no one is entirely sure what they needed. Excel becomes a tempting tool, because pretty much any end user can look at complicated data and slice/shape/chart/analyze it however they like. There's a good reason why they want to use Excel for everything: it's empowering to the users. When they have an idea for a new report, they don't need to go through six levels of IT management, file requests in triplicate, and have a testing and approval cycle to ensure the report meets the specifications. They just… make it.

There are packaged tools that offer similar, purpose built functionality but still give users all the flexibility they could want for slicing data. But they're expensive, and many organizations (especially government offices) will be stingy about who gets a license. They may or may not be easy to use. And of course, the time to procure such a thing was in the years before a massive virus outbreak. Excel is there, on everyone's computer already, and does what they need.

Still, they made the mistake, they saw the consequences, so now we know, they will definitely start taking steps to correct the problem, right? They know that Excel isn't fit-for-purpose, so they're switching tools, right?

From the BBC:

To handle the problem, PHE is now breaking down the data into smaller batches to create a larger number of Excel templates in order to make sure none hit their cap.

Oops.

[Advertisement] Keep the plebs out of prod. Restrict NuGet feed privileges with ProGet. Learn more.
Categories: Technology

CodeSOD: Switched Requirements

Mon, 2020-10-05 07:30

Code changes over time. Sometimes, it feels like gremlins sweep through the codebase and change it for us. Usually, though, we have changes to requirements, which drives changes to the code.

Thibaut was looking at some third party code to implement tooling to integrate with it, and found this C# switch statement:

if (effectID != 10) { switch (effectID) { case 21: case 24: case 27: case 28: case 29: return true; case 22: case 23: case 25: case 26: break; default: switch (effectID) { case 49: case 50: return true; } break; } return false; } return true;

I'm sure this statement didn't start this way. And I'm sure that, to each of the many developers who swung through to add their own little case to the switch their action made some sort of sense, and maybe they knew they should refactor, they need to get this functionality in and they need it now, and code cleanliness can wait. And wait. And wait.

Until Thibaut comes through, and replaces it with this:

switch (effectID) { case 10: case 21: case 24: case 27: case 28: case 29: case 49: case 50: return true; } return false; hljs.initHighlightingOnLoad(); [Advertisement] Otter - Provision your servers automatically without ever needing to log-in to a command prompt. Get started today!
Categories: Technology

Error'd: No Escape

Fri, 2020-10-02 07:30

"Last night&#39;s dinner was delicious!" Drew W. writes.

 

"Gee, thanks Microsoft! It's great to finally see progress on this long-awaited enhancement. That bot is certainly earning its keep," writes Tim R.

 

Derek wrote, "Well, I guess the Verizon app isn't totally useless when it comes to telling me how many notifications I have. I mean, one out of three isn't THAT bad."

 

"Perhaps Ubisoft UK are trying to reach bilingual gamers?" writes Craig.

 

Jeremy H. writes, "Whether this is a listing for a pair of pet training underwear or a partial human torso, I'm not sure if I'm comfortable with that 'metal clicker'."

 

[Advertisement] BuildMaster allows you to create a self-service release management platform that allows different teams to manage their applications. Explore how!
Categories: Technology

CodeSOD: Imploded Code

Thu, 2020-10-01 07:30

Cassi’s co-worker (previously) was writing some more PHP. This code needed to take a handful of arguments, like id and label and value, and generate HTML text inputs.

Well, that seems like a perfect use case for PHP. I can’t possibly see how this could go wrong.

echo $sep."<SCRIPT>CreateField(" . implode(', ', array_map('json_encode', $args)) . ");</SCRIPT>\n";

Now, PHP’s array_map is a beast of a function, and its documentation has some pretty atrocious examples. It’s not just a map, but also potentially a n-ary zip, but if you use it that way, then the function you’re passing needs to be able to handle the right number of arguments, and– sorry. Lost my train of thought there when checking the PHP docs. Back to the code.

In this case, we use array_map to make all our fields JavaScript safe by json_encodeing them. Then we use implode to mash them together into a comma separated string. Then we concatenate all that together into a CreateField call.

CreateField is a JavaScript function. Cassi didn’t supply the implementation, but lets us know that it uses document.write to output the input tag into the document body.

So, this is server side code which generates calls to client side code, where the client side code runs at page load to document.write HTML elements into the document… elements which the server side code could have easily supplied.

I’ll let Cassi summarize:

I spent 5 minutes staring at this trying to figure out what to say about it. I just… don’t know. What’s the worst part? json_encoding individual arguments and then imploding them? Capital HTML tags? A $sep variable that doesn’t need to exist? (Trust me on this one.) Maybe it’s that it’s a line of PHP outputting a Javascript function call that in turn uses document.write to output a text HTML input field? Yeah, it’s probably that one.

hljs.initHighlightingOnLoad(); [Advertisement] Utilize BuildMaster to release your software with confidence, at the pace your business demands. Download today!
Categories: Technology

CodeSOD: A Poor Facsimile

Wed, 2020-09-30 07:30

For every leftpad debacle, there are a thousand “utility belt” libraries for JavaScript. Whether it’s the “venerable” JQuery, or lodash, or maybe Google’s Closure library, there’s a pile of things that usually end up in a 50,000 line util.js file available for use, and packaged up a little more cleanly.

Dan B had a co-worker who really wanted to start using Closure. But they also wanted to be “abstract”, and “loosely coupled”, so that they could swap out implementations of these utility functions in the future.

This meant that they wrote a lot of code like:

initech.util.clamp = function(value, min, max) { return goog.math.clamp(value, min, max); }

But it wasn’t enough to just write loads of functions like that. This co-worker “knew” they’d need to provide their own implementations for some methods, reflecting how the utility library couldn’t always meet their specific business needs.

Unfortunately, Dan noticed some of those “reimplementations” didn’t always behave correctly, for example:

initech.util.isDefAndNotNull(null); //returns true

Let’s look at the Google implementations of some methods, annotated by Dan:

goog.isDef = function(val) { return val !== void 0; // tests for undefined } goog.isDefAndNotNull = function(val) { return val != null; // Also catches undefined since ==/!= instead of ===/!== allows for implicit type conversion to satisfy the condition. }

Setting aside the problems of coercing vs. non-coercing comparison operators even being a thing, these both do the job they’re supposed to do. But Dan’s co-worker needed to reinvent this particular wheel.

initech.util.isDef = function (val) { return val !== void 0; }

This code is useless, since we already have it in our library, but whatever. It’s fine and correct.

initech.util.isDefAndNotNull = initech.util.isDef;

This developer is the kid who copied someone else’s homework and still somehow managed to get a failing grade. They had a working implementation they could have referenced to see what they needed to do. The names of the methods themselves should have probably provided a hint that they needed to be mildly different. There was no reason to write any of this code, and they still couldn’t get that right.

Dan adds:

Thankfully a lot of this [functionality] was recreated in C# for another project…. I’m looking to bring all that stuff back over… and just destroy this monstrosity we created. Goodbye JavaScript!

hljs.initHighlightingOnLoad(); [Advertisement] Keep the plebs out of prod. Restrict NuGet feed privileges with ProGet. Learn more.
Categories: Technology

CodeSOD: Taking Your Chances

Tue, 2020-09-29 07:30

A few months ago, Sean had some tasks around building a front-end for some dashboards. Someone on the team picked a UI library for managing their widgets. It had lovely features like handling flexible grid layouts, collapsing/expanding components, making components full screen and even drag-and-drop widget rearrangement.

It was great, until one day it wasn't. As more and more people started using the front end, they kept getting more and more reports about broken dashboards, misrendering, duplicated widgets, and all sorts of other difficult to explain bugs. These were bugs which Sean and the other developers had a hard time replicating, and even on the rare occassions that they did replicate it, they couldn't do it twice in a row.

Stumped, Sean took a peek at the code. Each on-screen widget was assigned a unique ID. Except at those times when the screen broke- the IDs weren't unique. So clearly, something went wrong with the ID generation, which seemed unlikely. All the IDs were random-junk, like 7902699be4, so there shouldn't be a collision, right?

generateID() { return sha256(Math.floor(Math.random() * 100)). substring(1, 10); }

Good idea: feeding random values into a hash function to generate unique IDs. Bad idea: constraining your range of random values to integers between 0 and 99.

SHA256 will take inputs like 0, nice long, complex strings like "5feceb66ffc86f38d952786c6d696c79c2dbc239dd4e91b46729d73a27fb57e9". And a mildly different input will generate a wildly different output. This would be good for IDs, if you had a reasonable expectation that the values you're seeding are themselves unique. For this use case, Math.random() is probably good enough. Even after trimming to the first ten characters of the hash, I only hit two collisions for every million IDs in some quick tests. But Math.floor(Math.random() * 100) is going to give you a collision a lot more frequently. Even if you have a small number of widgets on the screen, a collision is probable. Just rare enough to be hard to replicate, but common enough to be a serious problem.

Sean did not share what they did with this library. Based on my experience, it was probably "nothing, we've already adopted it" and someone ginned up an ugly hack that patches the library at runtime to replace the method. Despite the library being open source, nobody in the organization wants to maintain a fork, and legal needs to get involved if you want to contribute back upstream, so just runtime patch the object to replace the method with one that works.

At least, that's a thing I've had to do in the past. No, I'm not bitter.

hljs.initHighlightingOnLoad(); [Advertisement] ProGet’s got you covered with security and access controls on your NuGet feeds. Learn more.
Categories: Technology

The Watchdog Hydra

Mon, 2020-09-28 07:30

Ammar A uses Node to consume an HTTP API. The API uses cookies to track session information, and ensures those cookies expire, so clients need to periodically re-authenticate. How frequently?

That's an excellent question, and one that neither Ammar nor the docs for this API can answer. The documentation provides all the clarity and consistency of a religious document, which is to say you need to take it on faith that these seemingly random expirations happen for a good reason.

Ammar, not feeling particularly faithful, wrote a little "watchdog" function. Once you log in, every thirty seconds it tries to fetch a URL, hopefully keeping the session alive, or, if it times out, starts a new session.

That's what it was supposed to do, but Ammar made a mistake.

.comment {border: none} function login(cb) { request.post({url: '/login', form:{username: config.username, password: config.password}, function(err, response) { if (err) return cb(err) if (response.statusCode != 200) return cb(response.statusCode); console.log("[+] Login succeeded"); setInterval(watchdog, 30000); return cb(); }) } function watchdog() { // Random request to keep session alive request.get({url: '/getObj', form:{id: 1}, (err, response)=>{ if (!err && response.statusCode == 200) return console.log("[+] Watchdog ping succeeded"); console.error("[-] Watchdog encountered error, session may be dead"); login((err)=>{ if (err) console.error("[-] Failed restarting session:",err); }) }) }

login sends the credentials, and if we get a 200 status back, we setInterval to schedule a call to the watchdog every 30,000 milliseconds.

In the watchdog, we fetch a URL, and if it fails, we call login. Which attempts to login, and if it succeeds, schedules the watchdog every 30,000 milliseconds.

Late one night, Ammar got a call from the API vendor's security team. "You are attacking our servers, and not only have you already cut off access to most of our customers, you've caused database corruption! There will be consequences for this!"

Ammar checked, and sure enough, his code was sending hundreds of thousands of requests per second. It didn't take him long to figure out why: requests from the watchdog were failing with a 500 error, so it called the login method. The login method had been succeeding, so another watchdog got scheduled. Thirty seconds later, that failed, as did all the previously scheduled watchdogs, which all called login again. Which, on success, scheduled a fresh round of watchdogs. Every thirty seconds, the number of scheduled calls doubled. Before long, Ammar's code was DoSing the API.

In the aftermath, it turned out that Ammar hadn't caused any database corruption, to the contrary, it was an error in the database which caused all the watchdog calls to fail. The vendor corrupted their own database, without Ammar's help. Coupled with Ammar's careless setInterval management, that error became even more punishing.

Which raises the question: why wasn't the vendor better prepared for this? Ammar's code was bad, sure, a lurking timebomb, just waiting to go off. But any customer could have produced something like that. A hostile entity could easily have done something like that. And somehow, this vendor had nothing in place to deal with what appeared to be a denial-of-service attack, short of calling the customer responsible?

I don't know what was going on with the vendor's operations team, but I suspect that the real WTF is somewhere in there.

hljs.initHighlightingOnLoad(); [Advertisement] Utilize BuildMaster to release your software with confidence, at the pace your business demands. Download today!
Categories: Technology

Error'd: Where to go, Next?

Fri, 2020-09-25 07:30

"In this screenshot, 'Lyckades' means 'Succeeded' and the buttons say 'Try again' and 'Cancel'. There is no 'Next' button," wrote Martin W.

 

"I have been meaning to send a card, but I just wasn't planning on using PHP's eval() to send it," Andrew wrote.

 

Martyn R. writes, "I was trying to connect to PA VPN and it seems they think that downgrading my client software will help."

 

"What the heck, Doordash? I was expecting a little more variety from you guys...but, to be honest, I gotta wonder what 'null' flavored cheesecake is like," Joshua M. writes.

 

Nicolas wrote, "NaN exclusive posts? I'm already on the inside loop. After all, I love my grandma!"

 

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

CodeSOD: A Generic Comment

Thu, 2020-09-24 07:30

To my mind, code comments are important to explain why the code what it does, not so much what it does. Ideally, the what is clear enough from the code that you don’t have to. Today, we have no code, but we have some comments.

Chris recently was reviewing some C# code from 2016, and found a little conversation in the comments, which may or may not explain whats or whys. Line numbers included for, ahem context.

4550: //A bit funky, but entirely understandable: Something that is a C# generic on the storage side gets 4551: //represented on the client side as an array. Why? A C# generic is rather specific, i.e., Java 4552: //doesn't have, for example, a Generic List class. So we're messing with arrays. No biggie.

Now, honestly, I’m more confused than I probably would have been just from the code. Presumably as we’re sending things to a client, we’re going to serialize it to an intermediate representation, so like, sure, arrays. The comment probably tells me why, but it’s hardly a necessary explanation here. And what does Java have to do with anything? And also, Java absolutely does support generics, so even if the Java trivia were relevant, it’s not accurate.

I’m not the only one who had some… questions. The comment continues:

4553: // 4554: //WTF does that have to do with anything? Who wrote this inane, stupid comment, 4555: //but decided not to put comments on anything useful?

Not to sound judgmental, but if you’re having flamewars in your code comments, you may not be on a healthy, well-functioning team.

Then again, if this is someplace in the middle of your file, and you’re on line 4550, you probably have some other problems going on.

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

Pages