4
\$\begingroup\$

I wrote this method and I would like to refactor it so it is easier to understand and looks cleaner. However, I don't know how or where to start. If you could give it your best shot and explain your approach to how you did it, it would be very helpful to me. The function reduces the size of elements until the parent container reaches a certain height (in this case 25 or less).

function _AdjustHeightOwlDots() {
    let dotRowHeight = $(`#${get(this, 'elementId')} .owl-carousel .owl-dots`).height();
    if (!dotRowHeight || dotRowHeight === 25 || dotRowHeight === 0) {
      return;
    }
    this._waitFor(`#${get(this, 'elementId')} div.owl-dots > button > span`).then(() => {
      if (dotRowHeight && dotRowHeight > 25) {
        for (let index = 0; index < 10; index += 1) { // default size of dot is 10x10px
          // first the margin because it looks better
          document.querySelectorAll(`#${get(this, 'elementId')} div.owl-dots > button > span`).forEach((el) => {
            const elCopy = el;
            const elStyle = elCopy.currentStyle || window.getComputedStyle(elCopy);
            if (parseFloat(elStyle.margin, 10) > 1) {
              elCopy.style.margin = `${parseFloat(elStyle.margin, 10) - 1}px`;
            }
          });
          dotRowHeight = $(`#${get(this, 'elementId')} .owl-carousel .owl-dots`).height();
          if (dotRowHeight > 25) {
            document.querySelectorAll(`#${get(this, 'elementId')} div.owl-dots > button > span`).forEach((el) => {
              const elCopy = el;

              if (el.offsetHeight > 1) {
                elCopy.style.height = `${el.offsetHeight - 1}px`;
                elCopy.style.width = `${el.offsetHeight}px`;
              }
            });
          }
          const el = document.querySelector(`#${get(this, 'elementId')} div.owl-dots > button > span`);
          const elStyle = el.currentStyle || window.getComputedStyle(el);

          if (el.offsetHeight === 1 && parseFloat(elStyle.margin, 10) <= 1) { // cant get smaller
            break;
          }
          dotRowHeight = $(`#${get(this, 'elementId')} .owl-carousel .owl-dots`).height();

          if (dotRowHeight <= 25) { break; }
        }
      }
    })
      .catch((/* e */) => {
      //
      });
  },

Edit: Follow up: Find size for elements to fit in certain width

\$\endgroup\$
1
  • 1
    \$\begingroup\$ Welcome to Code Review! Please do not update the code in your question to incorporate feedback from answers, doing so goes against the Question + Answer style of Code Review. This is not a forum where you should keep the most updated version in your question. Please see what you may and may not do after receiving answers. \$\endgroup\$ – Heslacher Dec 11 '19 at 8:02
2
\$\begingroup\$

It looks difficult to decypher indeed. Let's try it though. Some little bits which came to my mind at first glance:

  • I'd recommend using 4 spaces of indentation
  • maybe it's personnal taste but I would try not mixing jquery and vanilla js
  • as I understood, this keyword always refers to the same object, doesn't it? in that case maybe you could declare the selectors at the top of the function?

    const thisElementId = get(this, 'elementId');
    const selectorOwlDots = `#${thisElementId} .owl-carousel .owl-dots`;
    const selectorButtonSpans = `#${thisElementId} div.owl-dots > button > span`;
    
  • declarations of const elCopy = el; are useless, just refer to el and rename it to something more meaningful

  • the handler of _waitFor should be declared as a function:

    this._waitFor( selectorButtonSpans ).then( changeSizes ).catch( handleError );
    
    function changeSizes() {
        ...
    }
    
    function handleError( err ) {
        ...
    }
    
  • you can also simplify the initial exit condition a bit

    let dotRowHeight = $(selectorOwlDots).height() || 25;
    if ( dotRowHeight <= 25 ) {
        return;
    }
    
  • you have two initial checks on dotRowHeight - I think you could reduce them to just one ( one is the one I simplified, the other is inside the _waitFor handler )

That should make the code a bit more readable BUT I think this code is doing the wrong thing altogether. The problem is - you are changing the size of elements in a loop until some condition is met. This is a bad practice - I've seen people ending up with infinite loops when doing such things. What I think you should do instead is:

  • read all the sizes you need
  • calculate sizes to apply to elements
  • apply sizes
  • if anything breaks - you do not have an infinite loop of elements jumping up and down on your page

Always try to minimize the number of interactions between your script and the UI.

\$\endgroup\$
2
  • \$\begingroup\$ Hey thanks a lot for the answer. I will implement your feedback. In case of the const elCopy = el, I use a linter and it recommends to do it like this. The rule is called no-param-reassign. \$\endgroup\$ – Gautama Dec 10 '19 at 12:15
  • \$\begingroup\$ Follow up here : codereview.stackexchange.com/q/233848/214731 \$\endgroup\$ – Gautama Dec 11 '19 at 8:35

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.