57

My coding style for nested function calls is the following:

var result_h1 = H1(b1);
var result_h2 = H2(b2);
var result_g1 = G1(result_h1, result_h2);
var result_g2 = G2(c1);
var a = F(result_g1, result_g2);

I have recently changed to a department where the following coding style is very much in use:

var a = F(G1(H1(b1), H2(b2)), G2(c1));

The result of my way of coding is that, in case of a crashing function, Visual Studio can open the corresponding dump and indicate the line where the problem occurs (I'm especially concerned about access violations).

I fear that, in case of a crash due to the same problem programmed in the first way, I won't be able to know which function has caused the crash.

On the other hand, the more processing you put on a line, the more logic you get on one page, which enhances readability.

Is my fear correct or am I missing something, and in general, which is preferred in a commercial environment? Readability or maintainability?

I don't know if it's relevant, but we are working in C++ (STL) / C#.

  • 17
    @gnat: you refer to a general question, while I am especially interested in the mentioned case of nested function calls and the consequence in case of crash dump analysis, but thanks for the link, it contains quite some interesting information. – Dominique Feb 22 '18 at 11:34
  • 9
    Note that if this example were to be applied to C++ (as you mention this being used in your project) then this is not only a question of style, as the order of evaluation of the HX and GX invocations may change in the one-liner, as the order of evaluation of function arguments is unspecified. If you for some reason depend on the order of side effects (knowingly or unknowingly) in the invocations, this ”style refactoring” could end up effecting more than just readability/maintenance. – dfri Feb 22 '18 at 22:00
  • 4
    Is the variable name result_g1 what you'd actually use or does this value actually represent something with a sensible name; e.g. percentageIncreasePerSecond. That would actually be my test to decide between the two – Richard Tingle Feb 22 '18 at 22:50
  • 3
    Regardless of your feelings on coding style, you should follow the convention that's already in place unless its clearly wrong (it doesnt seem it is in this case). – n00b Feb 22 '18 at 22:55
  • 4
    @t3chb0t You're free to vote however you like, but please be aware in the interest of encouraging good, useful, on-topic questions on this site (and discouraging bad ones), that the purpose of up or down voting a question is to indicate whether a question is useful and clear, so voting for other reasons such as using a vote as a means to provide criticism on some example code posted to aid the context of the question generally isn't helpful in maintaining the quality of the site: softwareengineering.stackexchange.com/help/privileges/vote-down – Ben Cottrell Feb 23 '18 at 16:34
111

If you felt compelled to expand a one liner like

 a = F(G1(H1(b1), H2(b2)), G2(c1));

I wouldn't blame you. That's not only hard to read, it's hard to debug.

Why?

  1. It's dense
  2. Some debuggers will only highlight the whole thing at once
  3. It's free of descriptive names

If you expand it with intermediate results you get

 var result_h1 = H1(b1);
 var result_h2 = H2(b2);
 var result_g1 = G1(result_h1, result_h2);
 var result_g2 = G2(c1);
 var a = F(result_g1, result_g2);

and it's still hard to read. Why? It solves two of the problems and introduces a fourth:

  1. It's dense
  2. Some debuggers will only highlight the whole thing at once
  3. It's free of descriptive names
  4. It's cluttered with non-descriptive names

If you expand it with names that add new, good, semantic meaning, even better! A good name helps me understand.

 var temperature = H1(b1);
 var humidity = H2(b2);
 var precipitation = G1(temperature, humidity);
 var dewPoint = G2(c1);
 var forecast = F(precipitation, dewPoint);

Now at least this tells a story. It fixes the problems and is clearly better than anything else offered here but it requires you to come up with the names.

If you do it with meaningless names like result_this and result_that because you simply can't think of good names then I'd really prefer you spare us the meaningless name clutter and expand it using some good old whitespace:

int a = 
    F(
        G1(
            H1(b1), 
            H2(b2)
        ), 
        G2(c1)
    )
;

It's just as readable, if not more so, than the one with the meaningless result names (not that these function names are that great).

  1. It's dense
  2. Some debuggers will only highlight the whole thing at once
  3. It's free of descriptive names
  4. It's cluttered with non-descriptive names

When you can't think of good names, that's as good as it gets.

For some reason debuggers love new lines so you should find that debugging this isn't difficult:

enter image description here

If that's not enough, imagine G2() was called in more than one place and then this happened:

Exception in thread "main" java.lang.NullPointerException
    at composition.Example.G2(Example.java:34)
    at composition.Example.main(Example.java:18)

I think it's nice that since each G2() call would be on it's own line, this style takes you directly to the offending call in main.

So please don't use problems 1 and 2 as an excuse to stick us with problem 4. Use good names when you can think of them. Avoid meaningless names when you can't.

Lightness Races in Orbit's comment correctly points out that these functions are artificial and have dead poor names themselves. So here's an example of applying this style to some code from the wild:

var user = db.t_ST_User.Where(_user => string.Compare(domain,  
_user.domainName.Trim(), StringComparison.OrdinalIgnoreCase) == 0)
.Where(_user => string.Compare(samAccountName, _user.samAccountName.Trim(), 
StringComparison.OrdinalIgnoreCase) == 0).Where(_user => _user.deleted == false)
.FirstOrDefault();

I hate looking at that stream of noise, even when word wrapping isn't needed. Here's how it looks under this style:

var user = db
    .t_ST_User
    .Where(
        _user => string.Compare(
            domain, 
            _user.domainName.Trim(), 
            StringComparison.OrdinalIgnoreCase
        ) == 0
    )
    .Where(
        _user => string.Compare(
            samAccountName, 
            _user.samAccountName.Trim(), 
            StringComparison.OrdinalIgnoreCase
        ) == 0
    )
    .Where(_user => _user.deleted == false)
    .FirstOrDefault()
;

As you can see, I've found this style works well with the functional code that's moving into the object oriented space. If you can come up with good names to do that in intermediate style then more power to you. Until then I'm using this. But in any case, please, find some way to avoid meaningless result names. They make my eyes hurt.

  • 20
    @Steve and I'm not telling you not to. I'm begging for a meaningful name. All to often I've seen the intermediate style done mindlessly. Bad names burn up my brain far more than sparse code per line does.I don't let width or length considerations motivate me to make my code dense or my names short. I let them motivate me to decompose more. If good names just ain't gonna happen consider this work around to avoid meaningless noise. – candied_orange Feb 22 '18 at 15:02
  • 6
    I add to your post: I've got a little rule of thumb: If you can't name it, it might be a sign that it's not well-defined. I use it on entities, properties, variables, modules, menus, helper classes, methods, etc. In numerous situations this tiny rule has revealed a serious flaw in design. So in a way, good naming not only contributes to readability and maintainability, it helps you verify the design. Of course there are exceptions to every simple rule. – Alireza Feb 24 '18 at 9:10
  • 4
    The expanded version looks ugly. There's too much w h i t e s p a c e there which r e d u c e s i t s e f f e c t i v e n e s s s i n c e e v e r y t h i n g i s e m p h a s i z e d w i t h i t, m e a n i n g n o t h i n g i s. – Mateen Ulhaq Feb 24 '18 at 10:51
  • 5
    @MateenUlhaq The only extra whitespace there is a couple of newlines and some indentation, and it's all carefully placed at meaningful boundaries. Your comment instead places whitespace at non-meaningful boundaries. I suggest you take a slightly closer and more open look. – jpmc26 Feb 25 '18 at 23:52
  • 3
    Unlike @MateenUlhaq I'm on the fence about the whitespace in this particular example with such function names, but with real function names (which are more than two characters in length, right?) it could be what I'd go for. – Lightness Races in Orbit Feb 26 '18 at 2:00
50

On the other hand, the more processing you put on a line, the more logic you get on one page, which enhances readability.

I utterly disagree with this. Just looking at your two code examples calls this out as incorrect:

var a = F(G1(H1(b1), H2(b2)), G2(c1));

is heard to read. "Readability" does not mean information density; it means "easy to read, understand and maintain".

Sometimes, the code is simple and it makes sense to use a single line. Other times, doing so just makes it harder to read, for no obvious benefit beyond cramming more onto one line.

However, I'd also call you out on claiming that "easy to diagnose crashes" means code is easy to maintain. Code that doesn't crash is far easier to maintain. "Easy to maintain" is achieved primarily via the code been easy to read and understand, backed up with a good set of automated tests.

So if you are turning a single expression into a multi-line one with many variables just because your code often crashes and you need better debug information, then stop doing that and make the code more robust instead. You should prefer writing code that doesn't need debugging over code that's easy to debug.

  • 37
    While I agree that F(G1(H1(b1), H2(b2)), G2(c1)) is hard to read, this has nothing to do with being crammed too dense. (Not sure if you meant to say that, but it could be interpreted this way.) Nesting three or four functions in a single line can be perfectly readable, in particular if some of the functions are simple infix operators. It is the nondescriptive names that are the problem here, but that problem is even worse in the multi-line version, where yet more nondescriptive names are introduced. Adding only boilerplate almost never aids readability. – leftaroundabout Feb 22 '18 at 14:43
  • 23
    @leftaroundabout: To me, the difficulty is that it's not obvious whether G1 takes 3 parameters or only 2 and G2 is another parameter to F. I have to squint and count the parentheses. – Matthieu M. Feb 22 '18 at 15:26
  • 4
    @MatthieuM. this can be an issue, though if the functions are well-known it is often obvious which takes how many arguments. Specifically, as I said, for infix functions it's immediately clear that they take two arguments. (Also, the parenthesized-tuples syntax most languages use exacerbates this problem; in a language that prefers Currying it's automatically clearer: F (G1 (H1 b1) (H2 b2)) (G2 c1).) – leftaroundabout Feb 22 '18 at 15:39
  • 5
    Personally I prefer the more compact form, as long as there's styling around it like in my prior comment, because it guarantees less state to mentally keep track of - result_h1 cannot be reused if it doesn't exist, and the plumbing between the 4 variables is obvious. – Izkata Feb 22 '18 at 16:09
  • 8
    I have found that code that's easy to debug generally is code that doesn't need debugging. – Rob K Feb 22 '18 at 20:57
