Code review comment for lp:~frankban/juju-quickstart/env-manage-edit

Revision history for this message
Gary Poster (gary) wrote :

https://codereview.appspot.com/44750044/diff/1/quickstart/cli/views.py
File quickstart/cli/views.py (right):

https://codereview.appspot.com/44750044/diff/1/quickstart/cli/views.py#newcode230
quickstart/cli/views.py:230: def use(env_data):
On 2014/01/02 17:51:04, frankban wrote:
> On 2014/01/02 15:19:28, gary.poster wrote:
> > I wonder if the env_detail function would look less forbidding at
first glance
> > if these internal functions were defined outside (_use,
_set_default, etc.)
> and
> > then hooked up as partials. I think the verbs are clear enough that
it would
> > read well.
> >
> > A counter-argument is that you are defining everything locally, so
you can
> read
> > what is going on inline, and it similar to what people are
accustomed to doing
> > with OO. If env_detail were in its own file as is, would it look
super easy,
> > because it would look similar to a class with methods? I don't
know.
> >
> > What do you think?

> This is a really good question. I like the possibility to use closures
so that
> you can share the scope and group code logic, but, as you mentioned,
this can be
> surprising at first sight. If you add to this the good point Benji
made below
> (it is easier to directly unit test separate functions), then we have
two good
> arguments to make this change. Since it requires a bit of code
reorganization
> I'd be inclined to tackle this separately, how does it sound?

Sounds great to me. Thank you.

https://codereview.appspot.com/44750044/

« Back to merge proposal