Obfuscation, yo



  • I mentioned this project I used to work on. I just remembered a great example of why I hated it.

    It's PHP (yes yes TRWTF) and it uses output buffers to capture content before going to the browser so that it can splice content into the page before it goes back to the browser. Originally this was used to splice the session id into links but has long since been bastardised into many other things.

    One of the exciting things is that at times, all output buffers have to be closed, thus:

    while (@ob_end_clean());
    

    The previous code was:

    while (ob_get_level() > 0)
        ob_end_clean();
    

    I won't get into the holy debate over whether braces should be used for a single line branch (prevailing code style was 'no', now it's been taken religiously because OMG TOO MUCH CODE)

    Now, I could get behind removing the comparison and letting the natural return of ob_get_level deal with it - it's an int in all cases, and 0 if no output buffers currently active.

    But to shorten it to that first line... icky.

    There's also cases of reducing a complex select/case to a largeish if/elseif/elseif...else branch because that took less code (even though it was slower)



  • I love you, IDisposable.


    Filed under: probably unrelated.



  • Oh, no, this isn't a case of releasing disposable resources, this is a case of dumping anything caught by the output buffer that you know you don't want.


  • Discourse touched me in a no-no place

    So… why isn't that a simple cleanup operation (à la IDisposeable)?



  • Because this is implicitly wrapped around the single most important thing in your script: its entire output.

    This application will always create at least one output buffer to capture content for manipulation before it going to the user (to avoid many many many 'if (condition) then this link is in this form otherwise it's in that form' issues). Very often there will be two or more nested buffers - all around STDOUT.

    Sometimes you're explicitly munging one or two of those. Sometimes you explicitly want to munge all of them. PHP doesn't offer a 'nuke all buffers right now' option so you have to step through all of them and close all of them (depending on whether you want to get the content or not!) and this loops through and closes all of them.

    But it's not an arduous two line deal. It's just one line too long, which also has the side benefit of making the code less readable.

    Did I mention, PHP?


  • ♿ (Parody)

    @Arantor said:

    But to shorten it to that first line... icky.

    Yeah, I hate that. Better would have been:
    @Arantor said:

    while (@ob_end_clean()){}

    @Arantor said:

    I won't get into the holy debate over whether braces should be used for a single line branch

    I will. No braces is wrong.



  • I gather performance isn't important in your company? The @ in front of functions can really take its toll, especially on repeated incarnations.

    It basically changes the error handler before the function call and then changes it back to the previous handler after.



  • @boomzilla said:

    I will. No braces is wrong.

    {}{}{}           {}          {}{}{}    {}{}{}      {}{}{}   {}{}{}
      {}           {}  {}      {}          {}   {}     {}       {}
      {}          {}    {}    {}    {}{}   {}{}{}      {}{}{}   {}{}{}
      {}         {}{}{}{}{}    {}    {}    {}    {}    {}       {}
    {}{}{}      {}        {}     {}{}      {}     {}   {}{}{}   {}{}{}


  • This is an "open source" project, not a company project. And performance matters.

    The guy who works on this measures performance by checking the time required to load a page and provided that it all stays under a certain time, it's fine. I should add, the guy who works on this who wrote his own CSS preparser will frequently and regularly rewrite the CSS in the application so that it saves bytes.

    Seriously: he measures the CSS result after gzipping and attempts to improve upon it at the byte count level. While awesome, this is something of a problem if, say, you're ever building an open source application where users might possibly want to customise something.


  • ♿ (Parody)

    @Arantor said:

    The guy who works on this measures performance by checking the time required to load a page and provided that it all stays under a certain time, it's fine.

    Kind of sad that there wasn't another WTF in performance measurement. Especially from a guy who turns around and does this:

    @Arantor said:

    I should add, the guy who works on this who wrote his own CSS preparser will frequently and regularly rewrite the CSS in the application so that it saves bytes.



  • Oh, there are many performance WTFs. There's cases where he'll load a few hundred KB of PHP on every page load whether it will be used or not, simply because it's cheaper according to his benchmarks to just load it rather than to conditionally load it.


  • ♿ (Parody)

    Well...was it really cheaper? Was his benchmark flawed? If so, why? I can imagine that bandwidth would obviously be a factor, but I guess it depends on the nature of the condition being checked.



  • That's just it, there's no way to actually meaningfully tell. There's simply too many factors to contend with in terms of loading a few hundred KB of code every page load that you probably don't need.

    On a typical environment in PHP for this application (which is the shared hosting space), you probably won't get the benefit of APC or Zend OpCache (since most shared hosts don't run PHP 5.5 yet) while I know he's running with one or the other - which will cache the compiled code, which invalidates a number of his benchmarks off the bat.


  • ♿ (Parody)

    Oh, so his tests weren't realistic then? Yes, WTF. I've never PHP'd, so I'm not familiar with the sorts of caveats you just mentioned.



  • @JazzyJosh said:


    {}{}{} {} {}{}{} {}{}{} {}{}{} {}{}{}
    {} {} {} {} {} {} {} {}
    {} {} {} {} {}{} {}{}{} {}{}{} {}{}{}
    {} {}{}{}{}{} {} {} {} {} {} {}
    {}{}{} {} {} {}{} {} {} {}{}{} {}{}{}

        {}{}{}{}{}{}{}{}{}{}{}{}{}{}{}{}{}{}{}{}{}{}{}{}{}{}{}{}
        {}  {}{}{}  {}      {}{}{}{}      {}{}    {}{}{}    {}{}
        {}    {}    {}  {}{}{}{}{}{}{}  {}{}  {}{}  {}  {}{}  {}
        {}          {}      {}{}{}{}{}  {}{}  {}{}  {}  {}{}  {}
        {}  {}  {}  {}  {}{}{}{}{}{}{}  {}{}  {}{}  {}  {}{}  {}
        {}  {}{}{}  {}      {}{}{}{}{}  {}{}{}    {}{}{}    {}{}
        {}{}{}{}{}{}{}{}{}{}{}{}{}{}{}{}{}{}{}{}{}{}{}{}{}{}{}{}
    


  • That's precisely my point. There are many holes in his benchmarks. But it's my fault for not trying nearly hard enough to point out his failures in testing.



  • How does this thing even work? Does ob_end_clean() return 0 when it has no buffers to close? Does it throw an error? Both?

    If ob_end_clean() returns a value that breaks you out the loop, then ob_get_level() call seems redundant. But why supress errors, then?



  • ob_end_clean returns true if it was able to close a buffer and false on failure, in this case, failing by trying to close a buffer that doesn't exist and wilfully suppresses the error in so doing.

    ob_get_level may seem redundant by consequence but I was always taught to check before doing something that might throw an error, and in any case the original code would be more readable than this.



  • @Arantor said:

    and in any case the original code would be more readable than this.

    Eh, I don't know. If it wasn't for PHP's utter retardedness with error suppressing (namely, that it negatively impacts performance), I'd probably go with the corrected version.

    while(do_something()) is a fairly common idiom for a boolean-returning function.



  • while(do_something()) with nothing in the loop? As in the entirety of the loop is do_something() until do_something() returns false?

    You see that a lot, do you?



  • @Arantor said:

    You see that a lot, do you?

    Not an awful lot, but it pops out here and there. I've seen it mostly when doing microcontroller programming as a busy-waiting loop (while(!check_pin()); or something like that).

    A "do something until you can't anymore" case is a bit less obvious, maybe, but still far from obfuscated.

    Also, what would you do if you didn't have the ob_get_level() function, instead only having ob_end_clean() return 0 on failure, positive value on success (which is true for a lot of standard C functions, though usually the other way)?


  • ♿ (Parody)

    @Arantor said:

    You see that a lot, do you?

    A fair amount. I mean not all the time, but what's wrong with it?


  • Trolleybus Mechanic

    @Arantor said:

    while(do_something()) with nothing in the loop? As in the entirety of the loop is do_something() until do_something() returns false?

    For maximum readability, I'd probably go with something like:

    bool keepGoing = true;
    
    while(keepGoing)
    {
          keepGoing = doSomething();
    }
    

    However, the keepGoing variable does seem kind of redundant in this case. This actually looks cleaner to me:

    while(doSomething())
         ;
    

    Filed under: braces included for those who can't do without, K&R



  • That's kind of the point... in a microcontroller environment, you're dealing with limited capacity and keeping things tight is important.

    My main beef with it is that it's not particularly readable and to understand what it's doing you have to understand several side details of how PHP works to understand what's going on. Going from two lines that are quite descriptive to one line that isn't. And doing so because the two lines offends you because it's always better to have fewer lines.

    I was always taught that code should be expressive in itself; while 'well written code shouldn't need documentation' is of course a lie, well written code should be at least somewhat descriptive of what it's doing. There's no comment even around this code explaining what's going on. It's just a single line buried inside a lengthy routine with no explanation as to what it does or how it does it.

    PHP may be many things but I find it generally quite a readable language, and munging something to one line as opposed to a slightly more readable two lines... ugh.



  • @Arantor said:

    That's kind of the point... in a microcontroller environment, you're dealing with limited capacity and keeping things tight is important.

    Actually, in arguing this point the exact opposite is true - in a microcontroller environment, you're not concerned with things like "yielding unused time", so you can busy-wait as you please.

    @Arantor said:

    My main beef with it is that it's not particularly readable and to understand what it's doing you have to understand several side details of how PHP works to understand what's going on. Going from two lines that are quite descriptive to one line that isn't. And doing so because the two lines offends you because it's always better to have fewer lines.

    But it's readable. Not natural language-level readable, but fairly obvious to anyone with Programming 101 experience. I probably wouldn't go as far as to change it, but it does make for one less function call. I am, however, pretty sure that my first instinct would be to write the "obfuscated" version.

    As I said - aside from error suppressing, there's nothing very unreadable about this, as long as you know ob_end_clean() returns false on failure. And throws an error to boot, which might or might not be a WTF in the function itself, but not so much in the usage.


  • BINNED

    @Arantor said:

    while(do_something()) with nothing in the loop? As in the entirety of the loop is do_something() until do_something() returns false?

    You see that a lot, do you?

    Bah, that's nothing. This is, from memory, an example of finding the last element in a linked list. From official learning materials. From a college:

    for(;item.next != NULL; item = item.next);
    


  • Surely in a microcontroller environment, while you're not worrying about yielding unused time, you would be worried about keeping it lean?

    As far as readability goes... I'm going to have to be all awkward and disagree with you. I can make sense of it, sure, and I did as soon as I saw it, but I'd been doing PHP for a number of years before I saw that and understood the ramifications of it. As much as PHP is the natural RWTF in discussions, most people using it are not used to languages like C where such things are naturally more commonplace.

    Amongst typical PHP programmers - most of whom are some level of WTF themselves, me most certainly included - this is obfuscated. If it returned false just on its own, that'd be fine, but it's not.

    What's the prevailing opinion on a try/catch block with nothing in the catch?



  • @Arantor said:

    What's the prevailing opinion on a try/catch block with nothing in the catch?

    Reply as New Topic


  • Trolleybus Mechanic

    @Arantor said:

    What's the prevailing opinion on a try/catch block with nothing in the catch?

    I know I've done it at least once, but would recommend against it as a general pattern.


    Filed under: master the rules before you break 'em, I did put a comment in the catch block to explain purpose



  • @GOG said:

    Filed under: master the rules before you break 'em, I did put a comment in the catch block to explain purpose

    try {
      something.makeItHappen();
    } catch (Exception e) {
      //it's ok, I've mastered the rules
    } 
    


  • So what's the difference here? You're doing a while loop on something specifically until it fails, with an exception-that-isn't-actually-an-exception-because-of-PHP-legacy as a side benefit.

    There is a specific function available for performing this check prior to running the function which would remove the need for error checking, and IMNSHO more descriptive than the new code, given the people who will play with it.

    There are so many more WTFs than this, this was just meant to be a representative sample of how not to PHP from someone that knows PHP.


  • Trolleybus Mechanic

    try
    {
    doSomething();
    }
    catch
    {
    // Nothing to see here, move along
    }



  • @Arantor said:

    You're doing a while loop on something specifically until it fails, with an exception-that-isn't-actually-an-exception-because-of-PHP-legacy as a side benefit.

    At that point, the exception handler is only dealing with a flawed design.


    Filed under: On Error Resume Next



  • @Onyx said:

    Bah, that's nothing. This is, from memory, an example of finding the last element in a linked list. From official learning materials. From a college:

    for(;item.next != NULL; item = item.next);
    ```</blockquote>
    
    I see your `for` loop abuse and raise you a comma operator abuse:
    
    ```C++
    while (MPI_Iprobe( MPI_ANY_SOURCE, MPI_ANY_TAG, MPI_COMM_WORLD, &recvFlag, &status), recvFlag)
    {
    	processMessages();
    }
    

    Written by yours truly, by the way. The point was to check the message queue (which sets the recvFlag), and if recvFlag is set, process the message that's in the queue.

    @Arantor said:

    Surely in a microcontroller environment, while you're not worrying about yielding unused time, you would be worried about keeping it lean?

    What do you mean about "lean"? In general, yes, you need to write small and performant code (much more than in standard programming), but in this particular case, that's a moot point.

    @Arantor said:

    Amongst typical PHP programmers - most of whom are some level of WTF themselves, me most certainly included - this is obfuscated.

    For typical PHP programmers, everything is obfuscated. They just live in their own little world very much detached from ours.

    @Arantor said:

    If it returned false just on its own, that'd be fine, but it's not.

    As I said - that makes for a WTF-y function, but the usage seems fine. I kind of assume PHP would just crap itself if it threw a non-surpressed error - so what else is the point of having the function return false?

    @Arantor said:

    What's the prevailing opinion on a try/catch block with nothing in the catch?

    Kind of a code smell, but I've done it once (I think it was some kind of WinForms weird case - I needed to access a TextBox from outside an UI thread, and when the app was closing, sometimes Invoke would fail due to the TextBox being already disposed and throw an exception - but since the app was closing, it couldn't matter less).


  • Trolleybus Mechanic

    The context here is using a library function that returns one - and only one - item from a collection or throws an exception if it finds more than one satisfying the criteria or does not find one at all.

    I only need to do something with the item if I get a valid return and it doesn't matter why I didn't if I don't. I could have done this otherwise, but it would have to be in a much more roundabout fashion.

    Like I said, I wouldn't recommend it as a general way of doing it and would only consider doing so in exceptional circumstances.


    Filed under: I never said it was PHP



  • Reminds me of using a GetKey() method without using a ContainsKey()-type check.



  • Oh, no, that's just it.

    PHP won't dump itself with a non-suppressed error. It will, configuration depending, output it to STDOUT, and in this application's case, it will still be caught by the app-level error handler (in any event) since it's non fatal and be logged to the error log, which means it has to be suppressed otherwise you'd be logging an error every page load.

    There are a worrying number of crappy hosts that don't bother to hide error output, meaning that any app with any PHP non-fatal errors will output them to STDOUT including the file path from whence the error originated which will be a full file path on the server.



  • @Arantor said:

    since it's non fatal and be logged to the error log, which means it has to be suppressed otherwise you'd be logging an error every page load.

    cough Response.Redirect cough ThreadAbortException

    @Arantor said:

    There are a worrying number of crappy hosts that don't bother to hide error output, meaning that any app with any PHP non-fatal errors will output them to STDOUT including the file path from whence the error originated which will be a full file path on the server.

    That's one thing that I like in Python, changing how that works with cgitb is so easy.



  • Response.Redirect isn't appropriate in this case because this is happening every page load in the shutdown phase where all content is aggregated and output to the user!

    As for changing error handling in general, PHP is only mildly insane about this. All errors will be output unless they're not, assuming error reporting is even on and the standard error handler may or may not be overridden.

    Oh wait... no, that's not mildly sane.



  • @Arantor said:

    Response.Redirect isn't appropriate in this case because this is happening every page load in the shutdown phase where all content is aggregated and output to the user!

    Yeah, it's more analagous to an exception in every Page_Load() event, but luckily, the .NET code that I come across is a bit more sane than that.

    @Arantor said:

    As for changing error handling in general, PHP is only mildly insane about this. All errors will be output unless they're not, assuming error reporting is even on and the standard error handler may or may not be overridden.

    Oh wait... no, that's not mildly sane.

    Now you know why PHP is considered TRWTF.



  • Oh, I've long known PHP is TRWTF but I have snowblindness to the very worst of it and it's only when rubber duck debugging that I remember why it is.



  • @Arantor said:

    Oh, I've long known PHP is TRWTF but I have snowblindness to the very worst of it and it's only when rubber duck debugging that I remember why it is.

    I have a rubber duck sitting on my book stack next to me, on top of The Humane Interface. Maybe you need one on top of a book called The Humane Language.



  • @Arantor said:

    PHP won't dump itself with a non-suppressed error. It will, configuration depending, output it to STDOUT, and in this application's case, it will still be caught by the app-level error handler (in any event) since it's non fatal and be logged to the error log, which means it has to be suppressed otherwise you'd be logging an error every page load.

    Still, it's pretty much as bad as dumping itself in this case. I guess it kinda depends on the intended usage of the function (should it crap out when it gets a non-existent buffer, or should it treat it as a valid situation?).



  • I am too dumb to understand the point you're trying to make here.



  • I have this book:

    But that wouldn't help you, because you need one to help you in dealing with PHP, which I'm pretty sure violates the Geneva Convention for its WTF-ery.



  • Ah, now I see, thank you.



  • Best is that PHP has some methods for outputting a CSV to a file, but not to get it as a string. Have to use weird stuff like opening an output buffer, opening php://output as a file, using fputcsv, then using ob_get_clean() to get it as a string.



  • PHP 5.3 added str_getcsv for just that reason. Even better, it lets you override what the escape character should be, something that fputcsv doesn't.


  • BINNED

    @Arantor said:

    str_getcsv

    Why? Why? Why?

    Or is it just me that hates the str_ naming scheme?



  • Those do opposite things though. fputcsv takes an array and turns it into a CSV. str_getcsv takes a CSV and turns it into an array.


Log in to reply