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

>> 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. Oh, OK, my bad... I like this design of Decorators. Why don't you use the same one for Strategies? Again I'm not sure about the distinction you make between decorators and strategies. You said it is based on the fact that decorators are optional and strategies are mandatory... In my opinion, strategies should also be optional. >> 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. OK, if you want to keep this design, I think you should have an "empty" strategy that does nothing, and be able to combine strategies (like you can combine decorators). >> 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. Because when I'm writing some code I don't think about the way I will remove the whitespaces or the comments afterward. A definition for code would be something like "some data that can be executed by a machine", not "some data and a way to transform it to a more compact but equivalent data that can be executed by a machine". I think Code is a good class name anyway... >> 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. Yes, I had a look to those strategies, and I find it would be more usefull if we could not have this dependency... Say I write a new strategy that renames variable to shorter names (blabla => a, foobar => b...). Now I may want to apply this strategy, but leave the formatting of the code unchanged (because I want to check everything is OK, I want to compare old to new code or whatever). I also may want to apply this strategy and the Route strategy, or this one and the Quiet one, or this one and the Pack one. This could be done by having a constructor that lets me create a RenameVariables strategy with another strategy (new RenameVariables(new Quiet) for example). Or this could also be done by changing the way a strategy is applied to a code: $code->apply(new RenameVariables()); $code->apply(new Quiet()); > $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? If the 'Header' of the reformat function was given as an argument to the constructor, I could have written $code = new ScriptReorganizer_Type_Decorator_AddHeader('HEADER', new ScriptReorganizer_Type_Decorator_AddFooter('FOOTER', new ScriptReorganizer_Type_Script( new ScriptReorganizer_Strategy_Route ) ) ); $code->load('scriptToLoad.php'); $script->save('fileToSave.php'); -- Vincent