lkml.org 
[lkml]   [2000]   [Aug]   [30]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH] af_rose.c: s/suser/capable/ + micro cleanups


On Wed, 30 Aug 2000, Rogier Wolff wrote:
>
> > source code smaller and more easier to read (yes, this is debatable,
> > I think it becomes more clean, other think otherwise, I'm just
> > following what Linus said he prefer).
>
> The kernel is a multi-million-lines-of-code piece of software.
> Software maintenance cost is found to correlate strongly with the
> number of lines-of-code.
>
> So, I would prefer the shorter version.

I disagree.

Number of lines is irrelevant.

The _complexity_ of lines counts.

And ?: is a complex construct, that is not always visually very easy to
parse because of the "non-local" behaviour.

That is not saying that I think you shouldn't use ?: at all. It's a
wonderful construct in many ways, and I use it all the time myself. But I
actually prefer

if (complex_test)
return complex_expression1;

return complex_expression2;

over

return (complex_test) ? complex_expression1 : complex_expression2;

because by the time you have a complex ?: thing it's just not very
readable any more.

Basically, dense lines are bad. And ?: can make for code that ends up "too
dense".

More specific example: I think

return copy_to_user(dst, src, size) ? -EFAULT : 0;

is fine and quite readable. Fits on a simple line.

However, it's getting iffy when it becomes something like

return copy_to_user(buf, page_address(page) + offset, size) ? -EFAULT: 0;

for example. The "return" is so far removed from the actual return values,
that it takes some parsing (maybe you don't even see everything on an
80-column screen, or even worse, you split up one expression over several
lines..

(Basically, I don't like multi-line expressions. Avoid stuff like

x = ...
+ ...
- ...;

unless it is _really_ simple. Similarly, some people split up their
"for ()" or "while ()" statement things - which usually is just a sign of
the loop baing badly designed in the first place. Multi-line expressions
are sometimes unavoidable, but even then it's better to try to simplify
them as much as possible. You can do it by many means

- make an inline function that has a descriptive name. It's still
complex, but now the complexity is _described_, and not mixed in with
potentially other complex actions.

- Use logical grouping. This is sometimes required especially in "if()"
statements with multiple parts (ie "if ((x || y) && !z)" can easily
become long - but you might consider just the above inline function or
#define thing).

- Use multiple statements. I personally find it much more readable to
have

if (PageTestandClearReferenced(page))
goto dispose_continue;

if (!page->buffers && page_count(page) > 1)
goto dispose_continue;

if (TryLockPage(page))
goto dispose_continue;

rather than the equivalent

if (PageTestandClearReferenced(page) ||
(!page->buffers && page_count(page) > 1) ||
TryLockPage(page))
goto dispose_continue;

regardless of any grouping issues.

Basically, lines-of-code is a completely bogus metric for _anything_.
Including maintainability.

> If it takes you a few seconds to look this over, that's fine. Even it
> the one "complicated" line take twice as long (per line) as the
> original 4 lines, then it's a win.

I disagree violently.

Linus

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
Please read the FAQ at http://www.tux.org/lkml/

\
 
 \ /
  Last update: 2005-03-22 12:38    [W:0.050 / U:9.924 seconds]
©2003-2018 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site