Refactor function with nested for loop - Javascript

Question

I need to delete nested for loop in a function I created. My function receive an associative array and I return a new array based on certain properties in order to group messages to use later. For example, I have two schools, many students. So I group them based on gender and grade. I don't how to refactor this function because I don't know much about algorithms. It doesn't matter if my logic need be erased completely or need to be done again. I must delete the second for loop. Also, I can return either an common array, associative array or just object. I tried to replicate my function with the same logic but different data:

var studentsArray = new Array();

studentsArray["SCHOOL_1"] = [
    // girls
    {id: '1', school: 'SCHOOL_1', grade: 'A', message: 'Congratulations!', isMan: false},
    {id: '2', school: 'SCHOOL_1', grade: 'A', message: 'Good work!', isMan: false},
    {id: '3', school: 'SCHOOL_1', grade: 'A', message: 'Ok', isMan: false},
    // boys
    {id: '4', school: 'SCHOOL_1', grade: 'A', message: 'Congratulations!', isMan: true},
    {id: '5', school: 'SCHOOL_1', grade: 'B', message: 'Good work!', isMan: true},
    {id: '6', school: 'SCHOOL_1', grade: 'B', message: 'Good work!', isMan: true},
    {id: '7', school: 'SCHOOL_1', grade: 'A', message: 'Congratulations!', isMan: true},
    {id: '8', school: 'SCHOOL_1', grade: 'B', message: 'Good work!', isMan: true},

];
studentsArray["SCHOOL_2"] = [
    // girls
    {id: '9', school: 'SCHOOL_2', grade: 'A', message: 'Congratulations!', isMan: false},
    {id: '10', school: 'SCHOOL_2', grade: 'A', message: 'Congratulations!', isMan: false},
    {id: '11', school: 'SCHOOL_2', grade: 'A', message: 'Congratulations!', isMan: false},
    {id: '12', school: 'SCHOOL_2', grade: 'B', message: 'Good work!', isMan: false},
    {id: '13', school: 'SCHOOL_2', grade: 'B', message: 'Nice!', isMan: false},
    // boys
    {id: '14', school: 'SCHOOL_2', grade: 'A', message: 'Congratulations!', isMan: true},
    {id: '15', school: 'SCHOOL_2', grade: 'A', message: 'Congratulations!', isMan: true},
    {id: '16', school: 'SCHOOL_2', grade: 'A', message: 'Congratulations!', isMan: true},
    {id: '17', school: 'SCHOOL_2', grade: 'B', message: 'Congratulations!', isMan: true},
];

function GroupMessages(schools, gender) {
    // Initialize object to return
    var result = [];
    // First loop
    for (var school in schools) {
        // Group students by gender
        var girls = schools[school].filter(student => !student.isMan);
        var boys = schools[school].filter(student => student.isMan);

        // Flag to determine unique grade per gender
        var boysHaveUniqueGrade = boys.map(student => student.grade).filter((v, i, a) => a.indexOf(v) === i).length === 1;
        var girlsHaveUniqueGrade = girls.map(student => student.grade).filter((v, i, a) => a.indexOf(v) === i).length === 1;

        // If exists a single student per gender, return the same
        if (girls && girls.length === 1) result.push(girls[0]);
        if (boys && boys.length === 1) result.push(boys[0]); 
 
    
        //////////////////////////
        //    Group by grades   //
        /////////////////////////

        if (boys && boys.length > 1 && boysHaveUniqueGrade && gender === 'man') {
            // Combine messages
            let messages = boys.map(boy => boy.message);
            // First student is the reference
            let student = boys[0];
            // Join messages
            student.message = messages.join('|');
            // Update object to return
            result.push(student);
        }

        if (boys && boys.length > 1 && !boysHaveUniqueGrade && gender === 'man') {
            // Group messages by level (maybe I don't need GroupByProperty function neither)
            let studentsByGrade = GroupByProperty(boys, 'grade');
            // Second loop. I return a boys students based on 'grade' property. (I NEED TO DELETE THIS SECOND FOR LOOP)
            for (let grade in studentsByGrade) {
                // First student is the reference
                let student = studentsByGrade[grade][0];
                // Combine messages
                let messages = studentsByGrade[grade].map(student => student.message);
                // Join messages
                student.message = messages.join('|');
                // Update object to return
                result.push(student);
                // Code continue but I stop code here...
            }
        }

        if (girls && girls.length > 1 && girlsHaveUniqueGrade && gender !== 'man') {
            // Combine messages
            let messages = girls.map(girl => girl.message);
            // First student is the reference
            let student = girls[0];
            // Join messages
            student.message = messages.join('|');
            // Update object to return
            result.push(student);


        }

        if (girls && girls.length > 1 && !girlsHaveUniqueGrade && gender !== 'man') {
            // Group messages by level (maybe I don't need GroupByProperty function neither)
            let studentsByGrade = GroupByProperty(girls, 'grade');
            // Second loop. I return a girls students based on 'grade' property. (I NEED TO DELETE THIS SECOND FOR LOOP)
            for (let grade in studentsByGrade) {
                // First student is the reference
                let student = studentsByGrade[grade][0];
                // Combine messages
                let messages = studentsByGrade[grade].map(student => student.message);
                // Join messages
                student.message = messages.join('|');
                // Update object to return
                result.push(student);
                // Code continue but I stop code here...
            }
        }
    }

    return result;
}

