Adding id to array twice

I have this code:

  app.get("/dash/tasks", (req,res)=>{
  if(!req.user) return res.redirect("/signin")
    Class.find({school: req.user.school}, (err, result)=>{
      Task.find({school: req.user.school}, (err, result2)=>{
        var classesthisstudentisin = []
        if(req.user.type=="student"){
          var promises = []
              result2.forEach((element) =>{
                promises.push(
                Class.findOne({_id: element.class},(err,doc)=>{
                 for (var i = 0; i < doc.students.length; i++) {
                   if(doc.students[i].uid==req.user._id){
                  classesthisstudentisin.push(doc._id)
                }
                 }
              }))
              })
          Promise.all(promises).then(() => {
            console.log(classesthisstudentisin)
          res.render("tasks", {user: req.user, Class: Class, classes: result, tasks: result2, classesthisstudentisin:classesthisstudentisin})
        })
        }else res.render("tasks", {user: req.user, Class: Class, classes: result, tasks: result2})
      })
    })
})

The console.log shows this:
image
How come it adds the same id to the array twice? There is only one class with that id, so there can’t be two. What am I doing wrong?

Uh you are pushing it once only right?

Yes, I only use push once, but end up with 2 ideas instead of one

Why don’t you try running classesthisstudentisin = [] before you run it just to be safe?

Try adding semicolons. Can you share a sample database document?

app.get("/dash/tasks", (req, res) => {
    if (!req.user) return res.redirect("/signin")
    Class.find({
        school: req.user.school
    }, (err, result) => {
        Task.find({
            school: req.user.school
        }, (err, result2) => {
            var classesthisstudentisin = []
            if (req.user.type == "student") {
                var promises = []
                result2.forEach((element) => {
                    promises.push(
                        Class.findOne({
                            _id: element.class
                        }, (err, doc) => {
                            for (var i = 0; i < doc.students.length; i++) {
                                if (doc.students[i].uid == req.user._id) {
                                    classesthisstudentisin.push(doc._id)
                                }
                            }
                        }))
                })
                Promise.all(promises).then(() => {
                    console.log(classesthisstudentisin)
                    res.render("tasks", {
                        user: req.user,
                        Class: Class,
                        classes: result,
                        tasks: result2,
                        classesthisstudentisin: classesthisstudentisin
                    })
                })
            } else res.render("tasks", {
                user: req.user,
                Class: Class,
                classes: result,
                tasks: result2
            })
        })
    })
})

Are you confident that the code works the way you think it does? Here are some ideas that might help with this:

  1. A common technique is to use indentation to visualize the nesting of control flow statements, callbacks, etc. Take a look at the reply from @CarlyRaeJepsenStan for an example of this in action.

  2. Using both callback style (err, doc) => { ... } and promises .then(() => { ... }) in the same code can complicate your analysis, no only because you now have to think about both, but also because it’s not always well documented in what order things happen. For example, in this code, does A happen first or B?

    Class.findOne(..., (err, doc) => {
      A
    }).then(() => {
      B
    })
    

Supposing you can convince yourself that the code is right, what about the data? Notably, this statement

classesthisstudentisin.push(doc._id)

happens in a loop. Sure there’s an if in the loop, but is it guaranteed that the condition is satisfied at most once per task?

You mention that there’s only one class with that ID, but what about the students in that class document? Really, the way this code is written, there’s a lot to verify if you want to make sure that a student’s ID is only added to each array once. Anything that writes to the students field of a Class document is a suspect.

You might want to think about whether it’s possible to design the program in some defensive way so that you only need to reason about a small part of the codebase to verify important properties such as this. The best way to do this depends on the application though, so I’ll leave it at this general recommendation here.

5 Likes

Wow! Thanks for the amazing reply @wh0! Really helped to get my brain thinking about how all the code actually works, considering this is like just what I threw together that seemed to work, almost. I managed to find a fix. I was doing it using the tasks that I found and doing a forEach on those tasks, meaning it was running/verifying more then it needed to. I got rid of that and changed the code a tad and it works great!

app.get("/dash/tasks", (req,res)=>{
  if(!req.user) return res.redirect("/signin")
    Class.find({school: req.user.school}, (err, result)=>{
      Task.find({school: req.user.school}, (err, result2)=>{
        var classesthisstudentisin = []
        if(req.user.type=="student"){
          var promises = []
                promises.push(
                Class.find({},(err,doc)=>{
                 for (var i = 0; i < doc.length; i++) {
                   for (var c = 0; c < doc[i].students.length; c++) {
                   if(doc[i].students[c].uid==req.user._id){
                  classesthisstudentisin.push(doc[i]._id)
                }
                 }}
              }))
          Promise.all(promises).then(() => {
            console.log(classesthisstudentisin)
          res.render("tasks", {user: req.user, Class: Class, classes: result, tasks: result2, classesthisstudentisin:classesthisstudentisin})
        })
        }else res.render("tasks", {user: req.user, Class: Class, classes: result, tasks: result2})
      })
    })
})

Mind you, there are a billion ways someone could do this better/more efficiently or whatever, but at this point, I don’t really care, as long as it works! :joy:
Thanks for the help :slight_smile: