> On April 29, 2015, 8:24 p.m., Joris Van Remoortere wrote:
> > src/master/framework.cpp, line 187
> > <https://reviews.apache.org/r/33376/diff/3/?file=939881#file939881line187>
> >
> >     1) Let's sync with BenH if we want to factor out logging like this. 
> > There are arguments on both sides, so let's follow up with a discussion, 
> > and maybe add it to the style guide!
> >     2) *If* we *do* decide to factor out the logging like this, let's 
> > change the name to be more meaninful, or even move this function into 
> > `updateFrameworkInfo` as a lambda. This message is clearly specific to that 
> > function, and is not usable by other functions in `Framework`.
> >     3) Since we're in the implementation file, we can do `using 
> > std::string` and remove the namespacing of `std::string` here.
> 
> Marco Massenzio wrote:
>     I actually wasn't quite sure myself (and the more I think about this, the 
> less I like my own choice): this is just a helper function, to avoid 
> violating the DRY Principle.
>     I would like to move it out of the Framework class, and just leave it the 
> cpp file as a local (static) helper method.
>     Again, to me, it scratches the itch of copy & pasted code, which I 
> positively dislike.
>     
>     If I change it to a local helper method, would that address your concerns?
> 
> Ben Mahler wrote:
>     >this is just a helper function, to avoid violating the DRY Principle.
>     
>     This is a great example of how to apply DRY! :)
>     
>     Namely, we have to be careful about introducing helper functions to 
> follow the DRY principle as it often comes at a cost: loss of readability. In 
> this case, the reader encountering `logWarning` has no _local_ clues to know 
> that `logWarning` is not a general way to log warnings, but rather it prints 
> a special message about updating framework information. That doesn't seem 
> very intuitive? Also, now we have a function at the Framework scope, callable 
> from any other place in the framework code, which is going to mean that when 
> a reader encounters it, he/she won't be sure that it's only used within the 
> updating FrameworkInfo code. So in the process of applying DRY we've made our 
> software harder to understand. Does this make sense?
>     
>     There are ways to follow DRY without sacrificing readability, for 
> example, can we collect the set of fields that we weren't able to change and 
> just print them together in one log message? If there's some reason to have 
> multiple log messages, could we have a local lambda for it? (Although I don't 
> see a need for having 1 log message per field).

Absolutely, Ben, thanks for clarifying.
That's actually what I meant by 'scratchin the itch' (I realize now I was being 
economical with words... I was typing single-handed... which is != from 
single-handedly typing :)
I've already reverted the code.


- Marco


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33376/#review82004
-----------------------------------------------------------


On April 22, 2015, 8:35 p.m., Marco Massenzio wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33376/
> -----------------------------------------------------------
> 
> (Updated April 22, 2015, 8:35 p.m.)
> 
> 
> Review request for mesos and Joris Van Remoortere.
> 
> 
> Bugs: MESOS-2633
>     https://issues.apache.org/jira/browse/MESOS-2633
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Created new file framework.cpp containing all the methods' implementations 
> for the `Framework` class (declared in master/master.hpp)
> 
> Declared `operator ==` for Task in type_utils.hpp 
> (it was implemented before, but not declared in the header file);
> 
> Refactored all the LOG(WARNING) to a single utility method.
> 
> 
> Diffs
> -----
> 
>   include/mesos/type_utils.hpp cdf5864389a72002b538c263d70bcade2bdffa45 
>   src/Makefile.am 93c7c8a807a33ab639be6289535bbd32022aa85b 
>   src/master/framework.cpp PRE-CREATION 
>   src/master/master.hpp 550d2c50cd01aa5830a7e7e03ab4a0240c221b15 
> 
> Diff: https://reviews.apache.org/r/33376/diff/
> 
> 
> Testing
> -------
> 
> All tests (make check) pass.
> 
> 
> Thanks,
> 
> Marco Massenzio
> 
>

Reply via email to