Code review for image gallery uploader
Antranig Basman
antranig.basman at colorado.edu
Thu May 19 06:58:04 UTC 2011
I had a look at the recently committed refactored version of the new "image gallery" with uploader at
https://github.com/colinbdclark/image-gallery/blob/0b4138369b78bda2f160d817194e9b8a17c32d6a/js/uploader.js,
as invited by Colin (let me know if I should look at any of the other files).
This reworked implementation looks pretty good - there is a good use of "event boiling" which is
interestingly paradigmatic, which we should keep a hold of should we need to direct people to useful
real-world examples of the value of this kind of thing. As a result of the boiling, the "simpleRenderer"
component is maximally reusable and could even be thrown into the framework - it's very easy to imagine
someone working with this component finding an easy "stop shop" to firstly configuring the markup for these
rows in the main options, and then perhaps, if they wanted more flexibility, configuring in a different
renderer which received events from this usefully semantic event stream.
In general this code sheds more light on the state of the framework than it does on itself :) - illustrating
the areas where the framework has stabilised well for 1.4 (general IoC, boiling, clearing) and areas in
which more work is needed (declarative use of changeApplier, model calculus, semantics for destruction) -
I've just JIRAed the latter as http://issues.fluidproject.org/browse/FLUID-4257 exposing a characteristic
blind spot in the author of this part of the framework.
ChangeApplier work raised by this impl is at http://issues.fluidproject.org/browse/FLUID-4258
I can think of only the most minor things to say about the impl itself...
i) "undefined" test at 140 seems a touch heavy-handed, would a "falsity" do instead?
ii) jQuery might well cache the fetched template we refetch at line 148, is it perhaps worth keeping these
around somehow? There will of course be proper framework support for this kind of thing as part of the
"antigens" work in 1.5
iii) "Those skilled in the art" will recognise the meaning of the templateURLSelector at line 125 as
conforming to the jQuery.load syntax, but a comment might be useful for those unfamiliar
iv) The id-based selectors at line 114 should probably be swapped out for class-based ones - in case this
app ends up as a portlet (not inconceivable, its previous incarnation did :P)
v) The "path to model" at line 106 is somewhat long. This is generally acceptable since this is a path into
the SAME options structure as ourselves and so should hopefully not be a point of fragility. However, it may
be worth factoring out the bit of state holding the original and modifiable "queueSettings" into a
standalone mini-modelComponent so that they can be accessed as something like "{queueSettings}.model" from
both places, which as well as being more stable would highlight more clearly to readers of the component
that this is an interesting piece of shared state.
In general, the "hit rate" of this component is extremely high - out of 220 lines in the file, only 41
(about 20%, 5:1) contain any kind of code. We could expect future versions of the framework to be able to
double this hit rate to something like 10:1 or even higher - if we count "simpleRenderer" as some kind of
"framework code", the only actual implementation code ultimately required in this whole file are the two
string manipulation lines 130 and 131 :) These last two lines could go away if we delivered i) a
framework-standard GUID facility, and ii) the "globally ginger world".
Cheers,
Antranig
More information about the fluid-work
mailing list