Re: [PEPr] Comment on Tools and Utilities::ScriptReorganizer

On Mon, 16 May 2005 22:30:54 +0200, Vincent Lascaux <vincent.lascaux@centraliens.net> wrote:

I don't think Type is a good name, Document would probably be better (if I understand what it does right: storing the document (script or library) that will be transformed by a strategy).

<Type>, as, without any doubts, you will have seen in the source, is the "abstract concept" of what _has_to_be_created_ and only by chance holds the "original document" to be processed. It's more the notion of what the output will be: "document" of <Type> script or of <Type> library, two different implementations of said concept.

OK... Looking at the code, it looks like Type is more like an abstract concept of a job: its role is not only to represent some code, but also the way it will be transformed (since it also stores the strategy and (eventually) the decorator).

No Vincent, <Type> does not store any <Decorator>s! <Type> is not aware of the fact that there is any further processing involved _before_ or _after_ the <Strategy> to apply. That's the nice thing about decorators ;-) Furthermore, the entity dealing with a <Type> object does not even know which it is currently using or if it is "decorated". Nice.

I still think that this class should not hold the strategies (or the decorators).

According to the Gang of Four, the Strategy pattern's intent is to: Define a family of algorithms, encapsulate each one, and make them interchangeable. Strategy lets the algorithm vary independently from clients that use it. I'm just following the new perspective of "Objects have responsibilities": the client <Type> only knows about the fact that it will apply a <Strategy>, but not which one. Conceptually the client manages several different implementations of the _same_ algorithm. The algorithms depend on the _context_ in which they occur, i.e. what the user (=== context) of the library wants to be achieved. Again, according to the Gang of Four, the Decorator pattern's intent is to: Attach additional responsibilities to an object dynamically. Decorators provide a flexible alternative to subclassing for extending functionality. If "Objects have responsibilities", then instead of controlling added functionality by having a control method, the <Decorator> pattern says to control it by chaining together the functions desired in the correct order needed. It _separates_ the dynamic building of this chain from the the client that uses it as well as from the chain components (e.g. <AddHeader>, <AddFooter> and <Pharize>). With this approach I'm avoiding long if ( ... ) else and/or switch ( ... ) constructs, there's no need to check what we are dealing with. It will just be done. Obviously somewhere along the lines there must be some checking code, but we have decoupled this part from the core library and thus made this classes more cohesive. You can achieve the same output result by following the PPP path. Correct you are. However, I have chosen the "New Perspective on Object-Oriented Design". Someone else would like to comment on this?

What about the name <Code>, which, IMHO, does reflect even better what is being looked at - with "Document" I do associate more something to deal with a word processor, a grafical tool etc. Just a thought.

I also prefer Code: it's a more specific (and thus better) name than Document. But a Code should not hold a decorator or a strategy, should it?

Why not? <Code> would only be the conceptual view of <Script> and <Type>. See my notes above.

I do follow you on this, but the reasoning behind the separation of stratagies and decorators is very simple: - Strategies are _mandatory_, i.e. it does not make sense to create a "Document" object without knowing in advance, which <Strategy> to apply (remember, that the context of ScriptReorganizer is the deployment of code and not a dynamic web application - it's just a work horse package) ... it simply can't be missed by chance and it's anchored with the instantiation of the "Document".

Actually, I find you library class (that implements Type) usefull, even without applying any strategy. Strategies should not be mandatory, and it would be nice if several strategies could be applied (like 1-remove comments, 2-remove unusefull whitespaces, 3-add header).

Please look at the source of the strategies <Route>, <Quiet> and <Pack> as hinted in the proposal: they are working in an incremental way. <Pack> (allows only one whitespace between each token) depends on <Quiet> (stripping off comments) depends on <Route> (stripping off only two or more consecutive blank lines). So there's no need to use several strategies in combination. The goal of <ScriptReorganizer> is to reduce the file size of the code to deploy. If you would like to use several decorators in an alternative way, just try for example: $code = new ScriptReorganizer_Type_Script( new ScriptReorganizer_Strategy_Route ); $code->load( 'scriptToLoad.php' ); $script = new ScriptReorganizer_Type_Decorator_AddHeader( $code ); $script->reformat( 'HEADER' ); $script = new ScriptReorganizer_Type_Decorator_AddFooter( $code ); $script->reformat( 'FOOTER' ); $script->save( 'fileToSave.php' ); Somehow cumbersome, but would that be a good compromize for you?

Unrelated: I had a look to you library type, wouldn't you have a problem with cyclic imports (like A.php contains require_once 'B.php'; and B.php contains require_once 'A.php';)?

Thanks Vincent, for spotting this bug! Find below the fixed method load, the cyclic source as well as the respective test method. I will release this code later today. In ScriptReorganizer_Type_Library add lines 14 and 15:

    // {{{ public function load( $file )

    /**
     * Loads the script's content to be reorganized from disk
     *
     * @param  string $file a string representing the file's name to load
     * @return void
     * @throws {@link ScriptReorganizer_Type_Exception ScriptReorganizer_Type_Exception}
     */
    public function load( $file )
    {
        parent::load( $file );

/*add*/ $baseDirectory = realpath( dirname( $file ) ); /*add*/ $this->imports[] = $this->retrieveRealPath( $file, $baseDirectory );

        $this->setContent( $this->resolveImports( $baseDirectory ) );

        $importException = '"< import of( file [^>]+)>"';

        if ( preg_match_all( $importException, $this->getContent(), $matches ) ) {
            throw new ScriptReorganizer_Type_Exception(
                'Import of' . PHP_EOL . '-' . ( implode( PHP_EOL . '-', $matches[1] ) )
            );
        }
    }

    // }}}

In the path ScriptReorganizer/Tests/files add the files A.php: <?php

       define( 'A', true );

       require_once 'B.php';

       ?>

B.php: <?php

       define( 'B', true );

       require_once 'A.php';

       ?>

In ScriptReorganizer_Tests_Type_LibraryTest add:

    // {{{ public function testPreventCyclicImports()

    public function testPreventCyclicImports()
    {
        $expected = '<?php' . PHP_EOL . PHP_EOL . "define( 'A', true ); define( 'B', true );" . PHP_EOL . PHP_EOL . '?>';
        $library = new ScriptReorganizer_Type_Library( new ScriptReorganizer_Strategy_Pack );
        $this->source = $this->target . 'A.php';
        $this->target .= 'cyclicImports.php';

        $this->xRescript( $library, $expected );
    }

    // }}}

Again, thanks for your valuable input!