CodeSOD: Re-Ports |
Crystal Reports falls into that category of tool which promises to help end users accomplish technical tasks easily. They can point it at a database, ask the database a question, and voila, a report pops out, complete with pretty fonts and colors.
Like any such tool, however, there's a point where it starts getting technical. Jon's company passed that point ages ago, and hired on a dedicated Crystal Reports Developer to write reports that were too complicated for the end users. But even that has its limits, and eventually, their reporting needs outgrew what a Crystal Report implemented by their dedicated developer could do.
So Jon's team was handed a pile of reports and told, "implement these as stored procedures in SqlServer." Now, there wasn't anything so fancy as being given requirements or any sort of description about what the reports were for, or what they did, or what would qualify as a successful port version an error. The reports themselves were the requirements: reverse engineer them exactly as written, doing a 1-to-1 mapping wherever possible.
That was mostly fine, but there were a few "special" points in the original reports, where the developer had implemented their own custom Crystal Report functions. Including their own custom date functions. Like this one:
shared DateTimeVar dateObj1;
shared DateTimeVar dateObj2;
If IsDate(ToText(dateObj1,{@DateFormat})) and IsDate (ToText(dateObj2, {@DateFormat}))
then
Now, looking at this, you can see that dateObj1
and dateObj2
are typed as DateTimeVar
s, which yes, means they only hold date times. And thus, in the conditional, we see a pattern we've seen many, many times before: we convert the actual date object back into text and then validate that the text contains a valid date. Which it will, because, again, we started with a date in the first place.
But the goal was to do a 1-to-1 mapping, since no one really knew what the reports actually depended on or how they worked, and no one was going to free up the budget to actually answer those questions. "Thoughtlessly port the code, as accurately as possible," was the order of the day.
So, despite every desire in his body, Jon replaced those ToText
calls with TO_CHAR
and kept the logic exactly the same.
Метки: CodeSOD |
CodeSOD: Query Lockup |
Another day, another time where someone from Brian's team needs to log into their MySQL database and kill a query. This particular query hangs while holding a lock, which hangs up every other query which needs to touch this table, which is a lot of them.
select count(*) INTO @fullCount
FROM SALLSTDM LEFT OUTER JOIN BUYBIDMB ON (MBBUYNBR = 597436 AND MBLOTNBR = SLLOTNBR)
INNER JOIN LOTFILFL FL1 ON (FL1.FLFILTYP = 'A1' AND FL1.FLLOTNBR = SALLSTDM.SLLOTNBR )
INNER JOIN LOTFILFL FL2 ON (FL2.FLFILTYP = 'A1' AND FL2.FLLOTNBR = SALLSTDM.SLLOTNBR )
INNER JOIN LOTFILFL FL3 ON (FL3.FLFILTYP = 'A1' AND FL3.FLLOTNBR = SALLSTDM.SLLOTNBR )
INNER JOIN LOTFILFL FL4 ON (FL4.FLFILTYP = 'A1' AND FL4.FLLOTNBR = SALLSTDM.SLLOTNBR )
INNER JOIN LOTFILFL FL5 ON (FL5.FLFILTYP = 'A1' AND FL5.FLLOTNBR = SALLSTDM.SLLOTNBR )
INNER JOIN LOTFILFL FL6 ON (FL6.FLFILTYP = 'A1' AND FL6.FLLOTNBR = SALLSTDM.SLLOTNBR )
INNER JOIN LOTFILFL FL7 ON (FL7.FLFILTYP = 'A1' AND FL7.FLLOTNBR = SALLSTDM.SLLOTNBR )
INNER JOIN LOTFILFL FL8 ON (FL8.FLFILTYP = 'A1' AND FL8.FLLOTNBR = SALLSTDM.SLLOTNBR )
INNER JOIN LOTFILFL FL9 ON (FL9.FLFILTYP = 'A1' AND FL9.FLLOTNBR = SALLSTDM.SLLOTNBR )
INNER JOIN LOTFILFL FL10 ON (FL10.FLFILTYP = 'A1' AND FL10.FLLOTNBR = SALLSTDM.SLLOTNBR )
WHERE 1=1 AND SALLSTDM.SLYRDNBR not in(450,451,452)
Now, I actually think the WTF is the code that generates this query, but Brian didn't send us that. Maybe Brian doesn't even know where that is. But for some reason, this query joins back to the LOTFILFL
table ten times. It's the same condition, and it's inner joins, so this isn't going to affect the count(*)
of the table.
In an ideal world, someone would track down the source of the query, drag it out behind the barn and be done with it. But we don't live in that world- instead, somebody from Brian's database team gets called outside of regular hours every time this query runs. Sometimes that's once a week. Sometimes that's three or four times a day. No one is empowered to fix it or address the root cause, so it just sits there, festering.
Метки: CodeSOD |
CodeSOD: Mostly Okay |
Taffer is the team lead on a team making security products. As such, they have very strict policies about how they write their code, they have very thorough code review systems, and they also have automated tests for everything.
And yet, things can still slip through.
Taffer submitted this change. It passed two code reviews. It didn't cause any unit tests to fail. It made it into the main branch, and sat there for two months. A team of very experienced, very senior developers didn't catch this glitch until a new hire happened to notice it during a review for something else.
status_t retval = internal_check_struct_integrity(input_struct);
if (retval != OK) {
return OK;
}
It's a simple mistake to make, and would hopefully be a simple mistake to catch, but alas, it turned out to be hard. Obviously, if the integrity check fails, we don't want to return OK
.
It's not all bad. It never made it into a released product, and it has additional benefits, as Taffer explains:
On the plus side, we have a great example of "anyone can make mistakes" to show new hires and interns when we're introducing them to our code review regime.
Метки: CodeSOD |
Error'd: Cinch |
Yankee Ezra A. explains the screenshot below at some length. Says he: "I live in Newton, MA, an affluent, wealthy suburb of Boston. In general, city services are excellent, although the home page of the website is a bit crowded, so I was glad to get an email with a link to the page where I could see how the city is handling my request/complaint about sidewalks, via the city's 311 service (I have no idea what the 311 stands for) When I went to the website, I found what you see in the photo. I guess one can't really complain about one small error in a large website." It's certainly an effective strategy for keeping the complaints box empty!
At the same time, German
Beatrix W.
uncovers a related truth about warranty service.
"I never realised before how hard checking checkboxes
can be. Also never seen them called tickbox before. The
form didn't work anyways and I just got an error. This
was just the experience I had with that company elsewhere."
Most organizations will make very certain that their
revenue-earning pages work well. They're not so
motivated to fix pages that might cost them something.
Right, Ezra?
"My Yams Are Cooked", cried Brian R. "Tried to change my password with my mobile provider, but I guess the joke's on me, MYAC MYAC MYAC."
Wandering Worf wonders "So can I Uber or Lyft a DeLorean back so I can log in and keep my password?"
BorrowerFirstName Christopher R. easily shaded the marketing department of Freedom Mortgage: "Subject lines are rarely so on point." It was a cinch.
Метки: Error'd |
Typos |
Guillaume's company frequently uses consultants. It's a pretty standard setup: Guillaume's employer has many multi-year projects in flight, all of which are layered atop an existing ecosystem of in-house "do everything" applications, each full of their own WTFs.
Because of the complexity, Guillaume's team has a pretty strict code review policy. Someone new to the team will write a merge request, a senior developer will coldly review it and provide huge amounts of comments. By the end of the process, the senior team member may have provided most of the code and architecture via those code review comments, and the junior member is left to just follow the instructions.
When Guillaume was tasked to review a senior contractor's submission though, that felt like a change of pace. Guillaume still left a lot of comments, but it was more obviously a good merge request. Still, "good" is not "ready for production", and Guillaume ended up in a conflict with management: they wanted the merge now, he wanted the merge right: in compliance with their standards, containing no obvious gotchas, and with clear comments and documentation.
"This is a senior consultant," Guillaume's boss said, "and they know the product. You need to have a little trust in your senior developers' skills."
Guillaume tried to explain that it wasn't about trust; everyone needed their code reviewed because more eyes was better. That didn't budge management, so Guillaume found a compromise: the merge could happen, if there was a follow up to fix the "less urgent comments". That made the users happy, that made Guillaume- well, if not happy, not irritated, and it definitely made the boss happy.
One of those "less urgent" comments was on this line:
// Step 1: retrieve the content from the container and filter bt Product Type
Obviously, the intent was "filter by product type", so Guillaume commented: "bt typo". This was the lowest of the low priority, but these small kinds of catches added up over time and kept the code base comprehensible. It was also not the clearest code review comment, but everyone on this project was a native English speaker.
So the consultant "fixed" the code review comment:
// Step 1: retrieve the content from the container and filter typo Product Type
Метки: Feature Articles |
CodeSOD: True Enough |
Managing true and false values is historically challenging. In the world of C, there's even a history to those challenges. Prior to the C99 standard, there wasn't a standardized version of boolean values, but there was a convention which most applications followed, based on how C conditionals and boolean logic works.
In C, anything non-zero is considered "true". So, if(0) { … }
won't execute the branch, but if(99) { … }
will. As a result, when people wanted to make boolean equivalents, they'd use the C preprocessors to specify something like:
#define TRUE 1
#define FALSE 0
This is, with some caveats, essentially what stdbool.h
added in C99. There's no reason why TRUE
needs to specifically be 1, as long as it's anything but zero. That's basically the only goal, as long as TRUE
is anything but zero, if(TRUE)
will behave as expected. I cannot stress enough that the only way to mess this up is to make TRUE
zero. If there's one rule about defining your own boolean constants in C/C++ don't make TRUE
zero.
Anyway, let me turn this over to Charles C:
I worked at a small-medium company that did a lot of subcontracted development work for a wide variety of clients. Every C or C++ project had to include the company's "standard" header field, which was to be included in every code file.
Buried halfway down this 300+-line file was this:
#define TRUE 0
#define FALSE 1
So, if (TRUE) {…}
in this would not execute the branch. If you were using these constants, you'd need patterns like if (variable==TRUE)
which is definitely an anti-pattern that hurts readability and frankly just annoys me. But at its core, this is just a hidden landmine in the code base, which is going to confuse anyone who isn't deeply entrenched in this code, and create huge piles of unnecessary errors.
Метки: CodeSOD |
CodeSOD: Magically True |
Matt P was spending a few hours trawling through a Java code base, working through the poorly documented logic and the bugs and the odd conventions. It was the kind of code base that rightly eschewed "magic numbers", favoring constants, but was also the kind of code base which took that to an extreme which arguably didn't help readability.
For example:
protected static final boolean BOOLEAN_TRUE = true;
protected static final boolean BOOLEAN_FALSE = false;
It's no file not found, but it's important that we clearly define what we mean by true
.
Метки: CodeSOD |
Classic WTF: Color Me Stupid |
It's a holiday in the US today, so we're flipping back through the archives to remember a classic WTF. These colors don't run, because they'll trip over their own shoelaces. Original. --Remy
Andy's company develops solutions for "Industrial" handheld devices. To make deployment and updates easier, they each run a thin client so only the server is different from project to project. This client was written by a long-gone employee in the early nineties, and had barely changed since because it "just worked". Updating it was discouraged for fear of breaking backward-compatibility.
Andy's new project was the first chance he'd had to use it, so he asked a colleague if there was some code that could be used to interface with it. What Andy received was essentially a giant method which responded to the client by cycling through a switch-statement to decide what to paint next based on the current state of the client. Andy took the initiative to create a library for making servers for these things a bit less spaghetti-like, and to encourage this new-fangled concept of code-reuse.
Another long-gone employee had written a fairly usable set of methods for interacting with it. A lot of these methods had a color parameter of type String, which was in turn sent directly to the client as a part of the command sequence. To get a hint of what this String should contain, Andy looked up some calling code and found that there was also a method which converted a standard .Net Color to a String the client would understand. Unfortunately, it looked like this:
private string translateColor(Color color) { if(color == System.Drawing.Color.Black) { return "&H0&"; } else if(color == System.Drawing.Color.Lime) { return "&HFF00&"; } else if(color == System.Drawing.Color.Red) { return "&HFF&"; } else if(color == System.Drawing.Color.Yellow) { return "&HFFFF&"; } else if(color == System.Drawing.Color.Orange) { return "&H80C0FF"; } else if(color == System.Drawing.Color.Blue) { return "&HFF0000"; } else if(color == System.Drawing.Color.LightGray) { return "&H808080"; } else if(color == System.Drawing.Color.DarkGray) { return "&HC0C0C0"; } else if(color == System.Drawing.Color.White) { return "16777215"; } else { //Error - using white return "16777215"; } }
Since Andy was a diligent engineer and wanted to do things properly, he thought that there must be a way to make this more clever, and spent a good hour trying to automatically convert generic Colors into what the accepted formats seemed to be. Unfortunately none of it was understood by the client. Combining the RGB values into one integer seemed promising, but it turned out to only work for white.
Annoyed, Andy spelunked through SVN for the client, found it and looked up the code which converts the String back:
Public Shared Function GetColor(ByVal strColor As String) As Color Select Case strColor Case "&H0&" Return Color.Black Case "0" Return Color.Black 'color Case "&HFF00&" Return Color.Lime Case "65280" Return Color.Lime Case "&HFF&" Return Color.Red Case "255" Return Color.Red Case "&H8000000F" Return Color.LightGray Case "&H808080" Return Color.LightGray Case "-2147483633" Return Color.LightGray Case "&H80000005" Return Color.White Case "16777215" Return Color.White Case "&HFF0000" Return Color.Blue Case "16711680" Return Color.Blue Case "&H80C0FF" Return Color.Orange Case "65535" Return Color.Orange 'bright yellow Case "&HC0C0C0" Return Color.DarkGray Case "&HFFFF&" Return Color.Yellow Case Else Return Color.Purple 'error End Select End Function
Andy pondered all the places from which this was called, and decided to move on.
https://thedailywtf.com/articles/classic-wtf-color-me-stupid
Метки: Feature Articles |
Error'd: Hi-Ho, Hi-Ho |
This week brings an ongoing installment in a long-running gag, and a plea for help with a truly execrable pun. Can someone please find me some map-related material in Idaho? I promise not to credit you directly!
Workaholic Stuart Longland flexes "Yes, I'm working on a Sunday. And yes, I've worked some long hours in the past. But 56 hours in one day? I don't know whether to crow or cry! But TRWTF is the fact these phantom work entries re-incarnate after I delete them." In his own defense, Stuart explains (and I concur) "Monday-Friday is fairly meaningless after COVID and the Queensland Floods."
Searching for the Rubicon, wandert Vivia wondered "Of those places, only Attica is in Greece. Maybe cities now exist in the cloud?"
Non-ex Patrick explains "I recently recalled several emails with mixed date formats, regarding the Fed-Ex updates for some goods I bought in January from Europe. Of the email subject, one email had European, and the other subject had American formatting, in addition to the body shown here. Luckily it only took 2 days, not 11 months, or -10 months." And Patrick, are you located in the US? I wonder if the email formatter was using the source locale for both timezone and formatting of the ship date, and the destination locale for both timezone and formatting of delivery date. There's a certain horrible logic to that. But clearly the right answer is RFC 3339 or nothing.
Coincidentally, Michael B. relates "Expected delivery by time machine?" Or another case of split locales, with shipping across the International Date Line?
Finally, Michael R. clocks in "The Date column is supposed to be the original date and New Date the new postponed one. My 20/02/2022 was especially tricky to figure out." Michael, I looked at the list and didn't see the problem. It must have been fixed since your encounter.
Метки: Error'd |
Minor Revisions |
In many places, driver's licenses work on a point system. As you commit infractions, you gain or lose points, when your point score hits a certain threshold, your insurance company raises your rates or you may even lose your driver's license. Where Christopher Walker lives, you start with twelve points, and each infraction takes a few away. Once a year, you have the option to attend a workshop on safe driving, where you can then regain a few of those points.
It's complicated and tedious, so several organizations, from the local department of motor vehicles to various insurance companies, have set up systems to manage this information. One of those organizations built a PHP application about fifteen years ago, and it gradually grew in kruft and complexity and confusion from that point forward. It works, but it's entirely unmaintainable.
So Christopher was hired to help upgrade it to something hopefully supportable. It's still in PHP, but it's redesigned to use some best practices, switch to Laravel as its framework, and basically be as modular and component-oriented as possible.
The real challenge was porting the existing data into the new system. The old schema was a mess. The "simple" problems were all around the fact that once upon a time the database only used ASCII, but was eventually upgraded to use UTF-8, but however that was done made it so that many characters like ''e' got mangled into '‡' or '§'.
But all of that was nothing compared to the problems updating the revision history tables. The other developers had given up on the revision/audit history many years ago. Instead of providing detailed reports, they simply displayed "[username] changed this participant."
The application tracked an audit log, and it was fairly thorough. At first glance, it even seemed pretty sensible. It had a timestamp, an action code (like "USRUPDATE" or "USRCREATE"), a "detailsaction" which contained what appeared to contain the new value of a modified field, and then a "request" which just seemed to log the raw SQL run to alter the table. That last one didn't seem important, so Christopher went ahead and started porting the old table to the new database.
That's when Christopher hit the first speed bump. Some of the records were sane, comprehensible audit logs. Some of them simply weren't. For some of them, the audit fields conveyed no information. For others, you needed to look at the request
field and try and reconstruct what happened from the raw SQL. Except that was easier said than done: many of the queries in the audit log referenced tables and fields which no longer existed, or had been renamed at some point. By combing through the huge pile of data, Christopher was able to determine that there were only about 20 different ways those queries got deprecated, so it wasn't too hard to come up with a script that could translate them into the new architecture.
The other unusual edge case were that instead of storing SQL in the field, many stored a condensed array representing the row that was altered, like:
a:23:{s:14:"participantsid";i:123456;s:5:"titre";s:8:"Monsieur";s:3:"nom";s:5:"[LAST_NAME]";s:6:"nom_jf";s:0:"";s:6:"prenom";s:6:"[FIRST_NAME]";s:10:"profession";s:1:"0";s:14:"naissance_date";s:10:"xxxx-xx-xx";s:14:"naissance_lieu";s:15:"STRASBOURG (67)";s:8:"adresse1";s:20:"[REDACTED]";s:8:"adresse2";s:0:"";s:11:"code_postal";s:5:"12345";s:5:"ville";s:9:"[REDACTED]";s:4:"tel1";s:14:"[REDACTED]";s:4:"tel2";s:0:"";s:5:"email";s:24:"[REDACTED]@gmail.com";s:6:"membre";s:0:"";s:15:"immatriculation";s:0:"";s:2:"ac";s:3:"NON";s:12:"permisnumero";s:12:"[REDACTED]";s:10:"permisdate";s:10:"2019-01-21";s:10:"permislieu";s:9:"PREFET 67";s:8:"remarque";s:0:"";s:14:"naissance_pays";s:0:"";}
That wasn't terrible to manage, aside from the fact that the dumps didn't actually reference existing tables and fields. Christopher could figure out what the replacement tables and fields were and map the data back to actual audit log entries.
That got Christopher 90% of the way there. But 90% isn't all the way, and the last ten percent was going to take a lot more time. Or perhaps was going to be impossible to do. Because the remaining audit log records stored queries that had nothing to do with the entity that was changed. Many of them weren't even modification statements.
For example, the audit log entry that seemed to be about updating a workshop's status from "active" to "cancelled" was purportedly done by this query: SELECT lieux.departement FROM lieux JOIN stages ON stages.lieuxid = lieux.lieuxid WHERE stages.types = 'PAP' AND stages.stagesid ='123456'
.
Christopher summarizes:
I don't know who decided that this was a good idea or even that this made sense, but I do understand why one of the previous developers of the app decided that "[username] changed this participant." was going to be the only info given in the revisions history.
Метки: Feature Articles |
CodeSOD: New Anti-Pattern Just Dropped |
Linda discovered a new anti-pattern, helpfully explained with comments.
try {
this.initializeConfig(this.configFile);
} catch (ADWException e) {
// something went terrible wrong... but we go on, since
// following errors will be thrown.
}
I'll call this the delayed catch pattern. We know something went wrong, and in this example, we seem to have a mildly specific idea of what went wrong, based on the exception type. We know that it's impossible for the program to continue, but we don't complain. We just ignore the exception and wait for somebody else to notice it, like the person who saw the mess in the breakroom, but are pretending they didn't, so they don't have to clean it up. They'll wait until somebody else needs coffee more than they do, and then act surprised when they discover Carole from down the hall had to spend 30 minutes cleaning up a mess left by god knows who. "Oh no," they'd say with all the sincerity of a used-car salesman, "I walked by but didn't notice the mess. I would have totally stopped and helped if I'd have noticed! Anywho, let me just grab some coffee quick, I've got a meeting in five."
https://thedailywtf.com/articles/new-anti-pattern-just-dropped
Метки: CodeSOD |
CodeSOD: Weakly Courses |
Kerin inherited a scheduling application for a university. This application stored the scheduled days for a class in the database… as one comma-separated field. This was a problem for Kerin, who was hired to add predictive scheduling and classroom density measurements to the system.
This particular function was written to take that data and transform it for display. Sort of.
function getDaysEnrollment($DayStr){
$Days = explode(',',$DayStr);
$StrDay = "";
if(sizeof($Days)>0){
foreach($Days as $day)
{
switch($day)
{
case 'M':
$StrDay .=" Mon,";
break;
case 'T':
$StrDay .=" Tue,";
break;
case 'W':
$StrDay .=" Wen,";
break;
case 'TH':
$StrDay .=" Thu,";
break;
case 'F':
$StrDay .=" Fri,";
break;
}
$StrDay = substr($StrDay,0,-1);
}
}
return $StrDay;
}
So, at its core, this function wants to loop through the list of days and convert them from short abbreviatiosn, like "M", to longer abbreviations, like "Mon". It then keeps concatenating each one together, but with a twist- it strips the commas. $StrDay = substr($StrDay,0,-1);
rips off the last character, which would be the comma. Except they hard coded the comma into the strings they're concatenating in the first place. It's completely superfluous. There's no need for that, they could have just not done the commas.
According to Kerrin, this isn't the worst thing in the code, but it is the "punchiest". I'll let Kerin explain some of the things in this codebase:
[I found this] nestled in amongst mountains of similarly-awful code, Slovenian-language comments, and - I'm being completely serious here - executing code loaded from a text file remote, foreign-language MMORPG blog's domain.
If I were to hazard a guess, the remote code is probably insurance - there were a couple other similar tricks like a concealed PHP shell and an emailer that phones home with the current admin details if the IP address changes.
Метки: CodeSOD |
CodeSOD: Nullable or Not |
Nullable types, at least in theory, make our code simpler and easier to maintain. If nothing else, we know when there's a risk of a null value, and can handle it with some grace. At least, that's how it works if we understand what they do.
Boaz's co-worker knows that nullables are valuable, but doesn't quite get it.
public ulong? ParentOpinionId
{
get
{
return DbAdapter.Parent_Opinion;
}
set
{
DbAdapter.Parent_Opinion = value.Value;
}
}
This is a pretty basic C# getter/setter, and it advertises itself as accepting nullable unsigned longs. And, sure enough, the database field it wraps also supports null values. So the signature advertises that it accepts nulls, the persistence layer supports nulls, but this setter doesn't.
Specifically, value.Value
on a null value throws an exception. This could be solved easily by just… not trying so hard. Parent_Opinion = value
solves this problem perfectly. That's the thing about this- the developer did extra work to get to the wrong result.
Метки: CodeSOD |
Error'd: Nice Work If You Can Get IT |
Danish cookie connoisseur Jorgen N. contributes our starter course this week. "Cloudera has an interesting way of implementing "Required only" cookies." It's an exercise for the frist poster to explain to the peanut gallery what's so distasteful about third-party cookies.
Studious Olivier sagely notes "Anatomy has been a subject of study ever since someone cut some other fellow open, but I don't think Android applications existed when Pontius Pilate washed his hands."
Coincidentally, Brett R. contemporaneously comments "That last build took a little more than 2,020 years, I think the software is already out of date."
Regular Peter G. breaks some big news for cosmology. "Looks like the dark matter mystery has been solved. Bunnings Warehouse in Australia reckons 150ccs of null weighs about 42g!" That must be inclusive of the packaging.
Speaking of cosmology, after Dan N's retirement portfolio took a haircut this week, he decided he might need to check his Social Security benefit for a little reassurance. Imagine his dismay at this bureaucratic brushoff. "I got an email from the SSA telling me that they'd published a new statement of my current progress to earning retirement benefits. When I went to their website to login I found this. They've got anywhere between 4 and 8.5 hours of scheduled downtime every single day! Unless the "database" backing their site is a room full of filing cabinets with hundreds of clerks to transcribe records when a teletype prints out requests I can think of no sane reason for this sort of downtime." Union rules?
https://thedailywtf.com/articles/nice-work-if-you-can-get-it
Метки: Error'd |
CodeSOD: The String Buildulator |
"Don't concatenate long strings," is generally solid advice in most languages. Due to internal representations, strings are frequently immutable and of a fixed length, so a block like this:
string s = getSomeString();
s = s + "some suffix";
creates three strings- the original, the suffix, and the third, concatenated string. Keep spamming instances, especially long ones, if you want to stress test your garbage collector.
While languages will do their best to optimize those kinds of operations, the general advice is to use string builders which can minimize those allocations and boost performance.
Or, you can do what Richard B's predecessor did, and abuse the heck out of string interpolation in C#.
StreamWriter sw = new StreamWriter(filename);
#region Export file header
string header = "";
header = "Title";
header = $"{header},\"First Name\"";
header = $"{header},\"Middle Name\"";
header = $"{header},\"Last Name\"";
header = $"{header},Suffix";
header = $"{header},Company";
header = $"{header},Department";
header = $"{header},\"Job Title\"";
header = $"{header},\"Business Street\"";
header = $"{header},\"Business Street 2\"";
header = $"{header},\"Business Street 3\"";
header = $"{header},\"Business City\"";
header = $"{header},\"Business State\"";
header = $"{header},\"Business Postal Code\"";
header = $"{header},\"Business Country/ Region\"";
header = $"{header},\"Home Street\"";
header = $"{header},\"Home Street 2\"";
header = $"{header},\"Home Street 3\"";
header = $"{header},\"Home City\"";
header = $"{header},\"Home State\"";
header = $"{header},\"Home Postal Code\"";
header = $"{header},\"Home Country/ Region\"";
header = $"{header},\"Other Street\"";
header = $"{header},\"Other Street 2\"";
header = $"{header},\"Other Street 3\"";
header = $"{header},\"Other City\"";
header = $"{header},\"Other State\"";
header = $"{header},\"Other Postal Code\"";
header = $"{header},\"Other Country/ Region\"";
header = $"{header},\"Assistant's Phone\"";
header = $"{header},\"Business Fax\"";
header = $"{header},\"Business Phone\"";
header = $"{header},\"Business Phone 2\"";
header = $"{header},Callback";
header = $"{header},\"Car Phone\"";
header = $"{header},\"Company Main Phone\"";
header = $"{header},\"Home Fax\"";
header = $"{header},\"Home Phone\"";
header = $"{header},\"Home Phone 2\"";
header = $"{header},ISDN";
header = $"{header},\"Mobile Phone\"";
header = $"{header},\"Other Fax\"";
header = $"{header},\"Other Phone\"";
header = $"{header},Pager";
header = $"{header},\"Primary Phone\"";
header = $"{header},\"Radio Phone\"";
header = $"{header},\"TTY/TDD Phone\"";
header = $"{header},Telex";
header = $"{header},Account";
header = $"{header},Anniversary";
header = $"{header},\"Assistant's Name\"";
header = $"{header},\"Billing Information\"";
header = $"{header},Birthday";
header = $"{header},\"Business Address PO Box\"";
header = $"{header},Categories";
header = $"{header},Children";
header = $"{header},\"Directory Server\"";
header = $"{header},\"E - mail Address\"";
header = $"{header},\"E - mail Type\"";
header = $"{header},\"E - mail Display Name\"";
header = $"{header},\"E-mail 2 Address\"";
header = $"{header},\"E - mail 2 Type\"";
header = $"{header},\"E - mail 2 Display Name\"";
header = $"{header},\"E-mail 3 Address\"";
header = $"{header},\"E - mail 3 Type\"";
header = $"{header},\"E - mail 3 Display Name\"";
header = $"{header},Gender";
header = $"{header},\"Government ID Number\"";
header = $"{header},Hobby";
header = $"{header},\"Home Address PO Box\"";
header = $"{header},Initials";
header = $"{header},\"Internet Free Busy\"";
header = $"{header},Keywords";
header = $"{header},Language";
header = $"{header},Location";
header = $"{header},\"Manager's Name\"";
header = $"{header},Mileage";
header = $"{header},Notes";
header = $"{header},\"Office Location\"";
header = $"{header},\"Organizational ID Number\"";
header = $"{header},\"Other Address PO Box\"";
header = $"{header},Priority";
header = $"{header},Private";
header = $"{header},Profession";
header = $"{header},\"Referred By\"";
header = $"{header},Sensitivity";
header = $"{header},Spouse";
header = $"{header},\"User 1\"";
header = $"{header},\"User 2\"";
header = $"{header},\"User 3\"";
header = $"{header},\"User 4\"";
header = $"{header},\"Web Page\"";
#endregion Export file header
sw.WriteLine(header);
The real killer to this is that there's no need for string concatenation at all. There's no reason one needs to WriteLine
the entire header at once. sw.Write("Title,");
Also, string interpolation is almost always more expensive than straight concatenation, and harder for compilers to optimize. I'm not about to benchmark this disaster to prove it, but I suspect this is going to be pretty much the most expensive option.
And don't worry, the same basic process follows for each individual row they're outputting:
string contactRow = "";
HtmlToText htmlToText = new HtmlToText();
bool extendedPropRetrieved = false;
#region Extract properties for export file
if (contact.CompleteName != null)
contactRow = $"\"{contact.CompleteName.Title}\""; // Title
else
contactRow = $"";
contactRow = $"{contactRow},\"{contact.GivenName}\""; // First name
contactRow = $"{contactRow},\"{contact.MiddleName}\""; // Middle name
contactRow = $"{contactRow},\"{contact.Surname}\""; // Last name
if (contact.CompleteName != null)
contactRow = $"{contactRow},\"{contact.CompleteName.Suffix}\""; //Suffix
else
contactRow = $"{contactRow},";
contactRow = $"{contactRow},\"{contact.CompanyName}\""; // Company
contactRow = $"{contactRow},\"{contact.Department}\""; // Department
contactRow = $"{contactRow},\"{contact.JobTitle}\""; // Job title
if (contact.PhysicalAddresses.Contains(PhysicalAddressKey.Business))
{
contactRow = $"{contactRow},\"{contact.PhysicalAddresses[PhysicalAddressKey.Business].Street}\""; // Business street
contactRow = $"{contactRow},"; // Business street 2
contactRow = $"{contactRow},"; // Business street 3
contactRow = $"{contactRow},\"{contact.PhysicalAddresses[PhysicalAddressKey.Business].City}\""; // Business city
contactRow = $"{contactRow},\"{contact.PhysicalAddresses[PhysicalAddressKey.Business].State}\""; // Business state
contactRow = $"{contactRow},\"{contact.PhysicalAddresses[PhysicalAddressKey.Business].PostalCode}\""; // Business postalcode
contactRow = $"{contactRow},\"{contact.PhysicalAddresses[PhysicalAddressKey.Business].CountryOrRegion}\""; // Business country/region
}
else
{
contactRow = $"{contactRow},";
contactRow = $"{contactRow},";
contactRow = $"{contactRow},";
contactRow = $"{contactRow},";
contactRow = $"{contactRow},";
contactRow = $"{contactRow},";
contactRow = $"{contactRow},";
}
// ... this goes on for about 600 lines
The physical address else
block is something really special, here.
Метки: CodeSOD |
CodeSOD: Failed Successfully |
Martin's company had written a set of command line tools which their internal analysts could then string together via shell scripts to do their work. It was finicky and fragile, but frankly didn't work too badly for most cases.
There was one tool, however, which seemed to be the source of an unfair number of problems. Eventually, Martin sat down with an analyst to see what was going wrong. The program would exit successfully, but wouldn't actually do any of the work it was supposed to. Instead of doing the normal thing and writing errors to STDERR, the tool wrote to a file. Which file, however, was determined by reading some shell variables, but the shell variables used by each of the tools were slightly different, because why would you build a consistent interface for your suite of analytical tools?
Eventually, Martin was able to figure out where the errors were going, and saw that it was failing to connect to the backend database. That was a simple matter- just fix the connection string- but why was it exiting successfully when it couldn't connect?
/* Connect to Oracle*/
iStatus = BDconnect();
if(iStatus != SUCCESS_STATUS)
{
write_log(ERROR_LOG_FILE,"BDconnect() FAILED sql:%d",iStatus);
exit(EXIT_SUCCESS);
}
It exited with a successful return code, and thus the shell scripts the analysts were using assumed, wrongly, that the application had succeeded. It wasn't too much to fix this specific case, but as it turned out, this "exit with success even when you fail" was an endemic pattern across many of these tools.
Метки: CodeSOD |
CodeSOD: The GUID Utillity |
Let's say you saw a method called StrToGuid
, in a C# codebase. Your first thought might be: "Wait, isn't there a built in parse? Well, I guess maybe they might do some sort of exception handling. But it still doesn't seem right." And then you'd take a look at the method signature and see that it takes both a string, and an integer named counter
, and you'd think: "Wait, what?"
Henrik H had a similar experience. His team hired a new developer, someone with 15+ years of experience. This is what they contributed to the codebase:
private Guid StrToGuid(string s, int counter)
{
Guid newGuid = new Guid();
if (counter < 10)
Utillity.ScreenAndLog("d", s);
try
{
newGuid = Guid.Parse(s);
_noOfOKGuids++;
}
catch(ArgumentNullException)
{
_noOfEmptyGuids++;
}
catch(FormatException)
{
_noOfErrorGuids++;
}
return newGuid;
}
So, if s
contains a valid GUID, this parses it into a GUID and returns it. Otherwise, it returns a new GUID. It also accepts that counter parameter which clearly exists to log every tenth GUID we attempt to parse, using the "Utillity"(sic) class. Instead of responding to exceptions, we just increment counters- counters which never get checked by any other method, by the way, so this is essentially just "swallowing the exceptions", as it were.
Henrik adds: "Yes, he is related to the boss."
Метки: CodeSOD |
CodeSOD: An Animated Block |
"There are a few more functions like this in the same file," writes Jenny, about today's submission. This is one which largely does speak for itself.
const gright = () => {
setIscountright(isCountright + 1);
if(isCountright === 0) {
setIsleft(!isLeft);
setIsfirstdot(!isFirstdot);
setIssecdot(!isSecdot);
setInfof('Once activated buttons on the right panel will appear');
setIssquareleft(!isSquareleft);
setIsanimBottRightIn(!isAnimBottRightIn);
}
if(isCountright === 1) {
setIssecdot(!isSecdot);
setIsthirddot(!isThirdtdot);
setInfof('Tap on them to change content of the projection on the wall');
setIselmscale(!isElmscale);
setIssquareleft(!isSquareleft);
setIsmap(!isMap);
setIsmapdot(!isMapdot);
setIsborderwhite(!isBorderwhite);
}
if(isCountright === 2) {
setIsright(!isRight);
setIsthirddot(!isThirdtdot);
setIsfourthdot(!isForthdot);
setInfof('Use the menu bar in top left corner to switch between pages');
setIssquareleft(isSquareleft);
setIsanimBottRightIn(!isAnimBottRightIn);
setIselmscale(!isElmscale);
setIsmap(!isMap);
setIsmapdot(!isMapdot);
setIsborderwhite(!isBorderwhite);
setIsindicator(!isIndicator);
setTimeout(():void => {
setAnimain(false);
setMainsec(true);
setIsindicator(false);
setIsindicator2(true);
}, 1000);
setTimeout(():void => {
setMainsec(false);
setMainth(true);
setIsindicator2(false);
setIsindicator3(true);
setShowdone(true);
}, 2200);
}
}
Clearly, this is setting all sorts of display properties inside of a UI component. But it's also using its own internal conventions and namings that just make everything harder to understand. gright
? setMainth
? And those capitalizations. Plus, look at these giant piles of setters- it reads like someone created a bunch of global variables and ther wrapped them in setter functions to make them feel less global.
The logic here isn't that complicated, but because of the names, because of the spammed negations, because of the conventions, I still don't really have any good sense of what it does. Maybe if I knew more of the conventions, I think this code was all gright, but as it is, I'm pretty sure it's all gwrong.
Метки: CodeSOD |
Error'd: Irony |
This week's edition of Err'd gets off to a flying start with one that came in "over the transom" as t'were. Ordinarily, expired certs are a bit mundane for this column, but in this case, where this foible fetched up is at least worth a chuckle.
Jim M. wrote directly to the editor with this explanation. "If you're looking for compliance reports to prove that your cloud provider has solid security practices, be wary of this WTF with Azure. Quoting the site, SOC 2 Type 2 attestation report addresses the requirements set forth in the Cloud Security Alliance (CSA) Cloud Controls Matrix (CCM), and the Cloud Computing Compliance Criteria Catalogue (C5:2020) created by the German Federal Office for Information Security (BSI). Sounds impressive! The link for Azure DevOps SOC 2 Type 2 attestation report goes to this link, https://docs.microsoft.com/en-us/compliance/regulatory/offering-soc-2, which shows that the cert for this page has expired. Try it here: https://servicetrust.microsoft.com/ViewPage/MSComplianceGuideV3 "
An anonymous New Yorker shared this from the land of traffic and tourists. "The real WTF is a function that converts 330 to 3.99," says he. Doesn't seem quite like a euro conversion at current rates. Any ideas?
Richi's no l"o"oli, writing in with a trick captcha: "This google captcha asked me to check all boxes that have a pedestrian crossing in them. As you can see, there is just a motorcycle." The trick is to simply submit the answer without selecting any squares, as there are none that match.
Deutscher Konrad seeks help translating Googlisch. "Thanks Google for the informative overlay. I'm totally okay with the HH:MM but what does the u in front of it want to tell me?" Live in 4 days, at uhm, hm, I don't know?
Luke H. "While other companies may claim to be redefining the modern career, IBM is going one step further and undefining it." I can't improve on that. Enjoy your weekend.
Метки: Error'd |