7
\$\begingroup\$

I have created a set of functions to calculate the aspect ratio of an image, set a height/width on document.ready and on window.resize resize the image. Now, I have got these functions working, however I feel like they could be more efficient/clean. So far I have to use cases. First, an image gallery I have setup and now, Iframes.

The below scriptfile includes the issue above, and some functionality for mobile, and an overlay.

Note: all these functions work (I do have an error imageResize('.com-background > img', ratio1); ratio1 is not defined within the window.resize function.

scriptfile.js

function mobile() {
    var ua = navigator.userAgent; var event = (ua.match(/iPad/i)) ? 'touchstart' : 'click';
    jQuery('.mobile-contain > a').bind(event, function() {
        jQuery('#menu-main').slideToggle(500);  
    });
};  


function overlay() {
    jQuery('.com-block').mouseenter(function() {
        jQuery(this).find('.com-title').css('display', 'none');
        jQuery(this).find('.com-desc').css('display', 'table-cell');
    });
    jQuery('.com-block').mouseleave(function() {
        jQuery(this).find('.com-title').css('display', 'table-cell');
        jQuery(this).find('.com-desc').css('display', 'none');
    });
}

function imageCalc(selector) {
    var obj=$(selector);
    var $imgWidth = obj.width();
    var $imgHeight = obj.height();
    var $imgAspectRatio =  $imgHeight / $imgWidth;
    // $(selector).css('margin-left', function( calcMargin ) { return parseInt($('.main-content').css('padding')) * -1 + "px"; }); fix for ie
    obj.css('margin-left', '-10px' );
    return $imgAspectRatio;
}
function setImageDims(selector, content_area, $imgAspectRatio) {
    var container = $(content_area);
    $(selector).css('height', function() { return $imgAspectRatio * container.width(); });
    $(selector).css('width', function() { return container.width() + 20; });    
}

function imageResize(selector, $imgAspectRatio) {
    $(selector).css('width', function() { return $("body").width() });
    $(selector).css('height', function() { return $imgAspectRatio * $(selector).width();  });
}

function cycleImages(){
  var $active = $('.com-background .active');
  var $next = ($active.next('img').length > 0) ? $active.next('img') : $('.com-background > img:first');
  $next.css('z-index',2);
  if ($('.com-background > img').length > 1) {
      $active.fadeOut(1500,function(){
      $active.css('z-index',1).show().removeClass('active');
          $next.css('z-index',3).addClass('active');
      });
  };
}

function setContainHeight() {
    var biggestHeight = "0";
    $(".com-background *").each(function(){
     if ($(this).height() > biggestHeight ) {
       biggestHeight = $(this).height();
     }
    });
    $(".com-background").height(biggestHeight);
}
function setImgMaxHeight() {
    var smallestHeight = "0";
    $(".com-background *").each(function(){
     if ($(this).height() > smallestHeight ) {
       smallestHeight = $(this).height();
     }
    });

    // Set the container height
    $(".com-background img").css('max-height', smallestHeight);
}

$( document ).ready(function() {
    $('.com-wrap').hide();

    ratio1 = imageCalc('.com-background > img');
    setImageDims('.com-background > img', '#main-content', ratio1);
    ratio2 = imageCalc('.blog-entry-content iframe');
    setImageDims('.blog-entry-content iframe', '#content', ratio2);
    // imageCalc('.com-background > img');
    // setImageDims('.com-background > img', '#main-content');
    // imageCalc('.blog-entry-content iframe');
    // setImageDims('.blog-entry-content iframe');
    setImgMaxHeight();
});
$(window).bind("load", function() {
    setContainHeight();
$('.com-more a').click(function(){
    $('.com-wrap').toggle();
    $('html, body').animate({
        scrollTop: $( $(this).attr('href') ).offset().top
    }, 500);
    return false;
}); 
    setInterval(function(){cycleImages()}, 7000);
    mobile();
    overlay();
});


$(window).resize(function() {
    setContainHeight();
    if ($("body").width() < 980 ) { 
        imageResize('.com-background > img', ratio1);
        // var ratio1 = imageCalc('.com-background > img');
        //  setImageDims('.com-background > img', '#main-content', ratio1);
    } else {
        setImageDims('.com-background > img', '#main-content');
    }
  if ($("body").width() < 770 ) {
    jQuery('.menu-main-container').css('display', 'none');
  }
  if ($("body").width() > 770 ) {
    jQuery('.menu-main-container').css('display', 'block');
  }
}).resize(); 

I have also created a rudimentary fiddle to illustrate the imageCalc(), setImageDims(), and imageResize() functionality here.

\$\endgroup\$
  • \$\begingroup\$ If you just set the width on an image, the height will automatically be set according to aspect ratio (and vice-versa if you set only the height), so I'm not sure what the code does. Just style the image with width:100%; height: auto or similar; no JS required at all. Meanwhile the jsfiddle just seems stretch and squash the image; I don't see it doing anything related to the aspect ratio. So it's all a little confusing. \$\endgroup\$ – Flambino Oct 10 '14 at 21:40
  • \$\begingroup\$ Flambino, an issue I have with using height: auto + width: 100% is the parent elements margin. I need the image to span the entire div, ignoring margin/padding. \$\endgroup\$ – Chris Oct 10 '14 at 22:54
  • \$\begingroup\$ Alright. Even so, it still smells to me like a CSS/HTML solution would be possible - and preferable. If the image must exceed the bounds of its parent element, it sounds to me like the image should simply have a different parent element. It may not be a possibility, but it'd certainly make things simpler. Again, I don't know the full context, but generally speaking I find that you very, very rarely need JavaScript to keep images sized correctly, since the browser can do that on its own given the proper markup and styling. \$\endgroup\$ – Flambino Oct 10 '14 at 23:26
5
\$\begingroup\$

There's a lot of code here, so I'll just start with a single function: setImageDims

  • I'm not a big fan of the name. For one, I rarely see the need to abbreviate words. For another, I'd just find resizeImage or maybe resizeToMatch to be more straightforward.
    Of course, you already have a resizeImage function which does the exact same thing - it's just hard coded to only use the body as its reference. But we'll get to that.

  • There's little system in your variable-naming. selector is fine (if non-descriptive), but content_area is snake_case whereas you use camelCase (which is the JavaScript convention) everywhere else. And you basically "rename" it to container inside the function, so perhaps that'd be a better name to begin with? And $imgAspectRatio would imply that it's a jQuery object. But it's just a simple number.
    Stranger still, when you actually call setImageDims in the resize event handler, you don't provide that value at all, meaning it's undefined. And undefined * container.width() is NaN - Not a Number. So nothing really makes sense there.

  • There's no need to use functions when setting the width/height. The functions don't add anything, so you can just use the plain values that the functions currently return:

    function resizeToMatch(image, container, aspectRatio) {
        var width = $(container).width();
        $(image).css('height', aspectRatio * width);
        $(image).css('width', width + 20);
    }
    

    I've also opted to fetch the width once and store it, instead of calling .width() twice.

  • Furthermore, you don't need to make that two lines, since .css() accepts an object as its argument:

    function resizeToMatch(image, container, aspectRatio) {
        var width = $(container).width();
        $(image).css({
            height: aspectRatio * container.width(),
            width: container.width() + 20
        });
    }
    

    This also avoids calling $(selector) twice (alternatively avoided by calling it once and storing the return value).

  • And, as mentioned in the comments, you don't even need both width and height. In this case, the width is the driver; it determines the height. In which case you simply don't set the height at all (or you set it to auto), and only set the width:

    function resizeToMatch(image, container) {
        $(image).css("width", $(container).width() + 20);
    }
    

    You'll notice that we've reverted back the original .css(property, value) usage since now there's only one property. You can still use an object, but there's little need now.
    And aspectRatio is gone, because we don't need it.

  • But why 20? Why resize the image almost proportionally, but not quite? This goes for much of the code; there are a lot of unexplained magic numbers. Presumably they're entirely dependent on how the page looks. Which in turn means your JavaScript is heavily dependent on style (this is an anti-pattern!). It also undermines the point of using an existing element as the scaling reference; isn't the image supposed to just match that other element's width as-is? And it of course doesn't scale proportionally anymore, which would presumably be the whole point.
    If you need to add 20px, there's something not quite right somewhere else (namely, the -10px margin in imageCalc). Perhaps you want .innerWidth() or .outerWidth() instead?

  • Now, back to your existing resizeImage function. Presuming you get rid of that hardcoded + 20 above, you don't need the imageResize function at all. You can just call

    resizeToMatch(image, document.body);
    

    This is also why you don't want magic numbers and weird super-specific stuff in your functions. As long as you have that + 20 there, you'll have to have 2 separate functions that essentially do the same thing. Without the magic number, you have just 1 function that does 1 thing but has many uses. You'll note I'm not passing a string selector for the 2nd argument; I'm passing a plain DOM object. Since the function calls $() with the argument, the argument can be anything that $() accepts. Namely raw DOM objects, string selectors, and other jQuery objects.

This was basically a review of one 3-line function. Here's a quicker one of imageCalc. Many of the same note apply, but of course you don't really need this function at all. Let the browser handle the aspect ratios and proportional scaling. Still, there are other points.

  • imageCalc - again with the abbreviations. But this time, it's also misleading. It doesn't just calculate, it also modifies the element you give it. This is a no-no. Calling jQuery's .width() doesn't also turn the background red, nor should your functions do anything except what it says on the tin.
    Of course, on the tin it says that it "calculates an image"; it actually calculates an aspect ratio.

  • Your variable-naming is exactly backwards. None of the $-prefixed variable are jQuery objects (which is what the dollar sign is meant to imply), while the only jQuery object is just called obj.

  • Your aspect ratio calculation is technically backwards too. An aspect ratio is width over height, not the other way around. Hence why displays are labelled 4:3, 16:9 and so forth. You do it the other way because that's what you need later, but in a sense that's not this function's problem. Besides, all you need to get the inverse aspect ratio is to, well, invert it: 1 / aspectRatio.

Rewritten, it might look like:

function calculateAspectRatio(element) {
    var $element = $(element);
    return $element.width() / $element.height();
}

I'd continue, but I'm out of time.

\$\endgroup\$
  • \$\begingroup\$ This is exactly the constructive criticism I was hoping for, the only way I will become a better programmer. I genuinely appreciate the time. \$\endgroup\$ – Chris Oct 11 '14 at 19:32
  • \$\begingroup\$ @Chris No problem, glad it was helpful! (By the way, I spotted and corrected some dumb mistakes in the code blocks in my answer. Just in case you've been looking at the code and wondering what the heck was going on.) \$\endgroup\$ – Flambino Oct 11 '14 at 23:13
3
\$\begingroup\$

This line is too long:

var ua = navigator.userAgent; var event = (ua.match(/iPad/i)) ? 'touchstart' : 'click';

It would be better to split into two:

var ua = navigator.userAgent;
var event = (ua.match(/iPad/i)) ? 'touchstart' : 'click';

You can also omit the redundant parentheses there and simplify to:

var event = ua.match(/iPad/i) ? 'touchstart' : 'click';

In this code:

var biggestHeight = "0";
$(".com-background *").each(function(){
 if ($(this).height() > biggestHeight ) {
   biggestHeight = $(this).height();
 }
});

Why use a string for biggestHeight? Maybe you meant this:

var biggestHeight = 0;

This looks like copy-paste error:

var smallestHeight = "0";
$(".com-background *").each(function(){
 if ($(this).height() > smallestHeight ) {
   smallestHeight = $(this).height();
 }
});

Probably you forgot to flip the >, and meant to write this instead:

var smallestHeight = 0;
$(".com-background *").each(function(){
 if ($(this).height() < smallestHeight ) {
   smallestHeight = $(this).height();
 }
});
\$\endgroup\$

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.