• ray10k (unregistered)

    Makes me wonder if any customers had trouble with random values changing from time to time.

  • LCrawford (unregistered)

    How did the customers reach back on shore? Did any customers opt out of using the new flag? It seems that their speed Westward would increase with each save, since their longitude would be doubled.

  • ray10k (unregistered) in reply to LCrawford

    The least unreasonable explanation I can think of is that the "original" locations came from some geolocation service, which would announce updates to its database, which in turn could prompt this system to re-fetch the actual locations. Possibly.

  • Little Bobby Tables (unregistered)

    Would this mean that customers who were located in, say, East Anglia, would progresively find themselves in e.g. Netherlands, Germany, Poland, Belarus and onward to Russia?

  • my name is missing (unregistered)

    My comment will include $number characters.

  • RLB (unregistered) in reply to Little Bobby Tables

    Would this mean that customers who were located in, say, East Anglia, would progresively find themselves in e.g. Netherlands, Germany, Poland, Belarus and onward to Russia?

    No, because those would be stored as , say, 1.2974, not as +1.2974. No plus sign, no addition.

  • Ross Presser (google)

    Sounds like a feature added for job security by the database programmer after he transitioned to contract status. "Hey Joe, can you fix the database again?" "Sure, just add 1 hour to my billable time for the week."

  • RLB (unregistered)

    OK, so this is all very well and bad, but... aren't we going to comment on that last line? "{$var1}" . $var2 ?

    Using PHP double-quote variable substitution: fine.

    Being scared of variable substitution and concatenating all your strings: you're showing the tigers your weakness, but all right then.

    Doing both in the same expression? No, no, no, no. That's borderline excusable if the concatenated term is a complicated expression or a long-winded function call, but with a simple variable, as here, it's a sure sign of a cookbook programmer.

    Also, I would really, really want to know what $number actually stood for -- and what shallow-fried dinkum named it that.

  • Roman (unregistered)

    Just to make it clear what $number stood for - not sure if the editor omitted it in the article or I forgot to send it in the snippet:

    $number = substr($value, 1);

    So if the longitude $value was -1.5, the code would subtract it from itself, making it -3.. again and again and again.

  • (nodebb)

    It's the attack of the arbitrary global value! In PHP! Run away! Run away!

  • someone (unregistered)

    Oof. That must've been fun to track down.

  • Mark Charsley (unregistered)

    This article title reminds me of the fun a family friend had when computers first reached post sorting offices. There is actually a town called Westward Ho! - https://www.google.co.uk/maps/place/Westward+Ho!/@51.0269252,-4.2510466,12.84z/data=!4m5!3m4!1s0x486c161fa158ab15:0x103194248c8586b9!8m2!3d51.038862!4d-4.238105

    I'll leave it to you to guess which character in its name caused problems with the post code software...

  • Dude (unregistered)

    Maybe it's just me (I don't know much about what looks like PHP), but it looks like it's account for numeric inputs that may contain simple formulas. This would allow the user to, for example, change the value of a given input to something like +5), which would add 5 to whatever value was originally there. Since the points in this case are effectively close to the Prime Meridian, there's very little difference in seeing points with longitude incremented by a fixed value and points with longitude effectively doubling.

    Without knowing where $number comes from it's not easy to say for sure, but I could see some programmer somewhere thinking it would be a good thing to have generically at the database access layer.

  • (nodebb)

    Change the geo coordinates to prefix with e or w. Stupid coding bypassed, simple.

  • Kashim (unregistered)

    TRWTF: "He added a flag to the update() method so that customers could disable the behavior upon request." So, TRWTF is that the solution for the problem was "put the onus on the customer to check a box in order to make the system work the way it should've the whole time."

    "Hey, we sell a service that puts you on the map, but if you want to be in the correct place on the map, you have to check this box." Has any customer willingly not checked the box? "No, thanks, I want it to look like we own our own island just off shore."

  • I'm not a robot (unregistered) in reply to Kashim

    To be fair, that could be a braino for "callers could disable the behaviour". Which is still a bit hacky, but not unreasonable as a minimal fix with little risk of breaking something else.

  • (nodebb)

    It's just a feature for adjusting location after Prius improves their Gas mileage. https://xkcd.com/687/

  • (nodebb) in reply to histocrat

    I wonder if Scotland will Remain

  • (nodebb) in reply to LCrawford

    It seems that their speed Westward would increase with each save, since their longitude would be doubled.

    No, their speed is governed by $number. Whatever that is.

  • ooOOooGa (unregistered)

    OK. This is probably the wrong group of people to ask this of, but here we go anyway...

    Is it normal or standard practice to hack fixes like this and then still call it a fix? Sure the original creator of this mess left the team a while ago. But you still have the source code. Can't people figure out where this code is being called from? This is obviously a terrible side effect for a standard 'update' function. If there is a place in the code that is using this behavior, that place should have its own 'update' function variant.

    Sure, this is going to take time. And testing resources. But to me that is just the standard cost of having a good code base.

    Am I the one that is crazy?

  • (nodebb)

    Would it be any worse if it was done in SQL instead?

    INSERT (PlayerId, AttributeName, AttributeValue) VALUES (@PlayerId, n.AttributeName, CASE n.Action WHEN 1 THEN n.AttributeValue WHEN 3 THEN REPLACE(RTRIM(REPLACE(REPLACE(RTRIM(REPLACE(cast(CAST( CASE --Standard number on number action WHEN LEFT(n.AttributeValue,1) = '+' and ISNUMERIC(substring(n.AttributeValue,2,32)) = 1 THEN 0 + (cast(substring(n.AttributeValue,2,32) as numeric(32,8)) * n.Qty) WHEN LEFT(n.AttributeValue,1) = '-' and ISNUMERIC(substring(n.AttributeValue,2,32)) = 1 THEN 0 - (cast(substring(n.AttributeValue,2,32) as numeric(32,8)) * n.Qty) WHEN LEFT(n.AttributeValue,1) = '*' and ISNUMERIC(substring(n.AttributeValue,2,32)) = 1 THEN 0 * (cast(substring(n.AttributeValue,2,32) as numeric(32,8)) * n.Qty) WHEN LEFT(n.AttributeValue,1) = '/' and ISNUMERIC(substring(n.AttributeValue,2,32)) = 1 and cast(substring(n.AttributeValue,2,32) as numeric(32,8)) != 0 THEN 0 / (cast(substring(n.AttributeValue,2,32) as numeric(32,8)) * n.Qty) END as NVARCHAR(4000))as DECIMAL(18,6)),'0',' ')),' ','0'),'.',' ')),' ','.') END )

    Addendum 2018-09-19 20:11: NFC what happened with the paste. Apparently bad.

  • (nodebb)

    So where was $number coming from? That would be buried in the <snip> no doubt. If it was a "global $number;" statement then that should have been included in the code for even more wtf-value.

    Judging from the string literal, it looks like the database is MySQL (what with the backticks for quoting identifiers). For the code to work $number itself would need to start with a "-" or "+" or the SQL wouldn't parse. The SQL snippet would expand to something like

    `longitide` = `longitude` -4.833 +0.003

    For an arbitrary $number of "+0.003".

    My suspicions: The code was originally "$field = $field $value[0]", which turned out to be completely wrong because the second mention of $field shouldn't be there. The developer didn't figure this out and instead thrashed around until cooking up this $number thingy in an attempt to correct it (somewhere in the elided code). Hence the random mix of variable interpolation, delimited variable interpolation, and variable concatenation going on in the string. The fix "worked", so that's what went into production. But over time, rounding errors were accumulating, causing the drift.

  • Paul (unregistered) in reply to Kashim

    You seem awfully sure that this is the ONLY thing it does? Are you the one who implemented it?

  • Albert (unregistered) in reply to Paul

    "Each time a customer logged into their account via the website and changed their data: if any of their data began with either a plus or minus sign, those values would have some mysterious value (contained in the variable $number) either added to or subtracted from them. "

    You are so right. Say one field is the customer balance, and it turns negative one day. This would then make that silently even more negative. Now we are talking money ? Maybe there is some customer account that slowly grows in increments of $NUMBER ?

  • FormalWare (unregistered)

    The feature was clearly for data obfuscation. And it clearly worked!

  • Eddie (unregistered) in reply to Ross Presser

    That was exactly my thought. We don't have to be curious or investigate to determine why an arbitrary value is being added to the longitude every time the customer logs in. There is obviously no reason to do this. (Even if we grant that there was some other error they were trying to correct for, this is clearly the wrong place to do it. What, are they trying to compensate for continental drift??)

    This was a timebomb left by the now-long-gone developer. There's no other reason to systematically alter the location of the customer.

  • Thomas (unregistered) in reply to ooOOooGa

    The issue is one of the difficulty of proving an absence except by exhaustion. Unless you can, for example, confirm that you have found all the places the code is called, and thus say "it shouldn't occur for any possible situations", how can you be sure that you haven't missed something?

    The issue in this case is that the code is called during the update process, so you know the caller (all the time), but you can't safely say "it should never be called for any database update", without checking every single potential update in the system, for every single possible data change, which is impossible, or at least not worth the effort.

    What's clever about Roman's fix is that it is effectively a deprecation process. As users slowly "opt out" (or are opted-out by support) from this behaviour, you will either get a bug report from one of them that exposes why this code exists (at which point you can investigate a proper fix), or you will reach the point where "COUNT(enabledUsers)==0" and at that point removing the code completely becomes a no-op.

Leave a comment on “Westward Ho!”

Log In or post as a guest

Replying to comment #:

Log Out

« Return to Article