Anyway, I wanted to demonstrate the problem in Scala. It did bite me today a little bit. I caught it quickly in my tests. The following two bits of code have different meanings:
private def updateOnOff = {
def calculateOnOff: boolean = {
incomingPowerSources.foreach( p => {if( p.isOn ) true; })
return false;
}
cachedOnOffBoolean = calculateOnOff;
}
private def updateOnOff = {
def calculateOnOff: boolean = {
incomingPowerSources.foreach( p => {if( p.isOn ) return true; })
return false;
}
cachedOnOffBoolean = calculateOnOff;
}
The purpose of the updateOnOff function is to simply set the cachedOnOffBoolean to true if any incoming power sources are on. If none of them are on, the boolean is set to false. The inner function, calculateOnOff is responsible for looping through the incoming power sources to see if any are on. If one is on the function should return true to the outer function which sets the boolean.
To be honest, I'm not sure which one is the local and which one isn't. I'm in the process of trying to figure it out. It seems natural to me though to assume that the first one is the local return. If anyone knows better, please let me know.
The first example is an example of local return. Local return returns from the most local block of code, and in this case its the closure function itself, not the calculateOnOff function. The closure here is what's inside the foreach call:
( p => {if( p.isOn ) true; } )
This is the function that will be returned from. It simply returns back to the loop, which goes on to the next item. Finally, when the loop is finshed, false is returned. So, in the first example false will always be returned! This is certainly not correct.
The second example is and example of non local return. Specifying "return" before false means that you intend to return from the enclosing method, not just the closure itself. You intend to break out of the loop. You've found what you were looking for, and you were done. The return returns true from the calculateOnOff, and the boolean is set to true, just as you wanted.
Its pretty straight forward when you know it but I'm sure Bloch is right, there are going to be lots of bugs. Developers who don't use unit testing religiously are going to get it. A simply copy past from a refactoring, and boom.
So what does this tell us? Well, at the least it says be careful, and write lots of unit tests. At the most it might mean that lots of idiots just stay in Java and the cool kids move on to Scala, and thats all right now baby, yeah.
You're right on which one is local vs. non-local. The rule of thumb in Scala is to avoid "return" unless you absolutely need non-local returns. So in your code I'd even get rid of the other usage of return.
ReplyDeleteJames, thanks for clearing that up.
ReplyDeleteIn this case though, I absolutely do need non-local return. I need to return true from the calculateOnOff method, not the closure. A PowerSource is on if any of its incoming power connections are on. The first case (the local return, where I didn't use "return") always returned false, which was incorrect.
Sorry, I wasn't clear. I mean "return false" could just be "false"
ReplyDeleteI wonder if there is a better functional approach to finding out whether any element is "on", than to break away from the function with a return statement. How would you do it if you were not in a context where you can just return?
ReplyDeleteI tend to do this:
val b = a filter (_.isOn)
val anyOn = b.length > 0
The bad thing is filter goes through the whole collection, which is not needed for this.
Please ignore my last comment :)
ReplyDeleteI just read the other post where somebody suggested to use find.
It's obviously the way to go.