juju-gui

Code review comment for lp:~gary/juju-gui/bug1090716

bug1090716
Merge into trunk

Revision history for this message

Gary Poster (gary) wrote on 2013-01-11:

On 2013/01/11 17:22:53, hazmat wrote:
> crappy network ate my first response, redo with brevity.

> On 2013/01/11 13:44:38, gary.poster wrote:
> > On 2013/01/11 12:51:03, hazmat wrote:
> > > On 2013/01/11 05:29:36, hazmat wrote:
> > > > On 2013/01/10 22:08:46, gary.poster wrote:
> > > > > Please take a look.
> > > >
> > > > this seems like a lot more code then just filtering at the model
/ delta
> > > stream
> > > > level.
> > >
> > >
> > > more specifically trying to keep every known view/render location
guarded
> with
> > a
> > > service filter is much more fragile and more code then simply
> > > removing/preventing in one place at ingest into the app models of
the gui
> > > service.
> >
> > Benji and I discussed the two options in a pre-implementation call.
We
> > preferred filtering on the view level for a few reasons.
> >
> > - We were not convinced that it would be any easier in the long run
to make
> the
> > change at the db/ingestion level rather than the view level. If we
had felt
> > that this ease was the primary driver for an important choice, we
would have
> > considered commissioning competing implementations. However, we had
other
> > reasons to not pursue the db/ingestion approach, given below.
> >
> > - This is a view-level feature: we don't want to confuse users by
presenting
> > certain charms. The db has nothing to do with the desired outcome.
In both
> of
> > our experiences, implementing view-level behavior in view code not
only makes
> > more sense intuitively but also is a better long-term design
decision.

> the purpose of content in the db is for presentation to the user, else
it has no
> purpose being in the client side db.

Certainly. However, we have a clear model/view distinction in our code.
The db/delta/model's job is a simple one at the moment: synchronize the
client db with the server db. Any one-off customizations from that
mission are a maintenance burden. Neither of us have proof that one
approach's maintenance burden is greater over time. As I said, IME
moving these kinds of very-high-level UI decisions down into the db
level is a mistake. You have a different perspective.

> the problem with this is a view level feature is that it has to be
implemented
> everywhere we render a service. If we grow new views we have to
duplicate the
> same filtering code there as well.

With trivial and already-implemented code reuse. Moreover, your "grow
new views" story is a less concrete example than the one I gave, with no
user story associated with it.

> From a maintenance and efficiency perspective
> it seems a clear win to me, to implement once vs. implement n.

In terms of code efficiency, it is true that simply not rendering the
GUI service rather than hiding it would reduce some computation.
However, that appears to be a negligible increase as I understand the
code paths.

In terms of developer efficiency, I think it is a matter of opinion,
given the current state of the app and future plans.

> >
> > - As a concrete example of why keeping the db truthful might be an
advantage,
> we
> > discussed a possible future feature in which hidden services could
be
> revealed.
> > For instance, let's say we want a proxy in front of the gui or some
other
> future
> > hidden service. If we can reveal hidden services with a gesture, we
can make
> > this happen, and we can also see why a proxy that appears to be
disconnected
> is
> > actually connected and performing a desired role.

> that future use case isn't well defined or even clear that its valid.
in future
> with jaas, the typical environment won't have these services.

It's one example story, which does have a real and reasonable use case
associated with it, whether or not we choose to pursue it. The point is
that it is a good example of other stories for which keeping
model/db/sync code simple and direct and accurate is valuable.

> >
> > >
> > > the utility functions here feel like they should be in model.utils
instead
> of
> > > view.utils.
> >
> > Given our perspective that this is a view-level feature, and that
these utils
> > are used in view-level code, the utilities make sense where they
are.
> Certainly
> > if the code is refactored to filter at the ingestion/db level then
the utils
> > belong somewhere else.
> >

> regardless of usage location, the fact that these only operated on
model
> attributes (or in future app state) was my differentor to definition
location.
> its a minor though, implementor's choice on that one.

View code works with model objects also. Moreover, these utilities also
operate on the controller-ish BoundingBox objects as well as models.

> > >
> > > needs fixing, imo.
> >
> > I think this is an "implementer's choice" disagreement, where both
sides have
> > reasonable arguments and the resolution should be in the hands of
the person
> who
> > did the implementation. That's your call.
> >

