I think I’ll start a new series, and a new blog category, called “How Not To Code”. In it, I’ll try to highlight those little things that we’ve tended to do over the years that, well, are tempting for expediency, but probably not something we should be doing. There are a lot of “Here’s the right way to do things”, so this series will concentrate on the “what not to do”. That will inevitably lead, of course, to a discussion of “What to do”, but that’s ultimately the point, I suppose.
Today’s topic is “Don’t directly passing function results as parameters to other functions and procedures”.
For instance, consider the following code:
TData = record
a, b: integer;
TMoreData = record
c, d: string
function GetSomeData(aInteger: integer; aString: string): TData;
function GetMoreData(aBoolean: Boolean; aInteger: integer): TMoreData;
procedure ProcessData(aData: TData; aMoreData: TMoreData);
Then, imagine that you wanted to call ProcessData with the result of calls to GetSomeData and GetMoreData. It would be mighty tempting to simply call:
ProcessData(GetSomeData(aIntVariable, 'ocelot'), GetMoreData(True, AnotherIntVariable));
After all, think of all the typing you’ll save!
You should avoid that temptation, because this way of doing things presents a few problems:
- It’s really hard to read.
- A line of code that does more than one thing requires more brain power to read that does code where each line does one thing. In this case, you have to convince your brain to parse all those parentheses. Before you can figure out what the code is really doing, you have to parse two function calls inside of a procedure call. Using your brain to parse unnecessarily complex semantics is a waste of effort.
- You can’t be sure by merely reading the code what the types of the parameters are that are being passed to ProcessData. This means that you have to look up the parameter types to gain full understanding. If the variables were declared locally, then it would be easy to see their types.
- It’s really hard to debug
- There are no intermediate results to check in the debugger.
- Stepping into the code requires that you step into the calls to GetSomeData and GetMoreData, when that might be something that you don’t want to have to do.
Instead of trying to compress everything into a single line of code, you probably would do something like this, instead:
TempData := GetSomeData(aIntVariable, 'ocelot');
TempMoreData := GetMoreData(True, AnotherIntVariable);
The above code solves the aforementioned problems – it is easier to parse in your head and you can easily see the data values in the debugger before they are passed to ProcessData.
Some good heuristics that fall out of this:
- If you have to stare at the code for a moment just to parse it in your head before you even figure out what is going on, then you have a problem.
- If you have a hard time figuring out where to put a breakpoint to get at the debugging information that you really want, then you have a problem.
- If you feel the need to create intermediate variables to improve your ability to debug code, then you probably should have put the intermediate variables there in the first place.
- All sorts of bad things happen when you try to be clever and save a few keystrokes by taking a code shortcut. Remember, the future maintainer is the person you should be writing code for, not the compiler.
- A line of code should do one thing and one thing only – if the explanation of what a single line of code does needs the word “and” to describe it, then it probably ought to be two lines of code.
Lesson for the Day: Always pass variables, never direct function calls, as parameters to another procedure or function.