function GroupByProperty(objectArray, property) {
    let result = objectArray.reduce((acc, obj) => {
       var key = obj[property];
       if (!acc[key]) acc[key] = [];
       acc[key].push(obj);
       return acc;
    }, {});

    return result;
}

GroupMessages(studentsArray, 'woman'); // any other gender works as 'man'

an array, associative array or object that group or filter students based on their gender and grades. You have to consider that gender and grades could be different. — Intenzion
– Intenzion, Commented Nov 19, 2020 at 20:41
There are no "associative arrays" in JavaScript. Use an object literal, do not use new Array. — Bergi
– Bergi, Commented Nov 19, 2020 at 21:20
Thanks @Bergi. I just added an argument to my example function. I hope someone could help me. — Intenzion
– Intenzion, Commented Nov 19, 2020 at 21:22
@Intenzion, can you please include the parts you left out that say code with another logic. it would also be helpful to see the expected output. — Mulan
– Mulan, Commented Nov 19, 2020 at 22:35

Mulan · Accepted Answer · 2021-12-16 17:55:15Z

reusable modules

This is great opportunity to learn about reusable modules. Your GroupMessages function is over almost 100 lines and it is tightly coupled with your data structure. The solution in this answer solves your specific problem without any modification of the modules written at an earlier time.

I will offer some code review at end of this answer, but for now we rename schools to grades becauase each item in the array represents a single grade of a single student at a particular school -

const grades =
  [ {id: 1, school: 'SCHOOL_1', grade: 'A', message: 'Congratulations!', isMan: false}
  , {id: 2, school: 'SCHOOL_1', grade: 'A', message: 'Good work!', isMan: false}
  , {id: 3, school: 'SCHOOL_1', grade: 'A', message: 'Ok', isMan: false}
  , {id: 4, school: 'SCHOOL_1', grade: 'A', message: 'Congratulations!', isMan: true}
  , {id: 5, school: 'SCHOOL_1', grade: 'B', message: 'Good work!', isMan: true}
  , {id: 6, school: 'SCHOOL_1', grade: 'B', message: 'Good work!', isMan: true}
  , {id: 7, school: 'SCHOOL_1', grade: 'A', message: 'Congratulations!', isMan: true}
  , {id: 8, school: 'SCHOOL_1', grade: 'B', message: 'Good work!', isMan: true}
  , {id: 9, school: 'SCHOOL_2', grade: 'A', message: 'Congratulations!', isMan: false}
  , {id: 10, school: 'SCHOOL_2', grade: 'A', message: 'Congratulations!', isMan: false}
  , {id: 11, school: 'SCHOOL_2', grade: 'A', message: 'Congratulations!', isMan: false}
  , {id: 12, school: 'SCHOOL_2', grade: 'B', message: 'Good work!', isMan: false}
  , {id: 13, school: 'SCHOOL_2', grade: 'B', message: 'Nice!', isMan: false}
  , {id: 14, school: 'SCHOOL_2', grade: 'A', message: 'Congratulations!', isMan: true}
  , {id: 15, school: 'SCHOOL_2', grade: 'A', message: 'Congratulations!', isMan: true}
  , {id: 16, school: 'SCHOOL_2', grade: 'A', message: 'Congratulations!', isMan: true}
  , {id: 17, school: 'SCHOOL_2', grade: 'B', message: 'Congratulations!', isMan: true}
  ]

