4
\$\begingroup\$

Currently I am working on a project for which I want to have the different links (div elements) placed randomly on my webpage. To accomplish this, I first generate a set of coordinates and later check whether they overlap (SO thread, user @Robson helped me figure things out).

However, since this is my first JavaScript project that is longer than 15 lines of code, I am am sure to have missed some best-practices or simply took a too complicated approach. I am happy to learn about my mistakes and how to come up with more efficient or clean code!

Maybe I can describe my thought process a little:

  • getMaxDimension: in Order to have the divs not overflow the site, I need to only spawn them in coordinates far enough away from the right and bottom border of the screen, since coordinates are calculated from the top left corner.
  • getOffset: To check whether two divs overlay, I check if any point of the polygon of div a is inside div b. To do that I calculate all the coordinates and store them in an object.
  • getOverlap: basically just checks if div a's points and div b's points are inside of each other. The code seems rather complicated, but is just an implementation of the mathematical notation
  • getChar: (I really think there are better ways to do this); In order to control every div uniquely, they all need different ID's. However, id=1, id=2 and so on didn't work (maybe they are not allowed?). Now in order to "convert" iteration variables i and j to the corresponding div I just chose letters.

MWE:

 // Returns largest div's width and height
 function getMaxDimension(arr) {
   var maxWidth = 0;
   for (var i = 0; i < div_selection.length; i++) {
     if (div_selection[i].offsetWidth > maxWidth) {
       maxWidth = div_selection[i].offsetWidth;
     }
   }
   var maxHeight = 0;
   for (var i = 0; i < div_selection.length; i++) {
     if (div_selection[i].offsetHeight > maxHeight) {
       maxHeight = div_selection[i].offsetHeight;
     }
   }
   var values = {
     maxWidth: maxWidth,
     maxHeight: maxHeight
   };
   return values;
 }

 // Retruns a random number x; min < x < max
 function getRandomNumber(min, max) {
   return Math.random() * (max - min) + min;
 }

 // returns the position in xy-space of every corner of a rectangular div
 function getOffset(element) {
   var position_x = element.offsetLeft;
   var position_y = element.offsetTop;
   var height_x = element.offsetWidth;
   var height_y = element.offsetHeight;
   var tolerance = 0; // will get doubled
   return {
     A: {
       y: position_y - tolerance,
       x: position_x - tolerance
     },
     B: {
       y: position_y + height_x + tolerance,
       x: position_x - tolerance
     },
     C: {
       y: position_y + height_x + tolerance,
       x: position_x + height_y + tolerance
     },
     D: {
       y: position_y - tolerance,
       x: position_x + height_y + tolerance
     }
   };
 }


 // Returns true if any corner is inside the coordinates of the other div
 function getOverlap(div1, div2) {
   coor_1 = getOffset(document.getElementById(div1));
   coor_2 = getOffset(document.getElementById(div2));
   return (
     (coor_1.A.x <= coor_2.A.x && coor_2.A.x <= coor_1.D.x) && (coor_1.A.y <= coor_2.A.y && coor_2.A.y <= coor_1.B.y) ||
     (coor_1.A.x <= coor_2.B.x && coor_2.B.x <= coor_1.D.x) && (coor_1.A.y <= coor_2.B.y && coor_2.B.y <= coor_1.B.y) ||
     (coor_1.A.x <= coor_2.C.x && coor_2.C.x <= coor_1.D.x) && (coor_1.A.y <= coor_2.C.y && coor_2.C.y <= coor_1.B.y) ||
     (coor_1.A.x <= coor_2.D.x && coor_2.D.x <= coor_1.D.x) && (coor_1.A.y <= coor_2.D.y && coor_2.D.y <= coor_1.B.y)
   );
 }

 // Number to Letter
 function getChar(n) {
   var ordA = 'a'.charCodeAt(0);
   var ordZ = 'z'.charCodeAt(0);
   var len = ordZ - ordA + 1;

   var s = "";
   while (n >= 0) {
     s = String.fromCharCode(n % len + ordA) + s;
     n = Math.floor(n / len) - 1;
   }
   return s;
 }

 var div_selection = document.getElementsByClassName("random");

 maxDimensions = getMaxDimension(div_selection);
 var widthBoundary = maxDimensions.maxWidth;
 var heightBoundary = maxDimensions.maxHeight;

 for (var i = 0; i < div_selection.length; i++) {
   var isOverlapping = false;
   var attemptCount = 0;
   do {
     randomLeft = getRandomNumber(0, window.innerWidth - widthBoundary);
     randomTop = getRandomNumber(0, window.innerHeight - heightBoundary);
     div_selection[i].style.left = randomLeft + "px";
     div_selection[i].style.top = randomTop + "px";     
     isOverlapping = false;
     for (var j = 0; j < i; j++) {
        if (getOverlap(getChar(i), getChar(j))) {
        isOverlapping = true;
        break;
      }
     }
   } while (++attemptCount < 50 && isOverlapping);
 }

 // check every element
 for (var i = 0; i < div_selection.length; i++) {
   for (var j = i + 1; j < div_selection.length; j++) {
     console.log(i, j)
     console.log(getChar(i), getChar(j))
     console.log(getOverlap(getChar(i), getChar(j)))
   }
 }
