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

[Jim Meyering]

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.

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

It's only a suggestion (directed to someone else, even) after all,
so you're free to ignore it.

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.

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