Software Engineering Stack Exchange is a question and answer site for professionals, academics, and students working within the systems development life cycle. It only takes a minute to sign up.
Sign up to join this community
In a recent homework assignment I ended up calling my functions in an ugly way uglyReceipt(cashParser(cashInput())) the program itself worked perfectly but I still felt like I was doing something wrong.
Is calling functions like this bad practice and if so: What should I be doing instead?
This really depends on how much nesting you use. After all, you are allowed to use function results directly in expressions to improve readability. Both, code that does not use nested expressions (like assembler code), and code that uses too much nested expressions is hard to read. Good code tries to strike a balance in between the extremes.
So lets look at some examples. The one you gave in your question seems quite legit to me, so nothing to worry here. However, a line like
foo(bar(baz(moo, fab), bim(bam(ext, rel, woot, baz(moo, fab)), noob), bom, zak(bif)));
would definitely not be tolerable. Likewise, code like
double xsquare = x*x;
double ysquare = y*y;
double zsquare = z*z;
double xysquare = xsquare + ysquare;
double xyzsquare = xysquare + zsquare;
double length = sqrt(xyzsquare);
would not be very readable as well. sqrt(x*x + y*y + z*z) is much easier to understand, even though it combines a total of six different operation in one expression.
My advice is to pay attention to what expressions you can still parse in your head easily. The moment you need to take a second look to grasp what a single expression does, it's time to introduce an additional variable.
xsquare really offers no value over x*x (I would even say it’s worse). But in another example, say, the quadratic formula, the b^2 - 4*a*c term could reasonably be extracted and named properly as the discriminant.
Dec 28, 2021 at 1:37
map call, too. E.g. in Swift: sqrt([x, y, z].map { $0 * $0 }.reduce(0, +))
Dec 28, 2021 at 1:41
The concept underlying your question is so important I feel it needs another answer rather than just a comment (as I had started to do).
The other 3 answers thus far provide some useful points of consideration on whether a given situation merits using what you call "nested function calls". But perhaps a more important point is hidden in the comments under your question: in case you missed the subtlety in what those erudite folks are suggesting, Carl, you have discovered for yourself the topic actually called functional programming. If you have never seen the term, you might not have thought it was really a "thing" in @HighPerformanceMark's comment.
But indeed it is! Functional programming has been written about for decades, since John Hughes' seminal paper Why Functional Programming Matters. There are some languages that are functional languages (i.e. they only let you write in a functional programming style), languages like Erlang, Lisp, OCaml, or Haskell. But there are many more languages that are hybrid imperative/functional languages. That is, they are traditionally imperative languages but offer some support for functional programming as well, including Perl, C++, Java, C#, and many more. Wikipedia's entry on functional programming provides a nice section showing a comparison of functional style vs. imperative style for a number of languages.
There is much to say on the differences between imperative and functional styles, but the key starting point is that with functional programming, functions or methods have no side effects, making it in general easier to both understand and debug programs.
For further reading, you might also take a look at Reginald Braithwaite's Why "Why Functional Programming Matters" Matters and another interesting post here on SO, Why functional languages?
I think whether it's good or bad depends a lot on context. The main reason it might be considered bad is that it arguably makes the code harder to read and debug. This is especially true when you are first learning to program. As your coding skills and code gets more advanced, there are times when this is acceptable.
For example, consider an error message like this:
line 1492: missing argument "foo"
How do you know if the missing argument is to cashInput, cashParser, or uglyReceipt? Depending on the language the error message might tell you, but it might not.
If were were to break those function calls apart, and the error message still pointed you to line 1492, you would know instantly where the problem lies:
1491: input = cashInput()
1492: parsed_value = cashParser(input)
1493: receipt = uglyReceipt(parsed_value)
With the steps broken out separately, it is much easier to debug since it is possible to set a breakpoint at any step, and you can easily inject values by changing the value of the local variables.
It is absolutely not a bad practice in general. Functions call accept values and one way of producing a value is by calling another function.
When I see a variable being defined, like:
parsed_value = cashParser(input)
... I have to consider that parsed_value might be used more than once and I'll probably have to check if this is true or not (what if my change breaks something elsewhere?). When you start adding more variables, it can become more complex for the reader to keep track of all of them and how they are related. So I am relieved when I see:
receipt = uglyReceipt(cashParser(input))
... because the intermediate value's scope/lifetime is obvious. Now, as always, splitting a long expression into separate statements might help, especially if a variable name can give more precision about the purpose of a value:
user_name = input()
Some pro's and con's regarding readability and the debugging experience have been given. The most obvious one in my view is still missing though: by stringing calls together you pass on the opportunity to tell the logical story.
By assigning intermediate results to variables you express what they mean through your variable identifiers.
SendDoc(ReadFile(GetFilePath()), context.GetUsersByType(UserType.LoggedOn | UserType.Recent));
That's a lot to take in. The reader needs to parse the statement and make assumptions as to what is happening or just can't be bothered because it is too much. Apparently some document is sent...
string reportFilePath = GetFilePath;
Report report = ReadFile(reportFilePath);
Users[] activeUsers = GetUsersByType(UserType.LoggedOn | UserType.Recent)
SendDoc(report, activeUsers);
This makes it more manageable to the head: a report is read from a file and gets sent to active users.
It is also easier to modify and a change would be easier to follow. If you need an extra step that would be one or more extra lines rather than a disassembly of the one-liner or an extra insertion in it.