div {
  width: 60px;
  height: 60px;
  position: absolute;
}

#a {
  background-color: pink;
}

#b {
  background-color: lightblue;
}

#c {
  background-color: lightgreen;
}

#d {
  background-color: silver;
}

#e {
  background-color: yellow;
}
<html>
  <body>
    <div class="random" id="a">Div1</div>
    <div class="random" id="b">Div2</div>
    <div class="random" id="c">Div3</div>
    <div class="random" id="d">Div4</div>
    <div class="random" id="e">Div5</div>
  </body>
</html>

\$\endgroup\$

1 Answer 1

3
\$\begingroup\$

Style

Some styling points

  • Names. You have some poor naming

    • The JavaScript naming convention is camelCase avoid using snake_case
    • Use the context of the function to imply additional meaning. Eg you have maxWidth and maxHeight in a function called getMaxDimension. As they are the only variables (appart from i) declared and as such can just be width and height
    • getRandomNumber could be randomNumber or randomNum or randNum
    • arr Avoid naming variables after their type. arr could be elements. Also always try to name arrays or array like objects using plurals.
  • Don't repeat code. The two loops in getMaxDimension can be just one loop.

  • Don't add unused code. The variable arr in getMaxDimension(arr) is never used. You access the array of elements from its global name. div_selection

  • Avoid single use variable unless they help reduce long cluttered lines.

  • When finding only a max value use Math.max rather than an if statement. Same with minimums Math.min

  • Comments should not state the obvious. If you must add comments make sure they are grammatically correct.

    DO NOT add comments that directly conflict with the code they are commenting on. You have // Retruns a random number x; min < x < max but the function returns min <= x < max Anyone reading this code will be unsure of your intent.

  • Use short form where possible.

    • Use for of loops over for loops unless you need the index or if performance is super critical.
    • Use property shorthand to define object literals values = {maxWidth: maxWidth, maxHeight: maxHeight}; can be values = {maxWidth, maxHeight};
    • Use arrow functions for simple one line functions.
    • You seldom need to reference window as it is the default object. Eg window.innerWidth is the same as innerWidth. You dont add window when accessing other window properties like window.document so why selectivly do it with others?
    • Use destructure assignment when pulling properties from object or arrays. EG const {maxWidth, maxHeight} = getMaxDimension(div_selection)

Rewrite

This rewrites only parts of your code as your code is far too complex for what it does, the example at the bottom of answer does not use any of your code,

The function getRandomNumber, getChar, getMaxDimension have been rewritten and renamed.