> i think this one is more an architectural/maintenance question then
the normal
> implementor's choice. i'd like to do one more round of discussion
before we
> resolve via other mech.

I have a meta concern.

We want to have high-quality code and design. We should try not to
achieve that via rework requests. I feel we have had too many rework
requests in our project's history, as most recently demonstrated by this
branch and the one for https://bugs.launchpad.net/juju-gui/+bug/1074423
.

We ask for rework during reviews. Reviews are one of the tools we have
to encourage high-quality code. They are also among the most expensive
tools when they ask for rework, rather than small changes.

Reworking is expensive in terms of time, money, workflow, and even
morale. Asking for rework should be a red flag for a possibly broken
process, and a suggestion that we are not using our other tools for
quality as well as we might. It should also be challenged: is the
rework request actually necessary?

What are our other tools for quality?

- We clearly write down our broad expectations and standards: tests need
to pass, code needs to conform to style guides, and so on. We document
those in our docs/process.rst right now. Robert Collins documented his
SOA expectations at
https://dev.launchpad.net/ArchitectureGuide/ServicesRequirements .
These sorts of expectations ideally should be SMART-style--measurable,
achievable, realistic, and so on. Vague expectations like "KISS" are a
lot harder to expect much from here.

- We also clearly write down our individual tasks in bugs and kanban
cards. When I write a bug, I try to specify the goals for that bug.
Ideally, the bug is written such that it specifies necessary goals and
leaves unspecified everything else, for the implementer to decide.
Ideally, if a solution conforms to the written goals and our written
standards, it will pass review.

- Prototypes and pre-implementation calls are the best times to try and
direct KISS and other vaguer ideas.

The less one is involved in these tools, the less one can expect from
the results

If an issue isn't caught by these tools, and leads to a potential rework
request in a code review, the first thing we should ask is if we could
have used those other tools better. The second thing we should ask is
whether we are confident that the rework request is worth the cost.

Sometimes it is worth the cost. For what it is worth, we have one other
tool, which in part is supposed to reduce the cost of rework requests:
keeping our individual tasks as small as possible, and trying to deliver
value as quickly as possible. If we are going down a wrong path, it's
better to know sooner rather than later. If we only have to rework a
day or two's worth of work, as with this branch, because we are landing
a small branch soon, that's a lot better than reworking a week's or a
month's worth of work.

But the cost of rework is still high, it's still quite possibly
indicative of a process problem, and it still should be challenged.

So, perhaps this branch is worth a rework request to you. I ask that
you please wield these requests with great care, and each time you are
tempted to use them, consider whether it indicates an improvement we
should make in our process and tools, and whether rework is really worth
the cost.

Thanks

Gary

https://codereview.appspot.com/7069068/

On 2013/01/11 17:22:53, hazmat wrote:
> crappy network ate my first response, redo with brevity.

> On 2013/01/11 13:44:38, gary.poster wrote:
> > On 2013/01/11 12:51:03, hazmat wrote:
> > > On 2013/01/11 05:29:36, hazmat wrote:
> > > > On 2013/01/10 22:08:46, gary.poster wrote:
> > > > > Please take a look.
> > > >
> > > > this seems like a lot more code then just filtering at the model
/ delta
> > > stream
> > > > level.
> > >
> > >
> > > more specifically trying to keep every known view/render location
guarded
> with
> > a
> > > service filter is much more fragile and more code then simply
> > > removing/preventing in one place at ingest into the app models of
the gui
> > > service.
> >
> > Benji and I discussed the two options in a pre-implementation call.
We
> > preferred filtering on the view level for a few reasons.
> >
> > - We were not convinced that it would be any easier in the long run
to make
> the
> > change at the db/ingestion level rather than the view level.  If we
had felt
> > that this ease was the primary driver for an important choice, we
would have
> > considered commissioning competing implementations.  However, we had
other
> > reasons to not pursue the db/ingestion approach, given below.
> >
> > - This is a view-level feature: we don't want to confuse users by
presenting
> > certain charms.  The db has nothing to do with the desired outcome.
In both
> of
> > our experiences, implementing view-level behavior in view code not
only makes
> > more sense intuitively but also is a better long-term design
decision.

> the purpose of content in the db is for presentation to the user, else
it has no
> purpose being in the client side db.

Certainly.  However, we have a clear model/view distinction in our code.
  The db/delta/model's job is a simple one at the moment: synchronize the
