Here are my thoughts:
Don't mutate function parameters unless there is good reason to do so.
node.depth = level;
The above statement basically breaks the contract of the function. The function claims to locate the deepestNode, but in fact it does findDeepestNodeAndSetDeepestPropOnAllNodes (s...
If the function tells the client that it's going to permanently modify (mutate) the objects, then it's OK. For example, Array#push clearly modifies the object it's called on rather than returning a new copy. In your case, the function deepestNode is similar to Array#find in that it performs a search. You wouldn't want Array#find to mess with the objects it was supposed to search through, I'd guess.
Since we kind of moved away from a strict recursion and holding state in a variable, I think we do not need to pass the currentLevel to traverse. Something like this: pastebin.com/4k5N8vVw
We can do that, but what is gained? The idea is to keep the code as tight as possible, using as few variables and conditionals as we can. If we move level outside of traverse, we extend the scope of level (similar to a global variable). The value of currentLevel in a given stack frame is hard to determine from reading the code. Parameters are good in this case, I would argue, because it's clear what level is and how it's changing from one frame to the next because it's local to each frame. Having to foo++ and then foo-- later on just to keep state correct should be a red light.
We do, but sometimes it's just more convenient to bump something up a scope. I can get away with deepest being outside of the local scope because it's very intuitive when it gets modified and the alternative of passing it in as a reference parameter or using a return value doesn't really gain a whole lot of safety since we have a nested function that has tight scope already. In your code, notice that you've had to introduce more complex conditionals and variables to make it all work, counteracting the safety. I still prefer my original, but right idea! Which code is easier to understand?
Also, check out the iterative version for comparison. In this case, all the needed state is on the explicit stack but it's not such a terrible thing that we have a "best" variable in the outer scope. I'd say the code is about as clean as the recursive version.
level is still "local" to the while block in the iterative version, but it'd be a bit silly to go and put deepest in there on the stack as well when it'd just be a reference to the outer scope anyway.
Not necessarily--it's a subjective thing, so you can make a good argument for your latest version. At that point, my biggest beef would be the lack of braces :)
Discussion between ggorlen and Koray …
Discussion between ggorlen and Koray …