Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Decorator pattern #132

Open
bromagosa opened this issue Apr 16, 2015 · 2 comments
Open

Decorator pattern #132

bromagosa opened this issue Apr 16, 2015 · 2 comments

Comments

@bromagosa
Copy link
Collaborator

I've recently found out the name of the pattern we're using over and over is actually decorator, not smart proxy.

Anyway, I've been thinking a lot about this pattern in BeetleBlocks... this is not a Snap! extension anymore, it's a complete mod, which is why I'm not sure it makes that much sense to decorate so many functions.

In the end I think it'd be easier to merge major, important Snap! changes back to BeetleBlocks, and it'd make our job much easier.

I believe heavy usage of this pattern does make a lot of sense in extensions, such as Snap4Arduino or Snapi, where you absolutely want to be up to date with the trunk, and where Snap! keeps being Snap!, but just with additional features. In BeetleBlocks this is not the case though, we don't even have sprites or a stage (strictly speaking) anymore. We are not even sharing the UI with Snap!, so applying this pattern over and over just makes for a slower loading of the environment and an incresed development difficulty.

On the other hand, the decorator pattern kind of keeps things in order. You can quickly jump into the beetleblocks folder and see what changes have been applied to Snap!, which is specially good for people who want to read our code and learn how to extend Snap.

I'm a bit divided here, what do you guys think? @jmoenig, do you agree?

@jmoenig
Copy link
Collaborator

jmoenig commented Apr 16, 2015

Hmm.... I'm really not a Git ninja (that's you, @bromagosa) so I don't know about the additional overhead this pattern creates (except for loading, but that doesn't worry me, I don't even notice any additional loading time in Beetleblocks at all).

I do agree that Beetleblocks is currently a fork of Snap, and not so much an extension. To be honest, however, I'm not all that happy with this decision. Well, that's not true, I'm very happy with this decision, because it makes things a lot easier to handle. But ultimately what I want is to really modulize Snap! in a way that lets us do things like Beetleblocksas plug-in modules. For this we'll have to rework the Snap code into whatever will be a nice module system that fullfills the qualification of not annoying me (such as IIFEs do), and it may well take a lot longer for all of this to happen (even years...). In the meantime I do enjoy being able to browse the peculiarities of Beetleblocks quickly and almost at a single glance in your current pattern, which I basically consider to be an applied Smalltalk changeset :-)

So, my vote at this time would be to stick with the current pattern. I do like it. But I understand if it becomes too cumbersome for you to manage or to get your head around in. You are, after all, Beetleblock's primary developer right now, so I'd leave it up to your judgement.

@bromagosa
Copy link
Collaborator Author

Basically, the overhead is in having to copy-paste big functions in cases where decorating is impossible, like in menus (see https://github.com/ericrosenbaum/BeetleBlocks/blob/master/beetleblocks/gui.js#L32). This is beginning to happen more and more because we're dealing with a lot of UI changes, and typically UI functions are long and can't be easily decorated.

For instance, some components don't keep references to all their submorphs (which is of course correct), so moving a button by decorating a function that paints that component would be even uglier than copy-pasting it.

Maybe if we can come up with a way to modify these components that doesn't involve writing code like this.children[3].setLeft(this.children[3].left() + this.children[2].width()) I will feel better about the BeetleBlocks codebase...

Let's keep thinking about this.

About Snap! extensions, I guess a "simple" way to deal with them would be to generalize what we've been doing in all these forks. Have an "extension" folder where you can dump files with decorated functions which Snap! will load right after their originals according to an extension definition file. Not optimal, but surely better than succumbing to IIFEs...

p.s. I'm not a Git ninja either, proof of it is the mess I created in my pull request to Snap! last week :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants