> On April 29, 2015, 8:24 p.m., Joris Van Remoortere wrote:
> >

I was surprised to see new functionality in this patch since the summary was 
code movement :)

Just a quick note on DRY below.


> 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?

>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).


- Ben


-----------------------------------------------------------
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