Javascript - Emptying Object instead of Creating new Object

Question

After each click, I intend to empty object editProductList. My code below is instead creating an additional new object editProductList instead of emptying the original editProductList. How do I ensure I'm emptying editProductList instead of creating a new editProductList after clicking it once more?

After the first clicking on the 'devices' column, then #edit_product_add_btn,

I'm logging:

product name, qty:  Remote Tag 6

After the second clicking on the 'devices' column, then #edit_product_add_btn, the previous object remains, and it updates both the original object and new one at the same time

product name, qty:  Remote Tag 7
product name, qty:  Remote Tag 6

Why is it creating an additional object of the same editProductList instead of emptying the original one?

EditableGrid.prototype.mouseClicked = function(e) {

    var editProductList = {};

    $.ajax({
        //...
        success: function(response) {
            editProductList = JSON.parse(response);
            console.log('editProductList direct from DB: ', editProductList);

            //adding products into editProductList
            $('#edit_product_add_btn').on('click', function(e) {

                var in_editProductList = false;

                    for (var product in editProductList) {
                        if (editProductList.hasOwnProperty(product)) {
                            if (editProductList[product].name === productType) {
                                //...
                                console.log('product name, qty: ', editProductList[product].name, editProductList[product].qty);

                                in_editProductList = true;
                                break;
                            }
                        }
                    }

                    if (in_editProductList === false) {

                        //...
                        var new_product_obj = { name: productType, qty: qty };
                        editProductList[Object.size(editProductList)] = new_product_obj;
                    }

            });
    });

}

You have posted nearly 80 lines of code, 90% of them irrelevant to your question. Do you thing you can reduce that to 8 lines? stackoverflow.com/help/mcve — Tomalak
– Tomalak, Commented Jan 25, 2015 at 8:04
Okay, that's a little better... Why are you defining an event handler in an Ajax response? That doesn't look right. — Tomalak
– Tomalak, Commented Jan 25, 2015 at 8:19
Sorry I'm quite new to this. May I ask why defining event handler's shouldn't be done inside an Ajax response? Is this what's causing it to create an additional object editProductList? — jchi2241
– jchi2241, Commented Jan 25, 2015 at 8:26
Because that would define a new event handler every time a response comes in. After 10 responses you would have 10 separate click event handlers. I don't think you want that. — Tomalak
– Tomalak, Commented Jan 25, 2015 at 8:28

Tomalak · Accepted Answer · 2015-01-25 09:11:03Z

1

After deconstructing your code example it became clear that you want to maintain a shopping cart.

If the user adds a product that also is already in the cart, it should simply increase the quantity of the existing item.
If the user adds a new product, it should append it to the cart.
In both cases the screen should be updated accordingly.

So there are two tasks here. Maintain a list and update the screen.

For updating the screen it is helpful to use an HTML templating library. This helps code readability and reduces the risk surface (no more manual HTML building from strings = less XSS risk). I used Mustache.js in the following example.

It is also helpful to separate the tasks so function size stays manageable.

Note how I use a custom jQuery event (dataUpdated) to decouple screen updating from list maintenance:

$(function () {
    var editProductList = [];
    var productListItems = Mustache.parse('{{#.}}<li class="list-group-item"><span class="badge">{{qty}}</span>{{name}}<button type="button" class="close" aria-hidden="true">&times;</button></li>{{/.}}');

    EditableGrid.prototype.mouseClicked = function (e) {
        if (this.getColumnName(columnIndex) == 'devices') {
            $.post('get_requested_devices.php', {
                table: this.name,
                request_id: this.getRowId(rowIndex)
            })
            .done(function (response) {
                editProductList = response;
                $('#edit_product_list').trigger("dataUpdated");
            });
        }
    };

    $('#edit_product_list').on("dataUpdated", function () {
        var listItems = Mustache.render(productListItems, editProductList);
        $('#edit_product_list').empty().append(listItems);
    });

    $('#edit_product_add_btn').on('click', function (e) {
        var qty = parseInt($('#edit_product_qty').val().trim(), 10);
        var name = $('#edit_product_type').text().trim();
        var existingProduct;

        if (qty > 0) {
            existingProduct = $.grep(editProductList, function (product) {
                return product.name === name;
            });
            if (existingProduct) {
                existingProduct.qty += qty;
            } else {
                editProductList.push({
                    name: name,
                    qty: qty
                });
            }
            $('#edit_product_list').trigger("dataUpdated");
        } else {
            alert('Enter a number greater than 0');
        }
    });
});

Warning The above code contains references to two undefined global variables (columnIndex and rowIndex). I have no idea where they come from, I just carried them over from your code. It is a bad idea to maintain global variables for a number of reasons, the biggest one being that many nasty bugs can be traced back to global variables. Try to replace those references, either by function results or by local variables.

Recommendation This situation is the perfect use case of MVVM libraries like Knockout.js. They are designed to completely take over all UI updating for you, so all you need to do is to maintain the data model of the shopping cart. You might want to consider switching.

edited Jan 25, 2015 at 9:11

answered Jan 25, 2015 at 9:02

Tomalak

339k68 gold badges547 silver badges635 bronze badges

Sign up to request clarification or add additional context in comments.

6 Comments

spelufo Over a year ago

Even though MV* libraries and templating frameworks might be suited to the task, they are no substitute for knowing the javascript language. The OP will get into trouble easily with or without a library if he doesn't get concepts like closures and prototypal inheritance.

jchi2241 Over a year ago

you clearly explained the task I wanted to achieve. I am very new to this and have only learned basic javascript and jQuery, and you guys can clearly see me struggling in keeping my HTML in sync with my javascript object editProductList. I took your advice from your earlier comment and unbinded each click handler before pulling data via AJAX again. This fixed the issue and I no longer seem to have the same object being created on top of the previous ones.

jchi2241 Over a year ago

What I'm assuming is there's a new object editProductList within each event handler being created. Is that what was going on? Again, I'm correct me if I'm wrong and point me towards a direction where I can understand how event handlers work.

Tomalak Over a year ago

@jchi2241 I tried to keep my code example very straight-forward and easy to read. You should not have too much trouble understanding what's going on from simply looking at it, even if you don't know everything I have used. Do take the time to go through it line by line and figure it out. I highly recommend using (sth. like) Mustache for all your HTML building needs. It takes only a few minutes to learn and definitely will have a positive impact on the rest of the JS code you write. Using Knockout is something to keep in mind for later, when you are more comfortable with what you're doing.

Tomalak Over a year ago

@jchi2241 Yes, the multiple event handlers were what was causing you trouble. Make sure event handlers are only set up once in your code.

|

spelufo · Accepted Answer · 2015-01-25 08:45:59Z

It's not so clear what you are trying to do.

You seem to be defining a click handler as a method of EditableGrid. So the user will need to click something for the ajax call to be executed, and then click #edit_product_add_btn to load the results into a variable which is local to the first handler? Presumably you are doing something with editProductList after the ajax call comes back, if not loading it at all is pointless, since you can't access it from anywhere else. Perhaps you want this.editProductList instead, so it is accesible from the rest of the "class"?

Are you sure you don't mean to do something like this?

EditableGrid.prototype.mouseClicked = function(e) {

  var self = this;

  $.ajax({
    //...
    success: function(response) {
      self.editProductList = JSON.parse(response);
    }
  });
};

About mutating the current object instead of creating a new one: I don't think you gain much from trying to do this. The old object will be garbage collected as soon as the variable points to the new one. You are creating an object as soon as you do JSON.parse, so there's no escaping that.

If I missundersood your intentions, please clarify your question.

Collectives™ on Stack Overflow

Javascript - Emptying Object instead of Creating new Object

2 Answers 2

6 Comments

Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

2 Answers 2

6 Comments

Comments

Your Answer

Sign up or log in

Post as a guest

Related