[Ovirt-devel] Re: broken windows [Re: [PATCH] Adds max/min methods ...

[mark wagner]

Jim Meyering wrote:
> mark wagner <mwagner at redhat.com> wrote:
>   
>> The Stats changes are actually mine, not Steve's. Since he alone
>> needed to integrate
>> with my changes and effectively tested them for me, I asked him to
>> just submit
>> them.
>>     
>
> Bear in mind that my review was addressed to Steve, since nothing
> suggested the changes were by anyone else.
>
>   
I was clarifying that the code was mine so as to clear Steve's
reputation. I'm guessing next time he will have me submit the code
on my own... :)

> ...
>   
>>> Hi Steve,
>>>
>>> A fine net change, overall.
>>>
>>> It's great to do whitespace clean-up, but please keep that sort of change
>>> separate from anything substantial.  Otherwise, it's more work for the
>>> reviewer to separate the trivially-ignorable whitespace changes from
>>> the ones that are significant.
>>>
>>>       
>> The whitespace changes are mine.  I thought you wanted them out and had
>> put such an edict in place.  Sorry for not understanding
>>     
>
> Eliminating all offending white space at once would be disruptive in
> the short term.  The current policy is gentler: add no _new_ instances.
>
> ...
>   
>>> Also, I noticed that the pre-existing style is to initialize variables at
>>> the top of a block or function.  It's better to avoid that style, and
>>> instead to place any initialization as near as possible to the first use.
>>> For example, if you move the initializations of my_max and my_min down
>>> so they're nearer their first uses, readers don't have to worry about
>>> whether they're used uninitialized (now the initialization is just before
>>> the loop), or if the values are modified between initialization and
>>> whatever use the reader is looking at.
>>>
>>>       
>> I was actually taught just the opposite, init at the top so its easy to find
>> the inits.  In my more performance sensitive c++ apps, I tend to init just
>> before the variable is first used (or just outside of the loop as needed)
>> if there is a chance of getting out of the function before the variable
>> would be used. Thus avoiding unneeded initializations.
>>
>> I appreciate the style suggestions, coming from a long C/C++ background,
>> I tend do things the way I've been doing them for the last 20+ years. I
>> try to be fairly flexible in how I write or change code. For instance I
>> matched the style in graph_controller.rb for previous changes that I've
>> made. However, since the ovirt program doesn't have a predefined style
>> guide and I am the one writing and maintaining the Stats code, I will
>> continue to write it in a manner that is easiest for me to support.
>>
>> I'm sorry if this upsets your style preferences, but functionality is
>> more important to me over style. I also tend to worry when people focus
>> on the style w/o specifying a style guide for the program.
>>     
>
> I make the suggestion to move declarations down whenever possible because
> not only does that make the code more readable, but it also makes it more
> robust in the face of continued development and maintenance, regardless
> of the language.  Reread the paragraph above, and imagine what can happen
> when the distance between initialization and first use increases.
>
>   
 From my personal experiences, I would disagree on the robustness and
readability. If I know that someone always declares and initializes their
variables at the top of a function I know exactly where to look for them.
It may be an inconvenience for me to page up once or twice, but I know
where to look and I know the scope. If they declare them just before use,
as a reviewer I need to hunt to find them as they may be a page or two up
based on the block scope. I also need to verify that the usage of the
variable is used correctly for the scope it has been declared in.

I have spent too much time in my career helping debug other peoples errors
caused by improper declarations and initializations. Whether it be
declaring and initializing something inside a loop or in an "if"
block and trying to use it in the "else" block, etc.

Putting them at the beginning of the function eliminates that issue.
As I mentioned in my previous email, there are certain times where I
will declare things differently, but that is mostly related to language
and performance requirements.

> It's only a suggestion (directed to someone else, even) after all,
> so you're free to ignore it.
>
>   
Maybe directed at someone else but it was about code that I authored,
so it implicitly directed at me.
> The bad white space policy is also important, and likewise, is not
> prompted by some whim or idle preference.  I have seen so many cases in
> which bad white space has led to bugs (resulting from mis-resolved merge
> conflicts that should never have arisen) that it is clearly worthwhile
> to avoid the root cause.  Bad white space can even be a direct source
> of bugs in Makefiles and in languages like C and C++ (in strings and
> CPP directives), not to mention Python.
>
>   
I clearly agree that trailing whitespace can cause issues in some cases
and languages.  In those cases it needs to be dealt with.

In Ruby I would think its fairly benign except a few isolated cases. I have
yet to see that the few instances of extra whitespace at the end of line
in any of the Ruby code I have written has caused any issues with the 
execution
of the code.

Well, in honesty, the only case has been where the script sent remove the
bad whitespace was not adequately tested and instead removed the new lines,
thus creating a big one line file.  Does that count ?

However, no more bad whitespace is now a rule for ovirt and I will
abide by it.
>> Don't you
>> think oVirt has bigger issues to deal with other than trailing whitespace
>> and where I init my variables?
>>     
>
> Maybe you haven't heard about the broken windows theory?
>
>   http://tinyurl.com/5dy4fw
>     aka
>   http://www.davecheong.com/2006/06/30/broken-windows-theory-in-software-and-your-personal-life/
>   
I have heard about it and believe in some of its premises, they seem
pretty common sense to me. However I tend to focus on the testing side of
the issues, not the styling. For instance, people should test their changes
before submitting them. There are still instances where crap is
getting committed to ovirt that couldn't have passed a basic smoke test
because stuff just doesn't work after it is submitted. To me that is the
top issue facing us.

Why is the build broken?
Did the submitter and the reviewers test the changes. I believe that those
are part of the working sets of rules we should be following. So I can
apply the "Broken Windows Theory" to this and say that we need to crackdown
on both the submitter and reviewer. (however see fears below)...


BTW - Did you do enough research to see that the original premise has 
pretty
much been disproven ?
http://en.wikipedia.org/wiki/Fixing_Broken_Windows

On a personal note the main thing I dislike about this "theory" is that
the original theory was based on results achieved by massive police 
crackdowns.
Exactly what I fear in software development. That the style police will 
come
after me because of where I put my curly braces when the real issue is
does the code work.

Keep in mind that I come from a state that prints "Live Free or Die" on the
front and back of my vehicle. It is a motto I try to live by.

My personal belief is that quality needs to be designed into a product.
It can't be tested or dictated in. However, testing is still extremely
important to ensure that the quality is there.

I will continue to focus on designing and implementing extensible, well
thought out code.  I will spend my time worrying about testing the code
paths, not coding style.

As with many things, these tend to be philosophical issues bordering
on religion to some.  Using my "Live Free or Die" mantra, feel free to
focus on what you think is important, I'll continue to do the same.

-mark