Messages in this thread |  | | | Date | Wed, 30 Aug 2000 10:04:12 -0700 (PDT) | | From | Linus Torvalds <> | | Subject | Re: [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/
|  |