4
\$\begingroup\$

I got this pretty interesting homework assignment, which was to finish off a very simple Javascript library. I couldn't use jQuery.

First, I will show you the raw piece of code that I received, then I will show you my solution and the last piece of code is what I think is the test of all the functions I had to make. I checked it and it worked pretty well- I mean the console output was as expected. I would like to ask you whether my solution is okay or not.

What I was given:

'use strict';

var dom = {
    // TODO: fill in library's API ...

    create: function(tag_name, attrs){

    },

    // Attributes getter / setter 
    attr: function(elem, name, value){

    },

    // Append element as last child
    append: function(elem, child){

    },

    // Prepend element as first child
    prepend: function(elem, child){

    },

    // Detach element from dom
    remove: function(elem){

    }
}

What I did:

'use strict';

var dom = {
    // TODO: fill in library's API ...

    create: function(tag_name, attrs){
        var temp = document.createElement(tag_name);
        for(var key in attrs) 
            {
            temp.setAttribute(key, attrs[key]);
            }
        return temp;
    },

    // Attributes getter / setter 
    attr: function(elem, name, value){
        if(!value) return elem.getAttribute(name);
        elem.setAttribute(name,value);
        return elem;
    },

    // Append element as last child
    append: function(elem, child){
        elem.appendChild(child);
    },

    // Prepend element as first child
    prepend: function(elem, child){
        elem.prepend(child);
    },

    // Detach element from dom
    remove: function(elem){
        var par = elem.parentNode;
        par.removeChild(elem);
    }
}

WHAT I THINK IS A TEST:

'use strict';

describe('Dom library', function () {


  it('should allow to create dom elements', function () {
    var div = dom.create('div');

    expect(div instanceof window.Element).toBeTruthy();
    expect(div.tagName).toBe('DIV');
  });

  it('should allow to create dom elements with attributes', function () {
    var div = dom.create('div',{
        test:'test_value'
    });

    expect(div instanceof window.Element).toBeTruthy();
    expect(div.getAttribute('test')).toBe('test_value');
  });

  it('should allow to append elements', function () {
    var div = document.createElement('div');
    div.innerHTML = '<div id="one"></div>\
    <div id="two"></div>\
    <div id="three"></div>';

    var appendee = document.createElement('div');
    dom.append(div, appendee)
    expect(div.children[div.children.length - 1]).toBe(appendee);
  });

  it('should allow to prepend elements', function () {
    var div = document.createElement('div');
    div.innerHTML = '<div id="one"></div>\
    <div id="two"></div>\
    <div id="three"></div>';

    var prependee = document.createElement('div');
    dom.prepend(div, prependee)
    expect(div.children[0]).toBe(prependee);
  });

  it('should allow to detach elements', function () {
    var div = document.createElement('div');
    div.innerHTML = '<div id="one"></div>\
    <div id="two"></div>\
    <div id="three"></div>';

    var target = div.children[0];
    var detached = dom.remove(target)
    expect(target).toBe(detached);
    expect(div.children[0]).not.toBe(target);
  });

  it('should allow get element attribute', function () {
    var div = document.createElement('div');
    div.setAttribute("test","test_value");

    expect(dom.attr(div, 'test')).toEqual('test_value');
  });

  it('should allow set element attribute', function () {
    var div = document.createElement('div');
    dom.attr(div, 'test', 'test_value');

    expect(div.getAttribute('test')).toEqual('test_value');
  });

});
\$\endgroup\$
4
\$\begingroup\$

First off, consider which browsers is this DOM library supposed to support. Then test that it works in all of them.

  • create()
    • Plain for-in loop is not safe as it also loops over keys in object prototype.
    • temp is bad name for a variable.
    • You're repeatedly testing that return value is window.Element.
    • window.Element can be written as just Element (or perhaps use HTMLElement for clarity).
  • attr()
    • What if the value is 0 or ""?
  • prepend()
    • elem.prepend() is not supported by all browsers.
  • remove()
    • par is unnecessary abbreviation of parent.

A few general remarks:

  • Check your code for consistent formatting. You're missing some semicolons and the indentation is inconsistent. Use a tool like ESLint.
  • Escaping newlines to create multiline strings is not officially supported in ECMAScript.
\$\endgroup\$
1
\$\begingroup\$

This is meant as a supplement for Rene Saarsoo's answer.

In general more testing and case handling could be a nice touch.

What happens if there is no parentNode? Or if the target isn't an instance of HTMLElement?

This is mostly a user-friendliness issue because it could be argued that the library should just be "used correctly", but it is something to keep in mind.

The snippet below shows some examples:

'use strict';
var dom = {

  create: function(tag_name, attrs) {
    //Test for valid paramenters
    if (typeof tag_name === "string" ? tag_name.length < 1 : false)
      return;
    var temp = document.createElement(tag_name);
    if (attrs !== void 0) {
      for (var key in attrs) {
        //Needs test: attrs.hasOwnProperty(key)
        if (attrs.hasOwnProperty(key))
          temp.setAttribute(key, attrs[key]);
      }
    }
    return temp;
  },

  /* ... */

  remove: function(elem) {
    //Test for valid paramenters
    if (elem instanceof HTMLElement == false)
      return;
    var par = elem.parentNode;
    //What if parentNode is Null?
    if (par === null)
      return;
    par.removeChild(elem);
  }

};

\$\endgroup\$

Your Answer

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

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