Sign up ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Would it serve much purpose to work at refactoring the latter parts of this code?

// This part is OK
function isScrolledIntoView(elem, trigger_height) {

  var docViewTop = $(window).scrollTop();
  var docViewBottom = docViewTop + $(window).height();
  var elemTop = elem.offset().top;

  if (trigger_height == false) {
    trigger_height = elem.height()
  }
  var elemBottom = elemTop + trigger_height;

  return ((elemBottom <= docViewBottom) && (elemTop >= docViewTop));
}

// This function happens when the page first loads
$("p").addClass('hidden').each(function() {
  $this = $(this);
  if (isScrolledIntoView($this, false)) {
    $this.removeClass("hidden");
  }
});

// This function happens on scroll, but basically repeats itself    
$(window).off('scroll').scroll(function() {
  $("p.hidden").each(function() {
    $this = $(this);
    if (isScrolledIntoView($this, false)) {
      $this.removeClass("hidden")
    }
  });
});

To me, it seems like there isn't – but at the same time its repetitive:

$this = $(this);
if (isScrolledIntoView($this, false)) {
  $this.removeClass("hidden")
}

Is there a general rule that anyone follows such as "only refactor if you're looking at > 5 lines of repeated code", or should you be refactoring absolutely everything as you go?

share|improve this question
    
The real question is what you want. If you feel it should be re-factored, re-factor it. If you feel it's OK, leave it be. –  Mast Jun 16 at 10:37

1 Answer 1

I can't really see much to refactor in your code except a few things, so I'll go through those.


if (trigger_height == false)

could be if (!(trigger_height))


$this = $(this);
if (isScrolledIntoView($this, false)) {
    $this.removeClass("hidden");
}

I don't really see why you need to initialise $this, you could just use $(this) later on instead.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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