• (nodebb)

    but this has been in the code base for longer than most of our careers.

    Most, sure, but not all. PHP itself, much less the code cited, didn't exist when my career began.

  • Quite (unregistered) 
    $FRIST->mysqlName = $row["FRIST"];
	if (!$FRIST->mysqlName) {
		$errMsg = "No comment named \"" . $FRIST->mysqlName ."\"";
		return(false);
  • MaxiTB (unregistered)

    "So the fact that this sets a pass-by-reference $errMsg variable and returns false on an error isn't the WTF in the code."

    This is actually in C# the well-known Try-pattern, which is also supported directly by the framework, specifically with handling non-nullable reference types. It's actually a pretty smart way to handle optional results, but has it's drawbacks with lambdas obviously.

  • (nodebb) in reply to Quite

    Shame you were actually snecod.

  • Robin (unregistered)

    The weird irony of this is that the only way the error would say anything else is if you had a table named false,

    It's mercifully been a few years since I've used PHP on a regular basis, so I could be wrong - but I don't see how this is true. Surely in this case the value would be the string "false", and even in PHP surely that's a "truthy" value? (Although actually I seem to recall the string "0" is falsy so it wouldn't be a shock if there was a second such weird exception. But having very quickly googled this, it seems that thankfully "false" coerces to true, as it would in a sane language.)

  • (nodebb)

    " and in this case may predate the current century" - ha. where I work there's plenty of code that predates the current century,

  • RLB (unregistered) in reply to Robin

    The weird irony of this is that the only way the error would say anything else is if you had a table named false,

    It's mercifully been a few years since I've used PHP on a regular basis, so I could be wrong - but I don't see how this is true. Surely in this case the value would be the string "false", and even in PHP surely that's a "truthy" value?

    Correct. It's even explicitly used as an example in the docs.

  • Barry Margolin (github)

    It's easy to understand why no one ever noticed the problem in the code -- it only manifests when the function is called with a nonexistent table name. This is just CYA validation for something that should never happen in the first place.

    Or maybe these broken error messages have happened, but fixing them never rose to a high enough priority, as long as the error is actually caught.

  • (nodebb)

    Error handling code is notorious for being the worst tested. Typically when the chances of such an error happening are rare so the code never actually ends up running for the developers. But it will run for someone, someday.

  • tbo (unregistered)

    Correct. It's really testing to see whether it's empty.

  • MaxiTB (unregistered) in reply to The_MAZZTer

    I agree that error code handling is the worst way to handle errors. But there's a difference between a result and an error code. People often forget that exceptions are not free, they are actually pretty expensive even when not triggered, especially in some nested contexts (good example is unwinding a state machine or memory management with nested exceptions) and worst of all, their performance is not predicable because they are not implemented the same way over multiple platforms (simply because different operating systems by themselves already have different implementations, it gets even worse considering different hardware architectures).

    So yeah, if you have for example a parsing method, you should start always with a method returning a boolean success code and optionally add a simple method that calls the the first method and throws and exception on false. This way, you have both options available and the developer can chose the appropriate method for his implementation. And yeah, obviously the other way around is pointless, the whole idea is to have a method that doesn't create an exception scope - so one doing basically try { return true; } { return false; } is a design anti-pattern.

  • (nodebb) in reply to Robin

    But having very quickly googled this, it seems that thankfully "false" coerces to true, as it would in a sane language.)

    Seems to me that a particularly sane language might avoid implicitly assigning boolean values to strings. :-)

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

    " and in this case may predate the current century" - ha. where I work there's plenty of code that predates the current century,

    Many of my clients had to prepare for Y2K, if that's any indication.

  • Sole Purpose Of Visit (unregistered) in reply to Bim Zively

    Sane?

    You're new to PHP, aren't you?

  • Robin (unregistered) in reply to Bim Zively

    Ha, I realised some would say that - but then I reflected even the most strongly typed languages usually have some way to explicitly convert other values to Boolean, even if you can't put non-booleans directly as a condition. So my point is still the same, regardless of your views on the evils or otherwise of implicit conversion. (Personally I think it is pretty evil in most cases, but don't mind it inside an if)

  • Jeremy (unregistered)
    Comment held for moderation.

Leave a comment on “Nothing Wagered, Nothing Errored”

Log In or post as a guest

Replying to comment #:

Log Out

« Return to Article