Breaking up a navigation menu

Question

This code works as I intended, but I am definitely a JS noob (C++ background), so I'm not sure if this is the best way to do it (for example using .each instead of some kind of a for loop was completely new to me).

https://github.com/TaylorHuston/menuBreak/blob/master/menuBreak.js

function menuBreak() {

var windowWidth = $(window).width();

var menuLength = 0;

$("#navUl > li").each( function (index, element) {
    menuLength += $(this).width();
});

if( menuLength > windowWidth*.9 ) {
    var newWidth= menuLength*.7;
    $('#navUl').css({"maxWidth":newWidth});
} else {
    $('#navUl').css({"maxWidth":windowWidth});
}

}

And in my HTML, not sure if this is the 'best' way to call it

<script>
  $(document).ready(function() {
    menuBreak();
  });

  $(window).resize(function() {
    menuBreak();
  });
</script>

Answer 1

From a once over:

• You could have used a loop instead of .each(), it would be marginally faster
• Your indenting is off, I assume you had trouble pasting the code
• .9 and .7 are magic constants, you should declare these with var and a good name

• I would use a ternary for this:

  if {
      var newWidth= ;
      $('#navUl').css({"maxWidth":newWidth});
  } else {
      $('#navUl').css({"maxWidth":});
  }
  

  could be

  var navWidth = ( menuLength > windowWidth *.9 ) ? menuLength *.7 : windowWidth;
  $('#navUl').css({"maxWidth":navWidth});
  

  of course with properly named constants this might stretch too much to be easily readable, up to you.

• With jQuery, the most common way to trigger on ready is

  $( function(){
    //Do Something
    menuBreak();
  });
  

  now, since $() expects a function, we might as well pass menuBreak immediately:

  $( menuBreak );
  

  The same things goes for resize:

  $(window).resize( menuBreak );