CodeSOD: Extra Strict |
One of the advantages of a strongly typed language is that many kinds of errors can be caught at compile time. Without even running the code, you know you've made a mistake. This adds a layer of formality to your programs, which has the disadvantage of making it harder for a novice programmer to get started.
At least, that's my understanding of why every language that's designed to be "easy to use" defaults to being loosely typed. The result is that it's easy to get started, but then you inevitably end up asking yourself wat?
Visual Basic was one of those languages. It wanted to avoid spitting out errors at compile time, because that made it "easy" to get started. This meant, for example, that in old versions of Visual Basic, you didn't need to declare your variables- they were declared on use, a feature that persists into languages like Python today. Also, in older versions, you didn't need to declare variables as having a type, they could just hold anything. And even if you declared a type, the compiler would "do its best" to stuff one type into another, much like JavaScript does today.
Microsoft recognized that this would be a problem if a large team was working on a Visual Basic project. And large teams and large Visual Basic projects are a thing that sadly happened. So they added features to the language which let you control how strict it would be. Adding Option Explicit to a file would mean that variables needed to be declared before use. Option Strict would enforce strict type checking, and preventing surprising implicit casts.
One of the big changes in VB.Net was the defaults for those changed- Option Explicit defaulted to being on, and you needed to specify Option Explicit Off to get the old behavior. Option Strict remained off by default, though, so many teams enabled it. In .NET, it was even more important, since while VB.Net might let you play loose with types at compile time, the compiled MSIL output didn't.
Which brings us to Russell F's code. While the team's coding standards do recommend that Option Strict be enabled, one developer hasn't quite adapted to that reality. Which is why pretty much any code that interacts with form fields looks like this:
Public i64Part2 As Int64
'later…
i64Part2 = Format(Convert.ToInt64(txtIBM2.Text), "00000")
txtIBM2 is, as you might guess from the Hungarian tag, a text box. So we need to convert that to a number, hence the Convert.ToInt64. So far so good.
Then, perplexingly, we Format the number back into a string that is 5 characters long. Then we let an implicit cast turn the string back into a number, because i64Part2 is an Int64. So that's a string converted explicitly into a number, formatted into a string and then implicitly converted back to a number.
The conversion back to a number undoes whatever was accomplished by the formatting. Worse, the format give you a false sense of security- the format string only supports 5 digits, but what happens if you pass a 6 digit number in? Nothing: the Format method won't truncate, so your six digit number comes out as six digits.
Maybe the "easy to use" languages are onto something. Types do seem hard.
[Advertisement]
ProGet can centralize your organization's software applications and components to provide uniform access to developers and servers. Check it out!
|
Метки: CodeSOD |
Error'd: Destination Undefined |
"It's good that I'm getting off at LTH, otherwise God knows what'd have happened to me," Elliot B. writes.
"Ummmm...Thanks for the 'great' deal, FedEx?" writes Ginnie.
David wrote, "Sure am glad that they have a men's version of this...I have so many things to do with my kitchen hands."
"I mean, the fact that you can't ship to undefined isn't wrong, but it's not right either," Kevin K. wrote.
Peter G. writes, "This must have been written by physicists, it's within +/- 10% of being correctly sorted."
"As if the thought of regular enemas don't make me clench my cheeks enough, there's this," wrote Quentin G.
[Advertisement]
Ensure your software is built only once and then deployed consistently across environments, by packaging your applications and components. Learn how today!
|
Метки: Error'd |
CodeSOD: I Fixtured Your Test |
When I was still doing consulting, I had a client that wanted to create One App To Rule Them All: all of their business functions (and they had many) available in one single Angular application. They hoped each business unit would have their own module, but the whole thing could be tied together into one coherent experience by setting global stylesheets.
I am a professional, so I muted myself before I started laughing at them. I did give them some guidance, but also tried to set expectations. Ignore the technical challenges. The political challenges of getting every software team in the organization, the contracting teams they would bring in, the management teams that needed direction, all headed in the same direction were likely insurmountable.
Brian isn’t in the same situation, but Brian has been receiving code from a team of contractors from Initech. The Initech contractors have been a problem from the very start of the project. Specifically, they are contractors, and very expensive ones. They know that they are very expensive, and thus have concluded that they must also be very smart. Smarter than Brian and his peers.
So, when Brian does a code review and finds their code doesn’t even approach his company’s basic standards for code quality, they ignore him. When he points out that they’ve created serious performance problems by refusing to follow his organization’s best practices, they ignore him and bill a few extra hours that week. When the project timeline slips, and he starts asking about their methodology, they refuse to tell him a single thing about how they work beyond, “We’re Agile.”
To the shock of the contractors and the management paying the bills, sprint demos started to fail. QA dashboards went red. Implementation of key features got pushed back farther and farther. In response, management decided to give Brian more supervisory responsibility over the contractors, starting with a thorough code review.
He’s been reviewing the code in detail, and has this to say:
Phrases like ‘depressingly awful’ are likely to feature in my final report (the review is still in progress) but this little gem from testing jumped out at me.
it('should detect change', () => {
fixture.detectChanges();
const dt: OcTableComponent = fixture.componentInstance.dt;
expect(dt).toEqual(fixture.componentInstance.dt);
}); This is a Jasmine unit test, which takes a behavioral approach to testing. The it method expects a string describing what we expect “it” to do (“it”, in this context, being one unit of a larger feature), and a callback function which implements the actual test.
Right at the start, it('should detect change',…) reeks of a bad unit test. Doubly so when we see what changes they’re detecting: fixture.detectChanges()
Angular, when running in a browser context, automatically syncs the DOM elements it manages with the underlying model. You can’t do that in a unit test, because there isn’t an actual DOM to interact with, so Angular’s unit test framework allows you to trigger that by calling detectChanges.
Essentially, you invoke this any time you do something that’s supposed to impact the UI state from a unit test, so that you can accurately make assertions about the UI state at that point. What you don’t do is just, y’know, invoke it for no reason. It doesn’t hurt anything, it’s just not useful.
But it’s the meat of the test where things really go awry.
We set the variable dt to be equal to fixture.componentInstance.dt. Then we assert that dt is equal to fixture.componentInstance.dt. Which it clearly is, because we just set it.
The test is named “should detect changes”, which gives us the sense that they were attempting to unit test the Angular test fixture’s detectChanges method. That’s worse than writing unit tests for built-in methods, it’s writing a unit test for a vendor-supplied test fixture: testing the thing that helps you test.
But then we don’t change anything. In the end, this unit test simply asserts that the assignment operator works as expected. So it’s also worse than a test for a built-in method, it’s a test for a basic language feature.
This unit test manages, in a few compact lines, to not simply be bad, but is “not even wrong”. This is the kind of code which populates the entire code base. As Brian writes:
I still have about half this review to go and I dread to think what other errors I may find.
[Advertisement]
ProGet can centralize your organization's software applications and components to provide uniform access to developers and servers. Check it out!
|
Метки: CodeSOD |
CodeSOD: A Short Trip on the BobC |
More than twenty years ago, “BobC” wrote some code. This code was, at the time, relatively modern C++ code. One specific class controls a display, attached to a “Thingamobob” (technical factory term), and reporting on the state of a number of “Doohickeys”, which grows over time.
The code hasn’t been edited since BobC’s last change, but it had one little, tiny, insignificant problem. It would have seeming random crashes. They were rare, which was good, but “crashing software attached to factory equipment” isn’t good for anyone.
Eventually, the number of crash reports was enough that the company decided to take a look at it, but no one could replicate the bug. Johana was asked to debug the code, and I’ve presented it as she supplied it for us:
class CDisplayControl
{
private:
std::vector m_vecIDoohickeys;
std::map m_vecIHelpers;
short m_nNumHelpers;
public:
AddDoohickey(IDoohickey *pIDH, IHelper *pIHlp)
{
// Give Helper to doohickey
pIDH->put_Helper(pIHlp);
// Add doohickey to collection
m_vecIDooHickeys.push_back(pIDH);
pIDH->AddRef();
int nId = m_vecIDooHickeys.size() - 1;
// Add Helper to local interface vector. This is really only done so
// we have easy/quick access to the Helper.
m_nNumHelpers++;
m_vecIHelpers[nId] = pIHlp; // BobC:CHANGED
pIHlp->AddRef();
// Skip deadly function on the first Doohickey.
if (m_nNumHelpers > 1)
{
CallThisEveryTimeButTheFirstOrTheWorldWillEnd();
}
}
}I’m on record as being anti-Hungarian notation. Wrong people disagree with me all the time on this, but they’re wrong, why would we listen to them? I’m willing to permit the convention of IWhatever for interfaces, but CDisplayControl is an awkward class name. That’s just aesthetic preference, though, the real problem is the member declarations:
std::vector m_vecIDoohickeys;
std::map m_vecIHelpers;Here, we have a vector- a resizable list- of IDoohickey objects called m_vecIDoohickeys, which is Hungarian notation for a member which is a vector.
We also have a map that maps shorts to IHelper objects, called m_vecIHelpers, which is Hungarian notation for a member which is a vector. But this is a map. So even if Hungarian notation were helpful, this completely defeats the purpose.
Tracing through the AddDoohickey method, the very first step is that we assign a property on the IDoohickey object to point at the IHelper object. Then we put that IDoohickey into the vector, and create an ID by just checking the size of the vector.
We also increment m_nNumHelpers, another wonderfully Hungarian name, since n tells us that this is a number, but we also need to specify Num in the name too.
It’s important to note: the size of the vector and the value in m_nNumHelpers should match. Then, based on the id, we slot the IHelper object into our map. This is done, according to the comment, “so we have easy/quick access to the Helper”.
Keep in mind, we just assigned the IHelper instance to a property of the IDoohickey, so we already have “quick/easy” access. Quicker, because these are Standard Template Library classes, and while the STL is a powerful set of data-structures, back then speed wasn’t really one of its attributes.
Also, note that BobC didn’t trust source control, which isn’t unreasonable for that long ago, but for only one of the lines changed. Though the tag, “CHANGED” doesn’t really give us much insight into what the change was.
Finally, we use than m_nNumHelpers to see if we’ve run this method at least once, because there’s a step that should only happen when we have more than one IDoohickey and IHelper combination. As Johana’s “corrections” to the code make clear- if we call this at the wrong time, the world will end. We can’t call it the first time through, but we must call it every other time through.
Which, if you carefully check the variable declarations, you’ll catch the root cause of the seemingly random crashes:
short m_nNumHelpers;
In Johana’s world, shorts are 16 bit integers. As these are signed, that means after it hits 32,767, it overflows and wraps back around to negative. So m_nNumHelpers > 1 becomes false, and we stop calling that method which we must call or the world will end.
Most of the time, the equipment gets power-cycled long before they hit the 32,767 invocations of this method, which is why this was so tricky to debug.
Speaking of “tricky to debug,” there’s one more thing I see lurking in here, which based on what I saw in this method, makes me worry. As we know, BobC isn’t super keen on counting, but we see calls to AddRef() in this code. I don’t know, but I suspect that BobC implemented his own reference counting garbage collector.
Real garbage collection, of course, would be to completely refactor this code.
[Advertisement]
Otter - Provision your servers automatically without ever needing to log-in to a command prompt. Get started today!
|
Метки: CodeSOD |
Representative Line: Don't Negate Me |
There are certain problem domains where we care more about the results and the output than the code itself. Gaming is the perfect example: game developers write "bad" code because clarity, readability, maintainability are often subordinate to schedules and the needs of a fun game. The same is true for scientific research: that incomprehensible blob of Fortran was somebody's PhD thesis, and it proved fundamental facts about the universe, so maybe don't judge it on how well written it is.
Sometimes, finance falls into similar place. Often, the software being developer has to implement obtuse business rules that accreted over decades of operation; sometimes it's trying to be a predictive model; sometimes a pointy-haired-boss got upset about how a dashboard looked and asked for the numbers to get fudged.
But that doesn't mean that we can't find new ways to write bad code in any of these domains. Ren'e works in finance, and found this unique JavaScript solution to converting a number to a negative value:
/**
* Reverses a value a number to its negative
* @param {int} value - The value to be reversed
* @return {number} The reversed value
*/
negateNumber(value) {
return value - (value * 2);
}
JavaScript numbers aren't integers, they're double-precision floats. Which does mean that you could exceed the range when you double. That would require you to be tracking numbers larger than 2^52, though, which we can safely assume isn't happening in a financial system, unless inflation suddenly gets cosmically out of hand.
Ren'e has since replaced this with a more "traditional" approach to negation.
[Advertisement]
Utilize BuildMaster to release your software with confidence, at the pace your business demands. Download today!
|
Метки: Representative Line |
CodeSOD: Selected Sort |
Before Evalia took a job at Initech, her predecessor, "JR" had to get fired first. That wasn't too much of a challenge, because JR claimed he was the "God of JavaScript". That was how he signed each of the tickets he handled in the ticket system.
JR was not, in fact, a god. Since then, Evalia has been trying to resuscitate the projects he had been working on. That's how she found this code.
function sortSelect(selElem) {
var tmpAry = new Array();
for (var i=0;inew Array();
tmpAry[i][0] = selElem.options[i].text;
tmpAry[i][1] = selElem.options[i].value;
}
tmpAry.sort();
while (selElem.options.length > 0) {
selElem.options[0] = null;
}
for (var i=0;ivar op = new Option(tmpAry[i][0], tmpAry[i][1]);
selElem.options[i] = op;
}
return;
}
This code sorts the elements in a drop down list, and it manages to do this in a… unique way.
First, we iterate across the elements in the list of options. We build a 2D array, where the first axis is the item, and the second axis contains the text caption and value of each option element.
Once we've built that array, we can sort it. Fortunately for us, when you sort a 2D array, JavaScript helpfully defaults to sorting by the first element in the second dimension, so this will sort by the text value.
Now that we have a sorted list of captions and values, we have to do something about the pesky old ones. So we iterate across the list to set each one to null. Well, not quite. We actually set the first item to null until the length is 0. Fortunately for us, the JavaScript length only takes into account elements with actual values, so this works.
Once they're all empty, we can repopulate the list by using our temporary array to create new options and put them in the list.
Credit to JR, I actually learned new things about JavaScript when wrying to understand this code. I didn't know how sort behaved with 2D arrays, and I'd never seen the while/length construct before, and was shocked that it actually works. Of course, I'd never gotten myself into a situation where I'd needed those.
The truly "god-like" thing is that JR managed to take the task of sorting a list of items and turned it into a task that needed to visit each item in the list three times in addition to sorting. God-like, sure, but the kind of god that Lovecraft warned us about.
[Advertisement]
Utilize BuildMaster to release your software with confidence, at the pace your business demands. Download today!
|
Метки: CodeSOD |
Error'd: Errors as Substitution for Success |
"Why would I be a great fit? Well, [Recruiter], I can [Skill], [Talent], and, most of all, I am certified in [qualification]." David G. wrote.
Dave writes, "For years, I've gone by Dave, but from now you can just call me 'Und'."
"Sure, BBC Shop, why not, %redirect_to_store_name% it is," wrote Robin L.
Christer writes, "Turns out that everything, even if data is missing, has a price."
"Well...I have been debating if I should have opted for a few dozen extra exabytes recently," Jon writes.
Dustin W. wrote, "$14 Million seems a bit steep for boots, but hey, maybe it's because the shoes come along with actual timberland?"
[Advertisement]
ProGet supports your applications, Docker containers, and third-party packages, allowing you to enforce quality standards across all components. Download and see how!
https://thedailywtf.com/articles/errors-as-substitution-for-success
|
Метки: Error'd |
Representative Line: Separate Replacements |
There's bad date handling code. There's bad date formatting code. There's bad date handling code that abuses date formatting to stringify dates. There are cases where the developer clearly doesn't know the built-in date methods, and cases where they did, but clearly just didn't want to use them.
There's plenty of apocalypticly bad date handling options, but honestly, that gets a little boring after awhile. My personal favorite will always be the near misses. Code that almost, but not quite, "gets it".
Karl's co-worker provided a little nugget of exactly that kind of gold.
formattedID = DateTime.Now.ToString("dd / MM / yyyy").Replace(" / ", "")
Here, they understand that a ToString on a DateTime allows you to pass a format string. They even understand that the format string lets you customize the separators (or they think spaces are standard in date formats). But they didn't quite make the leap to thinking, "hey, maybe I don't need to supply separators," so they Replace them.
There are probably a few other replacements that need to be made in the codebase.
[Advertisement]
ProGet supports your applications, Docker containers, and third-party packages, allowing you to enforce quality standards across all components. Download and see how!
|
Метки: Representative Line |
CodeSOD: Dating Automation |
Good idea: having QA developers who can build tooling to automate tests. Testing is tedious, testing needs to be executed repeatedly, and we're not just talking simple unit tests, but in an ideal world key functionality gets benchmarked against acceptance tests. API endpoints get routinely checked.
There's costs and benefits to this though. Each piece of automation is another piece of code that needs to be maintained. It needs to be modified as requirements change. It can have bugs.
And, like any block of code, it can have WTFs.
Nanette got a ticket from QA, which complained that one of the web API endpoints wasn't returning data. "Please confirm why this API isn't returning data."
It didn't take long before Nanette suspected the problem wasn't in the API, but may be in how QA was figuring out its date ranges:
private void setRange(int days){
DateFormat df = new SimpleDateFormat("yyyy-MM-dd")
Date d = new Date();
Calendar c = Calendar.getInstance()
c.setTime(d);
Date start = c.getTime();
if(days==-1){
c.add(Calendar.DAY_OF_MONTH, -1);
assertThat(c.getTime()).isNotEqualTo(start);
}
else if(days==-7){
c.add(Calendar.DAY_OF_MONTH, -7);
assertThat(c.getTime()).isNotEqualTo(start);
}
else if (days==-30){
c.add(Calendar.DAY_OF_MONTH, -30);
assertThat(c.getTime()).isNotEqualTo(start);
}
else if (days==-365){
c.add(Calendar.DAY_OF_MONTH, -365);
assertThat(c.getTime()).isNotEqualTo(start);
}
from = df.format(start).toString()+"T07:00:00.000Z"
to = df.format(d).toString()+"T07:00:00.000Z"
}
Now, the Java Calendar object is and forever the real WTF with dates. But Java 8 is "only" a few years back, so it's not surprising to see code that still uses that API. Though "uses" might be a bit too strong of a word.
The apparent goal is to set a date range that is one day, one week, one month, or one year prior to the current day. And we can trace through that logic, by checking out the calls to c.add, which even get asserted to make sure the built-in API does what the built-in API is supposed to do.
None of that is necessary, of course- if you only want to support certain values, you could just validate those and simply do c.add(Calendar.DAY_OF_MONTH, days). You can keep the asserts if you want.
But none of that is necessary, because after all that awkward validation, we don't actually use those calculated values. from is equal to start which is equal to the Calendar's current time, which it got from d, which is a new Date() which means it's the current time.
So from and to get to be set to the current time, giving us a date range that is 0 days long. Even better, we can see that from and to, which are clearly class members, are string types, thus the additional calls to DateFormat.format. Remember, .format returns a string, so we have an extra call to toString which really makes sure that this is a string.
The secret downside to automated test suites is that you need to write tests for your tests, which eventually get complicated enough that you need to write tests for your tests which test your tests, and before you know it, you're not writing any code that does real work at all.
Which, in this case, maybe writing no code would have been an improvement.
[Advertisement]
Continuously monitor your servers for configuration changes, and report when there's configuration drift. Get started with Otter today!
|
Метки: CodeSOD |
CodeSOD: Reasonable Lint |
While testing their application, Nicholas found some broken error messages. Specifically, they were the embarassing “printing out JavaScript values” types of errors, so obviously something was broken on the server side.
“Oh, that can’t be,” said his senior developer. “We have a module that turns all of the errors into friendly error messages. We use it everywhere, so that can’t be the problem.”
Nicholas dug in, and found this NodeJS block, written by that senior developer.
const reasons = require('reasons');
const handleUploadError = function (err, res) {
if (err) {
var code = 500;
var reason = reasons([{ message: 'Internal Error'}])
if (err === 'errorCondition1') {
code = 400;
reason = reasons([{message: 'Message 1'}]);
} else if (err === 'errorCondition2') {
code = 400;
reason = reasons([{message: 'Message 2'}]);
} else if (err === 'errorCondition3') {
code = 422;
reason = reasons([{message: 'Message 3'}]);
// else if pattern repeated for about 50 lines
// ...
}
return res.status(code).send({reasons: reasons});
}
res.status(201).json('response');
};We start by pulling in that aforementioned reasons module, and stuffing it into a variable. As we can see later on, that module clearly exports itself as a single function, as we see it get invoked like so: reason = reasons([{message: 'Internal Error'}])
And if you skim through this function, everything seems fine. At first glance, even Nicholas thought it was fine. But Nicholas has been trying to get his senior developer to agree that code linting might be a valuable thing to build into their workflow.
“We don’t need to add an unnecessary tool or checkpoint to our process,” the senior continued to say. “Just write better code.”
When Nicholas ran this “unnecessary tool”, in complained about this line: var reason = reasons([{ message: 'Internal Error'}]). reason was assigned a value, but it was never used.
And sure enough, if you scroll down to the line where we actually return our error messages, we do it like this:
return res.status(code).send({reasons: reasons});reasons contains the library function we use to load error messages.
This code had been in production for months before Nicholas noticed it while doing regression testing on some of his changes in a related module. With this evidence about the value of linters, maybe the senior dev will listen to reason.
[Advertisement]
ProGet can centralize your organization's software applications and components to provide uniform access to developers and servers. Check it out!
|
Метки: CodeSOD |
CodeSOD: The Sound of GOTO |
Let's say you have an audio file, or at least, something you suspect is an audio file. You want to parse through the file, checking the headers and importing the data. If the file is invalid, though, you want to raise an error. Let's further say that you're using a language like C++, which has structured exception handling.
Now, pretend you don't know how to actually use structured exception handling. How do you handle errors?
Adam's co-worker has a solution.
char id[5]; // four bytes to hold 'RIFF'
bool ok = false;
id[sizeof(id) - 1] = 0;
do {
size_t nread = fread(id, 4, 1, m_sndFile); // read in first four bytes
if (nread != 1) {
break;
}
if (strcmp(id, "RIFF")) {
break;
}
// ...
// 108 more lines of file parsing code like this
// ...
ok = true;
} while (time(0L) == 0);
// later
if (ok) {
//pass the parsed data back
} else {
//return an error code
}
This code was written by someone who really wanted to use goto but knew it'd never pass code review. So they reinvented it. Our loop is a do/while with a condition which will almost certainly be false- time(0L) == 0). Unless this code is run exactly at midnight on January 1st, 1970 (or on a computer with a badly configured clock), that condition will always be false. Why not while(false)? Presumably that would have been too obvious.
Also, and I know this is petty relative to everything else going on, the time function returns a time_t struct, and accepts a pointer to a time_t struct, which it can initialize. If you just want the return value, you pass in a NULL- which is technically what they're doing by passing 0L, but that's a cryptic way of doing it.
Inside the loop, we have our cobb-jobbed goto implementation. If we fail to read 4 bytes at the start, break to the end of the loop. If we fail to read "RIFF" at the start, break to the end of the loop. Finally, after we've loaded the entire file, we set ok to true. This allows the code that runs after the loop to know if we parsed a file or not. Of course, we don't know why it failed, but how is that ever going to be useful? It failed, and that's good enough for us.
This line: char id[5]; // four bytes to hold 'RIFF' also gives me a chuckle, because at first glance, it seems like the comment is wrong- we allocate 5 bytes to hold "RIFF". Of course, a moment later, id[sizeof(id) - 1] = 0; null-terminates the string, which lets us use strcmp for comparisons.
Which just goes to show, TRWTF is C-style strings.
In any case, we don't know why this code was written this way. At a guess, the original developer probably did know about structured exception handling, muttered something about overhead and performance, and then went ahead on and reinvented the goto, badly.
[Advertisement]
Continuously monitor your servers for configuration changes, and report when there's configuration drift. Get started with Otter today!
|
Метки: CodeSOD |
Error'd: Call Me Maybe (Not) |
Jura K. wrote, "Cigna is trying to answer demand for telehealth support, but apparently they are a little short on supply."
"While Noodles World Kitchen's mobile app is really great with placing orders, it's less than great at handling linear time," writes Robert H.
Hans K. wrote, "Whoever is in charge of sanitizing the text didn't know about C# generics."
"These PDFs might also be great in my Chocolate Cake!" Randolf writes. Hint: Look up "Bitte PDF drucken"
Carl C. writes, "I wanted to have plenty to read in my Kindle app while I was self-isolating at home, but 18 kajillion pages?"
"I mean, I guess the error message about the error message not working might be preferrable to an actual Stack Overflow," James B. wrote.
[Advertisement]
ProGet supports your applications, Docker containers, and third-party packages, allowing you to enforce quality standards across all components. Download and see how!
|
Метки: Error'd |
CodeSOD: A Quick Escape |
I am old. I’m so old that, when I entered the industry, we didn’t have specializations like “frontend” and “backend” developers. You just had developers, and everybody just sort muddled about. As web browsers have migrated from “document display tool” to “enh, basically an operating system,” in terms of complexity, these two branches of development have gotten increasingly siloed.
Which creates problems, like the one Carlena found. You see, the front-end folks didn’t like the way things like quotes were displaying. A quote or a single quote should be represented as a character entity- ', for example.
Now, our frontend developers could have sanitized the strings for display on the client side, but making sure the frontend got good data was a backend problem, to their mind. But the backend developer was out of the office on vacation, so what were our struggling frontend folks to do?
def CustomerHelper.html_encode(string)
string.to_str.gsub(";",";").gsub("<","<").gsub(">",">").gsub("\"",""").gsub("'","'").gsub(")",")").gsub("%","%").gsub("@", "@")
endWell, that doesn’t look so bad, does it? It’s a little weird that they’re escaping ) but not (, but that’s probably harmless. Certainly, this isn’t the best way, but it’s not terrible…
Except that the frontend developers didn’t wrap this around sending the data to the frontend. They wrapped this around the save logic. When the name, address, email address, or company name were saved, they’d be saved with HTML entities right in line.
After a quick round of testing, the frontend folks happily saw that everything worked for them, and went back to tweaking CSS rules and having fights over how whether CSS classnames should reflect purpose or behavior.
There was just one little problem. The frontend wasn’t the only module which consumed this data. Some of them escaped strings on the client side. So, when the user inputs their name as “Miles O’Keefe”, the database stores “Miles O'Keefe”. When client code that escapes on the client side fetches, they convert that into “Miles O'Keefe”.
The email sending modules, though, were the ones that had the worst time of it, as every newly modified email address became miles.okeefe@howmuchkeef.com.
Thus the system sat, until the back-end developer got back from their vacation, and they got to head up all the cleanup and desanitization of a week’s worth of garbage being added to the database.
[Advertisement]
ProGet supports your applications, Docker containers, and third-party packages, allowing you to enforce quality standards across all components. Download and see how!
|
Метки: CodeSOD |
Rushin' Translation |
Cid works for a German company. From day one, management knew that they wanted their application to be multi-lingual, if nothing else because they knew they needed to offer it in English. So from the ground up, the codebase was designed to make localization easy; resource files contained all the strings, the language specific ones could be loaded dynamically, and even UI widgets could flex around based on locale needs.
In the interests of doing it right, when it came time to make the English version, they even went out and contracted a translation company. A team of professional translators went through the strings, checked through the documentation and the requirements, even talked to stakeholders to ensure accurate translations. The English version shipped, and everyone- company and customers included were happy with the product.
Cid’s employer got a lot of good press- their product was popular in its narrow domain. Popular enough that a Russian company called Инитеч came around. They wanted to use the product, but they wanted a Russian localization.
“No problem,” said the sales beast. “We can make that happen!”
Management was less enthused. When localizing for English, they knew they had a big market, and they knew that it was worth doing it right, but even then, it was expensive. Looking at the bottom line, it just didn’t make sense to put that kind of effort into the project for just one customer.
The sales beast wasn’t about to let this sale slip through their fingers, though. And Инитеч really wanted to use their product. And hey, Инитеч had a few employees who had taken a semester of English in school at some point. They could do the translation! They weren’t even looking to score a deal on support, they’d buy the software and do the translation themselves.
“Free” sounded good, so management gave their blessing. Since the customer was doing all the work, no one put too much thought into timelines, or planning, or quality control. Which meant that timelines slipped, there was no plan for completing the translation, and the quality control didn’t happen until Cid had the bright idea of realizing that co-worker Marya was natively Russian and asked her to take a look at the translations.
“Oh, these are great,” Marya said, “if the translator doesn’t speak either German or Russian.” The translations were roughly equivalent to taking the German original, slapping it through Google Translate to get to English, then eventually migrating to Russian by way of Hindi and Portuguese.
The problems with the translation were escalated up to management, and a bunch of meetings happened to debate what to do. On one hand, these were the translations the customer made, and thus they should be happy with it. On the other, they were terrible, and at the end of the day, Cid’s employer needed to be able to stand behind its product.
At this point, Инитеч was getting antsy. They’d already put a lot of work into doing the translations, and had been trying to communicate the software changes to their users for months. They didn’t have anything at all to show for their efforts.
Someone in the C-level offices made the call. They’d hire a professional translator, but they’d aggressively manage the costs. They laid out a plan. They set a timeline. They established acceptance criteria.
They set their timeline, however, without talking to the translation company. Essentially, they were hoping to defeat the “triangle”: they wanted to have the translation be good, be cheap, and be done fast. Reality stepped in: either they needed to pay more to bring on more translators, or they needed to let timelines slip farther.
What started as a quick sale with only minimal upfront investment stretched out into a year of effort. With everyone rushing but making no progress, mistakes started cropping up. One whole module’s worth of text was forgotten in the scope document agreed to by the translation company. Someone grabbed an old version of the resource file when publishing a test build, which created a minor panic when everything was wrong. Relations with Инитеч started to break down, and the whole process went on long enough that the Инитеч employee which started the purchase changed jobs, and another contact came in with no idea of what was in flight.
Which is why, when the sales beast finally was able to tell Инитеч that they had a successful Russian localization, the contact at Инитеч said, “That… is nice? Is this a sales call? Are you trying to sell us this? We just purchased a similar product from your competitor six months ago.”
[Advertisement]
Continuously monitor your servers for configuration changes, and report when there's configuration drift. Get started with Otter today!
|
Метки: Feature Articles |
CodeSOD: The Evil CMS |
Content Management Systems always end up suffering, at least a little, from the Inner Platform Effect. There’s the additional problem that, unlike say, a big ol’ enterprise HR system or similar, CMSes are useful for just about everyone. It’s a quick and easy way to put together a site which anyone can maintain. But it never has enough features for your content. So you always install plugins- plugins of wildly varying quality and compatibility.
Lucio Crusca was doing a security audit of a Joomla site, found this block inside an installed plugin:
".$MyForm->formrow->scriptcode);
echo "//]]>\n";
echo "\n";
}
?>Let’s just focus on the echos to start. We’re directly outputting a
|
Метки: CodeSOD |
CodeSOD: A Tern Off |
Jim J's co-worker showed him this little snippet in the codebase.
foreach (ToolStripMenuItem item in documentMenuItem.DropDownItems)
{
item.Enabled = item.Enabled ? Document.Status == DocumentStatusConsts.New : item.Enabled;
}
Tracing through the ternary, if the menu item is currently enabled, set it enabled if the document in question is new, otherwise set it to itself (that is to say, disabled).
Or, to put it differently, if it's not enabled, make sure it's not enabled.
My suspicion is that the original developer just really wanted to use a ternary, even if it didn't make much sense.
Jim writes:
When one of my colleagues showed me his find I suggested him to add this line into the loop:
if (!item.Enabled) item.Enabled = false || item.Enabled;Just to be absolutely sure the item will be disabled.
[Advertisement]
ProGet can centralize your organization's software applications and components to provide uniform access to developers and servers. Check it out!
|
Метки: CodeSOD |
Error'd: Burrito Font |
"I've always ordered my burritos in Times New Roman. I'll have to make sure to try the Helvetica option next time I go in," Winston M. writes.
"Giving its all and another 5%. That's a battery that I can be seriously proud of," wrote Chris.
James S. writes, "What are the odds that the amount of entropy that went into my password would result in personal data of mine. Now if only I knew what it was!"
Paul writes, "Announcement about the cloud? Something about AI? Perhaps an massively useful new feature added to Windows. This email can be whatever you want it to be!"
"Well, I guess I can let the price slide, it is an estimate after all," Carl C. wrote.
Peter W. writes, "I've spent a bit of money to get the best laptop within my budget and when I looked up what type of hardware hp have put into their Omen device, I was glad to see they had the most excellent microprocessor cache, video graphics, and audio system (not visible here) that is available in the world.
[Advertisement]
Otter - Provision your servers automatically without ever needing to log-in to a command prompt. Get started today!
|
Метки: Error'd |
CodeSOD: WTFYou, Pay Me |
Julien’s employer has switched their payroll operations to a hosted solution. The hosted solution has some… interesting features. The fact that it has a “share” button, implying you can share your paystub infromation with other people is unusual (but good: keeping salaries confidential only helps management underpay their employees). More problematic is that this feature emails it, and instead of putting in an email address manually, you instead pick off a drop-down list- which contains the email of every user of the hosted system.
Seeing this, Julien had to take a peek at the code, just to see what other horrors might lurk in there.
Let’s open with some ugly regexes:
var regExtURL =/(http(s)?|ftp:\/\/.)?(www\.)?[-a-zA-Z0-9@:%._\+~#=]{2,256}\.[a-z]{2,6}\b([-a-zA-Z0-9@:%_\+.~#?&//=]*)/;
///^(?:(?:https?|ftp):\/\/)?[\w.-]+(?:\S+(?::\S*)?@)?(?:(?!(?:0|127)(?:\.\d{1,3}){3})(?!(?:169\.254|192\.168)(?:\.\d{1,3}){2})(?!172\.(?:1[6-9]|2\d|3[0-1])(?:\.\d{1,3}){2})(?:[1-9]\d?|1\d\d|2[01]\d|22[0-3])(?:\.(?:1?\d{1,2}|2[0-4]\d|25[0-5])){2}(?:\.(?:[1-9]\d?|1\d\d|2[0-4]\d|25[0-4]))|(?:(?:[a-z\u00a1-\uffff0-9]-*)*[a-z\u00a1-\uffff0-9]+)(?:\.(?:[a-z\u00a1-\uffff0-9]-*)*[a-z\u00a1-\uffff0-9]+)*(?:\.(?:[a-z\u00a1-\uffff]{2,}))\.?)(?::\d{2,5})?(?:[/?#]\S*)?$/;
function isValidURL(thisObj){
if (thisObj.value != '' && !regExtURL.test(thisObj.value)){
alert('Veuillez entrer une URL valide.');
return false;
}
};
var re = /^(([a-zA-Z0-9-_"+"]+(\.[a-zA-Z]+)*)|(".+"))@((\[[0-9]{1,3}\.[0-9]{1,3}\.[0-9]{1,3}\.[0-9]{1,3}\])|([0-9]{1,3}\.[0-9]{1,3}\.[0-9]{1,3}\.[0-9]{1,3})|(([a-zA-Z0-9])+(\-[a-zA-Z0-9]+)*(\.[a-zA-Z0-9-]+)*(\.[a-zA-Z]{2,})+))$/;
function isEmailKey(thisObj){
//var re = /^(([^<>()\[\]\\.,;:\s@"]+(\.[^<>()\[\]\\.,;:\s@"]+)*)|(".+"))@((\[[0-9]{1,3}\.[0-9]{1,3}\.[0-9]{1,3}\.[0-9]{1,3}])|(([a-zA-Z0-9]+\.)+[a-zA-Z]{2,}))$/;
//var re = /^(([^<>()\[\]\\.,;:\s@"]+(\.[^<>()\[\]\\.,;:\s@"]+)*)|(".+"))@((\[[0-9]{1,3}\.[0-9]{1,3}\.[0-9]{1,3}\.[0-9]{1,3}\])|([0-9]{1,3}\.[0-9]{1,3}\.[0-9]{1,3}\.[0-9]{1,3})|(([a-zA-Z0-9])+(\-[a-zA-Z0-9]+)*(\.[a-zA-Z0-9-]+)*(\.[a-zA-Z]{2,})+))$/;
if (thisObj.value != '' && !re.test(thisObj.value)){
alert('Please enter a valid email address.');
return false;
}
};I like that they kept their old attempts at validating email addresses right there in the code. Real helpful for us. Presenting errors via the built-in alert method is also really great UX.
Do you want a super complex and probably inaccurate date validator? We’ve got one of those:
function validateDateValue(obj, format, errorMesage){
try {
format = format.toUpperCase();
if(obj != null){
var dateValue = obj.value;
if (dateValue.length == 0)
return;
if (dateValue.length > 10)
dateValue = dateValue.substring(0, 10);
if (dateValue.length < 6) {
alert(errorMesage);
return;
}
var d = null;
var sep = getSeparator(format);
if (sep.length > 0)
d = stringToDate(dateValue, format, sep);
else d = Date.parse(dateValue.substring(0,4) + '-' + dateValue.substring(4,6) + '-' + dateValue.substring(6,8));
if (d == null) {
if (dateValue.length == 6 ) {
if (d == null)
d = stringToDate(dateValue,"ddMMyy","");
if (d == null)
d = stringToDate(dateValue,"MMddyy","");
} else if (dateValue.length == 8 ) {
d = Date.parse(dateValue.substring(0,4) + '-' + dateValue.substring(4,6) + '-' + dateValue.substring(6,8));
if(isNaN(d))
d = null;
if (d == null)
d = stringToDate(dateValue,"dd/MM/yy","/");
if (d == null)
d = stringToDate(dateValue,"dd-MM-yy","-");
if (d == null)
d = stringToDate(dateValue,"dd.MM.yy",".");
if (d == null)
d = stringToDate(dateValue,"dd MM yy"," ");
if (d == null)
d = stringToDate(dateValue,"MM/dd/yy","/");
if (d == null)
d = stringToDate(dateValue,"MM-dd-yy","-");
if (d == null)
d = stringToDate(dateValue,"MM.dd.yy",".");
if (d == null)
d = stringToDate(dateValue,"MM dd yy"," ");
if (d == null)
d = stringToDate(dateValue,"yy/MM/dd","/");
if (d == null)
d = stringToDate(dateValue,"yy-MM-dd","-");
if (d == null)
d = stringToDate(dateValue,"yy.MM.dd",".");
if (d == null)
d = stringToDate(dateValue,"yy MM dd"," ");
} else {
if (d == null)
d = stringToDate(dateValue,"dd/MM/yyyy","/");
if (d == null)
d = stringToDate(dateValue,"dd-MM-yyyy","-");
if (d == null)
d = stringToDate(dateValue,"dd.MM.yyyy",".");
if (d == null)
d = stringToDate(dateValue,"dd MM yyyy"," ");
if (d == null)
d = stringToDate(dateValue,"MM/dd/yyyy","/");
if (d == null)
d = stringToDate(dateValue,"MM-dd-yyyy","-");
if (d == null)
d = stringToDate(dateValue,"MM.dd.yyyy",".");
if (d == null)
d = stringToDate(dateValue,"MM dd yyyy"," ");
if (d == null)
d = stringToDate(dateValue,"yyyy/MM/dd","/");
if (d == null)
d = stringToDate(dateValue,"yyyy-MM-dd","-");
if (d == null)
d = stringToDate(dateValue,"yyyy.MM.dd",".");
if (d == null)
d = stringToDate(dateValue,"yyyy MM dd"," ");
}
}
if (d == null) {
alert(errorMesage);
} else {
var formatedDate = moment(d).format(format);
obj.value = formatedDate;
}
}
} catch(e) {
alert(errorMesage);
}
};In fact, that one’s so good, let’s do it again!
function validateDateValue_(dateValue, format, errorMesage) {
try {
format = format.toUpperCase();
if (dateValue.length == 0) {
return errorMesage;
}
if (dateValue.length > 10)
dateValue = dateValue.substring(0, 10);
if (dateValue.length < 6) {
return errorMesage;
}
var d = null;
var sep = getSeparator(format);
if (sep.length > 0)
d = stringToDate(dateValue, format, sep);
else
d = Date.parse(dateValue.substring(0, 4) + '-' + dateValue.substring(4, 6) + '-' + dateValue.substring(6, 8));
if (d == null) {
if (dateValue.length == 6 ) {
if (d == null)
d = stringToDate(dateValue,"ddMMyy","");
if (d == null)
d = stringToDate(dateValue,"MMddyy","");
} else if (dateValue.length == 8) {
d = Date.parse(dateValue.substring(0, 4) + '-' + dateValue.substring(4, 6) + '-' + dateValue.substring(6, 8));
if (isNaN(d))
d = null;
if (d == null)
d = stringToDate(dateValue, "dd/MM/yy", "/");
if (d == null)
d = stringToDate(dateValue, "dd-MM-yy", "-");
if (d == null)
d = stringToDate(dateValue, "dd.MM.yy", ".");
if (d == null)
d = stringToDate(dateValue, "MM/dd/yy", "/");
if (d == null)
d = stringToDate(dateValue, "MM-dd-yy", "-");
if (d == null)
d = stringToDate(dateValue, "MM.dd.yy", ".");
if (d == null)
d = stringToDate(dateValue,"ddMMyyyy","");
if (d == null)
d = stringToDate(dateValue,"MMddyyyy","");
} else {
if (d == null)
d = stringToDate(dateValue, "dd/MM/yyyy", "/");
if (d == null)
d = stringToDate(dateValue, "dd-MM-yyyy", "-");
if (d == null)
d = stringToDate(dateValue, "dd.MM.yyyy", ".");
if (d == null)
d = stringToDate(dateValue, "MM/dd/yyyy", "/");
if (d == null)
d = stringToDate(dateValue, "MM-dd-yyyy", "-");
if (d == null)
d = stringToDate(dateValue, "MM.dd.yyyy", ".");
if (d == null)
d = stringToDate(dateValue, "yyyy-MM-dd", "-");
}
}
if (d == null) {
return errorMesage;
} else {
var formatedDate = moment(d).format(format);
dateValue = formatedDate;
}
} catch (e) {
return errorMesage;
}
return "";
};Yes, that is basically the identical method, but some of the parameter names are different, and one of them has more sensible indentation than the other.
But that only handles dates. What about datetimes?
function validateDateTimeValue(obj, format, errorMesage){
format = format.toUpperCase();
format = format.substring(0, format.length-2) + "mm";
if(obj != null){
var dateTimeValue = obj.value;
if (dateTimeValue.length == 0)
return;
if (dateTimeValue.length == 8 || dateTimeValue.length == 10 || dateTimeValue.length == 6)
dateTimeValue = dateTimeValue + " 00:00";
if (dateTimeValue.length > 16)
dateTimeValue = dateTimeValue.substring(0, 16);
if (dateTimeValue.length < 12 || dateTimeValue.length > 16){
alert(errorMesage);
return;
}
var time = dateTimeValue.substring(dateTimeValue.length-6, dateTimeValue.length);
if (time.charAt(0) != ' '){
alert(errorMesage);
return;
}
var h = parseInt(time.substring(1, 3));
if (isNaN(h)){
alert(errorMesage);
return;
}
var m = parseInt(time.substring(4, 6));
if (isNaN(m)){
alert(errorMesage);
return;
}
var d = null;
var dateValue = dateTimeValue.substring(0,dateTimeValue.length-6);
var sep = getSeparator(format);
if (sep.length>0)
d = stringToDateTime(dateValue,format.substring(0, format.length-6), sep,h,m);
else d = Date.parse(dateValue.substring(0,4) + '-' + dateValue.substring(4,6) + '-' + dateValue.substring(6,8) + 'T' + h + ':' + m + ':00');
if (d == null){
if (dateTimeValue.length == 12 ){
if (d == null)
d = stringToDateTime(dateValue,"ddMMyy","",h,m);
if (d == null)
d = stringToDateTime(dateValue,"MMddyy","",h,m);
} else if (dateTimeValue.length == 14 ){
d = Date.parse(dateValue.substring(0,4) + '-' + dateValue.substring(4,6) + '-' + dateValue.substring(6,8) + 'T' + h + ':' + m + ':00');
if(isNaN(d))
d = null;
if (d == null)
d = stringToDateTime(dateValue,"dd/MM/yy","/",h,m);
if (d == null)
d = stringToDateTime(dateValue,"dd-MM-yy","-",h,m);
if (d == null)
d = stringToDateTime(dateValue,"dd.MM.yy",".",h,m);
if (d == null)
d = stringToDateTime(dateValue,"dd MM yy"," ",h,m);
if (d == null)
d = stringToDateTime(dateValue,"MM/dd/yy","/",h,m);
if (d == null)
d = stringToDateTime(dateValue,"MM-dd-yy","-",h,m);
if (d == null)
d = stringToDateTime(dateValue,"MM.dd.yy",".",h,m);
if (d == null)
d = stringToDateTime(dateValue,"MM dd yy"," ",h,m);
if (d == null)
d = stringToDateTime(dateValue,"yy/MM/dd","/",h,m);
if (d == null)
d = stringToDateTime(dateValue,"yy-MM-dd","-",h,m);
if (d == null)
d = stringToDateTime(dateValue,"yy.MM.dd",".",h,m);
if (d == null)
d = stringToDateTime(dateValue,"yy MM dd"," ",h,m);
if (d == null)
d = stringToDateTime(dateValue,"ddMMyyyy","",h,m);
if (d == null)
d = stringToDateTime(dateValue,"MMddyyyy","",h,m);
} else {
if (d == null)
d = stringToDateTime(dateValue,"dd/MM/yyyy","/",h,m);
if (d == null)
d = stringToDateTime(dateValue,"dd-MM-yyyy","-",h,m);
if (d == null)
d = stringToDateTime(dateValue,"dd.MM.yyyy",".",h,m);
if (d == null)
d = stringToDateTime(dateValue,"dd MM yyyy"," ",h,m);
if (d == null)
d = stringToDateTime(dateValue,"MM/dd/yyyy","/",h,m);
if (d == null)
d = stringToDateTime(dateValue,"MM-dd-yyyy","-",h,m);
if (d == null)
d = stringToDateTime(dateValue,"MM.dd.yyyy",".",h,m);
if (d == null)
d = stringToDateTime(dateValue,"MM dd yyyy"," ",h,m);
if (d == null)
d = stringToDateTime(dateValue,"yyyy/MM/dd","/",h,m);
if (d == null)
d = stringToDateTime(dateValue,"yyyy-MM-dd","-",h,m);
if (d == null)
d = stringToDateTime(dateValue,"yyyy.MM.dd",".",h,m);
if (d == null)
d = stringToDateTime(dateValue,"yyyy MM dd"," ",h,m);
}
}
if (d == null)
{
alert(errorMesage);
}
else{
var formatedDate = moment(d).format(format);
obj.value = formatedDate;
}
}
return true;
}And is it also duplicated? You know it is, following the same underscore naming convention: validateDateTimeValue_. (I won’t share it again)
Okay, complex regexes that you can’t debug is bad. Custom date handling code is a WTF. Duplicating that code for no clear reason is bizarre. But what’s the stinger?
How about “hardcoded credentials for connecting to the database”?
function Register(){
registerCurrentUserToFDB("Doe", "name surname", "abc@abc.com", "chgitem", "0fc6c0427cea929a3e21028f68cecf42");
}
function Login(){
loginUserToFDB("abc@abc.com", "0fc6c0427cea929a3e21028f68cecf42");
}
In this the backend is Firebase (that's the FDB above), so this is client side JS phoning home to a Firebase backend.
[Advertisement]
BuildMaster allows you to create a self-service release management platform that allows different teams to manage their applications. Explore how!
|
Метки: CodeSOD |
CodeSOD: Bad Code and Taxes |
Here in the US, “tax season” is extended into the summer. No one likes dealing with taxes, obviously, but we agree that the social benefits outweigh the costs.
I can’t speak to how folks feel in Italy. But Riccardo B was perusing the Italian Revenue Service’s (INPS) website, and was having a bad time of it. This website was recently “modernized”, which Riccardo tells us cost €300M (I wasn’t able to track down much on this, and since I don’t speak Italian, I’ll take Riccardo’s word on it), so “having a bad time” doesn’t seem like it should be part of the process.
Like most of us, when he started having problems, Riccardo pulled up the inspector and started debugging. Which is how he spotted this €300M treat:
function wait(ms) {
var start = new Date().getTime();
var end = start;
while (end < start + ms) {
end = new Date().getTime();
}
}Yes, that’s a busy-wait function. Yes, it’s constantly checking the date and time. No, you absolutely should never put such a thing in your code.
At the time of this writing, that code is still in production. It’s in a lovely 4,000 line file which clearly loves jQuery. Given that the file is named app.js I’ll give them the benefit of the doubt and assume it was bundled together from separate source files, but it’s also not minified, so who knows? It’s good for us anyway, as we get to “enjoy” the full ugly.
For example, how is wait used?
if ($(article).hasClass("article--with-sticky-nav")) {
$(window).scroll(function () {
wait(1000);
if ($(window).scrollTop() > article.offset().top) {
outline.addClass("article__nav--sticky--fixed");
} else if ($(window).scrollBottom() > article.offset().bottom) {
outline.addClass("article__nav--sticky--fixed");
}
else {
outline.removeClass("article__nav--sticky--fixed");
}
});
}Oh, it’s called inside of a scroll event handler. That’s about as fun as paying taxes. If the user tries to scroll when there’s a “sticky” article displayed, we’ll freeze for a second, then slap some CSS classes around. Bonus points for using an else if where an || would do.
As an aside, while CTRL+Fing for wait, I found waitForFinalEvent, which has other treats:
var waitForFinalEvent = (function () {
var timers = {};
return function (callback, ms, uniqueId) {
if (!uniqueId) {
// C85
uniqueId = "timersv0001";
}
if (timers[uniqueId]) {
clearTimeout(timers[uniqueId]);
}
timers[uniqueId] = setTimeout(callback, ms);
};
})();Here, we can see that someone understands at least something about using setTimeout, the correct way to wait for a period of time. Other than that, this is a pretty straightforward “Immediately Invoked Function Expression”, which lets them create a timers variable without that variable being placed in the global scope. That’s not a WTF, that’s just what you do in JavaScript if you’re not working with modules. The WTF is that someone in the shop knew the right tool, but whoever wrote wait didn't.
I assume, in that code, all the //C85 type comments represent changes tied to a ticket tracking system, and no one understands source control well enough to think about why that’s not a good practice.
[Advertisement]
ProGet can centralize your organization's software applications and components to provide uniform access to developers and servers. Check it out!
|
Метки: CodeSOD |
Representative Line: All the Small Things |
Kerry (previously) has a long held belief: people that can’t get the little things right have no hope of getting the big things right, and not just when it comes to software.
Personally, I don’t think that’s truly universal, but it’s certainly a good guideline. If nothing else, seeing certain simple mistakes gives you a hint of worse things to come. Like this interface definition:
long? saveEntry(CreateEntryViewModel dto, out long? IncidentNumber, out int incidentCount, string IncidentOrigination, bool ForceMilesAtPurchase);Speaking of “small stuff”, one of the smallest things we might expect from a method signature is consistent capitalization. Out of the gate, we fail on that- most of the parameters are PascalCase, but one camelCase entry sneaks in. The method returns a value, but it also has two out parameters- output parameters for when you have a method which doesn’t return a value. The return type and one of those outputs is nullable, why? Well, there’s no documentation, so who knows? We also sandwich our out params between more traditional input params, which as a general rule, we don’t want to mix up our outputs and our inputs. It’s unhealthy.
Now, there is no documentation, as I mentioned. But that doesn’t mean that Kerry hasn’t been able to figure out what some of this method does. First, while it’s called saveEntry, it only creates them. There’s a separate updateEntry, which is only mildly confusing. There’s no indication anywhere of how the output parameters are actually meant to be consumed, and from the handful of places this is called, it looks like they simply aren’t.
But the return value is used, and it is important. This method returns the entryId of the newly created entry- the unique ID of this entry. Well, maybe it does, because the return type is nullable- long?. There’s just one problem with that: the entryId should never be null. A null is an exception- you failed to save the entry. This method should never return null, it should just throw an exception.
For one line of code, that’s a lot of bad choices. Each one of them is just a small thing, but this interface’s implementation needs to do big, complicated things on the back end. As Kerry adds:
Needless to say, I’m apprehensive as to what I will find upon opening the service’s implementation.
[Advertisement]
BuildMaster allows you to create a self-service release management platform that allows different teams to manage their applications. Explore how!
|
Метки: Representative Line |