# Refactor/Review JS function which moves a banner on window resize

Okay, so I wrote this function yesterday for one of our websites here at the office, the site is responsive, but one of the banners wasn't behaving at a certain screen size (was getting chopped off when screen size was less than 1067px). Basically, this script checks for window size of 1067px (higher than 1068px, banner is fine) to 980px (lower than 981, banner is hidden)

function updateReadyGoBG(){
var vpWidth, difference, position = 0;
vpWidth = jQuery(this).width();

if(vpWidth > 980 && vpWidth < 1067){
difference = (position - (600 - (vpWidth - position) ) ) + 100;
}
}


Any ideas on how to improve this to improve performance or reduce calls? Any review, refactoring or just general thoughts would be highly appreciated. What did I do wrong? What would you change?

Thanks all!

EDIT: adding rudimentary html snipped this acts on

<style>
.banner-container{ width:1000px; height:150px; }
.banner{ width: 100% }
</style>

<div class="banner-container">
<div class="banner">
</div>
</div>

-

Ok, assuming:

• You didn't use media queries for cross-browser reasons
• This function is already bound to a resize event, since you didn't include what triggers the function call
• Assuming that this is the window object, judging from the way you call it vp as in viewport and getting its width.

Then:

function updateReadyGoBG(){

//you can actually merge declaration and assignment
var vpWidth   = jQuery(this).width(),
//cache the ready_go_bg jQuery object to avoid requerying
position  = 0,
difference;

if(vpWidth > 980 && vpWidth < 1067){

//you could use position instead of getting the CSS left
//to avoid getting a string, and chopping off the "px" part

difference = (position - (600 - (vpWidth - position) ) ) + 100;

//sadly, position is just a "getter".
//you could use .offset(value) but calculating relative
//to your offset parent would be too much work
//we use .css(prop,value) to set the left value
//if unit wasn't specified, jQuery assumes it's in pixels
}
}


function updateReadyGoBG() {
var vpWidth = jQuery(this).width(),
position = 0,
difference;
if (vpWidth > 980 && vpWidth < 1067) {
difference = (position - (600 - (vpWidth - position))) + 100;

• Never forget ; at every line. You may run into issues during minification if you forget them
• Though personal preference, but it's cleaner to use ' instead of ".
1. Use window instead of this, because it's confusing where this points to without the context where the function is bound. 2. readyGoBg.css('left' add 'px' to the end. Otherwise it's not clear what you're setting (percentages, unicorns?) –  Jan Jongboom Nov 8 '12 at 13:35
Most of your assumptions are correct. Media queries are being used, but the banner was breaking on active resize of the browser window. This function is indeed already bound to a jQuery(window).resize() event - thus the this instead of window to save a call. Thank you for this! More than I expected. –  jsanc623 Nov 8 '12 at 18:22