- Feature Articles
- CodeSOD
- Error'd
- Forums
-
Other Articles
- Random Article
- Other Series
- Alex's Soapbox
- Announcements
- Best of…
- Best of Email
- Best of the Sidebar
- Bring Your Own Code
- Coded Smorgasbord
- Mandatory Fun Day
- Off Topic
- Representative Line
- News Roundup
- Editor's Soapbox
- Software on the Rocks
- Souvenir Potpourri
- Sponsor Post
- Tales from the Interview
- The Daily WTF: Live
- Virtudyne
Admin
I've never understood why people are incapable of seeing why their code is bad when it's staring them in the face
Admin
frist = null
Admin
public static string ScrubFrist(string comment) { string newComment = string.Empty; if (string.IsNullOrEmpty(comment)) newComment = null; else newComment = comment; return newComment; }
Admin
Ignorance? Arrogance? Who knows. I've seen a lot of crappy code written by people who should have known better but were too lazy to actually try, they just wrote the first thing they could think of to finish the task and then move on to the next one.
Admin
There is a certain therapeutic satisfaction in having a job that consists of cleaning up shit code. It's like an academic exercise:
"Question 1: a) Identify the aspects of this code which fall short of optimum. b) Implement improvements for these areas of concern. (40 points)
Question 2: For the improvements implemented in question 1: a) estimate or calculate: i) the percentage improvement in run-time ii) the percentage improvement in size of codebase
b) indicate particular points of interaction at which customer satisfaction will be improved, and estimate the probability that, as a result of your having considerably improved her daily workload, you'll get to boff that tasty little brunette in Customer Services."
Admin
Admin
Admin
Without knowing C#, would the following work?
Admin
I can't even comprehend what was the motivation behind this code's existence.
"Oh look, sometimes when someone enters an empty string it goes ahead and does something we haven't got time to go into the codebase to work out. Shall we just make sure that when anyone enters nothing into a mandatory field we invoke a null pointer exception? Then we'll be able to catch it and feed the stack dump back to the user. That'll work."
Admin
TRWTF is a full rewrite rather than fixing
http://www.joelonsoftware.com/articles/fog0000000069.html
Admin
Admin
yes. however given the original function your function should utilize string.isNullOrEmpty
Admin
This is rather hard when it is YOUR code. Every one else code usually stinks anyway !
Admin
Admin
... actually I have it on good authority that the charmer in Sales is already married and has three children (all with different women, none of which is his wife, who is built like a brick shithouse and has been known to hospitalise her rivals). If the cute brunette wishes to continue with her less-than-sensible course of action and thus risk life limb and happiness, then she's not the girl for me, I thought she was more sensible than that. On the other hand, if she is open to listening to reason, and would prefer a straight-up guy who actually knows how to fix things, not just fuck them, then hey, how about it, babe?
Admin
TRWTF is Joel's URLs.
Admin
String b now is null, the connection with string a has been severed. String a can now be garbage collected.
Still a rather costly way to do this;
would be more concise and would not necessitate the creation of a temporary object.
Admin
Isn't that just how TDD ends up working?
Admin
Admin
If that's genuinely the case here, then TRWTF is that someone decided that the data scrubbing logic was "If the string is null or empty, make sure it's null, otherwise just use the string as-is".
I have to admit I once invoked code that involved something like
but only because in that situation passing an empty string produced more consistent behaviour than passing null. And I didn't call it 'Scrub' anything... ;)Admin
captcha: oppeto. As far as I remember oppedo (Latin) means "I fart"
Admin
Admin
Strings are interned anyway, so there's probably not much memory to save (yes, you can disable string interning, but that's rarely a good idea)
Admin
In C# is it considered good practice to set strings to NULL instead of empty?
TRWTF is the management policy of rating programmers by Lines Of Code produced. This is why they have an entire contingent of redundant scrubbers, instead of a one line function for all strings.
Admin
Depends what you're going to do with the string :p
No, seriously. It's entirely contextual. AFAIK there's no real advantage one way or the other (except perhaps for consuming code, which would know whether it needed to check for null, Empty, or both. Then again, that would rely on the code documenting whether it returned null, or Empty, or either (arbitrarily), which given the quality of this code, seems unlikely).
Admin
Alternately,
Management: "What do you mean you're scrubbing all data the same way?! Everybody knows that you can't scrub a customer name the same way you scrub a phone number - they're completely different! Now stop being lazy and go back and do it right this time!"
Admin
Well, it's hard to see the WTFs in your own code in part because when you write the code, it (hopefully) makes perfect sense to you and you have the full context of the project loaded in memory.
It's when you come back six months (or sometimes even a few days/weeks) later and the contextual memory in your brain has been overwritten many times that the WTFs start jumping out at you.
If you can't spot WTFery (or, at minimum, things that could have been done better) in your own well-aged code, then you seriously need to work harder on improving your skills.
Admin
Not quite right, faoileag. The .Net optimizer will convert the string constant "" to a reference to a predefined static (string.Empty), so there is no issue here. Also, variable a can indeed be "garbage collected" regardless of the value it was set to. however the memory referenced must still remain since another object still references it.
Admin
Throw yourselves into this video, to escape all this hideousness... you'll love it, you little tarts:
http://www.youtube.com/watch?v=WYDlX49yUSI
Admin
Correct...and sometimes you even have to set them to: DBNull.Value
Admin
Admin
Specifically, sometimes the old codebase CANNOT accept a very important change. The example I think of is when the old codebase, whatever its faults, cannot be ported cleanly, if at all, to a new platform. Imagine a crufty(*), brittle codebase that is heavily tied to use of COM / DCOM interfaces between components, and deep-juju Win32 threading weirdness. Now imagine that a major(**) customer says, "We love the product, but no Windows in our datacentre please. Linux is fine, but not Windows."
What do you do? Throw away the customer and rely on the one other customer you actually have?(*) Write a DCOM emulation layer for Linux, including support for all the marshalling madness, custom moniker implementations(**), and so on? Rewrite the code so that it is portable to any platform with a TCP/IP layer?
Bear in mind, in passing, that one GUI-less network server component had been written in VB6, for reasons that continue to defy my capacity to imagine any justification for such a thing.
No, in this case you rewrite it. The old codebase was brittle, and had extensive multithreaded locking / object lifetime management issue. It would crash and burn accessing deleted objects, or it would deadlock nine ways to Sunday at the slightest opportunity. If it got busy, it would sometimes fail when Win32's 2GB user address space was full of thread stack space, somewhere around 1500 threads(!).
The new codebase was largely platform independent, aside from a portability layer, and single threaded. I worked on the architecture of it with a colleague who had previously worked on routers at DEC, and as a result of his experience, we had a model that never completed anything in-line, but posted delayed callbacks (overlapped I/O style) to all requests. No code was allowed to block for any reason. The new code base came up and working in about half the time the previous one took, and just worked. It never deadlocked, nor did we ever have problems with long-term leaks, nor objects dying before their time.(*****) Furthermore, we also had ports of the platform layer for Win32, Linux, FreeBSD, OpenBSD, and VxWorks.
() It was crufty despite being less than three years old with only one production release. Go figure. () I.e. one of two, an ISP (the other being a California school district) () See (**). My company was a dotcom startup that eventually stopdowned, but not until late 2003. Complicating its continued existence was a tendency to sack almost the entire marketing and sales crew at intervals of 18 months or less, and what I suspect was poisonous jealousy on the part of the co-founder-CTO of some inventions that came from developers (me, mostly, if you must know). (****) Custom monikers aren't so bad in and of themselves, but the IMoniker API documentation is vague, unclear, and in some places just plain wrong. Or maybe it's right, but if so, Microsoft's ROT viewer doesn't follow the API as written.
(*****) The code was C++, and we had a set of intrusive dual refcount classes, combined with two types of reference objects. 'Intrusive' means simply that the refcounts were stored in the objects themselves. When the "action" refcount went to zero, a mandatory "deactivate" method was called. When both refcounts went to zero, the object deleted itself. ("delete this;" is perfectly valid C++, but a bit dangerous if you aren't careful. It makes a good technical interview question for non-beginner candidates.) Errors in the deactivate method tended to result in leaking thousands of objects, but all active objects were linked together by a double-linked list maintained by the refcount holder base class, and a diagnostic routine would dump a list of objects that were left behind. It was considered good practice to call it at the end of main(), after scrubbing all remaining references.
Admin
...I wish.
Admin
Whee! I've got a date!
Admin
Admin
Sometimes the initial assumptions made during the design phase are utterly wrong and have led development down a path where even minor changes cause problems.
Sometimes a regime of short-sightedness has led to an application being patched and re-patched, without regard for the overall design, to the point that the overall design is hardly discernible.
Admin
well the name is misleading and the effect could be done by a simple ?: operator but other than that I don't see much wrong with this code at all.
if you want to treat empty strings as if they were null it's a perfectly reasonable function to write
Admin
Naturally, those than can't program at all end up in management telling the rest of us how to do it (wrong) and accepting the accolades when we do it (right).
Admin
I was assuming the code was written as a simple stub with the idea that "we'll come back and fill in the guts later, with real data cleaning and validating and sanitizing when we have time." Of course, that never ever happened.
Admin
Also, programming environments become...outdated. The first programming job I had was maintaining code written in and around a (multi-valued, not relational) database system that was designed in 1968. It uses upper ASCII characters for field delimiters, and that caused great Fun when dealing with non-American customers. It's limited to ASCII characters, so no Unicode - in the event that Unicode support becomes necessary (which is pretty likely, given their customer base), the entire code base will require a rewrite on some other platform.
Admin
You've nailed one of the problems I have with Joel On Software. He tends to be very dogmatic, seeing everything in black and white, whereas the real world tends to be in shades of grey. A good software engineer has to be pragmatic and apply the right levels of the right principles to a given situation.
The other (which is a more specific case of the above) is that he seems to have a lot of contempt for his users, feeling that they are idiots who should be led down specific paths. I know it's usual to user-bash here, but there are cases where you need to cater to the idiots while allowing for the intelligent users, and Joel doesn't seem to get that.
Admin
Could you share some more details about the model? I'm interested mostly because:
in my experience (probably different domain, or just wrong kind of experience ;) ) most multithreading issues turned out to be caused by not doing as much as possible "inline" (and using excessive number of threads, but this aspect seems common to both cases, yours and mine),
I don't quite understand (and would like to) how it was single threaded and couldn't block: the main() function would need to use 100% CPU or return immediately when there is nothing to do, wouldn't it? (Or is it how it was supposed to work, i.e. do what is to be done and exit? Perhaps I wrongly assumed that it was a kind of a server app that was supposed to react to external requests...)
Admin
There was a callback proxy-stub generator that read descriptions of interfaces so that you didn't have to write the boring boilerplate code to post result notifications back to calling objects.
There are four main sources of threading-specific problems that I've seen in threaded code:
It's possible, as we did in the old system, to have all four categories of bug, even in the same pair of objects.
(*) The network layer was designed to be saner to program than the normal sockets API, with different objects for TCP connections, TCP listeners, and UDP listener-senders. Inside, it used a platform-specific layer to adapt to the specifics of the sockets API on the different platforms (or, even, the fact that a particular platform had a different API). There were subtle differences between the BSDs and Linux because of silly things like FreeBSD's unique (among the platforms we tried) behaviour of completing to-localhost connect() calls for non-blocking TCP sockets in-line rather than failing with EWOULDBLOCK.
Admin
Sometimes the code is so bad, and so intertwined that you can't easily fix it because you'd end up in a rat's nest of having to fix module A, but module A depends on Module B which needs to be fixed first, which needs Module C, and so on until you have to fix everything to fix everything.
Better in that case to declare "technical bankruptcy" and start from scratch.
Admin
That sounds like a PICK database. My first professional job used a PICK "multi-valued" record system. Not terrible, but less than ideal. But the real kicker was that we ran into the memory limit for executables for our main application. The "OS" was running on top of Windows NT on a 486 (50MHz!) with 4MB of RAM, but the "OS" wouldn't permit an exe over 32K.
The app was a spaghetti nightmare of UI code mixed with business logic. I was forced to rewrite most of the code in order to extract all data access into a separate application so that the main application could handle the UI and business logic. Ultimately, this failed to meet our needs and we rewrote it again in Visual Basic and used the PICK database/OS merely as a data store. I left the company, but they allegedly spent years trying to migrate to SQL.
Sometimes, you gotta bury bad code and start fresh. Not because it offends your sensibilities, but because it simply doesn't work.
Admin
Re:" If the cute brunette wishes to continue with her less-than-sensible course of action and thus risk life limb and happiness, then she's not the girl for me, I thought she was more sensible than that. On the other hand, if she is open to listening to reason, and would prefer a straight-up guy who actually knows how to fix things, not just fuck them, then hey, how about it, babe?" -- story of my life, they always continue with the less-than-sensible course of action...
Admin
Aside from those two issues, none of the application-level code needed any tweaking when doing a port. NONE of it. We were so pleased.
Admin
LMAO
Admin
Admin
Isn't she the President's daughter?