As you learned, JavaScript does not have associative arrays. Nor does it have any native data structures which support lookups using compound keys (selecting a value with more than one key). We're going to import some functions from our binary tree module, btree, create an identifier for your records, myIdentifier, and use it to initialise your tree, myTree -

import { nil, fromArray, inorder } from "./btree.js"

const myIdentifier = record =>
  [ record?.school ?? "noschool" // if school property is blank, group by "noschool"
  , record?.grade ?? "NA"        // if grade property is blank, group by "NA"
  , record?.isMan ?? false       // if isMan property is blank, group by false
  ]

const myTree =
  nil(myIdentifier)

The binary tree automatically handles grouping based on the customisable identifier, and any number of grouping keys can be used. We will use a basic filter to select all grades matching the queried gender. The selected grades array is passed to fromArray along with a merging function that handles tree updates. inorder is used to extract the grouped values from the tree -

function groupMessages (grades, gender)
{ const t =
    fromArray
      ( myTree
      , grades.filter(x => !x.isMan || gender === "man")
      , ({ messages = [] } = {}, { message = "", ...r }) =>
          ({ ...r, messages: [ ...messages, message ]})
      )
  return Array.from(inorder(t))
}

Let's see the output now -

console.log(groupMessages(grades, 'woman'))

[
  {
    "id": "3",
    "school": "SCHOOL_1",
    "grade": "A",
    "isMan": false,
    "messages": [
      "Congratulations!",
      "Good work!",
      "Ok"
    ]
  },
  {
    "id": "11",
    "school": "SCHOOL_2",
    "grade": "A",
    "isMan": false,
    "messages": [
      "Congratulations!",
      "Congratulations!",
      "Congratulations!"
    ]
  },
  {
    "id": "13",
    "school": "SCHOOL_2",
    "grade": "B",
    "isMan": false,
    "messages": [
      "Good work!",
      "Nice!"
    ]
  }
]

To complete this post, we will show the implementations of btree,

// btree.js

import { memo } from "./func.js"
import * as ordered from "./ordered.js"

const nil =
  memo
    ( compare =>
        ({ nil, compare, cons:btree(compare) })
    )

const btree =
  memo
    ( compare =>
        (value, left = nil(compare), right = nil(compare)) =>
          ({ btree, compare, cons:btree(compare), value, left, right })
    )

const isNil = t =>
  t === nil(t.compare)

const compare = (t, q) =>
  ordered.all
    ( Array.from(t.compare(q))
    , Array.from(t.compare(t.value))
    )

function get (t, q)
{ if (isNil(t))
    return undefined
  else switch (compare(t, q))
  { case ordered.lt:
      return get(t.left, q)
    case ordered.gt:
      return get(t.right, q)
    case ordered.eq:
      return t.value
  }
}

function update (t, q, f)
{ if (isNil(t))
    return t.cons(f(undefined))
  else switch (compare(t, q))
  { case ordered.lt:
      return t.cons(t.value, update(t.left, q, f), t.right)
    case ordered.gt:
      return t.cons(t.value, t.left, update(t.right, q, f))
    case ordered.eq:
      return t.cons(f(t.value), t.left, t.right)
  }
}

const insert = (t, q) =>
  update(t, q, _ => q)

const fromArray = (t, a, merge) =>
  a.reduce
    ( (r, v) =>
       update
          ( r
          , v
          , _ => merge ? merge(_, v) : v
          )
    , t
    )

function* inorder (t)
{ if (isNil(t)) return
  yield* inorder(t.left)
  yield t.value
  yield* inorder(t.right)
}

export { btree, fromArray, get, inorder, insert, isNil, nil, update }

Reusability is essential. Modules can import other modules! Above, btree imports from func and ordered — partial modules included below -

// func.js

function memo (f)
{ const r = new Map
  return x =>
    r.has(x)
      ? r.get(x)
      : (r.set(x, f(x)), r.get(x))
}

export { memo }

// ordered.js

const lt =
  -1

const gt =
  1

const eq =
  0

const empty =
  eq

const compare = (a, b) =>
  a < b
    ? lt
: a > b
    ? gt
: eq

