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?

Quill
12k5 gold badges41 silver badges93 bronze badges
asked Jun 14, 2014 at 15:33
\$\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.");
 }
 });
}
answered Jun 14, 2014 at 16:09
\$\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\$ Commented 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\$ Commented Jun 14, 2014 at 16:36

Your Answer

Draft saved
Draft discarded

Sign up or log in

Sign up using Google
Sign up using Email and Password

Post as a guest

Required, but never shown

Post as a guest

Required, but never shown

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

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.