CodeSOD: Anti-Injection |
SQL injection attacks are, in most environments, easy to avoid. Pass user input through parameterized commands and never do any string munging to build your SQL queries. And yet, we constantly see places where people fail to do this correctly.
Eric's co-worker is one of "those" people. They were aware of what SQL injection was, and why it was a risk, but instead of using PHP's built-in functionality for avoiding it, they reinvented the wheel- now in a triangular shape!
// Anti-SQL Injection
function check_inject()
{
$badchars = array(";","'","*","/"," \ ","DROP", "SELECT", "UPDATE", "DELETE", "drop", "select", "update", "delete", "WHERE", "where", "-1", "-2", "-3","-4", "-5", "-6", "-7", "-8", "-9",);
foreach($_POST as $value)
{
$value = clean_variable($value);
if(in_array($value, $badchars))
{
//detect($value);
die("SQL Injection Detected - Make sure only to use letters and numbers!\n
\nIP: ".getIpAddr());
}
else
{
$check = preg_split("//", $value, -1, PREG_SPLIT_OFFSET_CAPTURE);
foreach($check as $char)
{
if(in_array($char, $badchars))
{
//detect($value);
die("SQL Injection Detected - Make sure only to use letters and numbers!\n
\nIP: ".getIpAddr());
}
}
}
}
}
function clean_variable($var)
{
$newvar = preg_replace('/[^a-zA-Z0-9\_\-]/', '', $var);
return $newvar;
}
There are so many layers of WTFery here. First, we stuff a $badchars array with some characters and more substrings, including " \ ", and "-9", which I'm not aware of any SQL injection attacks built out of negative numbers, yet here we are.
Then we inspect every $_POST variable, even the ones that aren't going anywhere near the database. We clean_variable, which strips most of the characters in $badchars in the first place, but then we check the variable to see if it's in the $badchars array. I want to stress, this is checking to see if the entire variable matches any string in the $badchars array. So it protects against a DROP but not a DROP TABLE users.
But that's okay, because they then split it into characters using an empty regex, and check each character against the array which isn't made up of single character strings.
So while clean_variable will do a decent enough job protecting against SQL injection attacks (by destroying your input data and preventing loads of realistic strings), the actual check_inject function is otherwise useless.
Forget injection, this code needs to be ejected into the cold hard vacuum of space, never to be heard from again.
[Advertisement]
Utilize BuildMaster to release your software with confidence, at the pace your business demands. Download today!
|
Метки: CodeSOD |
CodeSOD: A Careless Comment |
Today is a short hit, as there's a comment I want to highlight. This comes to us from Benjamin Urquhart. It's his comment, it's his code, and it's his confession.
// See #ISSUE
// This is not the place to monkey patch this
// issue, but it's like 2am and quite frankly
// I don't care anymore.
He explains:
Was working on a new feature and needed this particular bug fixed, so I did it in the quickest way possible. Still not sure where else to put this fix (it's in a
ToStringcall, so I guess anywhere is better).
Comments like this are the cry of a developer who knows they should do something different, but just needs to get things done.
[Advertisement]
Continuously monitor your servers for configuration changes, and report when there's configuration drift. Get started with Otter today!
|
Метки: CodeSOD |
CodeSOD: Modus Pwned |
Conditionals are a constant source of problems for some developers. Russell F inherited some code which needed to take a list of parts and filter the list of parts down to something which customers could actually order.
The specific rule was that they needed to include only parts that were: not in stock and not in close out, in stock and not in close out, or in close out but also available. Which, given that business rule, that's exactly what the developer implemented:
var filtered = lstParts.Where(part => (!part.Stock && !part.CloseOut) || (part.Stock && !part.CloseOut)
|| (part.CloseOut && part.IsAvail == true));
The fact that these are all boolean values, and most of the time the developer understood that makes the closing part.IsAvail == true line extra painful. But, reading both the rule and the code, you might realize that, like a middle school word problem, there's some red herrings and irrelevant details.
Russell made a much more concise version of the code by thinking logically about the statement:
var filtered = lstParts.Where(part => part.IsAvail || !part.CloseOut);
But that's not the entirety of the WTF, here. Russell has a few other details to add.
Note: I changed some names here for clarity's sake; for instance, the list wasn't actually called lstParts. I also changed the capitalization of some properties because we have some very unusual rules for casing (properties that correspond to actual database fields are for some reason capitalized differently than properties that are calculated on object initialization, for instance) and I didn't want to get side-tracked into a discussion of those rules.
That sounds less like a side track and more like a WTF of its own.
Also, Russell has another aside worth mentioning. If you're anything like me, you've had the experience where you search for something, find a Stack Overflow question and see exactly what you need- and also discover that you either asked the question or posted the answer five years ago. Well, Russell has a… different version of that experience.
A few days ago I was assigned to a task where I couldn't remember exactly where the code was that I'd need to look at. However, I did remember that I'd submitted some of it to TDWTF, so I did a search of your site for my submissions to find it (it was A Ritual Approach).
[Advertisement]
ProGet’s got you covered with security and access controls on your NuGet feeds. Learn more.
|
Метки: CodeSOD |
CodeSOD: Starting Your Date |
So much bad date-handling code is based in people reinventing methods that already exist, badly. That's code that solves a problem people have, that's already solved. But the really special code solves a problem nobody understands or wants solved.
Take this C# function from Luke's co-worker.
/// So, this function "gets start date", from "two dates" and a "number of weeks". The comment tells me this, the method name tells me this, and the function parameters tell me that it actually takes a number of months, not weeks.
But let's trace through the code.
We start by setting our return value to dtFrom. Then we count how many weeks are between our dtTo and dtFrom. Then we take the month input and convert it into weeks, by simply multiplying by 4. That's… not accurate. I mean, it's the floor of the number of weeks per month, so maybe it's fine.
If the number of weeks between the origin dates is the same as the one calculated by months, we set the return value back to dtFrom, which is what we just set it to anyway. But if the number of weeks mismatches, we add or subtract days. Mind you, we add or subtract in a loop, instead of just doing some basic arithmetic.
Then it returns the resulting date. Why? To what end? What is this for?
Luke adds:
We haven't really been able to work out what it does, and the comments do a good job of confusing the issue even more. It was written by someone who was considered a senior developer, and I think the fact that there were some communication issues at times meant that people thought some of this was just a breakdown in communication. Frighteningly, they've moved into a more senior position elsewhere…
If anyone works out what it actually does, be sure to post it in the comments
[Advertisement]
Continuously monitor your servers for configuration changes, and report when there's configuration drift. Get started with Otter today!
|
Метки: CodeSOD |
Error'd: Time is Fleeting |
It's astounding...
The madness took its toll on Chris N. who highlighted Monday "Your Microsoft Teams is out of date! This is after pressing Calendar."
The void is calling Matthias J. who can't decide "Not sure if I need to add my contact data or the missing contact form source code?"
So dreamy Luke H. declared indignantly "I'm being objectified by PayPal when I try to remove my address."
And an anonymous guy with the devil's eyes had this to share (which took me by surprise) "Somehow, I do not believe that {:title} in {:location} is a popular job title. In fact, I have never seen an advertisement for this. Clicking on it, yields an empty Oh snap! error. Does Linkedin only know three job titles?"
With only time for one last stanza, Manuel H. calls out "A service that rarely encounters issues sounds expensive, and looking at the Pricing, that indeed seems to be the case."
[Advertisement]
Continuously monitor your servers for configuration changes, and report when there's configuration drift. Get started with Otter today!
|
Метки: Error'd |
CodeSOD: Valuable Comments |
When we share code comments, it's usually because they demonstrate some lack of awareness or some carelessness about what's going on. A comment warning "I know I shouldn't do this" or a comment that contradicts the code in a funny way, that's usually what we share.
But today's submission, from Dewey, is a bit different. Dewey wrote the comment, and it was in preparation for some serious refactoring. So this comment is the product of someone spending time to accurately analyze and understand a method, documenting its behavior, and explaining it so the code could be changed to something better. The WTF here isn't the comment, but the code it describes.
/** Get the field value, stripped of accelerator key information while
side-effecting some global variables that affect the way
@link #setAcceleratorMnemonic(JMenuItem)} behaves in subsequent calls.
...
*/
"while side-effecting some global variables" is exactly the kind of thing you want to see in a code comment. Well, maybe not, but it's the kind of thing I like to see when trawling through submissions.
It took Dewey thirty minutes and a careful trace through three different methods to be able to understand the code well enough to write this.
[Advertisement]
Keep the plebs out of prod. Restrict NuGet feed privileges with ProGet. Learn more.
|
Метки: CodeSOD |
CodeSOD: Old File |
Let's say you've got an older PHP application. Once upon a time, it was wired together by a pile of includes with no real organization or planning behind the organization. A developer went through and cleaned up the file organization.
That's a happy ending, isn't it? No, it isn't, not for Scott. Because the developer doing the cleanup didn't want to risk breaking any files, and thus didn't actually do any final cleanup. Instead, in the labirynth of a thousand include files, many of them are dead ends containing only:
|
Метки: CodeSOD |
Playing a Role |
Initech's latest offering, IniPrints, was a secure automation system for document management. The target audience was the banking industry, which meant that the system was sold as having robust and fine-grained role-based access control systems. As far as any one could tell, that was exactly what Initech was shipping, which meant IniPrints gained a reputation within IniTech as being a "good product", with "low maintenance".
When Alan was assigned support on IniPrints, he expected it to be pretty quiet. So he was surprised when three of the veterans of the project, Carole, Larry, and Arthur desceded on his cube with grim tidings.
"We've got some concerns about IniPrints. You should probably take a deeper look at the code."
With that ominious warning, the Fates- Carole, Larry and Arthur- vanished back into the cube farm. The trio had migrated from development to technical sales support roles, which meant they mostly interacted with customers anymore. They were not to be any help to Alan when it came to understanding the software.
Unfortunately, neither was the documentation. There was none in the code, none in the commit history, and the code itself was incomprehensible. There was one Word document which was meant to be "the documentation", but it was less clear than the code. When Alan complained about the lack of documentation, his request eventually got back to the Fates, who replied: "check the big red filing cabinet for a hard copy of the documentation".
Alan found the hard copy: it was the same incoherent Word document, printed out, and decorated with a series of coffee stains which did nothing to make the meaning more clear.
Alan did his best to understand the software anyway. When it came to roles, it shipped with two roles: User and Manager. These two roles were used in the sales demo, and 99% of their customers never dove deeper into the security model than that. Those two roles worked perfectly and had well understood behaviors.
Unfortunately, the Fates- Carole, Larry and Arthur- had just landed a contract with a new bank. And this bank was in the 1%- they needed a role-based access control system with much more advanced functionality. That's what IniPrints claimed to have, but the reality was different.
The bank sent them a pile of security issues. Alan was the one person doing support, which meant Alan needed to triage and prioritize. He grouped the issues into four categories:
There were a handful of issues in the first group, and shockingly only one in the last. But groups (2) and (3) had enough work for an entire team. Still, Alan dove in and started checking them off. The Fates went back to the client, smoothing over the problems and making promises about what the future would hold. With each issue Alan fixed, the customer got calmer about their needs… until there was only one issue left. The category (4) storm of crap.
The specific problem was that this was an automation system. It was scriptable, and those scripts logged out the commands being executed. Unfortunately, the part of role-based security that controlled access to the logged messages didn't work. Any user, including unprivileged ones, could read those logging messages. And, due to the messed up architecture of the whole thing, it would require a ground up re-write of the entire logging engine to fix.
So Alan communicated back to the client, through the Fates: "This would be a pretty massive upheaval. Could I have more information about why it's required? There may be a better way to meet the goal."
The Fates did their best to get that information. Management got involved, and generally did get the sense that "well, this is a lot of work for only one customer, but they're a very BIG customer." The customer kept trying to raise the severity of the issue, claiming that simply logging the commands executed was a critical security vulnerability.
Eventually, this escalated to a video conference. The bank's security team, including everyone from the CSO all the way down to a junior analyst was on the call. The Fates were on the call. And Alan, of course, was on the call.
"Could you tell us," Alan said as diplomatically as possible, "what exactly makes this use case so important to you? The easiest fix might be to perform some sort of transformation on the data we're logging, like filtering standard-in or something."
The bank security team sat in an uncomfortable silence for a long moment, before the CSO said "It's just surface area that shouldn't be exposed."
"Right, but we didn't design the system to support this. It's just a log of commands executed and some informational output, so it shouldn't contain anything confidential. I understand that it's not ideal, but we're not logging out passwords or anything, right?"
Alan laughed at his own joke. The bank's security team sat in silence. The silence stretched out to a length beyond awkward.
"Right?" Alan asked again.
It was the junior analyst who caved. "Almost all of our commands take clear-text passwords as arguments." The CSO stared daggers at the junior analyst, but the junior analyst didn't care- they were pleased to have confessed their sins.
The Fates also relaxed. Arthur, especially, looked pleased. "Ah," he said, "our documentation celarly states that nothing confidential should be contained in the input scripts, specifically because of this risk. This is no longer a critical issue, but we'll keep it on the backlog for a potential future release."
"But we need this," the CSO complained.
"It seems like you need to re-evaluate your password handling," Carole said.
"In any case, is there anything else on the agenda for this call?" Arthur said. "No? Larry can document our findings and follow up in email. Thanks Alan, for your time."
[Advertisement]
Otter - Provision your servers automatically without ever needing to log-in to a command prompt. Get started today!
|
Метки: Feature Articles |
CodeSOD: A Little History |
Source control history can go a long way to telling a story. Take Cassi, who needed to run some PHP software which depended on a few binaries and shell calls to do its job. We can see the initial attempt to locate the path of a binary:
function findPathOf($path)
{
if (file_exists("/usr/bin/$path")) return "/usr/bin/$path";
return "/usr/local/bin/$path";
}
Now, this worked, so long as the binary was in one of those two places, but in any other case, that's a problem. So someone changed it to:
function findPathOf($path) {
exec("which " . escapeshellarg($path), $output, $returnVar);
if ($output != 0) {
return null;
}
return $output[0];
}
This version is completely wrong. $returnVar would contain the shell return code, which would be a non-zero value in the case of an error. $output will always be an array, and even in PHP, an empty array is never going to equal zero. So this didn't work at all. So someone else fixed it.
function findPathOf($path) {
if (file_exists("/usr/bin/$path")) return "/usr/bin/$path";
if (file_exists("/usr/local/bin/$path")) return "/usr/local/bin/$path";
exec("which " . escapeshellarg($path), $output, $returnVar);
if ($output != 0) {
return null;
}
return $output[0];
}
This highlights the value of source control history, as this allowed the developer to just do… both. One which is right in only two cases, and one which is never right. Having successfully solved none of the problems with the original solution, someone else decided to give it their best shot.
function findPathOf($path) {
static $knownPaths = array();
if ($knownPaths[$path]) return($knownPaths[$path]);
if (file_exists("/usr/bin/$path")) return $knownPaths[$path] = "/usr/bin/$path";
if (file_exists("/usr/local/bin/$path")) return $knownPaths[$path] = "/usr/local/bin/$path";
exec("which " . escapeshellarg($path), $output, $returnVar);
if ($output != 0) {
return null;
}
$knownPaths[$path] = $output[0];;
return $output[0];
}
This version memoizes the search path, ensuring that if we've found the file before, we don't need to find it again. This is a handy optimization, but still fails to address the fact that this code doesn't work except in the specific cases where the file being searched for is in /usr/bin or /usr/local/bin.
Which, at this point, it's likely worth noting, they're not at any point searching for a path, despite the $path variable name, they're searching for an executable in a path, but honestly, is it actually worth noting? Probably not.
[Advertisement]
Keep the plebs out of prod. Restrict NuGet feed privileges with ProGet. Learn more.
|
Метки: CodeSOD |
Error'd: Time's Up |
Aspirational Caleb Su thinks this birth-year chooser is a WTF. "You have to be in at least 8th grade to join, meaning at the very latest you could be born in 2009." Not so, Caleb, not so! A precocious 8th grader might have been born as late as 2013. It could happen.
"I've never seen a furnace running Linux. Was it autocorrect gone crazy? Wonder if the Lennox man knows what sudo is for?" huffed Quentin G., heatedly.
"What's up?" Scott P. wonders. "On US Bank's site to look at my account, checked the free credit score monitoring they offer. We seem to have different definitions of up." I think it's an adjective.
Clever Paul eviscerates Bugsnag's business model. "From an online web map application, on how to report an error to their support service. How come I've never thought of that? Ask the customer to debug the code!"
While watch watcher John DeTreville clucks "Apple's double-talk generator must be set to 11."
[Advertisement]
Otter - Provision your servers automatically without ever needing to log-in to a command prompt. Get started today!
|
Метки: Error'd |
CodeSOD: Caught Something |
One of the easiest ways to create a bug for yourself is to drop an empty catch into your application.
Today's rather short CodeSOD comes from Dex, and was found in a payment process. This is code that handles actual financial transactions, which is why the comment attached to a common mistake is relevant here:
catch // complete fubar but don't crash
{
}
And yes, when this exception happened, things were "fubar"- broken beyond all recognition, and unable to be recovered from. That's what drew Dex's attention to the code in the first place- trying to understand the mangled transactions which were passing through the system.
As Dex summarizes:
This combines a whole bunch of WTFs:
- Error suppression in payment processing code
- Try/catch in place of checking error conditions
- Willful laziness, as documented the coder's comment
[Advertisement]
Utilize BuildMaster to release your software with confidence, at the pace your business demands. Download today!
|
Метки: CodeSOD |
CodeSOD: The Load Balancer Got Terned Off |
Ilsa's organization uses Terraform to handle provisioning their infrastructure. This mostly works fine for the organization, but one day it started deleting their load balancer off of AWS for no good reason.
Ilsa investigated, but wasn't exactly sure about why that was happening. What she did find, however, was this particular ternary expression.
resource "aws_lb_listener" "this" {
count = var.internal == true || var.provision == true ? length(var.listener) : 0 && var.internal == false || var.provision == true ? length(var.listener) : 0
...
}
As mentioned yesterday, variable == true in a conditional expression is one of my pet peeves, though I suppose I don't know HCL that well- it may be the case that there may be truthy values that are not true, so this might be necessary (or at least caused by bad choices elsewhere in the system).
In the end, I think this highlights the problem with complicated, nested ternaries. I suspect the reason this misbehaves is the second ternary, which has the condition 0 && var.internal == false || var.provision == true. Because of the first condition, we know provision is false, and anything && 0 is false. This is probably a typo that got munged until it was syntactically valid, and when the person writing it ran their tests, it probably worked just fine with the testing parameters they used.
And then they realased this accident into actual usage and started breaking production.
[Advertisement]
Otter - Provision your servers automatically without ever needing to log-in to a command prompt. Get started today!
https://thedailywtf.com/articles/the-load-balancer-got-terned-off
|
Метки: CodeSOD |
CodeSOD: Audit For Truth |
Tony sends us this snipped of C# code:
internal static bool TraceListner(ProjectInstance objPI,
Exception objError, int EntityID,
Enumerations.LogEntityType EntityType, int EntityInterfaceID,
Enumerations.LogReturnType ReturnType,
Enumerations.EventLevels EventLevel, string StartTime,
string EndTime, int UserID, string Description)
{
ProcessAudit objLog;
bool blnWrite;
try
{
// WRITE THE TRACE LOG IF PROCESS HAS AUDIT FLAG = TRUE
//AND THE CURRENT ACTIVTIY AUDIT FLAG = TRUE
// need to log all of the errors regardless of the activity
blnWrite = true;
// write to the log if it is an error type log
if (!(objError == null))
blnWrite = true;
if (blnWrite == true)
{
objLog = new PrjectAudit(objPI, objError, EntityID, Convert.ToInt32(EntityType), EntityInterfaceID, Convert.ToInt32(EventLevel), StartTime, EndTime, Convert.ToInt32(ReturnType), Description, UserID);
return objLog.Save();
}
}
catch
{
}
return false;
}
This code is helpfully commented to make the "logic" clearer. And also the history of how this code evolved. First, we wanted to only log if auditing was enabled, but that changed at some point. Presumably also around that time, we also wanted to log errors, so we added a check for that. Then, we realized we wanted to log all messages, so we removed the code which checked the audit flag, but not the comment, and added a hard-coded assignment to force logging. Finally, we include a if (blnWrite == true), which is one of my pet peeves in an if-statement (if (blnWrite) is identical and more clear in most cases).
Finally, for a bonus, we have incosistent spellings- ProjectInstance and PrjectAudit. It doesn't matter how you spell your symbols, so long as you can spell them consistently.
[Advertisement]
Otter - Provision your servers automatically without ever needing to log-in to a command prompt. Get started today!
|
Метки: CodeSOD |
CodeSOD: If We're Good, Or Else |
There are some coding practices which likely arise from a good intent, but sometimes feel misplaced. For example, the "only one return from a function" tends to make code more complex and harder to read, instead of easier, ever if it theoretically makes debugging easier. I can at least understand the intent and reasoning behind it, even if I don't like it.
Danny D's former co-worker had their own little rules. Specifically, sometimes when you write an if statement, you definitely need an else. Other times, you don't. That's confusing, because you have to make a choice. Instead of making a choice, just always use an else. Always.
if (!(tf.getType() == matchingTimeframe.getType() && StringUtils.equals(tf.getParameter(),
matchingTimeframe.getParameter()))) {
....
} else {
// we are still good
}
Of course, you need to include a comment to explain what condition the else represents. On every if-statement in the application. Even- no especially the ones with a somewhat confusing to read negation test, like we see here. We are still good. We are still good.
[Advertisement]
Continuously monitor your servers for configuration changes, and report when there's configuration drift. Get started with Otter today!
|
Метки: CodeSOD |
Error'd: Dirty Deeds ... cheap. |
I saw my doctor this week and he told me I needed to go on a new diet. So here you go: all natural, no snark added.
Overachiever
Bill T.
reflected on his life achievements:
"I had to fill out demographic information - but is this
really a cognitive test in disguise? Find your level
of education!"
"Is this even legal??" exclaims
Julien M-A.
"I knew big tech had no conscience and also that you can
buy anything on Amazon, but really I did not expect it
to sink to these depths. But my real question is what search
algorithm matched this result in response to a query on
funding from the French Ministry of International Affairs."
Even the disfrancophonic readers can probably guess that
"a petits prix" is translated "at low prices".
Virgin Pulser Jeff A. quavers "I’m afraid to open the app; I don’t have time to complete 1.3 Billion cards."
"I'm sensing some repressed rage," empathizes cheese fan Cong. "Perhaps they're not really thankful for my custom?"
Eagle-eyed Bruce C. went the distance to get more data displayed. Disappointed, he comments "Not only did they decide not to have scrolling, they actually don't show more years on larger screens. This was captured on a 4k monitor - no extra years displayed."
[Advertisement]
Otter - Provision your servers automatically without ever needing to log-in to a command prompt. Get started today!
|
Метки: Error'd |
CodeSOD: The Core Class |
We've had a bit of a run of PHP in the inbox lately, which is fine, but certainly isn't doing anything to help PHP's reputation as a WTF factory. This one comes from Lucio C, who was hired to fix a hacked WordPress installation.
Much of the fixing was figuring out what data was safe to recover, what files may have been tampered with, and generally assessing the damage.
While doing that assessment, Lucio found this perfectly legitimate file in a perfectly legitimate WordPress plugin. This file was not altered by the hackers, but…
/**
* The core class, where logic is defined.
*/
class Core
{
public $footer_content;
// [...]
public function footer_content()
{
if ( '' !== $this->footer_content && !is_admin() ) {
$html = ' . $this->footer_content . '';
// we should sanitize for security, but users
//want to include all kinds of content, including forms.
/*
$allowed_html = wp_kses_allowed_html( 'post' );
$allowed_html['style'] = [
'type' => true,
];
echo wp_kses( $html, $allowed_html );
*/
echo $html ;
}
}
The opening comment is a wonderfully useless comment on what is clearly a "god class" object, where just all the plugin logic got dumped.
But it's this comment which highlights the WTF:
// we should sanitize for security, but users want to include all kinds of content, including forms.
"We probably shouldn't let this have an XSS vulnerability, but we can't stop it because our users don't care." There's even a vestigial attempt at doing some sanitization, commented out in favor of a version that just dumps user supplied HTML directly to the page.
What could go wrong?
[Advertisement]
BuildMaster allows you to create a self-service release management platform that allows different teams to manage their applications. Explore how!
|
Метки: CodeSOD |
CodeSOD: Filter Your Index |
Nora found a curious little C# helper method. It was called in exactly one place and it is… not exactly necessary in any case.
/// This method wants to take a boolean and convert it into an integer- 1 if true, 0 if false. Right off the bat, this method isn't needed- Convert.ToInt32 would do the job. Well, maybe our developer didn't know about that method, which isn't really an excuse, but then they write this logic in the most verbose way possible, favoring a switch over a simpler conditional, doing the dance of "only one return from a method" (which I'm not a fan of as a rule).
Even so, you could compact this into an actually readable ternary: return item ? 1 : 0;
So let's just see how this unnecessary method gets invoked.
FilterIndex = GetFilterIndex(FilterCheckBox.Checked);
FilteredSortedItems = FilterIndex > 0 ? CurrentItemList.Where(info => Convert.ToInt32(info.IsFlagged) == FilterIndex).ToList() : CurrentItemList.ToList();
Okay, so they do know about the Convert.ToInt32 method. Good to know. But then their use of GetFilterIndex is to do a comparison to see if it`s greater than 0, which only happens if it's true, so there's no need for this filter index method at all.
FilterdSortedItems = FilterCheckBox.Checked ? …
Oh, and the Convert.ToInt32 used here is also unnecessary, since info.IsFlagged is also a boolean, so once again, we could just compare info.IsFlagged == FilterCheckBox.Checked.
It's a method written the long way around to solve a problem in one place that doesn't even need to be solved, and as a bonus, the use of the method, its name, and its role, just makes the code less clear and harder to understand.
[Advertisement]
Utilize BuildMaster to release your software with confidence, at the pace your business demands. Download today!
|
Метки: CodeSOD |
Perfect Uptime |
"So," Burt said, "our bonuses at the end of the year are based on our systems uptime?"
"Well," the middle manager introducing their new incentive program said, "not exactly. It's part of your team's performance targets, so we calcula-"
"Yeah, I get it," Burt said. "But your top tier goes up to 99.999% What about those of us who get 100% up-time?"
"Well, 100% is unreasonable to expect and-"
"Our phone system can. I set up the PBX myself. It's not going down. It's the most reliable system in the company. You should give a higher bonus to teams that can hit 100% uptime."
"Ah…" The manager paused, nodded, and said, "I'll look into it," in the tone that made it quite clear that nothing was getting looked into but the conversation needed to move on.
This was circa 2002, and Natalie had just joined Initech's operations team. Mostly, she was doing the unglamorous work of untangling the rats nets of Ethernet cables left by the last person in her position, discovering that spreadsheet containing server rack assignments was incorrect, and swapping in the new UPS system for the server room.
The door next to the server room had two large buttons next to it. One of them was an accessibility button which would trigger the automatic door opener. This was useful for Natalie, when she had a cart-load of lead-acid batteries to replace. The second was the emergency stop button, which would cut power to everything in the server room.
Now, everyone was quite aware that two similar buttons right next to each other but with wildly different functions was a problem. But upper management didn't want to spend money to alter the layout or replace the buttons with something to make them more visually distinct. But they needed to do something.
So Burt, ever the problem solver, solved it. He cut a hole in the bottom of an old 5 1/4" floppy disk storage container, attached it to the wall over the emergency button, and demonstrated two features. First, by lifting the lid, you could easily and trivially access the emergency stop button. Second, it was just held in place with the weakest of drywall anchors and could easily be ripped from the wall in a real emergency. Someone decided that this was safe enough, and that "mitigated" the risk of an accidental shutdown.
Near the time for yearly reviews, both Natalie and Burt happened to be doing some maintenance at a secondary site. Simultaneously, their phones rang: someone had pushed the big red button and shutdown the server room. They rushed back to address the problems that this caused, though something about the call stuck in Natalie's brain.
Reviewing the security footage, later, the culprit was the "better idiot" who foils any idiot proof system: someone carrying a cart full of print outs walked right past the door-opener and spent several moments fiddling with the floppy disk case to figure out how to lift the lid and press the button inside.
No one was concerned with the security footage when Burt and Natalie arrived. The server room was controlled chaos, as everyone was scrambling to manually reboot servers, resolve issues with machines not coming back up, and generally panicking-not-panicking because nobody had ever really tested a full recovery and no one was sure if their procedures worked.
And that's when Natalie realized what struck her as odd about the phone call. It had come from one of the company numbers. One of the ones that ran through their PBX. The PBX was in the server room, it theoretically should have gone down when the emergency stop happened, but no- it had never rebooted, it had just hummed along quietly while everything else crashed.
Natalie turned to Burt. "Wait, how is the PBX still up?"
"Oh, see that rack over there? It's got a rectifier and a bank of deep-cycle marine batteries. When the main UPS cuts out, our phone system has a backup power supply."
In the yearly review, Burt's PBX system got a perfect 100% uptime rating. Burt's performance was not given equally high marks, though. Creating a fire hazard in the form of your own private battery bank and homebrew UPS, and not integrating with any of the key safety systems in the server room doesn't endear you to management.
In the end, Burt got a scolding and a negative note in his employee file. His PBX got put on the main power supply, sharing the same main and backup power as everything else in the building. The next time someone accidentally pushed the big red button, Burt's phone system didn't maintain its improbable 100% uptime.
[Advertisement]
Utilize BuildMaster to release your software with confidence, at the pace your business demands. Download today!
|
Метки: Feature Articles |
CodeSOD: Living Fossil |
Checking browser compatibility is less of a thing these days than it once was. In an ideal world, we check for the specific features we want to use, and fallback to a different approach, or fail gracefully if they don't exist. We use shims and polyfills and pile on all sorts of logic to navigate this mess.
What we hopefully don't do is use user-agent sniffing. The user-agent string is, honestly, one of the kludgiest, messiest, and just generally offensive ways to divine browser behavior. It was, however, once the standard, and thus there is still plenty of code, usually older code, which does use user-agent sniffing.
Which brings us to this code, from Megan. It comes from the admin application for a major logistics company for a European nation. This logistics company handles 90% of deliveries for books within that nation's borders, and is notorious for its buggy software, its habit of just dropping orders with no explanation, and generally being an unstable mess where you may have to try six times before your order goes through.
function init() {
printed = "no";
if (window.print) {
nCanPrint = "yes";
} else {
nCanPrint = "no";
}
var agt=navigator.userAgent.toLowerCase()
nVersion = parseInt(navigator.appVersion);
nIE = (agt.indexOf("msie") != -1);
nWin = ( (agt.indexOf("win")!=-1) || (agt.indexOf("16bit")!=-1) );
nMac = (agt.indexOf("mac") != -1);
nIE4Win = (nIE && (nVersion == 4) && nWin);
doPrint();
}
Now, this code doesn't start with user-agent sniffing, and instead checks to see if window.print, the method which pops up a print dialog exists. Now, for as far back as CanIUse's data reaches, every browser supports window.print with the notable exception of Firefox for Android. So, if they are targeting the 0.28% of the global browser market running that, then I guess they need to do this check.
But they probably don't need to store the results as strings inside of a global variable.
But it's what we sniff for in the user-agent that is the real icing on this cake. They sniff for IE4, which last saw a release in 1999. They also consider the machine a nWin machine if the user-agent contains "win" or contains "16bit", which implies that they're checking for a 16-bit CPU, aka a 286, for some real legacy compatibility.
This code is the JavaScript equivalent of a coelacanth: something we thought we only knew from fossils, but still lives on, to this day, in the deep waters of a logistics company moving books.
[Advertisement]
ProGet’s got you covered with security and access controls on your NuGet feeds. Learn more.
|
Метки: CodeSOD |
Error'd: Synchronicity |
Due to a chance astrological alignment, two famously alcoholic holidays collided Thursday. Your editor, along with scores of ecumenicists, atheists, heretics and other drunkards, took the opportunity to get plotzed, plastered, schnockered or fershnikit in every tradition possible. This poor column is the proof.
Ruleoneian Colin has an interest in travel to far-off places, musing "I wonder what the currency in Rule-6 is."
Checking in from Rule 4, both Carlos AND Quentin dished up the same Error'd simultaneously. Deep thinking Quentin ruminated "If there is no subject line found, and you report that in the subject line, does it still count as a subject line? Where is Jean Paul Sartre when you need him?" But practical Carlos was "...just left wondering what C015 might have been." Am I the only one who assumed it was Caesar's salad?
Meanwhile, over in Rule 7, Amazon's mobile app is ferkakte. "we_also_apologize_for_missing_translation," our anonymous contributor dripped drily.
Ill-conceived Ilkka just wants to register on the F1 website but alas, he was born in the wrong era. Or at least, the wrong week. "Which way month and the date go again? As it turns out, field and form validators won't agree so anyone born after the 12th of the month is not welcome to F1"
Reveler Gordon S. is recovering from an excess of shouting. "Whole Foods is having some counting issues or I've had too much cough syrup." Nonsense, Gordon, they count just like the wtfers. Zeroth, Frist, Other Frist... Slainte'chaim!
[Advertisement]
Keep the plebs out of prod. Restrict NuGet feed privileges with ProGet. Learn more.
|
Метки: Error'd |