const all = (a = [], b = []) =>
  a.reduce
    ( (r, e, i) =>
        concat(r, compare(e, b[i]))
    , eq
    )

const concat = (a, b) =>
  a === eq ? b : a

export { all, compare, concat, empty, eq, gt, lt }

Your answer is very useful despite changing the input data type.

Scott Sauyet · Accepted Answer · 2021-12-17 17:17:45Z

This old question recently came to my attention. The answer from user Mulan is fantastic; I would recommend spending the time to understand it thoroughly.

But I want to offer a somewhat different approach. It similarly builds atop existing functions, but doesn't go as low-level as that approach.

First, here was my initial solution to the problem:

const call = (fn, ...args) => fn (...args)
const groupBy = (fn) => (xs) =>
  xs .reduce ((a, x) => call (key => ((a [key] = [... (a [key] || []), x]), a), fn (x)), {})

const groupMessages = (students, gender) => 
  Object .values (groupBy (x => `${x.school}|${x.grade}`) (Object .values (students) 
    .flat ()
    .filter (({isMan}) => isMan == (gender == 'man'))))
    .map ((students) => ({
      ... students [0],
      message: students .map (s => s.message) .join ('|')
    }))

const students = {SCHOOL_1: [/* girls */ {id: '1', school: 'SCHOOL_1', grade: 'A', message: 'Congratulations!', isMan: false}, {id: '2', school: 'SCHOOL_1', grade: 'A', message: 'Good work!', isMan: false}, {id: '3', school: 'SCHOOL_1', grade: 'A', message: 'Ok', isMan: false}, /* boys */ {id: '4', school: 'SCHOOL_1', grade: 'A', message: 'Congratulations!', isMan: true}, {id: '5', school: 'SCHOOL_1', grade: 'B', message: 'Good work!', isMan: true}, {id: '6', school: 'SCHOOL_1', grade: 'B', message: 'Good work!', isMan: true}, {id: '7', school: 'SCHOOL_1', grade: 'A', message: 'Congratulations!', isMan: true}, {id: '8', school: 'SCHOOL_1', grade: 'B', message: 'Good work!', isMan: true}], SCHOOL_2: [/* girls */ {id: '9', school: 'SCHOOL_2', grade: 'A', message: 'Congratulations!', isMan: false}, {id: '10', school: 'SCHOOL_2', grade: 'A', message: 'Congratulations!', isMan: false}, {id: '11', school: 'SCHOOL_2', grade: 'A', message: 'Congratulations!', isMan: false}, {id: '12', school: 'SCHOOL_2', grade: 'B', message: 'Good work!', isMan: false}, {id: '13', school: 'SCHOOL_2', grade: 'B', message: 'Nice!', isMan: false}, /* boys */ {id: '14', school: 'SCHOOL_2', grade: 'A', message: 'Congratulations!', isMan: true}, {id: '15', school: 'SCHOOL_2', grade: 'A', message: 'Congratulations!', isMan: true}, {id: '16', school: 'SCHOOL_2', grade: 'A', message: 'Congratulations!', isMan: true}, {id: '17', school: 'SCHOOL_2', grade: 'B', message: 'Congratulations!', isMan: true}]}
         
console .log (groupMessages (students, 'woman'))

.as-console-wrapper {max-height: 100% !important; top: 0}

It uses a groupBy function that I keep around and use a lot. That in turn depends upon call, which accepts a function and a list of parameters and calls that function with those parameters. (I use it here simply to keep local variables to to a minimum.)

This works, and is clearly much shorter than the original code. But it has one real ugliness to it, common to a lot of such JS code. It's compact, but the order of operations is hard to see. It takes a deep understanding of the code to see this:

    const groupMessages = (students, gender) => 
      Object .values (groupBy (x => `${x.school}|${x.grade}`) (Object .values (students)
        /* ^-- 5 */   /* ^-- 4 */                                 /* ^-- 1 */ 
        .flat ()  /* <-- 2 */
        .filter (({isMan}) => isMan == (gender == 'man'))))  /* <-- 3 */
        .map ((students) => ({  /* <-- 6 */
          ... students [0],
          message: students .map (s => s.message) .join ('|')
        }))

I tend to use a pipe function which passes the result of one function to the next one and the result of that one to the following one, and so on. This cleans things up a lot. But it does require having curried functions that reify things like array methods.

So, I find this breakdown much cleaner:

// reusable utility functions
const pipe = (...fns) => (arg) => fns .reduce ((a, f) => f (a), arg)
const map = (fn) => (xs) => xs .map (x => fn (x))
const filter = (fn) => (xs) => xs .filter (x => fn (x))
const call = (fn, ...args) => fn (...args)
const groupBy = (fn) => (xs) =>
  xs .reduce ((a, x) => call (key => ((a [key] = [... (a [key] || []), x]), a), fn (x)), {})
const flat = (xs) => xs .flat ()

// helper function
const groupStudentMessages = (students) => ({
  ... students [0],
  message: students .map (s => s.message) .join ('|')
})

// main function, as a pipeline
const groupMessages = (students, gender) => pipe (
  Object .values,                                     /* <-- 1 */
  flat,                                               /* <-- 2 */ 
  filter (({isMan}) => isMan == (gender == 'man')),   /* <-- 3 */
  groupBy (x => `${x.school}|${x.grade}`),            /* <-- 4 */
  Object .values,                                     /* <-- 5 */
  map (groupStudentMessages)                          /* <-- 6 */
) (students)

const students = {SCHOOL_1: [/* girls */ {id: '1', school: 'SCHOOL_1', grade: 'A', message: 'Congratulations!', isMan: false}, {id: '2', school: 'SCHOOL_1', grade: 'A', message: 'Good work!', isMan: false}, {id: '3', school: 'SCHOOL_1', grade: 'A', message: 'Ok', isMan: false}, /* boys */ {id: '4', school: 'SCHOOL_1', grade: 'A', message: 'Congratulations!', isMan: true}, {id: '5', school: 'SCHOOL_1', grade: 'B', message: 'Good work!', isMan: true}, {id: '6', school: 'SCHOOL_1', grade: 'B', message: 'Good work!', isMan: true}, {id: '7', school: 'SCHOOL_1', grade: 'A', message: 'Congratulations!', isMan: true}, {id: '8', school: 'SCHOOL_1', grade: 'B', message: 'Good work!', isMan: true}], SCHOOL_2: [/* girls */ {id: '9', school: 'SCHOOL_2', grade: 'A', message: 'Congratulations!', isMan: false}, {id: '10', school: 'SCHOOL_2', grade: 'A', message: 'Congratulations!', isMan: false}, {id: '11', school: 'SCHOOL_2', grade: 'A', message: 'Congratulations!', isMan: false}, {id: '12', school: 'SCHOOL_2', grade: 'B', message: 'Good work!', isMan: false}, {id: '13', school: 'SCHOOL_2', grade: 'B', message: 'Nice!', isMan: false}, /* boys */ {id: '14', school: 'SCHOOL_2', grade: 'A', message: 'Congratulations!', isMan: true}, {id: '15', school: 'SCHOOL_2', grade: 'A', message: 'Congratulations!', isMan: true}, {id: '16', school: 'SCHOOL_2', grade: 'A', message: 'Congratulations!', isMan: true}, {id: '17', school: 'SCHOOL_2', grade: 'B', message: 'Congratulations!', isMan: true}]}

console .log (groupMessages (students, 'woman'))

.as-console-wrapper {max-height: 100% !important; top: 0}

I hope that the order of operations in the main function is clear enough even without the annotations. To make that even more clear, we pull out the groupStudentMessages helper function, and make each pipeline step a one-liner.

Note that those first six functions are extremely helpful, reusable functions. All have counterparts in Ramda, of which I'm a founder. But these implementations are simple and are easy to move into any codebase.

btilly · Accepted Answer · 2020-11-19 22:02:12Z

-1

It seems odd to me that you have to delete the second for loop.

But still, this is the kind of problem that relational databases are meant to solve. If no other option exists, https://github.com/sql-js/sql.js is SQLite compiled to JavaScript.

answered Nov 19, 2020 at 22:02

btilly

47.8k3 gold badges69 silver badges101 bronze badges

1 Comment

Mulan Over a year ago

this is a comment, not an answer

Collectives™ on Stack Overflow

Refactor function with nested for loop - Javascript

3 Answers 3

1 Comment

Comments

1 Comment

Your Answer

Linked

Hot Network Questions

Collectives™ on Stack Overflow

3 Answers 3

1 Comment

Comments

1 Comment

Your Answer

Sign up or log in

Post as a guest

Linked

Related