1
\$\begingroup\$

I'm making a website for a friend (just for fun) where we have to move a lot of svg images in absolute positions. To make it easier I wrote some code to calculate the % position of each element so we can do it faster.

I'm wondering what you think about it and what can I do to improve it. And also any improvements to my style of coding are welcome.

<html>

<head>
    <!-- Just some style to differentiate the object -->
    <style>
        .draggable{
            width: 10px;
            height: 10px;
            background: red;
        }
    </style>
</head>

<body>
    <!-- The item that we will be moving around -->
    <div class="draggable"></div>
    <script>

    class Mover{
        constructor(htmlObject,parent, position){
            // Store the object to move
            this.htmlObject = htmlObject;
            // The parent of the object
            this.parent = parent;
            // The type of position : 'relative'/'absolute'
            this.position = position;
            // Modify the elements and start the listeners
            this.init();
        }

        init(){
            // Make the item draggable
            this.htmlObject.draggable = 'true';
            // Set its position type to the user input
            this.htmlObject.style.position = this.position;
            // Modify its zIndex to make sure that you can always see where it is.
            this.htmlObject.style.zIndex = '9999';
            // Add some border just make it easier to find the item.
            this.htmlObject.style.border = '1px solid black';
            // Add the listeners for the drop and drag events to the parent
            this.parent.ondrop = (event)=>{ this.drop(event) };
            this.parent.ondragover = (event)=>{ this.allowDrop(event) };
        }

        drop(e){
            e.preventDefault();
            let pos = this.calculatePos(e.x, e.y);
            // Set the object position equal to the event x and y pos in %
            this.htmlObject.style.top = pos.h+'%';
            this.htmlObject.style.left = pos.w+'%';
        }

        allowDrop(e){
            // Allow to drop.
            e.preventDefault();
        }

        calculatePos(w,h){
            // Get the parent Width and Height
            let mx = this.parent.offsetWidth;
            let mh = this.parent.offsetHeight;
            // -- Just testing stuff --
            // let mx = innerWidth;
            // let mh = innerHeight;
            // ------------------------
            // Calculate the % based on the parent size.
            return {w : (w/mx) * 100, h : (h/mh) * 100}
        }

    }

    // Initialize the movable item.
    const draggableElement = new Mover(document.querySelector('.dragable'),document.querySelector('body'),'absolute')   
    </script>
</body>

\$\endgroup\$
  • \$\begingroup\$ Is this code complete? I see draggableElement is assigned a newly created instance of Mover; however I do not see it invoked/referred/used anywhere. \$\endgroup\$ – Igor Soloydenko May 10 '18 at 4:51
  • \$\begingroup\$ In the current code you only need to declare it, the constructor calls the init function, probably i should use draggableElement.init() instead of having the init at the constructor! \$\endgroup\$ – ithan May 10 '18 at 11:47
1
\$\begingroup\$

About Constructor

It is considered a bad practice for a constructor to have side effects for multiple reasons. You can browse numerous resources online on this subject. Here are a few whys:

  • The reader/maintainer of the code does not expect a constructor to have any side effects;
  • Such constructor is hard or impossible to test;
  • It violates a whole bunch of principles, including Principle of least astonishment, SRP.

So, yeah instead of

constructor(htmlObject,parent, position){
  // ...
  this.init();
}

you should rather create an instance and invoke the init() on it; like this

const draggableElement = new Mover(
  document.querySelector('.dragable'),
  document.querySelector('body'),
  'absolute',
);
draggableElement.init();

If you are not planning to add the function that disables "moving" of an element, you may even not need a local variable holding the reference to a Mover:

new Mover(
  document.querySelector('.dragable'),
  document.querySelector('body'),
  'absolute',
)
.init();

Naming Things

Naming is important. Method init() is not informative. You may want to rename it to something like enableElementDragging(). A symmetric counter part is now too easy too name – disableElementDragging().


Nano variable names are evil. What is e? An error? An exception? An event? To figure that out I need to look at a place where the function is being invoked. That's mental burden.

Same applies to pos, w, and h... If you are in VS Code or any other smart editor, it's a matter of hitting F2 and renaming it.

Names mx and mh are particularly unfortunate since they are meant to be symmetric to/consistent with w and h. Thus, mx should be mw! Also, I could not decipher what the m prefix means in both of these variables.

Just spell things out: position, width, height, parentWidth, parentHeight, calculatePosition().

\$\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.