const randNum = (min, max) => Math.random() * (max - min) + min;
function getMaxSize(elements) {
    var width = 0, height = 0;
    for (const el of elements) {
        width = Math.max(width, el.offsetWidth);
        height = Math.max(height, el.offsetWidth);
    }
    return {widtyh, height};
}
function intToBase(n, digits = "abcdefghijklmnopqrstuvwxyz") {
    const base = digits.length, result = [];
    do {
        result.push(digits[(n |= 0) % base]);
        n = n / base;
    } while (n > 0);
    return result.reverse().join("");
}

Fitting boxes

This is a common problem in computer science and is a hard problem to solve depending on the constraints.

I will assume that the given elements can fit the area.

That is the area of the elements to fit is less than the area to fit and that the longest edge of the elements to fit is less than the longest corresponding axis of the area to fit.

This at least guaranties that one element can be placed. The algorithm will have average of 50 tries per box if it can not find a position.

Some Notes

  • You can use getBoundingClientRect to get the bounds of a element.

  • The object Bounds defines the bounding boxes and provides function to check for overlap, position, and finally place the element if needed.

  • Elements are first added to the array placing. Each element is then checked for overlap against elements in the array fitted. If no overlaps found the element is moved to fitted and removed from placing

  • Once you have the elements there is no need to query the DOM every time you need to know where it is. The elements you store in the array div_selection are references to the elements so any changes you make will be accessible as the stored reference.

  • The example allocates 50 tries per box. If a box does not use all its tries they become available for others to use.

  • Only if the box can be placed its color is changed to red. Boxes that could not be moved remain in the top left and are colored black.

;(() => {
"use strict";
const TRIES_PER_BOX = 50;
const randUint = range => Math.random() * range | 0;
const placing  = [...document.querySelectorAll(".random")].map(el => Bounds(el, 5));
const fitted = [];
const areaToFit = Bounds();
var maxTries = TRIES_PER_BOX * placing.length;
while (placing.length && maxTries > 0) {
    let i = 0;
    while (i < placing.length) {
        const box = placing[i];
        box.moveTo(randUint(areaToFit.w - box.w), randUint(areaToFit.h - box.h));
        if (fitted.every(placed => !placed.overlaps(box))) {
            fitted.push(placing.splice(i--, 1)[0].placeElement());
        } else { maxTries-- }
        i++;
    }
} 
function Bounds(el, pad = 0) {   
    const box = el?.getBoundingClientRect() ?? {
        left: 0, top: 0, 
        right: innerWidth, bottom: innerHeight, 
        width: innerWidth, height: innerHeight
    };
    return {
        l: box.left - pad, 
        t: box.top - pad, 
        r: box.right + pad, 
        b: box.bottom + pad,
        w: box.width + pad * 2,
        h: box.height + pad * 2,
        overlaps(bounds) { 
            return !(
                this.l > bounds.r || 
                this.r < bounds.l || 
                this.t > bounds.b || 
                this.b < bounds.t
            ); 
        },
        moveTo(x, y) {
            this.r = (this.l = x) + this.w;
            this.b = (this.t = y) + this.h;
            return this;
        },
        placeElement() {
            if (el) {
                el.style.top = (this.t + pad) + "px";
                el.style.left = (this.l + pad) + "px";
                el.classList.add("placed");
            }
            return this;
        }
    };
}
})();
.random {
  position: absolute;
  margin: 2;
  border: 1px solid black;
  font-size: xx-large;
  top: 0px;
  left: 0pc;
  
}
.placed {
  color: red;
  border: 1px solid red;
}
<div class="random">Div 1</div>
<div class="random">Div 2</div>
<div class="random">Div 3</div>
<div class="random">Div 4</div>
<div class="random">Div 5</div>
<div class="random">Div 6</div>
<div class="random">Div 7</div>
<div class="random">Div 8</div>
<div class="random">Div 9</div>
<div class="random">Div 10</div>
<div class="random">Div 11</div>
<div class="random">Div 12</div>

\$\endgroup\$
1
  • \$\begingroup\$ Thank you for your very thorough answer! I have learned a lot of new things from your post :) \$\endgroup\$
    – timlwsk
    Aug 15, 2021 at 12:22

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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