What can I do better?

$(function () {
    "use strict";

     *  Header text. Vergroot de tekst door op lees meer te klikken
    var heading = $(".header .text h2").height(),
        firstP = $(".header p:first").height(),
        areaSmall = heading + firstP,
        areaBig = $(".header .text").height(),
        box = $(".header .text"),
        readmore = $('.header .text a[title*="meer"]'),
        closeBox = $('.header .text a[title*="Sluit"]');

    $(".header .text").css({ height: areaSmall });

    readmore.click(function () {
        $(".header .text").animate({ height: areaBig }, 1000, "easeInQuart", function () {
            $(".header .text p").css({ visibility: "visible" });

    closeBox.click(function () {
        $(".header .text").animate({ height: areaSmall }, 1000, "easeOutQuart", function () {
            $(".header .text p").css({ visibility: "hidden" });
            $(".header .text p:first").css({ visibility: "visible" });


migrated from codereview.meta.stackexchange.com Nov 4 '11 at 12:39

This question came from our discussion, support, and feature requests site for peer programmer code reviews.


You have lots of calls to $(".header .text"), which you can replace with calls to box:

var areaBig = box.height();

Similarly, you can simplify calls to selectors within box, such as:

// Better than: readmore = $('.header .text a[title*="meer"]')
var readmore = box.find('a[title*="meer"]');

// Better than: $(".header .text").animate(...
box.animate({ height: areaBig }, 1000, "easeInQuart", function () {
  box.find("p").css({ visibility: "visible" });

These changes will not only make the code easier to read but also increase execution speed.

  • 1
    \$\begingroup\$ Just make sure that the definition of box comes first, or at least before any code that intends to use it. \$\endgroup\$ – cHao Nov 7 '11 at 19:12

I would:

  • call heights heights (or "ht"), as in areaSmall
  • name jQuery object to start with $, and use them for elements I reference multiple times
  • don't change names of things (header !== heading)
  • don't create variables that are only used once
  • remove unused variables (box)

Here's one take...

var $header = $('.header'),
    $text = $('.text', $header),
    $p = $('p', $text),
    areaSmallHt = $("h2", $text).height() + $("p:first", $header).height(),
    textHt = $text.height();

$text.css({ height: areaSmallHt });

$('a[title*="meer"]', $text).click(function () {
    $text.animate({ height: textHt }, 1000, "easeInQuart", function () {
        $p.css({ visibility: "visible" });

$('a[title*="Sluit"]', $text).click(function () {
    $text.animate({ height: areaSmallHt }, 1000, "easeOutQuart", function () {
        $p.css({ visibility: "hidden" });
        $("p:first", $text).css({ visibility: "visible" });

I am not sure of what you want to be better but, for better code legibility and maintainability I would say that you should replace all your string constants with variables:

var headerClassId = ".header";
var textClassId = ".text";
var aTextValueThatMeansSomething = "easeInQuart"; 

Do the same with all other constants:

var aVariableNameThatMeansSomething = 1000;
$(headerClassId +" "+textClassId).animate({ height: areaBig },
      aVariableNameThatMeansSomething , aTextValueThatMeansSomething, function ...
  • \$\begingroup\$ This really doesn't do much for legibility. headerClassId +" "+textClassId is less readable to me than ".header .text" -- for one thing, because it requires that i know the value and intent of headerClassId and textClassId. (Not to mention, *ClassId is a rather crappy name -- is it a class, or an id, or both? "Either" is a bad answer.) \$\endgroup\$ – cHao Nov 7 '11 at 19:18

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.