client db with the server db.  Any one-off customizations from that
mission are a maintenance burden.  Neither of us have proof that one
approach's maintenance burden is greater over time.  As I said, IME
moving these kinds of very-high-level UI decisions down into the db
level is a mistake.  You have a different perspective.

> the problem with this is a view level feature is that it has to be
implemented
> everywhere we render a service. If we grow new views we have to
duplicate the
> same filtering code there as well.

With trivial and already-implemented code reuse.  Moreover, your "grow
new views" story is a less concrete example than the one I gave, with no
user story associated with it.

>  From a maintenance and efficiency perspective
> it seems a clear win to me, to implement once vs. implement n.

In terms of developer efficiency, I think it is a matter of opinion,
given the current state of the app and future plans.

> >
> > - As a concrete example of why keeping the db truthful might be an
advantage,
> we
> > discussed a possible future feature in which hidden services could
be
> revealed.
> > For instance, let's say we want a proxy in front of the gui or some
other
> future
> > hidden service.  If we can reveal hidden services with a gesture, we
can make
> > this happen, and we can also see why a proxy that appears to be
disconnected
> is
> > actually connected and performing a desired role.

> that future use case isn't well defined or even clear that its valid.
in future
> with jaas, the typical environment won't have these services.

It's one example story, which does have a real and reasonable use case
associated with it, whether or not we choose to pursue it.  The point is
that it is a good example of other stories for which keeping
model/db/sync code simple and direct and accurate is valuable.

View code works with model objects also.  Moreover, these utilities also
operate on the controller-ish BoundingBox objects as well as models.

> i think this one is more an architectural/maintenance question then
the normal
> implementor's choice. i'd like to do one more round of discussion
before we
> resolve via other mech.

I have a meta concern.

We want to have high-quality code and design.  We should try not to
achieve that via rework requests.  I feel we have had too many rework
requests in our project's history, as most recently demonstrated by this
branch and the one for https://bugs.launchpad.net/juju-gui/+bug/1074423
.

We ask for rework during reviews.  Reviews are one of the tools we have
to encourage high-quality code.  They are also among the most expensive
tools when they ask for rework, rather than small changes.

Reworking is expensive in terms of time, money, workflow, and even
morale.  Asking for rework should be a red flag for a possibly broken
process, and a suggestion that we are not using our other tools for
quality as well as we might.  It should also be challenged: is the
rework request actually necessary?

What are our other tools for quality?

- We clearly write down our broad expectations and standards: tests need
to pass, code needs to conform to style guides, and so on.  We document
those in our docs/process.rst right now.  Robert Collins documented his
SOA expectations at
https://dev.launchpad.net/ArchitectureGuide/ServicesRequirements .
These sorts of expectations ideally should be SMART-style--measurable,
achievable, realistic, and so on.  Vague expectations like "KISS" are a
lot harder to expect much from here.

- We also clearly write down our individual tasks in bugs and kanban
cards.  When I write a bug, I try to specify the goals for that bug.
Ideally, the bug is written such that it specifies necessary goals and
leaves unspecified everything else, for the implementer to decide.
Ideally, if a solution conforms to the written goals and our written
standards, it will pass review.

- Prototypes and pre-implementation calls are the best times to try and
direct KISS and other vaguer ideas.

The less one is involved in these tools, the less one can expect from
the results

If an issue isn't caught by these tools, and leads to a potential rework
request in a code review, the first thing we should ask is if we could
have used those other tools better.  The second thing we should ask is
whether we are confident that the rework request is worth the cost.

Sometimes it is worth the cost.  For what it is worth, we have one other
tool, which in part is supposed to reduce the cost of rework requests:
keeping our individual tasks as small as possible, and trying to deliver
value as quickly as possible.  If we are going down a wrong path, it's
better to know sooner rather than later.  If we only have to rework a
day or two's worth of work, as with this branch, because we are landing
a small branch soon, that's a lot better than reworking a week's or a
month's worth of work.

But the cost of rework is still high, it's still quite possibly
indicative of a process problem, and it still should be challenged.

So, perhaps this branch is worth a rework request to you.  I ask that
you please wield these requests with great care, and each time you are
tempted to use them, consider whether it indicates an improvement we
should make in our process and tools, and whether rework is really worth
the cost.

Thanks

Gary

https://codereview.appspot.com/7069068/

« Back to merge proposal