Mir

Code review comment for lp:~alan-griffiths/mir/surface-states-simplification

  1. surface-states-simplification
  2. Merge into surface-states
Revision history for this message
Kevin DuBois (kdub) wrote :

> > Secondly, I disagree with the approach. There are no strong "dependencies"
> > here. The information in question is all optional to the class receiving it,
> > so should not be passed in constructors. The classes can function without
> > event sinks or surface IDs. They will work as they already do. Only event
> > emission would not happen. So it's optional relative to those classes, and
> > therefore not something to enforce in the constructor.
>
> Arguing that a subset of the functionality works without these attributes is
> an argument that the subset belongs in a class providing just that subset.
> While we could factor that subset out, I don't think we need to that now.

I think we're using the term 'coupling' too broadly to make much progress arguing :) I'll propose re-framing the argument, and then give my 2 cents

I think we can agree on this though:
1) Its better if a function call doesn't have any hidden prerequisites, like "you must call function A before you call function B". This is sort of 'coupling of calling requirements".
2) A class with a lot of constructor parameters has a lot of dependencies and might be trying to do too much. This is sort of 'coupling of object dependencies'

my 2cents...
so we have to find a way to balance.

I think that, with regards to notify_change/set_id, set_id() is a dependency that has to be called before notify_change() is called, so the id may as well be in the constructor. Another possibility (if we really don't want any more construction parameters for Surface) is to have another object with expanded functionality that takes

We've tried to mitigate the problem of #2 by having a lot of mocks and stubs and nulls that are easy to plug in and make the object easy to manipulate in test. Since its easier to read a constructor than it is to trace what the call order is, I'd rather plug in mocks/stubs to the constructor than have to trace a call order to figure out if we've fulfilled our usage requirements.

« Back to merge proposal