Why is assigning something to a variable then returning it a code smell?

Simple example:

double makeNumber(String[] numbers)
{
StringBuilder sb = new StringBuilder();
for (String n : numbers)
sb.append(Double.parse(number);

double result = Double.parse(sb.toString());
return result;
}

Why is this bad?

double result = Double.parse(sb.toString());

imagine is a more complicated assignment or calls another function that does some other weird stuff?

If i'm going to debug an issue, i m going to have to unwrap it anyway. So what's the cost of leaving it there?

error on line 5... lol

@xewl hm ok.. it's actually pseudocode

@billgates it caught my eye, wasn't meant to be degrading :p

@irene yes but its a code smell and our company-wide code analysis system doesn't like it. I'm just going, what's the difference?

It's not like the assignment requires copying all the data again like if you were resizing an array.

Not very smelly but you could save yourself some typing by just returning the value, unless the variable name acts as comment/documentation.

In this case is say it's redundant, just as redundant as writing "//here we return the result" - it's kinda implicit already.

I agree that the var is nice when debugging, but I wouldn't leave debug variables in production code.

If this was a code review I don't think I'd mention it tbh.

@ihatecomputers but what about all the people that write those 1 like commands that do 5 different things to get a result.

I guess sorta like functional programming. How do you debug a(b(c(d(x))) and figure out which step and what input it's crashing on?

@billgates I think the point-free style in FP is gorgeous 😍

If I had to debug

return pipe(
foo,
bar,
...,
input)

I'd (I think... I'm kinda new to this) put a break point inside of foo/bar or put a logging function between foo and bar so that I could put breakpoints there and inspect the arguments and then remove the debug func when I'm done.

double result = Double.parse(sb.toString());
double sandwich = result;
double tuna = sandwich;

return result;

you did the same thing, just on a lower scale. Don't write code noone needs. These are lines you could easily avoid. And less lines == shorter methods. And short methods == cleaner code

@ihatecomputers but I think having the state after each step is called makes debugging easier. Once a function exits you lose the state. And well since it's in one line I either need to debug all the functions that get called as they get called just to get their individual results.

Whereas if it were written like a = x(q)
b = y(a)
c = z(b)

return c

Now I can step in/over and also rewind (and even hot edit) without actually having to run everything again or step through everyone of the functions.

@billgates why don't you just check what the value is in the calling method itself?

I'm assuming it's Java/C++ here, so most debuggers show what was the value that was returned by a function call (provided not a void return type). So you can avoid a storing a new local reference, while still being able to debug with relatively the same amount of ease/pain (depending on your situation with the thing).

@mahaDev return a(b(c(d(x)))

throws an exception, now what function actually threw the exception and what was the input that caused it?

@billgates

Exception handling, enter stage left, cup of wine in hand.

If it's Java, it's relatively easy to figure out through the stack trace. C++ will require more work, but it will make the code more beautiful. (Personal opinion)

Okay what's the company's take on putting debug loggers? If nothing, you can use them as buffers between the line that's causing the code smell and the return and the smell will vanish.

@mahaDev exception does not store the state and if the exception was in a() then you have to run b, c, d, again to find out what went into it and even walk thru a(). And to do that you're going to have to rewrite the code to assign the values anyway.. So bother combining it into a one line call in the first place?

Okay the scenario that you've explained is something that people generally do for the exact reason you've mentioned and to keep the code clean.

Even otherwise, adding a few debug loggers and stepping into/over at the right place (because you know what can blow up where) can help when debugging.

Anyhow, there must have been a reason for setting up the rules (or atleast guidelines) behind for code quality check. And since stupid merge requests are rejected (at least in my company) if the quality check fails, I usually follow them with a few options open for debugging (loggers, semi redundant error checks and handling).

@mahaDev yes so hence the question. Because right now I see no major cost to assigning it to a variable. it's just a address pointer (a a few bytes in memory?)

So why is it bad?

@billgates maybe on a very large scale application which has like a few dozen modules all loaded at once, a couple tens of thousands of such small assignments and method invocations can take up a lot space when you're already trying to reduce the amount of resources you want your application?

Just spit balling, but I don't see any other reason for them to put up such rules.

But yeah, variables make code more readable. Trade off, I guess.

@irene local variables have lifespan of the function call stack, agreed. They get cleared out, agreed. For me, Majority of my variables are passed by reference because my teacher once told me it's unwise to return values because well, Java.

Besides, fucking GC time in my application is quite high at times as it is, so I'd rather not increase such factors even more.

@irene that's heap memory. References are stored on stack, which is small and fixed.

And why reserve precious space when you're not using it anyway ? It'll have to get cleaned up later anyway, without being used. Except for being used for a debug in this case. Instead, print thread stack, trace logs containing object info or take heap dump (on exception)

@irene hang on.. I'm confused now.

Functions return the reference, not the object itself.

If you don't assign the object reference to a variable and directly return it (as specified by the code smell rule) you'll not store the reference on the stack (for the local variable), but only once, in the calling function (provided you actually store it there as well)

So that just reduced the storage/removal of the reference from the stack once. In a long running app, or in a huge iteration loop, I think, at the least, it makes a dent.

@irene but denying this fact. Just saying that assignment to a (local) variable also stores on stack.

@irene I'm not talking about the object on the heap. A variable, which is a store for the reference to the object is kept on stack. Creating more variables (especially unused ones, like in the context of this post) takes up more space on the stack. This should be avoided.

I do think we're saying the same thing, in different ways.

@irene oh fuck yeah, how did that slip my mind? 🤦‍♂️

Now I want a reasoning for the rule which @billgates mentioned.