[asterisk-dev] Line length restrictions in code changes

[Richard Mudgett]

On Thu, Mar 16, 2017 at 2:54 PM, Matthew Jordan <mjordan at digium.com> wrote:

> Warning: bike shedding.
>
> On several code reviews, comments have been left along with a -1 on
> the review - indicating that it is not acceptable for merging - due to
> line length restrictions. While this is technically correct per our
> coding guidelines, it doesn't always result in the most readable code.
>
> Personally, I find this:
>
> if (a_good_variable_name->another_good_name &&
> do_something_to_it(a_good_variable_name->another_good_name)) {
>     // Do things
> }
>
> To be preferable to:
>
> if (a_good_variable_name->another_good_name
>     && do_something_to_it(a_good_variable_name->another_good_name)) {
>     // Do things
> }
>
> This is typically because the indentation of the second part of the
> boolean test lines up with the code that is to be executed if both
> conditions is true, which is more visually jarring than just reading
> the first line.
>
> I'll grant this is probably a personal preference, but we have
> certainly opted for the former over the latter in many places in the
> code.
>

It is done both ways in the code.

My preference is the opposite for a couple reasons:
1) Seeing && at the beginning of a line you then know that it is part of a
line
continuation because && is a binary operator and needs something to its
left.
2) On long lines you can miss seeing that there even is an operator on the
end of the line.  The line end may even be off the edge of the window.
Either
way you would not know how to interpret the expression unless you knew
what the operator is.  You are then forced to scroll the window to find out.


>
> While I know that we aren't all sitting around with dual screens, we
> also aren't living in 1979 with dumb terminals. The line length
> restriction of 90 columns seems reasonable until you end up with long
> function names, which - since we live in a land of void * - is
> generally something that we should encourage. I'd like to say that
> some common sense about when to apply the line length rule and when
> not to would "work for us", but that doesn't appear to be the case.
>
> Questions than:
> (1) Should there even be a line length rule?
>

Yes.  We live in the age of variable sized and multi-overlapping windows.
In a way this is worse than the 1979's fixed sized display because we
try to cram more on the screen.


> (2) If there is a line length, what is a reasonable length given some
> of our function names? (Looking at things like
> ast_sip_get_mwi_disable_initial_unsolicited)
>

The 90 line length is a good compromise.  You can look at side by side
differences and not have to scroll horizontally except when the line length
is too long.


> (3) Should we simply advocate for readability, with examples of what
> is readable and what is not?
>

There are ways to deal with the long identifier lengths creating very long
lines.  One of the simpler is to avoid putting assignments in conditional
test expressions.  There are better readability line breaks available in a
stand alone assignment than there are when you cram the assignment
into an if test expression.  Another is not to excessively indent parameters
to functions like python PEP8 encourages.  Function parameters tend
to get quite long themselves.  Horizontal line space is at a premium
compared to vertical space.

Richard
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20170316/56b5b6da/attachment.html>