• Brian (unregistered)

    First snippet - it may well be that "HR" needs to be singled out. But on Unixes readdir also returns "." (this dir) and ".." (parent dir) - these are also singled out. Which, from the rest of the code seems to be valid ....

  • thosrtanner (unregistered)

    Frist? Arrggggh

  • foxyshadis (unregistered) in reply to Brian

    Normally if you need to exclude a folder you just set up a filter for that folder, instead of instituting a THOU SHALT NOT rule that'll inevitably catch some junior intern someday.

    It'd be hilarious if HR was never supposed to be excluded, but because that's just the way the software worked, they worked around it by using other folders for their important files. Cargo cults are alive and well to this day.

  • thegoryone (unregistered)

    Snippet 1 - Done badly? Yes. But it'll work, as Brian mentioned. There are better ways - pre-populate an array of values to ignore, but it'll still work for intended purpose. Snippet #2 is just someone that's gotten confused as to where to put the Throw. Not a mistake anyone above graduate level should ever make though. However, the last snippet is just tragic .

  • Rubberduck (unregistered)

    Snippet #2 does too do something! It destroys the stack trace! Obviously a feature!

  • scragar (unregistered) in reply to Brian

    That's why you test for the presence of a leading dot on the filename.

    $imagename[0] != '.' && $imagename != 'HR' && ......
    

    Much easier than just excluding everything with 2 or less characters.

  • Gino (unregistered) in reply to Brian

    Yes, I was thinking that too. I was also thinking, if the string comparison is slower than the string length, (a good optimizer might even make this check that the third character isn't null, if it is, then the string is not longer than 2 characters), the only WTF is that the reason isn't documented and instead the maintainer had to go research why that code was the way it was instead of seeing that in a comment.

  • Steve_The_Cynic (nodebb) in reply to Gino

    "a good optimizer might even make this check that the third character isn't null,"

    I sincerely hope not. What is the third character of "A" ? Or did I just whoosh again?

  • JG (unregistered)

    strlen($imagename)>2

    it's clever than you think. It also excludes the . and .. directories

  • Brian Boorman (google) in reply to scragar

    Test for the leading dot. Ah yes - because a directory named .settings (for instance) is obviously not a valid name.

    Addendum 2016-09-27 08:32: apparently a LT slash s GT is stripped by the forum software. </s>

  • job (unregistered)

    strlen($imagename)>2 also has the advantage that it avoids a file or directory named "0" stopping the loop, because obviously the string "0" is FALSE...

  • Shawn H Corey (github) in reply to Gino

    In this case, strlen() is slower than string comparison because one of the strings is immutable. At maximum, strcmp will do 2 char comparisons. The WTF is premature optimization.

  • PWolff (unregistered) in reply to Gino

    "(a good optimizer might even make this check that the third character isn't null, if it is, then the string is not longer than 2 characters)"

    And what if the string has length one, and the memory cell at string position + 2 is filled with some random bits?

  • rohcQaH (unregistered)

    PHP's internal string type stores the length, so there's no need to "look at every character in one string" because strlen() is O(1). Not really relevant in a loop that does disk IO, but it proves the PHP version of Muphry's law: every post or article bashing PHP contains at least one misconception about PHP.

  • Greg (unregistered) in reply to job

    It does not: the while condition is checked before strlen. The while should be written like this: while(($imagename = readdir($dir)) !== false )

  • DiskJunky (unregistered)

    Encountered this gem in a production environment a good while back (in BASIC);

    i$ = i$

  • job (unregistered) in reply to Greg

    Of course, what was I thinking... My brain always starts to shut down when it tries to parse PHP

  • An developer (unregistered)

    There could be a justification for the second snippet, depending on context. By constructing a new exception rather than rethrowing, it removes the stack trace. I've done something similar when dealing with security-related code where exceptions needed to be logged by the client code, but the stack trace might contain passwords for example.

  • dufel (unregistered)

    TBH, when I look at #2, I just see someone planning to add some logging code, then never finishing. It's a little curious, but not that bad in the grand scheme of things.

  • anonymous (unregistered) in reply to An developer

    If the stack trace has a password in it, you're doing it wrong.

  • Ext3h (unregistered)

    #2 actually does something.

    It mangles every class inheriting from Exception into a plain exception. This can actually make sense, if you e.g. have a call stack of the form A -> B -> A, whereby component A is unaware that it indirectly called itself, and a typed exception bubbling through B would be caught unexpectedly. Stripping the exception type at the component boundary can avoid that.

    #3 is just a typo.

    It should have been unset($product_data), which is needed as $product_data would otherwise pollute the parent scope.

  • Carl Witthoft (google) in reply to Brian

    Yeah, but what if my $IMPORTANT_DIRECTORY name is "K" ? what then?

  • Herby (unregistered)

    All this proves is that in any given language, somebody will use it wrong, think they are using it right, understand their task incorrectly, fault the language, make fun of the language, and last but not least, make up a real 'WTF' mess for all of us to see,

    It happens everywhere, and will keep on happening as long as there are programmers.

    Then there are stories of ill guided management. Enough said.

  • Ron Fox (google)

    Yes well in the last snippet there is a pathology where that does something. Consider:

    https://github.com/leedavis81/vent

    and suppose the $product_data array was set to fire events on read?

    Yes it's utterly filthy...but then again, this is TDWTF!

  • isthisunique (unregistered) in reply to foxyshadis

    I would put it inside another root folder that simply isn't indexed.

  • Anon (unregistered) in reply to An developer

    If you need to redact something for the stack trace, or remove the stack trace etc., then doing so in a throw/catch in each low-level method is the wrong place.

  • rigrig (unregistered)

    For added fun, $scratch_products might actually be a Iterator object that does all kinds of hidden stuff. It might even spew out ArrayAccess implementations, doing their own thing when offsetSet('productid') is called.

  • Anon-Anon (unregistered) in reply to Anon

    Not really. It depends on what the rest of the method does. It's not really a WTF without more context. Maybe it's a bad idea. Maybe it's not. We don't know.

    Personally, I've done similar things before when I didn't think the full stack trace was necessary. Usually when interfacing with a third-party library. The basic thing i need is that "this" method is erroring in this location. I don't need 50 lines worth of trace from libraries i didn't write and can't modify.

    Usually I'll modify the message as well, but sometimes it's easier or beneficial (generally when working with sql queries) to percolate the original message back up.

    For example, if I'm using a third-party library that eventually hits an sql database, it's nice to get the -803 error back along with the knowledge that my attempt to call that third party library failed. But I don't need to know that my code called ThirdPartyLib->GetData() which called ThirdPartyLib->BuildQuery() which called ThirdPartyLib->SqlPrepper() which called ThirdPartyLib->SqlPrepper(string, string) which called ThirdPartyLib->SqlPrepperPreparedStatement(string, string, string), etc.

  • SolePurposeOfVisit (unregistered)

    Nothing wrong with PHP, apart from the fact that it wasn't really designed as a language in the first place and has been mutated into something it was never originally intended to be (prior to version 3.0, at least). And nothing wrong with PHP programmers, except that they choose to program in PHP -- or at least get paid to do so, and nobody can argue with that. And nothing wrong with the rational position that I, amongst many others, hold: statistically speaking, PHP programmers are far more likely to be dangerous idiots than, say, Java programmers. (I am not keen on Java programmers either, but I am realistic.) And PHP programs are, statistically speaking, far more likely to be abject crap than, say, Perl programs (I have written some pretty dire Perl programs, so I'll hold my hand up.) And for anybody who repeats the meme "you can program badly in any programming language," well, yes. Equally, you can write a program that performs precisely to the customer's requirements in Brainf*ck. But who would want to?

  • mkl (unregistered)

    The try-catch snippet is probably some kind of refactoring leftover. Like, you're enclosing a DB transaction within a try block and catch all exceptions just to roll the transaction back on any error, which is then re-thrown. Then at some point you get rid of the transaction (or move it elsewhere) and you're left with an idle try-catch-throw block... Sloppy? Yes. But really, WTF?

  • Norman Diamond (unregistered)

    Then there are stories of ill guided management.

    Sure, but this language even manages to misspell the third initial of that common abbreviation.

  • Norman Diamond (unregistered)

    Encountered this gem in a production environment a good while back (in BASIC);

    i$ = i$

    Was that left over from a debugging breakpoint where the developer just wanted to glance at the value of i$?

    At least I wouldn't have to resort to any similar stupidities when debugging C++ in Visual Studio. Or if I did, I would always remember to delete the line after finishing debugging. Or if I forgot, at least I'd catch it myself when reading my own code again a while later, and meanwhile the optimizer would optimize it out[*].

    [* because release versions get optimized. We're not allowed to distribute debug versions, just like laws allow car manufacturers to install seat belts during testing but require seat belts to be removed when selling cars to the public. Or something like that.]

  • Kyle (unregistered)

    Php is a c-style language. All the sillyness of noobs. Write a proper unit test. Kthxbai.

  • Nate Scherer (google) in reply to SolePurposeOfVisit

    "Nothing wrong with PHP..."

    You should check this out, it's a good read.

    https://eev.ee/blog/2012/04/09/php-a-fractal-of-bad-design/

  • Falco20019 (unregistered)

    Most interesting is, that in #3, although there is a foreach loop on $product_data, not the element $scratch_products is used but $product_data to access the field 'productid'. So the unset seems not to be the only problem here...

  • Steve_The_Cynic (nodebb) in reply to mkl

    Sort of, except that the try/catch isn't idle. It catches all exceptions derived from Exception (which is presumably all of them, I'd hope, given that this isn't C++ where you can throw anything, even stupid things like int - throw 5; is valid C++...), junks them, and throws one itself. This discards the stacktrace of the exception location, and costs CPU and memory resources. I'd presume that you can just re-throw the current exception using a simple throw; statement, like in C++ and others.

  • Charles Wood (google) in reply to scragar

    ...unless you want to catch config files that start with a dot...

  • Appalled (unregistered) in reply to Falco20019

    "Most interesting is, that in #3, although there is a foreach loop on $product_data, not the element $scratch_products is used but $product_data to access the field 'productid'. "

    It would help if you knew the language before making assumptions, speaking about it, and proving your ignorance. You're not the only one. Well over half of the preceding comments are clueless assumptions about the language as well.

    The foreach loop is on $scratch_products, END OF STORY.

    $scratch_products is an ARRAY (no chance it is anything else) END OF STORY.

    $scratch_products has at least field, called 'productid'. END OF STORY.

    $product_data is a local variable (within the scope of the loop ONLY) used as a pointer into the current row within $scratch_products

    Substitute $foo for $product_data and the results are the same.

    The ONLY WTF is destroying the results, a new array, $pids, containing ONLY productid in it, as soon as it was so carefully built.

    I will guess WHY he did this. First envision $scratch_products is a query result (in PHP they're still just simple arrays) with 112 columns. productid is the 87th column. Something was going wrong with them and he wanted to see them only so he produced a mini-array of just the PIDs to see what the heck was going on (dunno where the echos went). He left the mini-array in place, destroying it rather than removing it. Yes silly indeed. Perhaps he wished to be able to turn it on again the next time something crashed. There are easier ways though, // comes to mind. At any rate, there is no WTF other than a few wasted CPU cycles. The program flow is totally unaffected.

    PHP is a SIMPLE language for those who know how to and with a desire to keep things simple. Fields, arrays, manipulators, conditional flow, and IO. That's pretty much it, nothing more. WYSIWYG. As such it is also a beautiful language for programmers who enjoy the art of KISS.

    The one thing it lacks is a Debug/IDE platform. There is no way to Debug Trace run time flow. If there's a problem, it's back to the old ways, drop in a display, Echo "I'm here and " . $foo . " just got changed again dammit "; Figure out the problem and yank it or comment the display. If you're doing displays to often you're not KISS'ing up to the language. Find another job, or learn how to KISS, you're choice.

  • Florent Fievez (google) in reply to Rubberduck

    It destroy also the type of the exception

  • Ashley Sheridan (unregistered) in reply to scragar

    Don't ever test strings using array syntax in PHP. That syntax treats the strings as arrays of bytes, not arrays of characters. It's a subtle but important difference. If you absolutely have to take the first character, then do it properly.

  • Medinoc (nodebb) in reply to Brian Boorman

    @scragar and Brian Boorman:

    I don't remember where I've read it, but apparently the whole UNIX "files starting with dot are hidden files" "feature" was actually created accidentally with such a test. The code for dir skipped files starting with a leading dot in order to exclude "." and "..", and someone noticed it worked on any file with a leading dot...

    Addendum 2016-09-30 06:19: And by dir, I mean ls.

  • mb (unregistered) in reply to Appalled

    Php does have a really good IDE https://www.jetbrains.com/phpstorm/ with debugging tools a and a couple of different debuggers (https://xdebug.org/ mainly).

  • I dunno LOL ¯\(°_o)/¯ (unregistered)

    I find Snippet #3 to be... unsettling.

  • jgh (unregistered) in reply to Brian Boorman

    Conventionally, an object name beginning '.' is not listed anyway, so it could well be a valid design choice to skip all object names beginning with '.'. I've just done some such code earlier today.

  • Tomasz Struczyński (unregistered) in reply to Nate Scherer

    Just to be clear - there's also a 'debunking' of this article, probably more than one. Like here: https://news.ycombinator.com/item?id=4177516

    And yes, I'm PHP programmer (mostly), so my opinions are biased. But I really think, that with current frameworks (like, Symfony), good practices (whole PSR initiative), unit testing (PHPUnit) etc, not mentioning (well, mentioning actually) Travis and Scrutinizer support on GitHub, most of the new PHP code is of quite good quality.

    BTW - I'm not arguing, that it's hard to write a bad code in PHP. No, it's easy. But I'd argue, that writing a bad code is also easy in most of the languages, including C.

    Also, about the 2nd example. I'm actually doing something similar in my library. I implement kind of strategy pattern, where enclosed code can (should not, but can) throw arbitrary exceptions. I want to catch them all in my template function and convert to one type ProcessException. Something like that:

    try {
        // Do something by external classes, which may behave bad
    } catch (\Exception $e) {
        throw new ProcessException($e->getMessage(), $e->getCode(), $e);
    }
    

    Mind it, I'm maintaining reference to the previous exception, but 'unify' all into one type, so I can write in my class signature, which exceptions are thrown from it. I think that repackaging exceptions is not so bad and unique to PHP (if they were, Exception constructor in Java would not have the second argument).

Leave a comment on “Properly Handled PHP”

Log In or post as a guest

Replying to comment #:

« Return to Article