My favorite privacy settings

Question

I'm looking for general feedback on this module. Here is what it handles on the page:

This is a private "class" meaning none of its properties are available outside of it.

init is run at run-time. pre sets up parameters to be sent to the server via ajax and post receives the ajax response.

I can post the code for Class.create if needed, but I wanted to focus on the class itself.

$A.Class.create('private', {
    Name: 'Settings',
    E: {
        save_button:  '#save_button',
        cancel_button:  '#cancel_button',
        se_form: '#se_form'
    },
    init: function () {
        var self = this;
        $(this.E.save_button).on("click", function () {
            $A.Packet.push({model: self.Name});
        });
        $(this.E.cancel_button).on("click", function () {
            Page.flip('fave');
        });
    },
    pre: function (packet) {
        packet.h_token = Storage.getRemembered();
        packet.privacy = this.getSaved();
        packet.state = true;
        return packet;
    },
    post: function (packet) {
        Page.flip('fave_custom');
        return packet;
    },
    getSaved: function () {
        var form_elements = this.E.se_form.elements,
            send_value = '',
            iterator;
        for (iterator = 0; iterator < form_elements.length; iterator++) {
            if (form_elements[iterator].checked) {
                send_value = form_elements[iterator].value;
            }
        }
        return send_value;
    }
});

I'd add a bit more margin between the radiobutton/setting name and its description. That said, "general feedback" is implied by merely posting here, and "JavaScript" is covered with the javascript tag - I've edited the title to make it a bit more descriptive (and punny!), feel free to edit it again to improve it. — Mathieu Guindon
– Mathieu Guindon, Commented Apr 7, 2016 at 19:44
it has been done ... in the CSS ... hey, this is a JS question. — the man
– the man, Commented Apr 7, 2016 at 23:24

arc.slate .0 · Accepted Answer · 2016-04-07 23:14:09Z

The main improvement with this code would be your handling of jQuery elements. You should save them in the object literal like you did with plain elements.

Secondly, you can shorten up your code, using jQuery to pull the radio button value.

Your code might be a bit faster, as it works at a lower abstraction level, but if you do choose to use it, it might be better to abstract it out into a DOM function.

Below is the modified code.

$A.Class.create('private', {
    Name: 'Settings',
    J: {
        save_button:          $('#save_button'),
        cancel_button:        $('#cancel_button')
    },
    init: function () {
        var self = this;
        this.J.save_button.on("click", function () {
            $A.Packet.push({model: self.Name});
        });
        this.J.cancel_button.on("click", function () {
            Page.flip('fave');
        });
    },
    pre: function (packet) {
        packet.h_token = Storage.getRemembered();
        packet.privacy = this.getSaved();
        packet.state = true;
        return packet;
    },
    post: function (packet) {
        Page.flip('fave_custom');
        return packet;
    },
    getSaved: function () {
        return $('input[name=privacy]:checked', '#se_form').val(); 
    }
});

/* Put in a DOM abstraction library

            var form_elements = this.E.se_form.elements,
                send_value = '',
                iterator;
            for (iterator = 0; iterator < form_elements.length; iterator++) {
                if (form_elements[iterator].checked) {
                    send_value = form_elements[iterator].value;
                }
            }
            return send_value;
        }
*/

Joseph · Accepted Answer · 2016-04-07 23:15:16Z

For one, you're mixing " and ' in the same code. Try using just one of them (I prefer the ').

Probably personal preference, but a var for all variables is hard to read. You have to keep in mind the commas and you can't simply move the variables around without redefining the var.

Unless E is a construct of your framework, it's hard to read. What in the world is E. If it was morning and I'm hungry, I'd think of E as eggs. Where are my eggs??? :D Also, it would be great if you didn't name things relative to something else. For instance, pre and post... of what? Name your entities properly and precisely. That way, you leave no one guessing. Programming is for humans to read.

One nice thing about JS is dynamicity. One terrible thing about JS is also dynamicity. In this case, this.E.se_form is a string. When does it become an object with an element property? It's not explicitly defined anywhere at all. Having too many implicit operations can cost you time down the road. Best if you make it explicit, like defining se_form as an object, or adding a comment when it became an object.

I use E to hold DOM elements. I've seen el used in many other cases, so I think in the context of web programming, if you had to pick what E stands for, I would go with elements. — the man
– the man, Commented Apr 7, 2016 at 23:21

Stack Exchange Network

My favorite privacy settings

2 Answers 2

You must log in to answer this question.

Hot Network Questions

My favorite privacy settings

2 Answers 2

You must log in to answer this question.

Related

Hot Network Questions