- 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
Admin
Somebody needs a "change this you fucknut" flag in their code review system, one that blocks further processing of the pull or merge request.
Admin
I was thinking maybe that was a list of all the statuses that weren't in that list. But then they use 'a' in the other if clause which is neither in that list nor the comment.
And yeah an enum is clearly the right answer here.
Admin
Given the 'a' flag doesn't prompt a where clause, I'd assume it means all
Admin
Looks like the comment explains why the Resolved codes (f and s) are not included in the code. I suspect the developer (and maybe others - including the reviewer) think that these text codes are so well known that the request for an explanation was itself unreasonable.
Admin
Also coming soon to a querystring near you.
Whaddya mean you don't know precisely what ?x=1&q=54&t=a&g=34 is meant to do?
Admin
You don't? But it's so obvious.
Admin
Yes indeed. It means x equals one and q equals fifty-four and t equals a and g equals 34. It's a parameter list. I mean, du-uh!
Admin
Oh, your codebase and database don't contain any of this sort of thing, do they? Good for you. Nice to have a new codebase developed only with post-2015 tools.
The problem with successful apps is this: data lives far longer than the code that manipulates it.
The appropriate comment here might be a reference to a document describing the meanings of the flags. And maybe even describing the transitions between flags (the flags' state machine).
Admin
Except that it doesn't explain that. It says what they are, but not why they aren't included. And it should also explain why the specific codes are included.
But the most important thing is why.
Admin
That's right. The most difficult part is the philosophical documentation.
Not what this code does, but why this approach was chosen/what the developer was trying to accomplish.
Admin
This is the real WTF
Admin
It does, it sets "and review='a'" into the text that presumably ends up in the where clause
Admin
@Tom. Agree. But ...
When the entire codebase is a stinking swamp of sewage, there's a tremendous internal pressure to just leave it that way. If your team is updating 1000 lines a week of a 1 million line codebase, it'll take 1000 weeks = 20 years to touch it all. And we know from experience that 80% of the code will never be touched and the other 20 will be touched often.
In a world where managers think about tomorrow and C-level suits think about next quarter and workers expect to change jobs next year, embarking on a 20-year quest to improve the sewage seems like a luxury whose payoff will only accrue to somebody else.
It should not be this way; but it is. Until quality comes before schedule and comes before profitability, nothing will change.
Admin
Maybe the name of the function with this code block is
checkNotResolved, so it's obvious why it lists all the status except resolved.Admin
Azure DevOps has that flag; it's called "waiting for update" rather than "change this you nut-who-fucks", but it blocks merging until you change the flag. (Depending on how you set up reviews, you can get someone else to accept the PR and then kick off the person with the "waiting for update" flag, but that should be obvious when it happens. Especially since every single action on a PR sends an email. Soooooo much email....)
Admin
But you've just proved it is worthwhile. If only 20% is touched, and that will be touched often, then you get a great return on investment if you fix up the code that is touched whilst touching it the first time, in the knowledge it will probably be touched again. The aim doesn't need to be to replace all code, just the code that matters.
Admin
No, that branch is if it's not equal to 'a'
Admin
There was a push (back in the 90s) to get ";" accepted as an alternative for "&" for query strings. I know CGI.pm (Perl) accepted it, but I wonder how many systems that'd break today.
Addendum 2021-02-22 11:38: Edit: aha, wasn't joking... from wikip for query strings "W3C recommends that all web servers support semicolon separators in addition to ampersand separators[7] to allow application/x-www-form-urlencoded query strings in URLs within HTML documents without having to entity escape ampersands."
Admin
reminds me of a terrible "error checking" routine someone submitted long ago: If (problem 1) occurs, print "error". if (problem 2) occurs print "error". etc. totally unhelpful.
Admin
You'll be delighted to know that my company uses nulls internally (yes, C nulls. ASCII nulls. strlen nulls) for query strings.
I may have to hit them over the head with a W3C/Perl clue-bat.
Admin
Related to that "why" answer...
Don't comment to tell me what to code does. I can (most of the time) read that.
Comment to tell me what it is supposed to do.
Admin
I think the real WTF is single-character statuses in the database.
Admin
Agreed.
I allow 'i', 'j', and 'k' for loop variables. I accept 'x' and 'y' when dealing with coordinate systems. For anything else, you really need more than one letter. If nothing else, to avoid collisions when you have more than a small handful of variables.
And if you name a variable 'temp' it had better be dealing with temperature. Even then it would be better to name the variable 'temperature' to avoid the appearance of evil.
Admin
I was going to say this. In most long lived LOB applications it likely is true that 80% is a mixture of really basic scaffolding, and core process stuff that if it had to change it would probably not make much sense to try to shoehorn whatever the new way of doing things is into the old application and just make something entirely new. There probably is 20% or so 'living code'.
Provide said application isnt a steaming pile of global variables with all kinds of side effects and impossible to control or understand internal state; but at least something that has some proper interface boundaries around that 80% than I'd say document those interfaces and just worry about tidying up the living 20%. If you are doing something less trivial maybe you have to go spelunking a bit but cest la vie, that's kinda the job, make some notes about the implementation details that are relevant and leave that code alone.
Admin
That's the type of code you give to the customer, so that you've fulfilled the part of the contract that says they have access to the source, but you've made it incomprehensible enough for them to make any changes.
Admin
Amateurs, I don't count the amount of "market leading" companies I talked to which not only see any ORM as a disgrace but consider it useless and counterproductive... "especially for simple queries". Hardcoded SQL all the way! I must admit the use of positional parameters put those guys on the "pros" list, you understand it's 101 after all. And OFC, dynamic schemas for everyone!
Admin
Many consider ORMs as a disgrace, also people who know what they're talking about. I don't, but there is logic in keeping query responses and objects separate. But that is no excuse for hand-assembling queries and not using prepared statement.
Admin
ORM is a tool like any other. Those that hate it are just as bad as those that use it for EVERYTHING. Sadly it happens to be one of those tools like LINQ where people latch on to it as if it were the second coming of GOTO.
I'll newer forget that fool for GOTO by the way. GOTO has its uses and it should have been retained. I want proper GOTO in modern languges and not this castrated stuff that they left in if they left anyhting in at all.
Admin
Surely those codes are listed in some table in that database called ReviewStatus or similar? if not, that's the TRWTF
It's still not the right way to do this, but it is done that way so often I don't care anymore. Using an ORM would have just shifted the cruddy approach to a different layer, and possibly made it more (not less) brittle.
Admin
Believe it or not, the developer's comment is actually very useful. It basically clarifies the whole logic. What follows is my (informed) speculation based on similar patterns seen before.
Admin
You could have a status table with flags and just inner join on it.
The "getfilter_status" variable needs some explanation. Is this taking something from a text box, list box, or something else?
Admin
We kludged our old review system to have a flag like that, except more business friendly. We all knew it really meant that, though. At one point, I checked, and I was the person to set that flag something like 80% of the time it was set. 60% of the time, I set it on my own patch, more or less indicating, "Yikes, I messed up, don't merge this, I'll fix it soon." Something like 75% of the time someone else used the flag (or about 15% overall), it was one specific reviewer setting it on his own patch. 80% of the remainder, or 4% overall, were other people applying it to my patches. About half of those were resolved with just a discussion about the overall architecture of our stuff
Something like 30% of the time, people ignored the flag and merged and deployed anyway. The vast majority of that time, it wasn't someone ignoring a flag that they'd put there themselves. About 5% of the time, the response people who did not apply the flag to their own patches made was to re-submit their patch, unchanged, but ask different people to review it.
That only worked at all because the review process reviewed specific patches. Modern tools I've seen do reviews on branches instead of patches, so the tag would seem to be directed to the concept of the change instead of the particular implementation.
What we do now is stick the work in progress tag on it and explain why. However, we're probably too reticent to do this, and the system doesn't log when we do it properly.
Admin
The comment makes more sense than the code as it shows which statuses are excluded (it shows - or at least heavily suggests - that the query clause is equivalent to "review not in ('f','s')"). An enum would have made the code more robust and more easily searchable, but assuming that the status codes are stable it wouldn't matter a great deal.
For me the worrisome part is the "Strings.isNullOrEmpty(getFilter_status())" check. It suggests that the status has not been been sanitized (otherwise zero-length strings could and should have been normalized to null or vice versa, and hence a less generic check suffices). If the code is indeed unsure about which values its inputs might have, the second branch of the if-statement is vulnerable to injection.
Admin
Nah, they're obviously using the Latin equivalent of the Greek alphabet. Then you just have to look them up here: https://en.wikipedia.org/wiki/Greek_letters_used_in_mathematics,_science,_and_engineering
So 'g' means gamma and that's obviously the circulation in fluid dynamics or reflection coefficient, or hey, it's the gamma function!
And 't' would that be theta or tau um well it does have a meaning too! See? Dead simple!
'a' of course refers to the false positive, as in that I am falsely positive that this is exactly how they are using this system! :D