25

Your first example, the single-assignment-form, is unreadable because the chosen names are utterly meaningless. That might be an artifact of trying not to disclose internal information on your part, the true code might be fine in that respect, we cannot say. Anyway, it's long-winded due to extremely low information-density, which does not generally lend itself to easy understanding.

Your second example is condensed to an absurd degree. If the functions had useful names, that might be fine and well readable because there isn't too much of it, but as-is it's confusing in the other direction.

After introducing meaningful names, you might look whether one of the forms seems natural, or if there's a golden middle to shoot for.

Now that you have readable code, most bugs will be obvious, and the others at least have a harder time hiding from you.

17

As always, when it comes to readability, failure is in the extremes. You can take any good programming advice, turn it into a religious rule, and use it to produce utterly unreadable code. (If you don't believe me on this, check out these two IOCCC winners borsanyi and goren and take a look at how differently they use functions to render the code utterly unreadable. Hint: Borsanyi uses exactly one function, goren much, much more...)

In your case, the two extremes are 1) using single expression statements only, and 2) joining everything into big, terse, and complex statements. Either approach taken to the extreme renders your code unreadable.

Your task, as a programmer, is to strike a balance. For every statement you write, it is your task to answer the question: "Is this statement easy to grasp, and does it serve to make my function readable?"


