Martin Mucha has posted comments on this change.
Change subject: userportal,webadmin: moved duplicate UICommand creation to
Factory
......................................................................
Patch Set 9:
@Alexander Wels: that's not possible, since class is stateful. But it can be
made stateless.
@Vojtech Szocs: I'm not an expert on garbage colletion, especially not JS
garbage collection. In plain java I cannot see it as a problem in any gc
strategy I know, since while it's a strong reference from factory to something,
in current constructs the factory instance is thrown away immediately, so it
does not matter to what it points. It's just a "little inefficiency" (and we're
probably talking in tens of ns grade tops), which is tradeoff for OO design.
BUT current patch was just a pushed code to start discussion (and to see if
there's interrest in this, see that cancel is not replaced, only ok buttons).
This code can be easily refactored to different shape, this is just a proposal.
Possible alterations: we should keep in mind, that we are able to, someday,
support DI. So possible alterations are:
* static factories, ideally defined in UIcommand (drawback: overgrown class).
* non-stactic stateless methods of some factory.
* builder.
ad adding "title" parameter to every method: I'm against these methods can
serve any permutation of input; these methods should serve as a shortcut for
*common* UICommand instantiations, it should not try to be able to create
completely anything. If that would be required I'd consider to implement it as
a builder, like you've presented:
UICommand defaultCommand = new UICommand("OnSave",
iCommandTarget).asDefaultCommand();
But I think we should define our intentions mode declaratively: "I want default
ok button, give me one." instead of "give me a button, then make it default,
and yes, also name it OK, alright?", which is longer, more error prone, harder
to write and forces to copypasting. Also, when there's lot of stuff to be set,
it should be solved either by custom code (if it's not common case) or by
builder, since adding more parameters "is not possible". Having said that, we
should also handle close and cancel commands. Others are so infrequent so it
does not matter. Probably.
Base on your feedback I'd:
* change the factory so it's not stateful ~ ie. add iCommandTarget to all
methods. But those methods should not be static.
If needed we can make UICommnand setters make to return this to resemble
builder. This way we could craft all uncommon cases.
--
To view, visit http://gerrit.ovirt.org/33317
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I0b3b1344b1a3819e58de4db5093106bedf5291e6
Gerrit-PatchSet: 9
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Martin Mucha <[email protected]>
Gerrit-Reviewer: Alexander Wels <[email protected]>
Gerrit-Reviewer: Lior Vernia <[email protected]>
Gerrit-Reviewer: Martin Mucha <[email protected]>
Gerrit-Reviewer: Vojtech Szocs <[email protected]>
Gerrit-Reviewer: [email protected]
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches
