Move objects into embedded arrays

Question

I'm using a document store that stores files and folders in a database and can output the contents (or part of the contents) as a serie of nested objects, like this:

var a = [
  {
    id: 0,
    children: [
      {
        id: 1,
        parent: 0,
        children: [
          {
            id: 2,
            children: []
          }
        ]
      }, {
        id: 3,
        parent: 0,
        children: []
      }, {
        id: 4,
        parent: 0,
        children: [
          {
            id: 5,
            parent: 4,
            children: [
              {
                id: 6,
                parent: 5,
                children: []
              }, {
                id: 7,
                parent: 5,
                children: []
              }, {
                id: 8,
                parent: 5,
                children: []
              }
            ]
          }
        ]
      }
    ]
  }
]

I'm displaying this and allowing users to interact with it in the browser using the HTML5 drag and drop API. When a user drags an element and drops it into another I want to change the object's position in the data. All I have available to use is the object to be moved and the destination object.

I've written a function that handles the move. It works by:

1. Finding the item to move, the original parent and the new destination.
2. Removing the item from the parent's children array.
3. Updating the parent on the item to move.
4. Pushing the item to the destinations children array.

Here's the code

function findItem (array, id) {
  var result
  array.some(item => {
    if (item.id === id) {
      result = item
      return true
    }
    if (item.children.length) {
      let subResult = findItem(item.children, id)
      if (subResult) {
        result = subResult
      }
    }
  })
  return result
}

function move (itemId, destinationId) {
  let itemToMove, parentItem, destinationItem
  itemToMove = findItem(a, itemId)
  parentItem = findItem(a, itemToMove.parent)
  destinationItem = findItem(a, destinationId)

  // Remove the item from the the parents childen
  var index
  parentItem.children.find((child, idx) => {
    if (child.id === itemToMove.id) {
      index = idx
      return true
    }
  })
  parentItem.children.splice(index, 1)

  // Update the parent of the item to move
  itemToMove.parent = destinationId

  // Push it to the new parent
  destinationItem.children.push(itemToMove)
}

move(3, 1)

Is there a better way of doing this?

Answer 1

Your findItem function looks good as it takes all the input and return an output.

I think I could just hightlight few points here:

The parameters name

Using array and id is not so helpful. In your example is complicated to find a name for the first parameter as the data is very generic. But is not exactly an array, isn't it?

Rather it is a tree and the findItem is a recursive function.

If you use tree and node instead of item the language is more connected to the real data structure.

Of course findItem became findNode.

I'll suggest you to change id with filter just to have a more abstract concept, if you have to change it in something like a function in the future.

usage of array methods

You are using the some method of the array object, and assign a variable to get the result. Maybe you can solve the problem using just the find method:

function findNode(tree, filter) {
    return tree.find(function (node) {
        return (node.id === filter) || ((node.children.length) && findNode(node.chiltren, filter))
    })
}

As you see the code is more concise and you don't need assignment anymore.

If you consider to use arrow function it will became even more short:

const findNode = (tree, filter) => 
    (tree.find((node) => 
        (node.id === filter) || ((node.children.length) && findNode(node.chiltren, filter))
    ))

And using a function instead of id comparison:

// This returns a function that filter from a specified id
const findById = (searchId) => 
    (node) => (node.id === searchId)

const findNode = (tree, filter) => 
    (tree.find((node) => 
        filter(node) || ((node.children) && node.chiltren.find(filter))
    ))

// And then you use it so:
const node = findNode(tree, findById(1));

About the move function I think you should change the signature and add the tree object as you had done well for the findItem function.

The big advantage of this is on avoiding issues connected to global variable or renaming variables and so on.

So it became like:

function move (nodeId, destinationId, tree) 

I changed itemId in coherence with what I wrote before.

If you use let please continue for variables, and please start to use const to constants, I mean variables that don't need to be reassigned.

So don't use var if you don't have to write code that should be portable on old browsers versions.

I know about hoasting but what is the purpose to do that:

...
let itemToMove, parentItem, destinationItem
itemToMove = findItem(a, itemId)
parentItem = findItem(a, itemToMove.parent)
destinationItem = findItem(a, destinationId)
...

You can just write:

  const itemToMove = findItem(a, itemId)
  const parentItem = findItem(a, itemToMove.parent)
  const destinationItem = findItem(a, destinationId)

Applying the suggestion already provided you can rewrite this lines:

  const nodeToMove = findNode(tree, findById(nodeId))
  const parentNode = findNode(tree, findByParent(nodeToMove))
  const destinationNode = findNode(tree, findById(destinationId))

About the search and remove item in the children array, firstly you should avoid changing data structure, and start to create new version instead.

In this particular case you can change your code as follow:

parentNode.chiltren = parentNode.children
    .filter((node) => (node.id !== nodeToMove.id))

That returns a new array without the selected node.