4
\$\begingroup\$

I wrote a function to create one of those CSS triangle blocks and insert it right after one of the already created blocks.

function createArrow() {
    var arrowDown = {
        parent: document.querySelector('.landingItem'),
        div: document.createElement('div'),
        initDiv: function() {
            this.div.classList.add('arrow-down');
            this.div.style.borderLeftWidth = parseInt(document.documentElement.clientWidth)/2 + 'px';
            this.div.style.borderRightWidth = parseInt(document.documentElement.clientWidth)/2 + 'px';
            this.parent.appendChild(this.div);
        },
        checkDiv: function () {
            if(this.parent.querySelector('.arrow-down')) {
                this.parent.removeChild(this.parent.querySelector('.arrow-down'));
            }
            this.initDiv();
        }

    };

    arrowDown.checkDiv();
}

createArrow();
window.addEventListener('resize', createArrow);
.arrow-down {
  margin-top: -2px;
  border-top: 60px solid #501a70;
  border-left: 160px solid transparent;
  border-right: 160px solid transparent;
}
<div class="landingItem"></div>

What can I improve?

\$\endgroup\$
  • \$\begingroup\$ "am i doing everything right?" Doing 'everything' right is hard. Does it work as intended? \$\endgroup\$ – Mast Jul 21 '17 at 10:56
  • \$\begingroup\$ yeap, works fine \$\endgroup\$ – Sveta Jul 21 '17 at 11:03
  • 3
    \$\begingroup\$ @Sveta: I fixed the formatting and grammar and changed your code into a snippet. Make sure that it still works as intended, and that your question still reflects your thoughts, not necessarily mine. \$\endgroup\$ – Zeta Jul 21 '17 at 11:19
1
\$\begingroup\$

Is it an utility function or a module?

You named your function createArrow() and it contains an object with methods and at the end you just call one.

I think this is a bit of over engineered for such piece of code.

The first improvement I suggest you is to remove the plain object and method, and refactor the function to do stright forward the task.

Names are all

Please pay attention to the names you give to the functions/method.

When I use some kind of module or object, it is natural to me to call the initializer as the first thing.

But looking at your code, it would be the wrong way.

You just called a checkDiv().

Does is not just bad because it is unaspected, but aslo because if I look at your code, you didn't check the div!

    checkDiv: function () {
        if(this.parent.querySelector('.arrow-down')) {
            this.parent.removeChild(this.parent.querySelector('.arrow-down'));
        }
        this.initDiv();
    }

Here you search an element in the portion of the DOM tree of the this.parent, and if you find, you remove!

A check should be a safe action, not something that remove things.

I suggest you to call such method: reset()

This is more near to the task, and tell to other developer that if they call it they are going to loose what they have now.

Another thing is to remove from the method the last line:

 this.initDiv();

You should call reset() at the beginning of the init(), not the init() at the end of the reset().

And yes, about names: you don't need the Div part in the name in this case.

So, I would refactor your code as follow:

function createDownArrow() {
    const parent = document.querySelector('.landingItem');

    var arrow = parent.querySelector('.arrow-down'); 
    if (arrow) {
        parent.removeChild(arrow);
    }

    const element = document.createElement('div');

    element.classList.add('arrow-down');
    const borderWidth = parseInt(document.documentElement.clientWidth)/2;

    element.style.borderLeftWidth = borderWidth + 'px';
    element.style.borderRightWith = borderWidth + 'px';

    parent.appendChild(element);
}

window.addEventListener("load", createDownArrow);
window.addEventListener("reset", createDownArrow);
.arrow-down {
  margin-top: -2px;
  border-top: 60px solid #501a70;
  border-left: 160px solid transparent;
  border-right: 160px solid transparent;
}
<div class="landingItem"></div>

\$\endgroup\$
  • \$\begingroup\$ and yes it is the utility function \$\endgroup\$ – Sveta Jul 21 '17 at 13:23

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.