The point is, there is no single measurable statement complexity that can decide, what is good to be included in a single statement. Take for example the line:

double d = sqrt(square(x1 - x0) + square(y1 - y0));

This is quite a complex statement, but any programmer worth their salt should be able to immediately grasp what this does. It is a quite well known pattern. As such, it is much more readable than the equivalent

double dx = x1 - x0;
double dy = y1 - y0;
double dxSquare = square(dx);
double dySquare = square(dy);
double dSquare = dxSquare + dySquare;
double d = sqrt(dSquare);

which breaks the well known pattern into a seemingly meaningless number of simple steps. However, the statement from your question

var a = F(G1(H1(b1), H2(b2)), G2(c1));

seems overly complicated to me, even though it's one operation less than distance calculation. Of course, that's a direct consequence of me not knowing anything about F(), G1(), G2(), H1(), or H2(). I might decide differently if I knew more about them. But that's precisely the problem: The advisable complexity of a statement strongly depends on the context, and on the operations involved. And you, as a programmer, are the one who must take a look at this context, and decide what to include into a single statement. If you care about readability, you cannot offload this responsibility to some static rule.

14

@Dominique, I think in your question's analysis, you're making the mistake that "readability" and "maintainability" are two separate things.

Is it possible to have code that is maintainable but unreadable? Conversely, if code is extremely readable, why would it become unmaintainable on account of being readable? I've never heard of any programmer who played these factors off against each other, having to choose one or the other!

In terms of deciding whether to use intermediate variables for nested function calls, in the case of 3 variables given, calls to 5 separate functions, and some calls nested 3 deep, I would tend towards using at least some intermediate variables to break that down, as you have done.

But I certainly do not go as far as to say function calls must never be nested at all. It's a question of judgment in the circumstances.

I would say the following points bear on the judgment:

  1. If the called functions represent standard mathematical operations, they're more capable of being nested than functions which represent some obscure domain logic whose results are unpredictable and cannot necessarily be evaluated mentally by the reader.

  2. A function with a single parameter is more capable of participating in a nest (either as an inner or outer function) than a function with multiple parameters. Mixing functions of different arities at different nesting levels is prone to leave code looking like a pig's ear.

  3. A nest of functions which programmers are accustomed to seeing expressed in a particular way - perhaps because it represents a standard mathematical technique or equation, which has a standard implementation - may be more difficult to read and verify if it is broken down into intermediate variables.

  4. A small nest of function calls that performs simple functionality and is already clear to read, and is then broken down excessively and atomised, is capable of being more difficult to read than one that was not broken down at all.

  • 3
    +1 to "Is it possible to have code that is maintainable but unreadable?". That was my first thought, too. – RonJohn Feb 22 '18 at 16:24
4

Both are suboptimal. Consider Comments.

// Calculating torque according to Newton/Dominique, 4th ed. pg 235
var a = F(G1(H1(b1), H2(b2)), G2(c1));

