• 🤷 (unregistered)
    if (Strings.isNullOrEmpty(getComments())) {
    	sql.append(" and review in ('f','r','i','s','t','')");
    }
    
  • (nodebb)

    Somebody needs a "change this you fucknut" flag in their code review system, one that blocks further processing of the pull or merge request.

  • Best Of 2021 (unregistered)

    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.

  • Jaloopa (partially registered) (unregistered) in reply to Best Of 2021

    Given the 'a' flag doesn't prompt a where clause, I'd assume it means all

  • Mobeer (unregistered)

    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.

  • Steve (unregistered)

    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?

  • 516052 (unregistered) in reply to Steve

    You don't? But it's so obvious.

  • Prime Mover (unregistered) in reply to Steve

    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!

  • Ollie Jones (unregistered)

    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).

  • (nodebb) in reply to Mobeer

    Looks like the comment explains why the Resolved codes (f and s) are not included in the code.

    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.

  • Scott (unregistered) in reply to Steve_The_Cynic

    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.

  • Tom (unregistered)

    Before Molly could reply back, someone else on the team approved and merged the pull request.

    This is the real WTF

  • Best Of 2021 (unregistered) in reply to Jaloopa (partially registered)

    It does, it sets "and review='a'" into the text that presumably ends up in the where clause

  • WTFGuy (unregistered)

    @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.

  • (nodebb) in reply to Steve_The_Cynic

    Maybe the name of the function with this code block is checkNotResolved, so it's obvious why it lists all the status except resolved.

  • (nodebb) in reply to Steve_The_Cynic

    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....)

  • The Mole (unregistered)

    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.

  • Jaloopa (partially registered) (unregistered) in reply to Best Of 2021

    No, that branch is if it's not equal to 'a'

  • Randal L. Schwartz (google) in reply to Steve

    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."

  • eric bloedow (unregistered)

    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.

  • Sole Purpose Of Visit (unregistered) in reply to Randal L. Schwartz

    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.

  • Anon (unregistered)

    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.

  • Loren Pechtel (unregistered)

    I think the real WTF is single-character statuses in the database.

  • ooOOooGa (unregistered) in reply to Loren Pechtel

    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.

  • Hal (unregistered) in reply to The Mole

    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.

  • Is That Really A Crosswalk? (unregistered)

    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.

  • (nodebb)

    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!

  • Dennis (unregistered) in reply to UserK

    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.

  • 516052 (unregistered)

    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.

  • MiserableOldGit (unregistered)

    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.

  • Andy (unregistered)

    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.

    1. This piece of logic is dynamically building a search query using a review status filter passed in from front end (retrieved using getFilter_status() call)
    2. If no review status filter is selected, then you retrieve all active records. Here's where the developer comment actually helps. They are effectively saying to ignore all the status given in the code because the actual requirement is to fetch everything but the two Resolved status.
    3. Given #2, better logic would have been to use NOT IN ('f', 's'). Most likely they thought about doing it that way, but then somebody said "NOT IN will cause performance issues". Either that or the column contains nulls. Either way, this is TRWTF. The comment at least helps focus on the real relevant statuses.
    4. Second if statement is handling "All statuses" filter. This "a" pseudostatus means don't filter out any status (including Resolved ones), but in that case, they will put a limit in the select statement to restrict to first x records.
  • Jay (unregistered)

    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?

  • Some Ed (unregistered) in reply to Steve_The_Cynic

    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.

  • van Dartel (unregistered)

    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.

  • Erk (unregistered)

    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

Leave a comment on “Self-Documented”

Log In or post as a guest

Replying to comment #523409:

« Return to Article