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