> On June 20, 2013, 11:16 p.m., Ben Mahler wrote:
> > I see why you added the helper, but it seems odd that it's only needed /
> > used by initialization. I realize initialize is fairly large, but my
> > preference would be to keep it in there. Alternatively, could the helper be
> > static, taking as input the flags, and returning a Resources object?
>
> Vinod Kone wrote:
> I really don't want to put it up in initialize(), because as you noted,
> its ~200 lines already! I think it reads pretty clear when you see it in
> initialize(). Infact, I would even like all the installs to be refactored
> into something like setupRoutes().
>
> Regarding a static helper, I don't understand the reasoning?
>
> Ben Mahler wrote:
> I guess this boils down to philosophy, I find it easier to read a long
> version of initialize() instead of jumping into helpers. With these helpers I
> would say they are especially confusing because they do not reduce
> complexity, they merely pull code from a function into a sub-function (but
> that anyone can call!). Ultimately, this actually increases complexity of the
> code (same code, but more lines to express it, and I have to jump into
> different contexts to understand initialize()).
>
> The static helper would at least have the benefit of not modifying state
> in the slave, meaning one can clearly see how it is used ("given a Flags
> object, this produces a Resources object and has no effect on the Slave
> because it's a static function").
>
> Vinod Kone wrote:
> Yup, I think this is more philosophical than technical. For me the
> important thing is that a function has to do one thing and is easy to
> understand based on its signature. And yes, I don't like reading functions
> that have 100s of lines. I tend to get lost trying to understand what is
> happening.
>
> Benjamin Hindman wrote:
> I'd like to keep a monolithic 'initialize'. It gets called exactly _once_
> and is very procedural in nature so it's helpful to be able to walk through
> it line by line rather than jumping between functions. Moreover, as BenM
> points out, moving code into sub-functions tends to increase complexity: (a)
> you have to decide what should go where (which changes over time) and (b) if
> there are dependencies it can get hard to reason about (i.e., does an
> instance variable get set in function 'setupFoo' or in 'setupBar', because
> 'setupBaz' needs it and gets invoked before 'setupFoo' but after 'setupBar',
> etc). In all other circumstances we should keep functions small and
> "primitive" in nature.
>
> In addition, any time we can write an idempotent/stateless function we
> should. And we should always make these static methods or "top-level"
> functions (preferring the latter if the functionality has nothing to do with
> the class).
>
> The 'Flags' abstraction should really provide "programmatic defaults",
> but since that functionality doesn't exist yet the code you moved to
> 'setupResources' ended up in
> 'initialize'. I'm all for moving this code out of 'initialize', but lets
> do it by fixing stout Flags. I created an issue at 3rdparty/stout!
I don't completely buy the "procedural" / "context switch" arguments here,
because they can be argued both ways. For example, why do we need
Framework::launch/destroyExecutor(), Executor::add/removeTask() which are not
idempotent? Why not inline them wherever they are used? Every function involves
a context switch. The tradeoff we need to consider is whether that context
switch is "worth" it. For me the "worth" is ease of understanding and ability
to modify later on.
I personally like this
initialize()
{
setupResources();
setupRoutes();
setupFoo();
setupBar();
}
than
initialize()
{
// 100 lines of something
// 100 lines of something else
// ....
}
The former tells me what this function is doing in general. If I am not sure
what is happening in a sub-function (a sign that it is a badly designed
function) then I can look into it. I remember that we talked about bloated
functions before and agreed that any function exceeding ~40 lines needs
refactoring. Now that doesn't mean a 400 line function should be chopped into
10 functions. It is likely that there is a bigger underlying problem.
Regarding your points a) and b). a) Arent' we always making this decision. I
don't think its that difficult to understand that a function called
setupResources() sets up resources or a function called addTask() adds task to
an internal structure. b) Again, isn't this always true. Isn't that why we have
things like pre/post conditions.
But perhaps, thats just me. Maybe setupResources() is not as "primitive" or
"easy to understand" as I thought it is.
- Vinod
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/12010/#review22208
-----------------------------------------------------------
On June 20, 2013, 11:02 p.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/12010/
> -----------------------------------------------------------
>
> (Updated June 20, 2013, 11:02 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ben Mahler, and Jiang Yan Xu.
>
>
> Description
> -------
>
> See summary
>
>
> Diffs
> -----
>
> src/slave/slave.hpp d1ba82ed0c87331c6c06b109bbc2b267a07cf335
> src/slave/slave.cpp 952bd149247c2cc44b87d20fe63a45d011f5578b
>
> Diff: https://reviews.apache.org/r/12010/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Vinod Kone
>
>
