3
\$\begingroup\$

In this project I've taken my first shot at OO JavaScript, and I'm not sure how it turned out. The script dynamically makes a tooltip with different text if you so choose. As of now it takes a lot to create the tooltip and show it, is there a way to make this smaller?

Here is the code at JSFiddle. Let me know if it would be easier to paste in here.

/*global window, document*/

var note;

function Note() {
    'use strict';

    this.identifier = 0;
}

Note.prototype.makeWrap = function () {
    'use strict';
    var wrap;

    wrap = document.createElement('div');
    wrap.id = 'wrap' + this.identifier;

    wrap.style.border    = '1px solid #43484A';
    wrap.style.boxShadow = '-1px -1px 5px #292929, 1px 1px 5px #292929';
    wrap.style.height    = '185px';
    wrap.style.opacity   = '.95';
    wrap.style.position  = 'absolute';
    wrap.style.width     = '300px';

    return wrap;
};

Note.prototype.makeHead = function () {
    'use strict';
    var head;

    head = document.createElement('div');
    head.id = 'head' + this.identifier;

    head.style.backgroundColor = '#43484A';
    head.style.color           = '#EEEEEE';
    head.style.height          = '30px';
    head.style.paddingLeft     = '6px';
    head.style.position        = 'relative';
    head.style.width           = '294px';

    return head;
};

Note.prototype.makeTitle = function (text) {
    'use strict';
    var title;

    title = document.createElement('div');
    title.id = 'title' + this.identifier;

    title.style.backgroundColor = 'inherit';
    title.style.color           = 'inherit';
    title.style.float           = 'left';
    title.style.fontFamily      = 'Georgia, "Times New Roman", serif';
    title.style.height          = '17px';
    title.style.paddingTop      = '6px';
    title.style.paddingBottom   = '7px';
    title.style.position        = 'relative';
    title.style.width           = '80%';

    title.innerHTML = text;

    return title;
};

Note.prototype.makeMini = function () {
    'use strict';
    var mini;

    mini = document.createElement('div');
    mini.id = 'min' + this.identifier;

    mini.style.backgroundColor = 'inherit';
    mini.style.color           = '#EEEEEE';
    mini.style.cursor          = 'pointer';
    mini.style.float           = 'left';
    mini.style.fontFamily      = 'Consolas, monospace';
    mini.style.height          = '17px';
    mini.style.paddingTop      = '6px';
    mini.style.paddingBottom   = '7px';
    mini.style.position        = 'relative';
    mini.style.textAlign       = 'center';
    mini.style.width           = '10%';

    mini.innerHTML = '—';

    return mini;
};

Note.prototype.makeClose = function () {
    'use strict';
    var close;

    close = document.createElement('div');
    close.id = 'close' + this.identifier;

    close.style.backgroundColor = 'inherit';
    close.style.color           = '#EEEEEE';
    close.style.cursor          = 'pointer';
    close.style.float           = 'left';
    close.style.fontFamily      = 'Consolas, monospace';
    close.style.height          = '17px';
    close.style.paddingTop      = '6px';
    close.style.paddingBottom   = '7px';
    close.style.position        = 'relative';
    close.style.textAlign       = 'center';
    close.style.width           = '10%';

    close.innerHTML = 'X';

    return close;
};

Note.prototype.makeBody = function (passage) {
    'use strict';
    var body;

    body = document.createElement('div');
    body.id = 'body' + this.identifier;

    body.style.backgroundColor = '#EEEEEE';
    body.style.color           = '#22282A';
    body.style.fontFamily      = 'Georgia, "Times New Roman", serif';
    body.style.height          = '118px';
    body.style.overflow        = 'auto';
    body.style.padding         = '6px';
    body.style.position        = 'relative';
    body.style.width           = '288px';

    body.innerHTML = passage;

    return body;
};

Note.prototype.makeFoot = function () {
    'use strict';
    var foot, link;

    foot = document.createElement('div');
    foot.id = 'foot' + this.identifier;

    foot.style.backgroundColor = '#EEEEEE';
    foot.style.heigth          = '16px';
    foot.style.padding         = '4px 6px 5px 6px';
    foot.style.position        = 'relative';
    foot.style.textAlign       = 'right';
    foot.style.width           = '288px';

    link = document.createElement('a');
    link.href = '#';

    link.style.color = '#0088CC';
    link.style.textDecoration = 'none';
    link.onmouseover = function () {
        this.style.borderBottom = '1px solid #0088CC';
    };
    link.onmouseout = function () {
        this.style.borderBottom = 'none';
    };
    link.innerHTML = 'Link';

    foot.appendChild(link);

    return foot;
};

Note.prototype.create = function (titleText, bodyText) {
    'use strict';
    var wrap, head, title, mini, close, body, foot;

    wrap  = this.makeWrap();
    head  = this.makeHead();
    title = this.makeTitle(titleText);
    mini  = this.makeMini();
    close = this.makeClose();
    body  = this.makeBody(bodyText);
    foot  = this.makeFoot();

    head.appendChild(title);
    head.appendChild(mini);
    head.appendChild(close);

    wrap.appendChild(head);
    wrap.appendChild(body);
    wrap.appendChild(foot);

    document.body.appendChild(wrap);
    this.identifier = this.identifier + 1;
};

