Re: PATCH: Refactor script-column.cc for improved reading and fewer lines

[David Kastrup]

Carl Sorensen <address@hidden> writes:

> Following up on David's comments, I've refactored script-column.cc.
>
> I've also added a combination of Script and TextScript objects to the
> script-column, to ensure that the two priorities work together properly.
>
> Please review the code at
>
> http://codereview.appspot.com/166057

I have a very hard time picking the code apart.  default_outside_staff
and last_outside_staff are used somewhat interchangeably, likely one of
the two should go.

What effects on the priorities does the
Side_position_interface::add_support function have, if any?

Could you describe in simple words what the behavior is supposed to
achieve?  If you do that, I promise to submit some simple code that does
that.

I've unwrapped a bit of code of the form

for (a;b;c) {
  if (condition)
    short something
  else {
    Large something
  }
  z
}

into

for (a;b;z,c) {
  if (condition) {
    short something
    continue;
  }
  Large something
}

which helps a bit keeping track of what happens where.  But the flurry
of variables and tests used does not really make a picture that is
coherent enough for me to be sure I get the idea behind the code.

If you could list the cases that should be handled and the exceptions,
that would help.  After all, this should likely boil down to something
which can be explained in a few sentences rather than "Some magic
involving the following properties happens".  So that people have a
chance to manipulate the properties and get predictable results.

Thanks

-- 
David Kastrup