[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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





Jim Meyering wrote:
mark wagner <mwagner 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


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]