[Merge] lp:~abreu-alexandre/webbrowser-app/add-uninstall-click-hook-to-webapps into lp:webbrowser-app
Alexandre Abreu
alexandre.abreu at canonical.com
Tue Nov 18 15:49:14 UTC 2014
> I didn't review it thoroughly, but I'd like to suggest two changes:
>
> 1) Of course, this is about the style :-) Make it consistent, in some places
> you are using "type& var", in some others "type &var".
done :)
> 2) I think that the OptionalData class (and the Fallible template) can be
> removed and the code simplified a bit. The Data() default constructor already
> does the right thing (setting the optional fields to "false") and it doesn't
> look like you really need to distinguish the case when the hook file is absent
> or if it's present and the keys are set to false.
I thought a bit about this one, I agree that it would simplify the code a bit
and as things are now would still make things work, but I find really annoying
(not to say bad) that there is no way (unless you complexify the function code a bit
and add cumbersome return arguments) to tell if a data is a valid data or not
(obtained by way of a valid click hook file). It adds a bit of overload on the data
structure which should also make sure that its default values produce no-ops when
there is a failure of any sort ...
This is a rather clean solution to sparkling those logic bits around the code (return
function arguments, etc.),
--
https://code.launchpad.net/~abreu-alexandre/webbrowser-app/add-uninstall-click-hook-to-webapps/+merge/241584
Your team Ubuntu Phablet Team is subscribed to branch lp:webbrowser-app.
More information about the Ubuntu-reviews
mailing list