2
\$\begingroup\$

I made a function in jQuery for my home-made grid system. When a row has too many columns in it, it removes the HTML code from that row.

Normally in my grid system it fits 12 columns in 1 row, but with the function this can easily be changed.

One column with the size of 1 (declared as: column size-1) has a width of 1/12 (or 8.3333%).

function validateRowWidth() {
    var _row = $('.row');
    _row.each(function (index, element) {
        var totalWidth = 0,
            _columnSize = $(element).find('[class*="size-"]');
        _columnSize.each(function (index, element) {
            var width = $(element).outerWidth(true),
                inProcent = Math.round((width / $(element).parent().width()) * 100);
            totalWidth += inProcent;
        });
        if (totalWidth > 100) {
            console.log('You have to many columns into your '+(index + 1)+'th row');
            $(element).remove();
        }
    });
}

Is there any way I can improve this?

\$\endgroup\$

1 Answer 1

1
\$\begingroup\$

Things I pondered:

  • What's with the underscores everywhere?
  • Why not use regular jQuery convention and use this instead of element in your each iterator?
  • What's the use-case for this? A regular user will never see the console, so logging to it doesn't help anyone.
  • Even if someone is watching the console, what's the point of warning them when the row is just summarily removed? At least give the user a chance to fix the problem. It's worse if you're not looking at the console - then stuff is just gone with no explanation.
  • And the warning, "You have too many columns", isn't really truthful. The problem is that the columns - even if it's just 1 - are too wide. You code isn't checking how many there are.
  • But mostly: Why use percentages? Just compare widths directly. Saves you a whole lot of trouble (and code)

This seems clearer to me:

function validateRowWidth() {
  $('.row').each(function (index) {
    var row = $(this), // use `this`
        widthSum = 0;

    // sum up the (direct descendant) column widths
    row.children('[class*="size-"]').each(function () {
      widthSum += $(this).outerWidth(true); // 1 line is all you need here
    });

    // compare and warn if need be
    if(widthSum > row.width()) {
      // alert is not a great choice, but it's better than console.log,
      // if this is user-facing. Explain the problem, and propose solutions.
      alert("Row " + index + " is too wide. Remove one or more columns or make them narrower.");
    }
  });
}
\$\endgroup\$
2
  • \$\begingroup\$ First of all, thank you for your time :). And yeah, i dont know either, it made the code more clear for me. And some guy told me that the element argument was more precise then $(this). Yeah and that grammatica wasnt my strongest point either. Doing this with widths is going alot better indeed, but i had to call Math.floor on the widthSum += to make it actually work. \$\endgroup\$
    – Bas
    Jun 14, 2014 at 16:26
  • 1
    \$\begingroup\$ @Dreiba No prob. Wait and see if there are a other answers, otherwise hit the checkmark. As for element being more precise, that's just nonsense. Sure, this changes value based on context, so this in the outer function is not the same as this in the inner functions. But that's what we want. And the same thing value-changing thing happened it your code anyway, because you called the argument element in both the inner and outer function, so the latter shadowed the former. \$\endgroup\$
    – Flambino
    Jun 14, 2014 at 16:36

Your Answer

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

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