Make your code more maintainable: The evils of the Return statement
What does it mean to make code more maintainable? Certainly obfuscated code is hard to understand, by definition.
A big part of maintainability is making it easier for others to read and understand what the code is doing. Your code may have been working for years, but then somebody comes along and wants to add a feature, which might break your code.
Sometimes the behavior of code isn’t quite right, so you might need to investigate somebody else’s code.
(The repro scenario of the issue might be quite complicated, involving several tedious minutes of (perhaps ambiguous) keystrokes and clicks.
You might be single stepping some code, and suddenly you find yourself past the point where something seems to have gone wrong. <Sigh>. You have to restart the entire scenario, putting a breakpoint before the offending code is executed. Or do you? Set Next Statement is your friend. Often you can use Set Next Statement to go back a bit. You can perhaps modify some local variables and try to execute the code again, without having to repeat the tedious repro scenario.)
The Return statement exists in many languages, including C#, VB, FoxPro, C++. This discussion pertains to all of them.
Each of these languages allow you to create a very complex expression, perhaps with inline conditionals, precede it with the keyword “Return” and the value of the expression is the return value of the current method.
Suppose you had “simple” functions Foo and Bar:
int Foo(someparam)
{
return Bar(someparam)->SomeMethod(SomeOtherParam);
}
int Bar(someparam)
{
<...some code...>
return AnotherFoo(SomeFunc(SomeOtherValue));
}
Another return statement sample with 20 calls in it:
return (HasFlag(Flags, GetSomeVal(Flags)) &&
!HasFlag32(Result, SXF_LPE) &&
!(IsSomeValPointer(Result) &&
// Rule out Pointers to values of types.
!(Result->AsSomeValPointerExpression().Left->type == type_WRD &&
Result->AsSomeValPointerExpression().Left->AsWordPointerExpression().BasePointer &&
!HasFlag32(Result->AsSomeValPointerExpression().Left->AsWordPointerExpression().BasePointer, SXF_LWORD) &&
ValHelpers::IsValue(Result->AsSomeValPointerExpression().Left->AsWordPointerExpression().BasePointer->ResultType)&&
// make sure SomeSomeVal() still works for value types(bug 32804)
!(Result->AsSomeValPointerExpression().Left->AsWordPointerExpression().BasePointer->type == type_SYM &&
Result->AsSomeValPointerExpression().Left->AsWordPointerExpression().BasePointer->AsWordPointerExpression().Word->PVariable()->IsGood()))))
Suppose you’re debugging a program and single stepping and reach the end of Bar(). Even after stepping over this statement, you can’t easily see the value (or intermediate values) that’s being returned. You can’t even see the type of the return value unless you can see the method signature on the screen: “Function MyMethod() As SomeType”
The Autos window does show the return value/type for native code, (but not VB or C#), but I find that the other info it displays is useless. It tries to guess the nearby values that you might want to see: I much prefer the Locals window. It would be nice if the return value from the Autos window were displayed in the Locals window. So I’ve resorted to showing just the top couple of lines of the Auto window under my Locals window. However, the Autos window doesn’t allow you to change a returned value, whereas the Locals window does.
Also, seeing intermediate values by hovering your mouse works for local values, but not for method calls.
(If you’re debugging C++ code, then if you open the Debug->Windows->Registers window, you can see and change simple return values like Bools and Integers in the EAX register (32 bit) or RAX (64 bit), but watch out for implied destructor calls)
So you step out of the function Bar(), then you’re on another complex return statement in Foo(), where you still can’t see the returned value. So you step out again, and yet another Return statement. You might need to step out a few levels before you can see what the value is. If you want to reexamine this path, you can Set Next Statement back, but then you’re at a call stack level that’s far from the original code, so you have to go drill back down the callstack.
Now suppose you think the return value of Bar() isn’t quite right, or you want to reexamine what is happening.
Right click and choose “Step Into Specific” ,which might list 20 different functions into which you could step. However, depending on conditionals and prior values, not all of the 20 may be called. So you Step into the first, see that it’s uninteresting, Step into the Next, etc. until you see something interesting.
(look for a future blog post on Just My Code for native debugging that will remove a lot of the uninteresting Step Intos., like library or template functions or operator overloads)
So we’ve discussed two maintainability issues:
· make the return value of your function more visible while debugging, so the user can verify functionality more easily and can use Set Next Statement.
· Break up complex expressions, especially in Return statements.
The simple solution to both of these issues: Introduce new local variables to have intermediate results and break up nested conditional clauses to be separate If statements.
· Set Next Statement and Breakpoint granularity is finer.
· Intermediate results can be seen and modified in the Locals window
· Mouse hover shows the values
· The local variable names can be further indications of intent.
Simply changing the code from:
Return <ComplexExpr>
To
Int RetVal = <ComplexExpr>
Return RetVal
will make a huge difference in this debugging scenario. The user can Step Over and the RetVal can be seen in the debugger and, if it’s wrong, the user can Set Next Statement back and reexecute the code, Stepping In, rather than Stepping Over (watch out for side effects of reexecution). The user stays within the method without having to back up a few levels of the call stack. Also, it’s easier to see the type of the return value.
int Foo(someparam)
{
SomeType Barval = Bar(somparam);
Int RetVal = Barval->SomeMethod(SomeOtherParam);
return Retval;
}
int Bar(someparam)
{
<...some code...>
SomeFuncType SomeFuncVal = SomeFunc(SomeOtherValue));
Int Retval = AnotherFoo(SomeFuncVal);
return Retval;
}
Modern optimizing compilers can optimize out those variables and can generate identical code.
I once heard of a chess program written in one (very long) line of code, but imagine trying to maintain that puppy!