2
\$\begingroup\$

I don't want any feedback on the regexes as I know what needs to be updated here. Also, don't need any feedback on naming conventions.

I'm looking for feedback on the structure and correctness of the class.

/***************************************************************************************************
**SForm - validates and manipulates form data
*/
    var SForm = $A.Class.create({
        Name: 'SForm',
         S: {
            domain:    /:\/\/(www\.)?([\.a-zA-Z0-9\-]+)/,
            url:       /:\/\/(www\.)?[\x00-\x7F]{1,1800}\.[\x00-\x7F]{1,200}/,
            email:     /\S{1,64}@[\x00-\x7F]{1,255}\.[\x00-\x7F]{1,255}/,
            tweet:     /\S{1,40}/,
            title:     /\S{1,32}/,
            name:      /\S{1,64}/,
            pass:      /\S{6,20}/,
            pre_url:   /(http:)|(https:)\/\//,
            full:      /\S+/,
            google:    'https://plus.google.com/_/favicon?domain='
        },
        constructor : function (form_elements) {
            this.form = {};
            $A.someKey(form_elements, function (val) {
                if (val.type !== 'checkbox') {
                    this.form[val.name] = val.value;
                } else if (val.type === 'checkbox') {
                    this.form[val.name] = val.checked;
                }
            }, this);
        },
        get: function (key) {
            return this.form[key];
        },
        set: function (key, value) {
            this.form[key] = value;
        },
        getObj: function () {
            return this.form;
        },
        checkField: function (key) {
            return this.S[key].test(this.form[key]);
        },
        checkFull: function () {
            var key;
            for (key in this.form) {

                // if it is not a boolean and it is not full
                if (!$A.isBoolean(this.form[key]) && !this.S.full.test(this.form[key]) ) {
                    return false;
                }
            }
            return true;
        },
        checkFullAM: function () {
            var key;
            for (key in this.form) {

                // if the fields is empty and it is not the tag field
                if (key !== "tag" && !this.S.full.test(this.form[key])) {
                    return false;
                }

                // if no tag is set, set one by default
                if (key === "tag" && !this.S.full.test(this.form[key])) {
                    this.form[key] = "no-tag";
                }
            }
            return true;
        },
        addURLPrefix: function () {

            // if there is no prefix, add http by default
            if (!this.S.pre_url.test(this.form.url)) {
                this.form.url = 'http://' + this.form.url;
            }
        },
        setDomain: function () {
            var domain = this.form.url.match(this.S.domain);
            if (domain) {
                this.form.domain = domain[2];
            }
        },
        setFaviconTemp: function () {
            this.form.favicon = this.S.google + this.form.domain;
        }
    }, 'constructor');
\$\endgroup\$
5
  • \$\begingroup\$ I would abstract the validation process from the data, and the data from the DOM. It seems you have to create a validation object with methods and everything for every form, or is it re-usable? \$\endgroup\$
    – elclanrs
    Jan 23 '14 at 2:12
  • \$\begingroup\$ What do you mean? I don't access the DOM in this class. \$\endgroup\$
    – user34330
    Jan 23 '14 at 2:14
  • \$\begingroup\$ I see that, just as general idea I mean. How do you use your class with the DOM? \$\endgroup\$
    – elclanrs
    Jan 23 '14 at 2:18
  • \$\begingroup\$ did you have a chance to read the code? \$\endgroup\$
    – user34330
    Jan 23 '14 at 20:47
  • \$\begingroup\$ I did, that's why I'm asking about it. Are you matching form elements strictly by name to use it in the DOM?. This return this.S[key].test(this.form[key]) is what looks limited at first sight, but I don't know how you interact with the library. \$\endgroup\$
    – elclanrs
    Jan 23 '14 at 20:58
2
\$\begingroup\$

A quick review, as the illustrious user34330 is no longer with us.

  • This:

    if (val.type !== 'checkbox') {
        this.form[val.name] = val.value;
    } else if (val.type === 'checkbox') {
        this.form[val.name] = val.checked;
    }
    

    should be

    if (val.type !== 'checkbox') {
        this.form[val.name] = val.value;
    } else
        this.form[val.name] = val.checked;
    }
    

    because if it is not not a checkbox, then... you know it is checkbox, you could also consider a ternary here:

    this.form[val.name] = (val.type === 'checkbox') ? val.checked : val.value;
    
  • getObj should be getForm, since it returns the form
  • checkFullAM the name does not tell me what this does, due to a lack of comments I am mystified at what this function should do. <- That's bad
  • I would have expected to be able to pass a prefix to addURLPrefix, so that I could set https if I wanted to
  • setDomain seems to have no possible use, there are no comments to enlighten the reader why/how this could be useful
\$\endgroup\$
1
  • \$\begingroup\$ R.I.P user34330 :( \$\endgroup\$
    – Morwenn
    Apr 4 '14 at 13:42

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy