Javascript groupBy implementation only produces 1 group

user_889053 0
Viewed 108

I am trying to implement my own groupBy method and, everything I see says this should work, but I only get 1 group when I use it with an array, even though the grouping is fine. What am I missing:

const merge = (array) => array.reduce((a, b) => Object.keys(a).map(key => {
  return {
    [key]: a[key].concat(b[key] || [])
  };
}).reduce(((a,b) => Object.assign({},a,b))))

Array.prototype.groupBy = function (grouper) {
  const groups = this.map(e => {
    return {
      [grouper(e)]: [e]
    };
  })
  console.log("Groups:\n",JSON.stringify(groups))

  return merge(groups)
}
const one = {
  1: [1,2,3],
  0: [4,5,6]
}
const two = {
  1: [7,8,9],
  0: [10,11,12]
}
const three = {
  1: [13],
  0: [16]
}


const array1 = merge([one,two,three])
console.log("case1:\n",JSON.stringify(array1,null,4))

const array2 = [1,2,3,4,5,6,7,9,10].groupBy(e => e % 2)
console.log("case2:\n",JSON.stringify(array2,null,4))

Outputs below, expected is 'case1':

case1:
 {
    "0": [
        4,
        5,
        6,
        10,
        11,
        12,
        16
    ],
    "1": [
        1,
        2,
        3,
        7,
        8,
        9,
        13
    ]
}

Groups:
 [{"1":[1]},{"0":[2]},{"1":[3]},{"0":[4]},{"1":[5]},{"0":[6]},{"1":[7]},{"1":[9]},{"0":[10]}]
case2:
 {
    "1": [
        1,
        3,
        5,
        7,
        9
    ]
}
functional-programming javascript
3 Answers

The first reduce in your merge method has a dependency on the keys of the first object in the array.

objs.reduce((a, b) => Object
  .keys(a)
//      ^-- Takes only the keys from `a`
  .map(key => ({ [key]: a[key].concat(b[key] || []) })
//                                    ^^^^^^-- only merges in those keys from `b`
)

To see the issue in action, take away the 0 or 1 key from your one object.

To fix it without deviating from your current approach too much, you could make sure you take both keys from a and b:

objs.reduce((a, b) => Object
  .keys(Object.assign({}, a, b))
  // etc...
)

It still feels a bit wasteful to first map to key-value-pair type objects and then merge those.

user_32972910

Final solution (removes another bug):

Array.prototype.groupBy = function (grouper) {
  const keysOf = (...objs) => Object.keys(Object.assign({}, objs))
  const groups = this.map(e => {
    return {
      [grouper(e)]: [e]
    };
  })
  const merge = (array) => array.reduce((a, b) =>
    keysOf(a, b).map(key => {
      return {
        [key]: (a[key] || []).concat(b[key] || [])
      };
    }).reduce((a, b) => Object.assign({}, a, b)))
  return merge(groups)
}
const array2 = [1,2,3,4,5,6,7,9,10].groupBy(e => e % 2)
console.log("case2:\n",JSON.stringify(array2,null,2))
user_8890530

@user3297291 points out the issue. I would recommend a different merge altogether. First we write merge2 helper which destructively merges b into a -

function merge2 (a, b)
{ for (const [k, v] of Object.entries(b))
    if (a[k])
      a[k] = [ ...a[k], ...v]
    else
      a[k] = v
  return a
}

Now you can write merge to accept any number of objects. Since it initialises the reduce with a fresh {}, no input objects will be mutated -

const merge = (...all) =>
  all.reduce(merge2, {})

Now groupBy works the way you write it, simply applying the mapped elements to merge -

const groupBy = (arr, f) =>
  merge(...arr.map(v => ({ [f(v)]: [v] })))

const result =
  groupBy([1,2,3,4,5,6,7,9,10], e => e % 2)

Expand the snippet below to verify the result in your own browser -

function merge2 (a, b)
{ for (const [k, v] of Object.entries(b))
    if (a[k])
      a[k] = [ ...a[k], ...v]
    else
      a[k] = v
  return a
}

const merge = (...all) =>
  all.reduce(merge2, {})

const groupBy = (arr, f) =>
  merge(...arr.map(v => ({ [f(v)]: [v] })))

const result =
  groupBy([1,2,3,4,5,6,7,9,10], e => e % 2)

console.log(JSON.stringify(result))

{"0":[2,4,6,10],"1":[1,3,5,7,9]}

If you want to make merge2 using a pure functional expression, you can write it as -

const merge2 = (a, b) =>
  Object
    .entries(b)
    .reduce
       ( (r, [k, v]) =>
          r[k]
            ? Object.assign(r, { [k]: [...r[k], ...v] })
            : Object.assign(r, { [k]: v })
       , a
       )

You could skip the whole merge song and dance and write groupBy in a more direct way -

const call = (f, v) =>
  f(v)

const groupBy = (arr, f) =>
  arr.reduce
    ( (r, v) =>
        call
          ( k =>
              ({ ...r, [k]: r[k] ? [...r[k], v] : [v] })
          , f(v)
          )
    , {}
    )


const result =
  groupBy([1,2,3,4,5,6,7,9,10], e => e % 2)

console.log(JSON.stringify(result))

{"0":[2,4,6,10],"1":[1,3,5,7,9]}

Another option is to use Map as it was designed, and convert to an Object after -

const call = (f, v) =>
  f(v)

const groupBy = (arr, f) =>
  call
    ( m =>
        Array.from
          ( m.entries()
          , ([ k, v ]) => ({ [k]: v }) 
          )
    , arr.reduce
        ( (r, v) =>
            call
              ( k =>
                  r.set
                    ( k
                    , r.has(k)
                        ? r.get(k).concat([v])
                        : [v]
                    )
              , f(v)
              )
        , new Map
        )
    )

const result =
  groupBy([1,2,3,4,5,6,7,9,10], e => e % 2)

console.log(JSON.stringify(result))

user_6331830
Related