Declaring a variable only to immediately return or throw it is a bad practice.
Some developers argue that the practice improves code readability, because it enables them to explicitly name what is being returned. However, this variable is an internal implementation detail that is not exposed to the callers of the method. The method name should be sufficient for callers to know exactly what will be returned.
Despite that first statement, writing code to be readable is never a bad practice. In fact its probably the single most important reason to write code in specific ways. Its also not the only reason to declare a local and return that local. It is useful to use a convention like this when the result that is returned is not simplistic and you need to inspect it while debugging at a point where the value is known and can be inspected prior to returning.
In the examples given for this rule (which are kind of awful and I dont think would even transpile or function correctly), a well known time calculation is given.
let returnValue = existingEventSet.add(JSON.stringify({code: targetEvent.code, parentId: targetEvent.parentId}));
return returnValue;
Having that assign to a local and return the local would make a marked improvement in the debugging experience because we can inspect and possibly modify the value before returning it.
This kind of thing should not be getting reported as a code smell (for any language). Readability should be the the most important consideration when it comes to these rules and never compared to a bad practice.
I am a bit puzzled how I should take your message. On one side it contains reasonable arguments, on another a lot of aggression and dislike.
I see and understand your point of view on this code pattern, and in SonarSource we believe it’s more readable to avoid creation of the variable here. I don’t have any wish to over-persuade you, so you can deactivate any rule you don’t like, including this one.
Be not puzzled, its a logical explanation. Any aggression perceived is perceived within your own mind. I didn’t put it there. I’m just a guy sitting at a desk scratching my head as to why someone would make a recommendation like this, say my argument is logical but then try to dismiss it despite it being a majority opinion. I can only guess that you are mistaking directness for aggression.
There arent any “sides”. I said the reasoning was incomplete and misses an important consideration.
Declaring a variable only to immediately return or throw it is a bad practice.
Some developers argue that the practice improves code readability, because it enables them to explicitly name what is being returned.
“Some developers” - who are these people? What about the other developers that werent in that group of “some”?. There is some data regarding this specific rule; take a look at this SO question and note that the most popular answer that is supporting rules like this one has only 25 upvotes but the first comment to that answer that disagrees and does not support this rule has almost 2x as many upvotes.
That means that “Most Developers include a placeholder variable like this because it helps debugging, and will be removed or mitigated by the compiler anyway” which is the reason I said was missing.
Since the readability in this case is subjective and could be either way and neither way is wrong, the rule should not exist as it is. If you take that question I linked as evidence, the opposite rule should be what is implemented.
We’re not talking about logic, but about opinions.
This topic has not been solved.
I’m not sure what are your expectations here.
As explained in the rule description and as mentioned by @Lena , at SonarSource we have a different opinion about this rule. If you think otherwise feel free to disable the rule.