Store returned values in variables


Link to this posting

Postby Ursego » 21 Feb 2013, 11:31

Don't directly use values, returned by expressions and user-defined subroutines, as an input of other expression and subroutines; instead, store them in interim variables, and use these variables in the rest of the method.

There are at least two reasons to do that:

1. In nested expressions, breaking one complicated line into a few simpler and clearer lines makes code easier to understand.

2. It simplifies debugging - you can see the returned value immediately, before it is "buried" somewhere, or even cannot be checked at all (like if (age < getMinAllowedAge())), forcing you to STEP IN to see what the function returns.

These two reasons dictate to store the return value of a function in a variable always - even if the function is called only once (if more than once, then no questions at all - that will eliminate code duplication!). The only exception from this rule - a well-named Boolean function which is called only once in the method and is used directly in an if statement: in this case we can understand the returned value - true or false - looking in the debugger if the program flow goes into the if or skips it (or goes into the else section if present). Ensure the called function doesn't return null which is treated as false by the program flow (see Don't return null from Boolean functions).

If you need to call a same deterministic function (i.e. giving always the same output for the same input in the given context) more than once in order to re-use its return value in different fragments, then there are additional advantages of storing the returned value in a variable:

1. Better performance (the function is run only once), especially if the function is not extremely efficient (but even an efficient function can start calling a stored procedure with a heavy SQL SELECT tomorrow).

From the book "97 Things Every Programmer Should Know":

A big bank with many branch offices complained that the new computers it had bought for the tellers were too slow. This was in the time before everyone used electronic banking, and ATMs were not as widespread as they are now. People would visit the bank far more often, and the slow computers were making the people queue up. Consequently, the bank threatened to break its contract with the vendor.

The vendor sent a performance analysis and tuning specialist to determine the cause of the delays. He soon found one specific program running on the terminal that consumed almost all the CPU capacity. Using a profiling tool, he zoomed in on the program and he could see the function that was the culprit.
The source code read:

Code: Select all
for (i = 0; i < strlen(s); ++i) {
   if (... s[i] ...) ...
}

And string s was, on average, thousands of characters long. The code (written by the bank) was quickly changed, and the bank tellers lived happily ever
after...

Shouldn't the programmer have done better than to use code that needlessly scaled quadratically?

Each call to strlen traversed every one of the many thousand characters in the string to find its terminating null character. The string, however, never
changed. By determining its length in advance, the programmer could have saved thousands of calls to strlen (and millions of loop executions):

Code: Select all
n = strlen(s);
for (i = 0; i < n; ++i) {
   if (... s[i] ...) ...
}


2. Signals that the called function is deterministic (otherwise developers can think it returns different values on each call).

So, call the function only once, and use the returned value in the rest of the method as many times as you need. This advice is especially important when the result of a function is used as a stop condition in a loop!

The rule to store returned values in variables naturally brings us to the idea to avoid complex nested expressions:

Try not to write more than one executable statement in one line of code, especially if you call developer-defined methods, nested in each other.

Good developers break one long, complicated method into a number of short, simple sub-methods. The same philosophy works for complex, super-nested expressions: we should break one long, complicated line of code into a number of short, simple lines. That will make the code longer, but it's better than an annoying and time-consuming attempt to solve an intricate puzzle (not always successful :lol: ).

*** BAD code: ***

Code: Select all
countryName = this.getCountryName(this.getCountryId(this.getCityId(rowNum)));

*** GOOD code: ***

Code: Select all
cityId = this.getCityId(rowNum);
countryId = this.getCountryId(cityId);
countryName = this.getCountryName(countryId);

The last example demonstrates how this approach simplifies debugging (in addition to better readability!): if the variable countryName has not been populated as expected, then you can see in the debugger (without STEP IN) which step exactly fails. In the nested version, if you want to STEP IN getCountryName to debug it then you are forced firstly to STEP IN (and STEP OUT from) each one of the inner methods, beginning from the most nested getCityId.

Pay attention...

I wrote "Don't directly use values, returned by expressions and user-defined subroutines...". I meant functions, written by you or your teammates. Of course, you can apply this rule always, but it doesn't make sense to use variables to accommodate values, returned from built-in/framework functions, which are tested million times and [almost :lol: ] 100% predictable - that will make code longer bringing virtually no benefits. The following lines of code are absolute legitimate (I just copy-pasted them from the Java project I am developing right now):

Code: Select all
InputStream stream = getClass().getClassLoader().getResourceAsStream(FILE_NAME);
...
ZCRMModule module = ZCRMRestClient.getInstance().getModuleInstance(moduleName);
...
String brokerName = excelRow.getCell(EXCEL_CELL_NUM__BROKER_NAME).getStringCellValue();
...
if (excelRow.getCell(0).getStringCellValue().trim().isEmpty()) return;
...
MainApplication.getLogger().info("Converted " + recordsArrayList.size() + " rows from sheet " + sheet.getSheetName());
User avatar
Ursego
Site Admin
 
Posts: 143
Joined: 19 Feb 2013, 20:33



Ketones are a more high-octane fuel for your brain than glucose. Become a biohacker and upgrade yourself to version 2.0!



cron
Traffic Counter

eXTReMe Tracker