CodeSOD: Identify Yourself |
Brian B stumbled across a bit of code to generate UUIDs. Seeing that tag-line, I was worried that they invented their own UUID generator. The good news, is that they just use java.util.UUID. The bad news is that they don’t understand how if statements work.
public class UuidGenerator implements IdentifierGenerator {
@Value("${spring.profiles.active}")
private String profile;
@Resource
private Map map;
@Override
public Serializable generate(SessionImplementor session, Object object) throws HibernateException {
UUID id = UUID.randomUUID();
if(session.getFactory().getDialect() instanceof H2Dialect){
return UUID.randomUUID();
}
if( session.getFactory().getDialect() instanceof org.hibernate.dialect.PostgreSQLDialect ){
return id;
}
return id;
}
}
This class generates IDs so that Hibernate can properly store objects in the database. It starts by generating a randomUUID. If, however, the type of database they’re talking to uses the H2Dialect, they’ll… generate an entirely new UUID and return that. If, on the other hand, it’s a Postgres database, they’ll… return the original UUID. If it’s none of those cases… it returns the original UUID.
There are other problems in this code- it loads a profile object from the config file, but never uses it. It creates a map which it never uses.
This code was written in 2016. It hasn’t been touched since. Only one developer has ever touched it, and perhaps they had grand plans for a complex ID generator, or perhaps they just blindly copy/pasted code which they didn’t understand.
Brian isn’t allowed to touch it, or to remove it. The entire module it’s part of works and no one is entirely certain why. So there it lives, forever and ever.
[Advertisement]
Forget logs. Next time you're struggling to replicate error, crash and performance issues in your apps - Think Raygun! Installs in minutes. Learn more.
|
Метки: CodeSOD |
CodeSOD: Strongly Unrecommended |
Asynchronous programming is hard. Because it’s so difficult, developers are constantly trying to find ways to make it simpler, whether it’s promises or callbacks, or the async/await pattern. It gets more difficult when you need to deal with handling exceptions- when a task fails, trying to recover from that failure in a separate thread is an extra special challenge.
Which brings us to Betty’s co-worker. Using C#’s Task objects, which tie into the async/await pattern, they wanted to simply ignore any exceptions thrown by one of those tasks. That’s your first WTF, of course. Their approach, however, is a larger one:
public static void Forget(this Task task)
{
task.ContinueWith(task2 =>
{
try
{
task.Wait();
}
#pragma warning disable 168
catch (Exception ex)
#pragma warning restore 168
{
// ignored
}
}).ConfigureAwait(false);
}
First, let’s talk about ContinueWith. ContinueWith is meant as a continuation for a task. By continuation, of course, we mean after the original task has finished. So, our very first line here starts by saying, “When the task (which may have thrown uncaught exceptions) has finished, start a new task”. The goal of catching unhandled exceptions has already left the building.
So what do we do in our second task (the lambda passed to ContinueWith)? Why, we wait on the original task to complete. As stated, our second task won’t start until the original task has completed, so calling Wait does nothing.
Well, not entirely nothing. If the original task had an exception and entered a faulted state, Waiting on that task… throws a new exception. Which we fortunately have a catch block to handle. And ignore. Helpful #pragmas conceal the compiler warnings for that overly broad catch block, which tells me this developer is a pro at ignoring compiler warnings.
The best part, though, is the comment on this method. Which I left out, to save for last.
///
/// Strongly unrecommended unless you are certain all
/// exceptions are eaten.
///
///
I strongly unrecommend this code. I don’t care how many exceptions you eat, don’t use this code.
|
Метки: CodeSOD |
CodeSOD: The Key to Using Dictionaries |
It's easy to use dictionaries/maps to solve the wrong kinds of problems, but deep down, what's more elegant than a simple hashed map structure? If you have the key, fetching the associated value back out happens in constant time, regardless of the size of the map. The same is true for inserting. In fact, hash maps only become inefficient when you start searching them.
Concetta recently started a new job. Once upon a time, a developer at the office noticed that the user-facing admin pages for their product were garbage. They whipped up their own internal version, which let them accomplish tasks that were difficult, time-consuming, or downright impossible to do in the "official" front end. Time passed, someone noticed, "Hey, this is better than our actual product!", and suddenly the C# code that just lived in one folder on one developer's machine was getting refactored and cleaned up into an application they could release to the public.
Concetta wasn't surprised to see that the code wasn't exactly great. She wasn't surprised that it constantly threw exceptions any time you tried to change anything. But she was surprised by this:
var result = (from kvp in HubProxies where kvp.Key == hubType select kvp.Value).FirstOrDefault();
if (result != null)
return result;
result = hubConnection?.CreateHubProxy(hubType.Name);
HubProxies.Add(hubType, result);
return result;
HubProxies was a dictionary, mapping Type keys to HubProxy objects. it was pretty clear where the previous developer had stumbled: if a certain value of hubType had never gotten a HubProxy associated with it, you'll get a key error when trying to Get the value there.
Of course, C# dictionaries have a wonderful TryGetValue method, which will accomplish two things: it will get the value and put it in an output parameter without enumerating each individual key, if that key exists, and it will return a boolean telling you whether or not the key exists.
It's the latter part which actually drew Concetta's attention to this block of code: she was getting duplicate key exceptions. This block of code was attempting to add a value for a key which already existed. It's not hard to see why. The FirstOrDefault() line will return either the first match or if there are no matches, null. But what if the dictionary contains nulls?
Concetta's first attempt to fix this code was to use TryGetValue, but that lead to downstream null reference exceptions. As it turned out, the dictionary might contain nulls, but shouldn't contain nulls. It wasn't hard to make sure an actual, concrete value was returned every time. This was no billion dollar mistake, but Concetta was impressed by how much the original developer got wrong in so few lines.
https://thedailywtf.com/articles/the-key-to-using-dictionaries
|
Метки: CodeSOD |
Error'd: The Error is ...Terror? |
"Lasterror...Las terror...Terrorist...Zoroaster...They're all so close! Which one do I choose??" wrote Ralph.
"From time to time I check into Amazon for new flavors of M&Ms. This time, I think I'll pass on 'Shoe-leather'," writes Mike S.
Gary S. wrote, "Oh, it's ok Illustrator, I'll come back later!"
"Since the maximum permitted word length is 8 letters, entering the first Jumble Bonus Word is going to be a bit of a challenge," Louise H. writes.
Tobias writes, "Oh {curse word}!! The virus is going to destroy my {brand} {model}! {other curse word}"
"I'm not a Bruce Springsteen fan myself, but Google News must really dislike his concerts," Bill T. wrote.
|
Метки: Error'd |
Announcements: Tokyo TDWTF Meetup: Bonenkai |
Tokyo readers, it's been quite a while since our last Tokyo/TDWTF nomihoudai. It's always a fun time, and we've got a good group of regulars now. Here's a pic of a group of us from a past meetup:

If you're unaware, nomihoudai is an easy way for a group of folks to get as much food and drink from the menu as they'd like for a set price over a set duration, without fussing over details like who ordered what and how many. And bonenkai, well... it's a a sort of year-end celebration, where you try to forget all of the year's woes through drinking.
So, if you're up for getting together on Friday, December 14 in the Shibuya area, please drop me a note via the contact form or direct, apapadimoulis/inedo.com.
https://thedailywtf.com/articles/tokyo-tdwtf-meetup-bonenkai
|
Метки: Announcements |
CodeSOD: Stringed Out |
The line between objects and maps can sometimes get a little blurry. In languages like JavaScript, there’s really no difference between the two. In Python, the deep internals of your classes are implemented essentially as dicts, though there are ways around that behavior.
In a language like C#, however, you’ve got types, you’ve got property definitions. This can offer a lot of advantages. When you layer on features like reflection, you can inspect your objects. Combine all this, and it means that if you want to serialize a data object to XML, you can usually do it in a way that’s both typesafe and generally doesn’t require much code on your part. A handful of annotations and a few method calls, and boom- any object gets serialized.
Unless you work at Kara’s office, of course. When they have an object that requires serialization, they must inherit from SerializableObjectBase.
public abstract class SerializableObjectBase
{
public Dictionary properties = new Dictionary();
public virtual void SerializeMe(XmlElement parent)
{
foreach (KeyValuePair item in properties)
{
parent.AppendChild(
parent.OwnerDocument.CreateElement(item.Key)
).InnerText = item.Value;
}
}
// Deserializer omitted for brevity.
}
All serializable properties must be stored in the properties dictionary. This dictionary is conveniently public, and stringly typed. The serialization method also produces a conveniently stringly-type XML document, so we don’t have to worry about anything so pedantic as schemas.
So, for example, if you wanted to create a serializable object, you might do something like this:
public class Foo : SerializableObjectBase
{
}
Look how easy that is! Of course, if your custom class has any reference types, they can’t be stored in the properties dictionary, so you’ll have to write that yourself. Something like:
public class Foo : SerializableObjectBase
{
public override void SerializeMe(XmlElement parent)
{
base.SerializeMe(parent);
if (this.BarReference != null)
{
var elem = parent.OwnerDocument.CreateElement("Bar")
parent.AppendChild(elem)
this.BarReference.SerializeMe(elem);
}
}
}
Enjoy doing that for every property that can’t be stored as a string. You may have noticed that, since the properties dictionary is public, I didn’t add any property accessors to my class. 90% of the classes in their codebase followed that pattern. You were lucky to find a class that actually bothered to implement typed accessors. Of course, since you had to store any serializable property in your properties dictionary, the property accessors usually took the form:
public int myProperty
{
get
{
if (properties.ContainsKey("myProperty"))
return int.Parse(properties["myProperty"]);
return 0;
}
set
{
properties["myProperty"] = value.ToString();
}
}
What could be simpler?
|
Метки: CodeSOD |
CodeSOD: Golf Buddies |
Hiring people you know is a double-edged sword. You already have an established relationship, and shared background, and an understanding of how they think and act. You’re helping a friend out, which always feels good. Then again, good friends don’t always make good co-workers, and if you limit your hiring pool to “people I know” you’re not always going to find the best people.
Becky’s boss, Chaz, tends to favor his golf buddies. One of those golf buddies got hired, developed for a few months, then just gradually ghosted on the job. They never quite quit or got fired, they just started coming in less and less until they stopped coming in at all.
Chaz passed the code over to Becky to fix. By “passed”, that is to say, he emailed her a zip file of the source, which was the only working copy of the code. There was no documentation, no source control, certainly no tests, and no description of what the program was actually supposed to do. “Just fix the bugs,” Chaz said.
M m = new M(true, C);
Mc mc = new Mc();
mc.AccountReference = Mb.AccountReference;
mc.Originator = Mb.ShortCode;
IEnumerable e = from x in m
group x by x.To into y
select y.First();
string r = string.Join(",", from x in e select x.To);
Msg msg = new Msg();
msg.Body = Mb.Text;
msg.Type = MessageType.SMS;
msg.Recipients = r;
mc.Items.Add(msg);
res = ms.Send(mc);
Mb.LocalStatus = LocalStatus.Sent;
Update(Mb.BatchID);
if (res.Ids.Count != e.Count())
{
Mb.LocalStatus = LocalStatus.Failed;
}
Obviously, this “golf buddy” was also a bit of a fan of keyboard golf. I mean, look at this line. Look at this.
M m = new M(true, C);
I could just stare at that line all day. Every developer tends to use a little bit of shorthand, but this whole block is amazing in its opacity. I’m convinced that the fact the class Mc has fields named AccountReference is a sign that there was at least one other developer on this project, who was trying desperately to use words.
They obviously LocalStatus.Failed in that.
|
Метки: CodeSOD |
CodeSOD: Chunks of Genius |
Brian recently started a new job. That means spending some time poking around the code base, trying to get a grasp on what the software does, how it does it, and maybe a little bit of why. Since the users have some complaints about performance, that's where Brian is mostly focusing his attention.
The "good" news for Brian is that the his predecessors were "geniuses", and they came up with a lot of "clever" solutions to common problems. The actually good news is that they've long since moved on to other jobs, and Brian will have a free hand in trying to revise their "cleverness".
void ReportInstance::WriteData(SQLConn & conn)
{
XSQL_QUERY("delete from report_data where report_id = " << GetInstID(), conn);
XString sXML(GetDetailAsXML());
{
XBuffer buff(sXML);
buff.ZipCompress();
sXML = RawToHex(buff.GetBuff(), buff.GetSize());
}
int iSize(sXML.GetLength());
int iRow(0);
for (int i = 0; i < iSize; i += 248)
{
XString sFrag("");
if ((iSize - i) > 248)
{
sFrag = sXML.Mid(i, 248);
}
else
{
sFrag = sXML.Mid(i);
}
XSQL_QUERY("insert into report_data (report_id, seq, chunk) values ("
<< GetInstID() << iRow << ZString("[" + sFrag + "]")
<< ")", conn);
iRow++;
}
}
Even just skimming this code sets my eye to twitching, mostly from the number of XML related objects in it. This is a "clever" solution to the problem of running a report and saving the results.
Run the query, and capture the results as XML. Take that XML, run it through zip compression. Then, split the zipped content into 248 character chunks, and save those back into the database.
This elegant solution is easily reversed to reassemble the report data. Even better, this removes the challenge of dealing with obscure and difficult database datatypes like blobs. The chunk column in the database is, as you might expect, VARCHAR(250).
|
Метки: CodeSOD |
Tales from the Interview: A Reusable Application |
Jay J had been helping a friend with the job hunt. As an experienced developer, with a strong network, Jay had a sense of who was hiring and what jobs were promising. One of his connections turned up a lead at Initech. Jay pointed his friend in that direction, and wished for the best.
"They won't let me apply," the friend explained when Jay asked how things were going. "Here, try it. These are my details. This is the link for the web application. Fill in the form and see what happens."
The first thing Jay noticed when pulling up the form was that it was clearly built from a toolkit of reusable widgets. The way validations appeared, the way the page laid out- this was a bolt-together HR application built out of some enterprise solution. Nothing inherently wrong with that approach- it can save time by using reusable components.
The trick, of course, is that reusable components have to be used correctly.
Jay filled in the from. Name. Address. Birth date. Attach a resume. Re-enter 90% of the information that's already on the resume. Click submit.
Error: dteBirthDate cannot be a holiday
A date picker with reusable validations is a good idea. Blindly using the validation configuration from someplace else in the system is not.
"Y'know," Jay said, "maybe you don't want to work there anyway."
|
Метки: Tales from the Interview |
Error'd: Cash vs File Cache |
"This was the ATM in the lobby of a certain hotel in Vienna," wrote Buddy, "The nice lady working at the reception desk said that 'it's been doing that, but it usually works.'"
"I know the US refuses to use the metric system, but I feel like Con Ed is taking it to the next level," writes George.
Adam G. writes, "Apparently, native CSS is now integrated in the latest Java SE release."
"Right - I don't know where that is, Google Maps, that's why I asked," writes Tom.
Bob B. wrote, "The request URI is too large, just like the font!"
"Of the things for Amazon to get wrong on Amazon Prime Day..." Neil D. writes.
|
Метки: Error'd |
A Dark Cloud Looming |
Buford was a contract developer working at a mid-sized financial firm. He had just wrapped up a lengthy project and was looking for something new to sink his teeth into. Tanner, the manager in his area, tasked him with moving their implementation of Jenkins into "this great new thing they call The Cloud."
Tanner recently returned from a conference with a bunch of swag from a company called PuffyCloud. They claimed to have the easiest cloud-based implementation of Jenkins in the business. "It's pretty much just a copy-paste job according to this whitepaper they gave me. Take a look, create some user stories, and have it done by the end of the next sprint," Tanner instructed.
Buford opened up the whitepaper and soon found that PuffyCloud was certainly full of puffery. They boasted about how their approach was "production-ready for an enterprise environment" and "dozens of organizations have revolutionized their systems with PuffyCloud and the magic of our simple Docker Compose code."
After getting through several pages of drivel, Buford already had a better, cheaper way in mind. He returned to Tanner's office to explain, "Look, I don't know what these PuffyCloud guys will charge, but I'm certain I can get the same result for only the cost of my time. I can make my own Docker script to install everything on a cloud-based server that we control without a costly middle-man license from them."
Tanner furrowed his brow before responding, "Bah! Doing this yourself will take way too long. I guess you didn't hear me the first time I explained this PuffyCloud thing. Copy. And. Paste. Do that and you'll finish easily with time to spare in the sprint.
Buford went back to reading the mind-numbing whitepaper since Tanner was clearly insistent on going with PuffyCloud. Ten minutes later, he finally got to the "production-ready for an enterprise environment" script:
jenkins_master:
image: jenkins_master
cpu_shares: 100
mem_limit: 500M ports:
ports:
- "8080:8080",
“50000:50000”
volumes_from: jenkins_dv
jenkins_dv:
image: jenkins_dv
cpu_shares: 100
mem_limit: 500M
Buford had so many questions from so little code. It sure as hell wasn't copy-paste-production. There was no way in hell 500M of memory would be enough, even for a bare-bones deployment. The image didn't even include a Docker repository. And that's before you take into account obvious syntax errors.
Predictably, the setup and implementation of PuffyCloud took Buford longer than a two week sprint. Since that didn't meet Tanner's expectations, Buford's contract was terminated. He was glad to be free of that mess but felt slightly bad that some other poor soul would have to deal with the PuffyCloud crapstorm.
|
Метки: Feature Articles |
Project Scheduling by T-Shirt |
In early 2002, Bert landed a job at Initech, which released its own protocol analyzer tool. Technically, they released a whole slate of protocol analyzers, data loggers, analytics tools, with overlapping features and business cases. Their product catalog had grown over the years, and was a bit of a thicket.
The team Bert joined was a decent mix of talent. Some, like him, were new to the industry. There were some more experienced devs, who knew the product and the low-level internals their software needed to navigate. And then there was Herb.
Herb was every stereotype of the 70s era hacker, aged up into the 00s. He had a collection of buckling spring keyboards, thought 64k of RAM was an incomprehensibly large amount, could code Assembly faster than most of the rest of the team could do C, and he knew every line of code in every product, and knew exactly what it did and why it was there.
Jack, the team's project lead, was Herb's polar opposite. Young, with a shiny new MBA, a focus on the kind of networking skills that didn't involve CAT5 cable, and absolutely no useful knowledge about anything. "Our team exists to serve the business," Jack was fond of saying. "So whatever the business needs, we do. No. Matter. What."
The business decided that their complicated product catalog was hurting their sales. Customers were confused about which product to purchase, some tools were receiving barely any updates, and there really wasn't enough opportunity to "upsell" additional functionality, since every product already had more functionality than most users needed. As part of this process, two older protocol analyzers and an unrelated data logger would be merged into a single product.
Well, the business needed something, and Jack was confident that his team would make it happen. Since this was "just" merging software together, it couldn't possibly be all that much work. It was nothing more than some mechanical repackaging, a little tweaking, and the biggest part of the work was probably updating the help files. Jack didn't need to talk to anyone on the development team to decide how they were going to serve the business.
The new product, Initech's INILYZER, would release in four months. Since it was replacing three existing products, those could be put into end-of-life. Since the INILYZER was definitely coming out in four months, they could be EOLed in four months as well.
Jack's next task was to communicate this to his team at the project kickoff meeting. Jack loved to turn project kickoffs into his own personal motivational speaking exercise, because "That's how you build team morale!" So the kickoff meeting launched with dramatic intro music (provided by a tiny CD player in the corner), a rousing speech, and then the pi`ece de r'esistance: freshly made t-shirts for the whole team, emblazoned with: "The INILYZER: August 30, 2002"
August 30th was Jack's predetermined release date.
"Well," Jack said, gleefully doing his best impression of a stadium t-shirt cannon, "does anyone have any questions before we go out and totally crush this awesome project?"
Most of the team cringed, except for Herb. He held up a shirt and pointed at the date. "Yeah. Where'd this timeline come from?"
"It's driven by business need," Jack said.
"Okay, but you're never going to hit this date."
"Um, Herb, we're doers at this company. We have a date, and we are going to hit that date."
This meeting was on a Friday. Over the weekend, Bert and the rest of the team chucked their t-shirts into the rag pile and basically forgot about them. But not Herb. On Monday, Herb walked into the office, proudly wearing his project shirt.
With a small modification, scratched in with red marker. It now read: "The INILYZER: August 30, 20023".
Jack didn't notice it until about 10AM, and when he did, it prompted a meltdown. He started screaming at Herb about insubordination, respect, and team spirit. "I want that shirt off, right now! Or you're fired!"
"I didn't bring anything to change into," Herb said.
Jack stomped off, fuming.
The project continued. August 30th came and went. Summer turned to autumn, autumn into winter. Six months after officially EOLing major products without having a replacement ready to go, there was a management purge throughout the company. Jack got fired, and there was much rejoicing.
Jack's replacement was Emma. It didn't take long for Emma to get a read for the new team, understand the source of the mess, and start working to get it as cleaned up as it could. When upper management started looking at Bert's team as another possible cut, it was Emma who made sure they understood that they were doing the best they could with an unrealistic timeline. Nobody else from the team got fired.
August 30th, 2003, the Initech INILYZER shipped. Emma, having heard about the now infamous t-shirt story, had a small plaque made up to celebrate Herb's accuracy.
Of course, with the unrealistic timeline, unrealistic goals, and management problems, the INILYZER shipped as a pile of barely usable crap. As it turns out, pretty much no customer wanted those products glued together in any way, making the whole effort pointless. It was a technical and commercial flop. The project to replace it started the very same day as its release.
Emma was the PM. Unlike her predecessor, she talked with the developers, especially the most experienced team members, to figure out what the timeline and scope actually were. The project kickoff involved no t-shirts.
[Advertisement]
Forget logs. Next time you're struggling to replicate error, crash and performance issues in your apps - Think Raygun! Installs in minutes. Learn more.
https://thedailywtf.com/articles/project-scheduling-by-t-shirt
|
Метки: Feature Articles |
A Big Change |
Circa 2009, Marylou took a position at an e-commerce firm. It was a small company, which had done its startup phase right in the midst of the DotCom Crash, but somehow made it out the other end with a steady revenue stream.
That itself turned out to be a problem, as once you turn a profit, investors stop investing. The company founders spent the next few years looking for investment to expand, and when they finally got it, they went on a hiring spree. That's where Marylou came in.
The company had been founded by developers, but over the years, they took on more and more leadership responsibilities, and handed off the development responsibilities to Joel. Joel was Marylou's new boss.
"So, the first thing you'll want to do is install our CVS client and our Eclipse plugin," Joel explained, working with Marylou to get her development box set up.
Marylou reached for the Eclipse plugin manager and started searching for a CVS plugin.
"No, no," Joel said. "Our CVS client and plugin. I forked CVS so we can use a customized version."
Marylou paused, waiting for the punchline. Joel took this silence as an opportunity to steal the mouse and start clicking around on her machine for her. "Y'see, we needed a system that let us easily stage releases before going into production. So I built one."
"Um… most organizations use a separate tool that sits on top of source control for that."
"Yeah, but this way, it's integrated!"
Once Marylou was set up, and had the code pulled to her machine, Joel started showing off some of his "clever" solutions. "Like, I wrote this using recursion!"
while(someMethod()) {
// Do bunch of stuff
}
"That… that isn't recursion," Marylou said.
"Well, it's like recursion."
"Right. So… what should I start working on?"
Joel pointed her to a configuration tool he'd built. Instead of building an admin page into the web UI ("That'd be too risky to expose to the Internet!"), he'd written a Java Swing GUI which connected directly to the database. It was a mess of tabs, most of which looked basically identical, did basically the same thing, but were just different enough to absolutely confuse and befuddle any user.
"So, we just need to change the behavior a little bit when someone clicks the save button," Joel explained. The change was a small alteration to one of the verification rules.
Marylou made the change that afternoon, and fired up the application to test. She quickly discovered that she hadn't made the change she thought she had. During Joel's tour of the code and highlights of the bits she'd need to change, she had gotten the absurd idea that all these panels shared a common codebase and inherited from a base class. That, of course, was a crazy idea. Joel had written the code to power one panel, and then copy/pasted it into the next panel, with minor changes. Rinse, repeat, and suddenly you've got a UI with 15 panels that all basically do the same thing but use copy/pasted variations on the code. To change the behavior of the save button required going to each and every panel and making the change. In total, there were 30,000 lines of code that were essentially duplicates of each other.
Marylou made the change, and got it set up through the broken CVS staging system for review. Then she went back, and looked through the duplicated panels. And then started refactoring. And refactoring. It took, in total, about a month of picking apart Joel's mess to build something that acted as a reusable widget with all the required features, which she kept plugging away at while working on all the other code. The 30KLoC shrunk to a "mere" 3KLoC. 15 unique widgets collapsed into one single class which could be dropped onto any screen. Future changes could easily be made.
It was a huge change, and it involved some serious changes to the class hierarchy. It was big enough that when Marylou shipped it off for Joel to code review, she expected it would take him awhile to look at, and would probably receive some pushback.
It was a reasonable expectation, but Joel had his own style. Minutes after she sent the request, Joel approved it, and filled in the required "Code Review Comment" field with "Looks great."
His glowing recommendation didn't help Marylou's confidence about the change, so she tracked down another peer, Chris, who had been hired in the same spree, but had a bit more industry experience. They sat down, went through the code together, and spent a few hours walking through the reasoning behind the changes and the actual implementation thereof. With Chris's seal of approval, Marylou felt much more confident in her changes.
This, of course, proves why Joel led the team. It took Marylou and Chris hours to decide that a massive refactoring "looks great," while Joel could tell at a glance. That's experience.
|
Метки: Feature Articles |
Representative Line: Without a Parent |
Rob M caught a ticket for a bug in a C# application. Specifically, when the user picked an item off a menu, that item wouldn't get highlighted, thus defeating the purpose of the menu. Strangely, the code hadn't been touched since its first commit, back in 2015.
var sortedParentChildItems =
matchedMenuItems.OrderBy(x => x.ParentID ?? x.ParentID).ThenBy(x => x.ParentID);
Somehow, this particular line made it through code review, which was notable because management was extremely proud about how "rigorously enforced" their code review process was. The "rigorous enforcement" took the form of someone filling out forms and running a meeting that hopefully contained more programmers than managers. It created a lot of hours on the time-sheet marked "code review", so it must be rigorous.
In this case, this line has several steps of silliness. The ?? is the null coalescing operator- if the left-hand operand is null, use the right one. So, we'll sort by the ParentID of a menu item, unless it's null, in which case we sort by the ParentID. Once they're sorted by ParentID, we'll sort within that list, by ParentID. If it's null this time, enh, whatever, we don't care.
Arguably, the root cause of this bug was a null value snuck into ParentID somewhere, and that broke the sort order. Realistically, the root cause was a process which was more focused on having a process than doing a useful code review.
|
Метки: Representative Line |
CodeSOD: Classic WTF: A Spoonful of Sugar |
It's Black Friday in the US, which is a fancy way of saying that we're all busy murdering each other in WalMart over PS4s. In case we don't survive, remember us by this classic article about Cold Fusion. Original -- Remy
John S. was doing some work on the search feature of a client's website when he noticed that he would receive a 500 Server Error if he tested against the API with an empty string.
This struck John as being pretty strange since not only had the search feature had been in place for years, but also, he could go to the search page, click on the "Search" button without entering anything and receive an "Item Not Found" response.
Curious, John poked around the underlying web code behind the search page and found that a previous developer had been aware of the error and had implemented a quite precocious workaround.
https://thedailywtf.com/articles/classic-wtf-a-spoonful-of-sugar
|
Метки: CodeSOD |
CodeSOD: Classic WTF: zzGeneralFunctions |
It's Thanksgiving in the US today, so we're running a classic WTF. What are you thankful for? How about not needing to support this application. Original --Remy
A codefile whose name is prefixed with “zz” can be one of two things. It's either a file that someone wanted to get rid of but was afraid to delete, or it's an intentional naming scheme to keep the file at the bottom as part of some crude code-organization technique. There used to be a third option - the file's a part of an application commissioned by a certain American rock trio known for their beards and cheap sunglasses - but the band dropped that requirement a long time ago.
However, when Mark Arnott stumbled across "zzGeneralFunctions.asp" as part of a maintenance project he was assigned, it was pretty clear why the file existed. Its first line contained the following comment:
' If you do something more than once, put it in a function here.
After a quick skim through the tens of thousands of lines of code, it was fairly obvious that, while the developers followed that "more than once" comment to the letter, they didn't follow the implied advice: before you put a function here, check if that function already exists. Take, for example, calculating a given month's name.
function MonthName(mNum) dim m on error resume next m=0 MonthName="??" 'if IsDate(mNum) then m=month(mNum) m=int(mNum) if m=1 then MonthName="January" if m=2 then MonthName="February" if m=3 then MonthName="March" if m=4 then MonthName="April" if m=5 then MonthName="May" if m=6 then MonthName="June" if m=7 then MonthName="July" if m=8 then MonthName="August" if m=9 then MonthName="September" if m=10 then MonthName="October" if m=11 then MonthName="November" if m=12 then MonthName="December" end function
Ignoring the fact that there's already a built-in VBScript function that does just that, and that the built-in function is also named MonthName, this code seems perfectly logical. In fact, so logical that it was repeated a few lines down.
function GetMonthName(thisNum) dim m on error resume next m=0 GetMonthName="??" 'if IsDate(thisNum) then m=month(thisNum) m=int(thisNum) if m=1 then GetMonthName="January" if m=2 then GetMonthName="February" if m=3 then GetMonthName="March" if m=4 then GetMonthName="April" if m=5 then GetMonthName="May" if m=6 then GetMonthName="June" if m=7 then GetMonthName="July" if m=8 then GetMonthName="August" if m=9 then GetMonthName="September" if m=10 then GetMonthName="October" if m=11 then GetMonthName="November" if m=12 then GetMonthName="December" end function
And then there was this function which did mostly the same thing, and also replaced the built-in functionality of VBScript's MonthName.
function MonthAbbr(m) on error resume next MonthAbbr="??" if IsDate(m) then m=month(m) if m=1 then MonthAbbr="Jan." if m=2 then MonthAbbr="Feb." if m=3 then MonthAbbr="Mar." if m=4 then MonthAbbr="Apr." if m=5 then MonthAbbr="May" if m=6 then MonthAbbr="Jun." if m=7 then MonthAbbr="Jul." if m=8 then MonthAbbr="Aug." if m=9 then MonthAbbr="Sep." if m=10 then MonthAbbr="Oct." if m=11 then MonthAbbr="Nov." if m=12 then MonthAbbr="Dec." end function
The combinations and permutations of almost identically named and functioning date/time methods were astonishing. At some point, functions names started to end with a number. And then came the prefixes.
function zzFormatDate4(thisdate)
dim tmp, m
on error resume next
tmp = ""
if (IsDate(thisdate)) then
m = month(thisdate)
tmp = ""
if m=1 then tmp="January"
if m=2 then tmp="February"
if m=3 then tmp="March"
if m=4 then tmp="April"
if m=5 then tmp="May"
if m=6 then tmp="June"
if m=7 then tmp="July"
if m=8 then tmp="August"
if m=9 then tmp="September"
if m=10 then tmp="October"
if m=11 then tmp="November"
if m=12 then tmp="December"
tmp = tmp & " "
tmp = tmp & right("00" & day(thisdate),2)
tmp = tmp & ", "
tmp = tmp & year(thisdate)
end if
zzFormatDate4 = tmp
end function
By the time Mark got to zzFormatDate4, he just gave up looking. It was going to be a very long maintenance project.
https://thedailywtf.com/articles/classic-wtf-zzgeneralfunctions
|
Метки: CodeSOD |
CodeSOD: Classic WTF: Let Me Sleep On It |
We're starting our Thanksgiving break a day early this year. To make up for it, we're dipping back into the archives for a classic WTF. Original
"Perl is a language for getting your job done," is the underlying philosophy of the language. The only right way to write a Perl program is whatever way works. The ultimate flexibility of Perl is a breeding ground for WTFs . That's doubly true when you're new to the language, like Dave once was.
To get Dave started with Perl, his boss paired him up with Alvin, the veteran Perl programmer. He'd been using Perl since version 4, and had a reputation for wielding regexes like a scalpel. After Dave had a few days of ramp up, Alvin started sending him code from their codebase so that Dave could try and understand how their applications worked.
Alvin's style might have used a lot of doSomething(foo) unless condition; constructs , but his coding style was fairly consistent and clear. Dave followed his code pretty well- although one convention really confused him:
#after writing a bunch of stuff to a file close(MYHANDLE); sleep(5);
Every close had a matching sleep. Dave couldn't help but ask Alvin, "Um… why is that sleep there? "
Alvin gave Dave the Stare™- that alpha-nerd, hairy-eyeball that makes you wonder, "Did I just say something incredibly stupid?" Just before Dave caved in and said "never mind", Alvin replied. "It's to make sure the file handle is really closed before the program continues."
"I don't think close works that way," Dave said.
"I'm pretty sure it does," Alvin said.
"I don't think so," Dave said. He made a copy of the script in question and with what little regex-fu he had learned so far, he replaced all the close/sleep pairs with just a plain old close. He ran the script, and as you might expect, it worked flawlessly and took about 15 seconds fewer to run. "See?" Dave said, "You don't really need those."
"Apparently," Alvin said.
They spent the rest of the afternoon tracking down major places where there were close/sleep pairs. Alvin grudgingly let Dave remove them and check the changes into CVS after testing them to prove it still worked. Dave went home feeling pretty clever. He really was learning this Perl stuff.
Dave slept on it, and he felt pretty good about everything until he got into work the next day. Over night, Alvin had gone back through the code and added in each and every one of those sleeps, and he made them all one second longer. Dave stopped down at Alvin's cube. "Hey, about those sleeps- you put them back?"
Alvin gave Dave the Stare™. And he didn't stop until Dave retreated to his cube. Apparently, those sleeps really were important to the application.
[Advertisement]
Forget logs. Next time you're struggling to replicate error, crash and performance issues in your apps - Think Raygun! Installs in minutes. Learn more.
https://thedailywtf.com/articles/classic-wtf-let-me-sleep-on-it
|
Метки: CodeSOD |
CodeSOD: Class Warfare |
Setting aside cross-browser quirks, CSS is a fiendishly complicated specification. There’s a lot to it. From styling rules and how they interact with the DOM hierarchy, to the complexities of using selectors to navigate the DOM- it’s a complex tool that is also very powerful. I mean, it’s Turing complete.
Shiv works with a self-proclaimed “CSS Ninja”- yes, that was actually in their resume when they got hired. They were hired on the strength of their portfolio- it looked very nice. Unfortunately, the implementation left something to be desired.
For example, imagine you had twenty elements on a page which needed to be styled the same. You might choose to apply a class attribute to them, and create a single styling rule for that entire class. But that’s not what a CSS ninja does.
.placeholder1, .placeholder2, .placeholder3, .placeholder4,
.placeholder5, .placeholder6, .placeholder7, .placeholder8, .placeholder9, .placeholder10,
.placeholder11, .placeholder12, .placeholder13, .placeholder14, .placeholder15,
.placeholder16, .placeholder17, .placeholder18, .placeholder19, .placeholder20 {
border-top: 1px solid #333333;
color: #FFFFFF;
height: 180px;
padding-top: 15px;
width: 140px;
}
That’s right, a true CSS Ninja creates a unique class for every single element you want styled, and then creates a stylesheet rule that selects all of those unique elements. It’s brilliant, because this way, if the style for .placeholder6 ever needs to change, you can just make a new rule for that one.
Bonus Ninja points for using absolute units for sizes and dimensions, which I’m certain is never going to cause a problem rendering on different display sizes.
[Advertisement]
Forget logs. Next time you're struggling to replicate error, crash and performance issues in your apps - Think Raygun! Installs in minutes. Learn more.
|
Метки: CodeSOD |
CodeSOD: A Clever Switch |
Today's anonymous submitter has this to say about this code: "It works fine, it's just... clever."
I'm not certain about the relative cleverness of this solution, myself.
switch (true) {
case (d <= 15000):
m.values[0]++;
break;
case (d > 15000 && d <= 30000):
m.values[1]++;
break;
case (d > 30000 && d <= 45000):
m.values[2]++;
break;
default:
m.values[3]++;
break;
}
This JavaScript lives in a web dashboard for monitoring an internal system. Like most such systems, it was slapped together in a rush with no real thought, and nobody actually cares too much about it so long as all the indicator lights on the dashboard stay green.
The obvious thing about this switch is that it should be an if. Arguably, it should even be a loop, which would allow you to iterate across a series of breakpoints, so that you could have a histogram with an arbitrary number of buckets. Perhaps that's premature abstraction, but at least this should be an if.
I'd suggest that maybe they were trying to play some code-golf, since case (...) : is shorter than else if (...) {, but all those savings are lost if you count the break.
That said, I have an idea to make it worse. This is JavaScript, so you could actually do this instead:
m.values[0] += d <= 15000;
m.values[1] += d > 15000 && d <= 30000;
m.values[2] += d > 30000 && d <= 45000;
m.values[3] += d > 45000;
It's less efficient, as you check every case every time, but look at how much more compact it is. And this is for a web page, nobody cares about efficiency in web development anymore. Just look at how clever it is in its (ab)use of the JavaScript type system!
May the gods save us from "clever" programmers.
|
Метки: CodeSOD |
Error'd: A Right to Remain Ever Conscious Blooms |
Eion R wrote, "Sure Google Voce, that is exactly what I was looking for."
"One might think this game is titled 'Alpha Blending and the Revenge of Floating Point' but it's not. It's just broken," writes Ashley A.
Bryan S. wrote, "I just hope that whoever created this survey doens't work on their online banking app."
"Now that it's the end of the day, I'm pretty sure I've walked some finite, non-negative number of steps," Drew C. writes.
"Why yes, I am interested in supporting the https project on Indiegogo. I think it'll be a big hit," writes Adam R.
"I guess I can't fault the hotel for trying to make some of its money back by including ads in their 'goodbye' note," wrote Andreas R.
[Advertisement]
Forget logs. Next time you're struggling to replicate error, crash and performance issues in your apps - Think Raygun! Installs in minutes. Learn more.
https://thedailywtf.com/articles/a-right-to-remain-ever-conscious-blooms
|
Метки: Error'd |