Or specific functions rather than general ones:

var a = Torque_NewtonDominique(b1,b2,c1);

When deciding which results to spell out, keep in mind cost (copy vs reference, l-value vs r-value), readability, and risk, individually for each statement.

For example, there's no added value from moving simple unit/type conversions onto their own lines, because these are easy to read and extremely unlikely to fail:

var radians = ExtractAngle(c1.Normalize())
var a = Torque(b1.ToNewton(),b2.ToMeters(),radians);

Regarding your worry of analyzing crash dumps, input validation is usually far more important - the actual crash is quite likely to happen inside these functions rather than the line calling them, and even if not, you don't usually need to be told exactly where things blew up. It's way more important to know where things started to fall apart, than it is to know where they finally blew up, which is what input validation catches.

  • Re the cost of passing an arg: There are two rules of optimization. 1) Don’t. 2) (for experts only) Don’t yet. – RubberDuck Mar 1 '18 at 1:28
1

Readability is the major portion of maintainability. Doubt me? Pick a large project in a language you don't know (perferably both the programming language and the language of the programmers), and see how you'd go about refactoring it...

I would put readability as somewhere between 80 and 90 of maintainability. The other 10-20 percent is how amenable it is to refactoring.

That said, you effectively pass in 2 variables to your final function (F). Those 2 variables are created using 3 other variables. You'd have been better off passing b1, b2 and c1 into F, if F already exist, then create D that does the composition for F and returns the result. At that point it's just a matter of giving D a good name, and it won't matter which style you use.

On a related not, you say that more logic on the page aids readability. That is incorrect, the metric isn't the page, it is the method, and the LESS logic a method contains, the more readable it is.

Readable means that the programmer can hold the logic (input, output, and algorithm) in their head. The more it does, the LESS a programmer can understand it. Read up on cyclomatic complexity.

  • 1
    I agree with all what you say about readability. But I don't agree that cracking a logical operation into separate methods, necessarily makes it more readable than cracking it into separate lines (both techniques which can, when overused, make simple logic less readable, and make the entire program more cluttered) - if you crack things too far into methods, you end up emulating assembly language macros and lose sight of how they integrate as a whole. Also, in this separate method, you would still be faced with the same problem: nest the calls, or crack them into intermediate variables. – Steve Feb 23 '18 at 13:01
  • @Steve: I didn't say to always do it, but if you are thinking about using 5 lines to get a single value, there's a good chance that a function would be better. As for the multiple lines vs complex line: if it's a function with a good name both will work equally well. – jmoreno Feb 24 '18 at 20:46
1

Regardless if you are in C# or C++, as long as you are in a debug build, a possible solution is wrapping the functions

var a = F(G1(H1(b1), H2(b2)), G2(c1));

You can write oneline expression, and still get pointed where the problem is simply by looking at the stack trace.

returnType F( params)
{
    returnType RealF( params);
}

Of course, if case you call the same function multiple times in the same line you cannot know which function, however you can still identify it:

  • Looking at function parameters
  • If parameters are identical and the function does not have side effects then two identical calls become 2 identical calls etc.

This is not a silver bullet, but a not so bad half-way.

Not to mention that wrapping group of functions can even be more beneficial for readability of the code:

type CallingGBecauseFTheorem( T b1, C b2)
{
     return G1( H1( b1), H2( b2));
}

var a = F( CallingGBecauseFTheorem( b1,b2), G2( c1));
1

In my opinion, self-documenting code is better for both maintainability and readability, regardless of language.

The statement given above is dense, but "self documenting":

double d = sqrt(square(x1 - x0) + square(y1 - y0));

When broken into stages (easier for testing, surely) loses all context as stated above:

double dx = x1 - x0;
double dy = y1 - y0;
double dxSquare = square(dx);
double dySquare = square(dy);
double dSquare = dxSquare + dySquare;
double d = sqrt(dSquare);

And obviously using variable and function names that state their purpose clearly is invaluable.

Even "if" blocks can be good or bad in self-documenting. This is bad because you cannot easily force the first 2 conditions to test the third ... all are unrelated:

if (Bill is the boss) && (i == 3) && (the carnival is next weekend)

This one makes more "collective" sense and is easier to create test conditions:

if (iRowCount == 2) || (iRowCount == 50) || (iRowCount > 100)

And this statement is just a random string of characters, viewed from a self-documenting perspective:

var a = F(G1(H1(b1), H2(b2)), G2(c1));

Looking at the above statement, maintainability is still a major challenge if functions H1 and H2 both alter the same "system state variables" instead of being unified into a single "H" function, because someone eventually will alter H1 without even thinking there is an H2 function to look at and could break H2.

I believe good code design is very challenging because there re no hard-and-fast rules that can be systematically detected and enforced.

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.