- Feature Articles
- CodeSOD
- Error'd
-
Forums
-
Other Articles
- Random Article
- 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
It's absolutely the frist thing to do on the journey towards a rewrite or refactoring.
Admin
While this is indeed terrible code and terrible processes, and I feel a lot of sympathy for Chris, I'm rather surprised he wasn't able to divine anything about what this function does and what the values mean. Even if there were zero other developers he could ask (maybe this was something produced by a third party years ago?), surely tracing through the places in the code where this is called would eventually shed some light? Not that it won't be a long and painful process of WTF discovery, but as long as the code isn't deliberately obfuscated - which isn't the case in the snippet shown in the article - it should be possible to discover something. And then document it (sensible tests would help with this even if nothing else is done!) so future developers don't have to go through the same process.
Unless of course Chris was told to write the entire test suite in some impossibly short space of time and his superiors wouldn't listen to sense - in which case that, not the dodgy code, is TRRWTF.
Admin
Starting by writing tests is not a WTF in itself, as it'd be conceivable to use test-driven development for the refactoring.
TRWTF is to test the old, bad, non-understandable code, rather than writing tests for the new code that'll (hopefully someday) replace the old code, or than writing less-detailed tests on the entire functionality at once (rather than on the testing those crappy individual functions).
Admin
From the casing I think this is C++, so it is actually not that bad. Granted, I would drop the redundant -1 cases, but the rest is fine and most compilers will optimize it anyway.
This looks like a simple getter where depending on the type you get returned a priority. I guess this number is later used for scheduling purposes, the highest number first down to the lowest.
So yeah, not really a WTF honestly, actually pretty well done. Now if in this case someone would have created a std::map and tried to look values up, now that would have been a WTF.
Admin
That's a good example of how writing unit tests is ... a lot more difficult than you might think.
I've seen examples of a unit test that basically reads in values from a database (default values from the EC5 specs for structural wood engineering, fwiw) and then checks that the values are ... um ... exactly what you expect them to be. I've even done it myself, although in my defence, the unit tests at least checked filtering of default values by country code. Still almost entirely useless, although it did catch at least one bug.
And perhaps more to the point, what's with this switch nonsense? No enums (which would count as documentation of a sort)? No dictionary/map? No abstraction whatsoever?
Seems to me that unit tests are often allocated to the most junior or most confused or possibly most brain-dead member of staff, with results like this. I wouldn't accept it on a code review of actual code, but I'm positing that nobody really spends any time code-reviewing a unit test.
Admin
@SPV: I believe the code shown is NOT the unit test. It's the code to be unit-tested.
And yes, like you I'd hope some sort of map would be used in the testing code to, well, map the inputs to outputs. Leading to eventually refactoring the working code in the same manner.
Admin
What you got to do is to make sure the code after the rewrite does exactly the same as the code before the rewrite.
So you assemble your test suite for the before-rewrite, and arrange for the results of the tests to be assembled into some form where they can be directly compared against the results of the parallel tests in the after-rewrite. At each stage of the development of each module you make sure the results match. If they don't, then there is either a bug in the before code or in the after code. If the latter, fix it. If the former, document it and bodge your after code to show the same bug.
You may say: how do I know whether the difference is caused by a bug in the before code or not? I don't know what the before code is supposed to do.
Aha -- this is the stage at which you are starting to develop some domain knowledge. You can't work on a project without getting at least some idea of what the program is supposed to do.
During the course of this process you begin to understand the program, and that's when your insight into how the program can be improved takes hold.
Admin
First order of business should be to determine if this function is actually called anywhere. If not, problem solved. :)
Admin
Not so fast, this seems to be C++ ;-)
Admin
Not that hard. Short of it being exported out of DLL or function pointer/object simple text search will cover nearly all the cases. (Rest can be found out by debugging or logging code)
Admin
I've had to go back and retrofit multiple codebases with tests (though the code wasn't this bad.) In my experience, the best attack here is to work outside-in: don't start with unit tests, but instead with super high-level acceptance tests a la Cucumber. The app has some kind of interface (an REST API, a GUI, input/output files, something) so test it end-to-end to make sure that its behavior is stable no matter how it's implemented or what changes happen. Then work inwards to finer grained integration tests, and then finally to unit tests at the end.
Admin
Ah, I think that was written by someone who dropped out of my C class. A certain someone who thought a switch statement was a good replacement for 'strlwr()'.
Admin
Well, you can write characterization tests for code you don't understand. Such tests don't prove that the behavior is correct, but they document the current behavior and ensure it's not accidentally changed. With good characterization tests, you can refactor with reasonable certainty that you're not breaking anything.
Admin
If you merely want to check that a rewrite has not changed behaviour, you don't need to write code to test the old implementation - just run both old and new, and compare the results. Only if you need to understand the code or change the behaviour do you need to write test code - but then the test code must reflect the desired behaviour and not the current behaviour.
Admin
I was referring to templates ;-)
Admin
"At this point, the application has plenty of tests which confirm the application does what it currently does, which is better than no tests at all" - is it really though? If what it does is bad, such tests enforce it staying bad. And besides, the only way to write these tests is to basically copy/paste the awful code from the sources into the tests, and what that in turn means is you now have awful code in two places instead of one.
Admin
So… you don’t see any WTF in undocumented copy-pasted magic numbers ?
Admin
Differentiating "Pinning" tests from "Correctness" tests is a key part of any strategy. For legacy code the goal is to catch unintended changes to behavior, so the initial version of the test is it fail if there is ANY change in behavior, not make any judgement if the current behavior is correct/desirable.
Admin
More likely Java. Very very dispiriting Java, from the bad old days before
enum.Admin
Thiis reminds me of the ticket prioritization scheme that is used, or was, at IBM (AT&T had a similar scheme). When you called in a ticket, you would be asked for a priority. Choices were 1-Urgent, 2-important and etc.
No matter what you chose, you would get a call back whenever they felt like getting around to it (or Hell froze over, you know) .
In order to get actual service, you had to know about another field on the ticket, which was named something obscure like "Service Level." To get immediate service you had to tell the ticket taker to set that field to 1. The "Priority" field they so diligently had you fill in wasn't even relevant.
The mappings in the function in this article look like somethinig similar: The customer speifies priority 1, which maps to RealPriority of -1, which translates to "when Hell freezes over." Only if you specify the magic priority number 18, which gets RealPriority mapped to 15, will you get an immediate callback.
Admin
Assuming those -1 returns in the switch are "redundant" is wrong.
There is a logical difference between the -1s that are returned in response to a recognised input and the -1 that is returned because none of the case values matched.
As you already said, the compiler/optimiser will generate the same code - but to the reader/maintainer of that code the difference between "valid value handled by returning -1" and "invalid value being handled by returning -1" is very important when "invalid value" is dictated to suddenly have some different behaviour.
Don't get me started on switch() statements of an enum which have a "default" case ... :)
Admin
cool really
Addendum 2022-12-27 15:47: It's not a problem to hire really talented developers right now. I already know for a fact that anyone can do it. But the best place to look at is of course here and read more details about hire ukrainian developers , they really helped me and I am very happy about it, because they are really worth hiring if you want to get quality results. I hope that it will be useful to you and you will not have any problems in these cases. Good luck and success, I hope it was helpful for you all to learn more.