Calculating absolute position in %

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>

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

<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>


• 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. – Igor Soloydenko May 10 '18 at 4:51
• 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! – ithan May 10 '18 at 11:47

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.

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().