Does this antipattern have a name?



  • Wow ... just ... wow ...

    I don't even know how to count the number of WTFs here. The biggest WTF of course, is the guy who made this was supposed to be team lead, and better at this stuff than the rest ... or so the in... ehm, CONsulting firm claimed.

    <?php
    class lfSyncManager
    {
    
        protected $syncSystem;
    
        function __construct($sync_system, $options = array())
        {
            if (is_string($sync_system))
            {
                $sync_system_name = sprintf('lfSync%s', ucfirst($sync_system));
                if (!class_exists($sync_system_name))
                {
                    throw new sfException(sprintf('Unknown synchronization system type "%s"', $sync_system));
                }
                $this->syncSystem = new $sync_system_name($this->loadConfiguration($sync_system));
            }
            else
            {
                throw new sfException('Unrecognized synchronization system.');
            }
        }
    
        public function __destruct()
        {
            unset($this);
        }
    
        /**
         * Creates an instance of synchronization system and returns it.
         *
         * @param string $sync_system Synchronization system, e.g. FOO.
         * @return lfSyncFOO an instance of synchronization system.
         */
        public static function getInstance($sync_system)
        {
            if (is_string($sync_system))
            {
                $sm = new lfSyncManager($sync_system);
                return $sm->getSyncSystem();
            }
            else
            {
                throw new sfException('Unrecognized synchronization system.');
            }
        }
    ?>
    


  • WTFs I see:

    • The class is supposed to be a singleton, but the constructor is publicly accessible. (You may count that as two WTFs if you consider the singleton pattern to be an antipattern.)
    • The destructor. (Counts as two WTFs if it actually works.)

    What am I missing?



  • Identifying your synchronisation system by a string which is the name of the class?

    Some people should not be trusted with dynamic typing.



  • @Psychosmurf said:

    Does this antipattern have a name?

     

     


     http://c2.com/cgi/wiki?OverUseOfPatterns



  • I have no idea of how GC in PHP works, but since almost every language today has GC, everytime I see a "destruction" method, I know the person has no idea of WTF he's doing.



  • Basically, the getInstance method is a factory method, returning an instance of the specified type. What's really bugging me, is that it first creates a new instance of lfSyncManager class, which initializes an object of the required type. Then that object is pulled from the bowels of the new lfSyncManager object, via a completely unneccesary getter method, and the lfSyncManager object is then discarded ...



  • The double check on is_string for the $sync_system is also awesome. But really, what in the name of baby Jesus is this code supposed to do besides completely unnessecarily wrapping "$class = 'lfSyncFOO'; $sync_system = new $class" in an object that's a greater waste of space than your average Eighties act?

    (Out of morbid curiosity, what do 'lf' and 'sf' stand for, and why does an lfSomething throw an sfException?)



  •  It looks like this is an app developed with the Symfony framework http://symfony.com/

    sfException is just a system logger class. Not sure what the IfSomething is though.....

     



  •  With the syncing, I assumed the 'f' stood for 'feed'. The 'l'? I read 'line' automatically, but that wouldn't really make sense I guess :) Good call on the Symfony use, that sounds logical. Although I haven't touched it in years.



  • @ubersoldat said:

    I have no idea of how GC in PHP works, but since almost every language today has GC, everytime I see a "destruction" method, I know the person has no idea of WTF he's doing.
     

    So, in a GC language, where should people delete temporary files, close network connections and so on?



  • @Mcoder said:

    @ubersoldat said:

    I have no idea of how GC in PHP works, but since almost every language today has GC, everytime I see a "destruction" method, I know the person has no idea of WTF he's doing.
     

    So, in a GC language, where should people delete temporary files, close network connections and so on?

    In Java, all of those should be done in a 'finally' clause.
    I try real hard not to know anything about php.

     


  • ♿ (Parody)

    @Mcoder said:

    @ubersoldat said:
    I have no idea of how GC in PHP works, but since almost every language today has GC, everytime I see a "destruction" method, I know the person has no idea of WTF he's doing.

    So, in a GC language, where should people delete temporary files, close network connections and so on?

    I think it depends on the sort of GC you're dealing with. In something deterministic, using refcounts, destructors are reasonable things to use. But most GC'd languages these days aren't like that, and if you aren't creating sufficient memory pressure, nothing may be collected for a long time, or even until your program ends. So the answer is, "when you're done with them."



  • @anonymous_guy said:

    WTFs I see:

    • The class is supposed to be a singleton, but the constructor is publicly accessible. (You may count that as two WTFs if you consider the singleton pattern to be an antipattern.)
    • The destructor. (Counts as two WTFs if it actually works.)

    What am I missing?

    sprintf() all over the place instead of string concatenation.

    Also, I see nothing that indicates it's supposed to be a singleton.

    I don't know what this code is trying to do, either, but it seems like a real mess.



  • @ubersoldat said:

    I have no idea of how GC in PHP works, but since almost every language today has GC, everytime I see a "destruction" method, I know the person has no idea of WTF he's doing.

    PHP uses refcounts (although recent versions have GC, too). If you don't create circular references, the destructor will actually do what you expect.



  • @boomzilla said:

    I think it depends on the sort of GC you're dealing with. In something deterministic, using refcounts, destructors are reasonable things to use. But most GC'd languages these days aren't like that, and if you aren't creating sufficient memory pressure, nothing may be collected for a long time, or even until your program ends. So the answer is, "when you're done with them."

    Even if you're using refcounts, the destructor may not be a reasonable location to dispose of unmanaged resources. Whether or not it is appropriate to dispose of them there depends on what exactly happens when the refcount hits 0. The instance could be immediately destroyed, but it could also be placed on a list of items that are collectively destroyed at some later time in one single 'pass' (much like modern generational GCs work).

    You'd need to be aware of quite a few internal details. For that reason alone it is always better to dispose of unmanaged resources explicitly.



  • @Psychosmurf said:

    $sync_system_name = sprintf( ... );
    ...
    $this->syncSystem = new $sync_system_name( ... );

    what?!


    but


    how is this even possible


    why would you ever want to


    i don't

    i



  • @PSWorx said:

    @Psychosmurf said:
    $sync_system_name = sprintf( ... );
    ...
    $this->syncSystem = new $sync_system_name( ... );

    what?!


    but


    how is this even possible


    why would you ever want to


    i don't

    You can also call strings as functions, leading to this:

    $callback = $_GET['callback'];
    $callback( $result );


  • @Ben L. said:

    You can also call strings as functions, leading to this:

    $callback = $_GET['callback'];
    $callback( $result );

    Wow! I knew PHP had some bad security holes - but I didn't know they actually have special syntax so it's easier to write new ones.
    I think I'll propose !!foo as shorthand for "execute URL parameter foo as a root shell, then wipe all logs." for the next version.



  • @Ragnax said:

    The instance could be immediately destroyed, but it could also be placed on a list of items that are collectively destroyed at some later time in one single 'pass' (much like modern generational GCs work).

    It is immediately destroyed. What's more, PHP is meant to be used to handle short-live web requests, so anything you don't cleanup explicitly is cleaned up automatically, so it's somewhat irrelevant for most people.

    @Ragnax said:

    You'd need to be aware of quite a few internal details. For that reason alone it is always better to dispose of unmanaged resources explicitly.

    [citation needed] You need to be aware of quite a few internal details when working with lots of languages. I don't see how it's better to avoid using features than it is to learn how the features work and use them appropriately.



  • @Ben L. said:

    You can also call strings as functions, leading to this:

    $callback = $_GET['callback'];
    $callback( $result );

    Which is possible in pretty much any language that has reflection capabilities.



  • @morbiuswilters said:

    @Ben L. said:
    You can also call strings as functions, leading to this:

    $callback = $_GET['callback'];
    $callback( $result );

    Which is possible in pretty much any language that has reflection capabilities.

    Yeah, but generally people who write this kind of code can't figure out reflection.



  • @PSWorx said:

    Wow! I knew PHP had some bad security holes - but I didn't know they actually have special syntax so it's easier to write new ones.

    How is that any different than any language that allows dynamic method invocation?



  • @Ben L. said:

    Yeah, but generally people who write this kind of code can't figure out reflection.

    So your position is that we should make languages more complicated and difficult to use in order to improve security? Wow, that's.. that's quite the statement.



  • @Monomelodies said:

    The double check on is_string for the $sync_system is also awesome. But really, what in the name of baby Jesus is this code supposed to do besides completely unnessecarily wrapping "$class = 'lfSyncFOO'; $sync_system = new $class" in an object that's a greater waste of space than your average Eighties act?

    I suppose it is a Java idiom applied to a language where it could have been written much more simply. I see those a lot. Unfortunately most books about design patterns use Java and than people apply the patterns to other languages where they are completely unnecessary because they were created as workaround on lack of Java deficiencies (like everything having to be in a class, complete lack of any kind of function types, lack of value types etc.) in the first place.


Log in to reply