note = new Note();
note.create(
    'Lorem',
    'Lorem ipsum dolor sit amet, consectetur adipiscing elit. Curabitur eu orci nibh. Class aptent taciti sociosqu ad litora torquent per conubia nostra, per inceptos himenaeos. Duis posuere rutrum pellentesque.'
);
\$\endgroup\$
0

3 Answers 3

8
\$\begingroup\$

Woahhhh, that is a LOT of repetition. You have a lot of similar functionality, all your functions do basically the same stuff:

  • Create an element
  • Set the ID
  • Apply styles
  • Maybe do something else

You should look into DRYing your code by:

  1. Factoring out often used features into helper functions
  2. Adjusting structure of the code in order to allow for more of the above
  3. Go back to 1 and repeat until it's no longer feasible

As a quick start I refactored and dryed your code quite a bit, I left out re-adding all the styling stuff, but it should give you a pretty good idea on how to structure your code.

/*global window, document*/

// Let's use an anonymous wrapper
(function() {
    'use strict'; // only one pragma needed

    // You are using alot of .style assignments... let's create a helper
    function css(elem, styles) {

        for(var i in styles) {
            if (styles.hasOwnProperty(i)) {
                elem.styles[i] = styles[i];
            }
        }

    }

    // You are also creating a lot of similiar elements, so will use another wrapper
    function element(type, id, styles) {

        var elem = document.createElement(type);
        elem.id = id;
        css(elem, styles);

        return elem;

    }

    function Note() {
        this.identifier = 0;
    }

    // Prototype is just an object, let's get rid of all those assignments
    Note.prototype = {

        create: function (titleText, bodyText) {

            'use strict';
            var wrap, head, title, mini, close, body, foot;

            wrap  = this.element('div', 'wrap', {

                border: '1px solid #43484A',
                boxShadow: '-1px -1px 5px #292929, 1px 1px 5px #292929',
                height: '185px',
                opacity: '.95',
                position: 'absolute',
                width: '300px',

            });

            // etc....

            this.identifier++;

        },

        element: function(type, name, styles) {
            return element(type, name + this.identifier, styles));
        }

    }

    window.Note = Note; // expose the local stuff to the window

});

note = new Note();
note.create(
    'Lorem',
    'Lorem ipsum dolor sit amet, consectetur adipiscing elit. Curabitur eu orci nibh. Class aptent taciti sociosqu ad litora torquent per conubia nostra, per inceptos himenaeos. Duis posuere rutrum pellentesque.'
);

Also have a look at JavaScript Design Patterns book by O'Reilly.

PS: I'd further suggest that you move out your CSS into actually CSS class declarations and use those on your elements.

\$\endgroup\$
2
  • \$\begingroup\$ Thank you for the great answer, I really appreciate it. Your answer raises another question which I noticed while browsing jQuery, what is the difference between doing (function(){window.Note = Note;}()) vs var Note; (function(){ Note = Note; }())? Thanks again! \$\endgroup\$
    – dunnza
    Commented Aug 27, 2011 at 23:32
  • \$\begingroup\$ +1 Design principles to the rescue, en.wikipedia.org/wiki/Don't_repeat_yourself \$\endgroup\$ Commented Aug 28, 2011 at 15:53
3
\$\begingroup\$

As @IvoWetzel already said : CSS has no place in javascript code. Instead you should have configurable classnames for elements of that Note widget. Leave styling of elements to CSS in *.css file.

\$\endgroup\$
4
  • \$\begingroup\$ The thing is is that this will eventually be a project where people who desire to use it will simply put <script src='project.js'></script> at the bottom of their html and I want it to work without them having to add more stuff . . . what is wrong with styling in JavaScript, anyway? Just curious . . . \$\endgroup\$
    – dunnza
    Commented Aug 27, 2011 at 23:36
  • \$\begingroup\$ @Zach , 2 major reasons : 1) preformance - each time you change any of style attribute via JS for any tag it causes browser's rendering engine to perform reflow. 2) code quality - in frontend you have 3 "layers" : html, css and js .. or content, presentation and behavior. They should be kept separate. Besides , your usecase is flawed. Do you really expect people to design pages around your widget's design ? \$\endgroup\$
    – tereško
    Commented Aug 28, 2011 at 0:19
  • \$\begingroup\$ if by design you mean the colors and stuff, then no. I already have a customization system in the works. Also thank you for the article about reflow. What do you think about adding a css file to the user's webpage via JavaScript? \$\endgroup\$
    – dunnza
    Commented Aug 28, 2011 at 0:31
  • \$\begingroup\$ @Zach make the javascript simpler by requiring consumers to do one more little simple step: <link rel="stylesheet" type="text/css" href="theme.css" /><script src='project'js' /> this also makes it very explicit to the consumer how they can alter the styling to match their page. \$\endgroup\$ Commented Aug 28, 2011 at 15:52
1
\$\begingroup\$

The most obviuos way is to move all styles to the external css file and add a link to it dynamically. Disqus is doing like this. The next thing is to stop doing the same again and again.

In fact you need only one function with definition like this:

funciton createElement(name, attrs, styles, parent)

Also you can add more specific function to stop further duplication in your case

function createDiv(id, styles, parent, innerHTML) {
    var el = createElement('div', {id: id}, styles, parent);
    if(innerHTML) el.innerHTML = innerHTML;
    return el;
}

With this function you could create all the DOM in much shorter way:

var wrap = createDiv('wrap', wrapStyles),
        header = createDiv('head', headStyles, wrap),
            title = createDiv('title', titleStyles, headerm titleText);
..etc..
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.