Conversation
Little bit of refactoring to simplify the flow. - No need for an else branch when the if branch ends with a return - path length is compared to 0 at the beginning of the function, no need to check it's not 0 at the end
findContext() is really doing two jobs, so renamed to describe what else it's doing. It's probably best to split into multiple functions, but this seemed simplest for now.
tkurki
left a comment
tkurki
left a comment
There was a problem hiding this comment.
All in all a really nice job! I've done the same a few times in open source projects, documenting how an algorithm works just to wrap my head around it. Nice to be on the receiving end!
Would you mind addressing my minor comments?
| }, this.sources) | ||
|
|
||
| // Uh, shouldn't something be done with the result of the reduce? What if | ||
| // the pointed to value isn't found? |
There was a problem hiding this comment.
You already documented what this does: creating elements as needed, using reduce cursor as a temp variable.
There was a problem hiding this comment.
I'm still confused. All this does is create elements, it doesn't actually update anything. Notice that context and timestamp aren't referenced in the body of the method. (Which means my comment is wrong, it's descending into this.sources, not context.)
There was a problem hiding this comment.
Ok, so
- it is handling this.sources and
contextis not needed nor used here, should be removed - it is not updating timestamp, which it should be doing, so a bug - add a FIXME comment in this documentation PR?
- your comment about descending into sources of the context is not correct, so to get this PR merged please remove that
2 participants
Add code comments to the code to aid understanding.
I'm trying to wrap my head around some of the details of the signalk implementation and adding comments seemed like the most efficient way to do this. I figured others would benefit from the work.
There's some very light refactoring in here where it simplified the explanation.