Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I am currently building a web app where I need to randomly switch the font and color of a title (inside the #content div) and the background of the page every time a button is clicked.

I wrote the following code that works perfectly, but I'm wondering if it could be optimized (or at least written in a more elegant way).

// Switch font randomly
var font = ["lobster", "shadows", "oswald", "josefin", "gloria", "pacifico"];
var color = ["blue", "purple", "yellow", "red", "orange", "green"];
var background = ["bg-blue", "bg-purple", "bg-yellow", "bg-red", "bg-orange", "bg-green"];

$("#try-me").click(function() {
    $("#content")
        .hide()
        .removeClass()
        .addClass(font[Math.floor(Math.random()*font.length)]) // Font
        .addClass(color[Math.floor(Math.random()*color.length)]) // Color
        .fadeIn(600);

    $("html")
        .removeClass()
        .addClass(background[Math.floor(Math.random()*background.length)]); // Background
});
share|improve this question
1  
It's pretty clear and elegant as it is. –  Carlos Aug 12 at 13:14
    
I agree with Carlos, looks clear and elegant as it is. –  jsanc623 Aug 12 at 14:31
1  
Perhaps you should ensure that the randomly chosen background and foreground colours are different from each other. –  200_success Aug 12 at 20:57

3 Answers 3

up vote 5 down vote accepted

The repetition of getting a random font, color, background class cries to be extracted to a utility function:

function pickRandom(arr) { 
    return arr[Math.floor(Math.random() * arr.length)];
}

$("#try-me").click(function() {
    $("#content")
        .hide()
        .removeClass()
        .addClass(pickRandom(font))
        .addClass(pickRandom(color))
        .fadeIn(600);

    $("html")
        .removeClass()
        .addClass(pickRandom(background));
});

I also agree with @MainMa about defining background in terms of color.

Other than that, I think it's nice code!

Ensuring change

If you want to make sure that each font/color/background are different every time the user clicks on the button, then perhaps you can create another helper:

function pickRandomExcept(arr, previous) { 
    while (true) {
        var pick = arr[Math.floor(Math.random() * arr.length)];
        if (pick != previous) {
            return pick;
        }
    }
}

However, in the caller you will need to keep track of the previous picks of each. For example:

var picked_color;

$("#try-me").click(function() {
    picked_color = pickRandomExcept(color, picked_color);
    $("#content")
        .hide()
        .removeClass()
        .addClass(pickRandom(font))
        .addClass(picked_color)
        .fadeIn(600);
share|improve this answer
    
Great, that's the kind of improvement I was looking for. Thanks! Any idea how I could make sure that each font/color/background are different every time the user clicks on the button? –  morgi Aug 13 at 9:49
    
Added an extra note for that. –  janos Aug 13 at 22:26

One possible improvement consists of removing code duplication between those two arrays:

var color = ["blue", "purple", "yellow", "red", "orange", "green"];
var background = ["bg-blue", "bg-purple", "bg-yellow", "bg-red", "bg-orange", "bg-green"];

Instead, you can build the second array from the first one:

var color = ["blue", "purple", "yellow", "red", "orange", "green"];
var background = color.map(function (c) { return "bg-" + c; })

This has two benefits:

  • If colors change, you have to do the change only once.

  • If the source of colors change (for example you start loading colors from a database or a configuration file), you have to change only the first line.

On the other hand, the drawbacks are:

  • That the code may be more difficult to understand by the beginners.

  • That in your particular case, the duplication is not an important issue: there are only six colors and arrays are easily modifiable by hand.

share|improve this answer
1  
Nice. You could also just prepend the "bg-" when adding the class, i.e. addClass('bg-' + ...), instead of mapping the array upfront. The benefit would be IE 6/7/8 support, since they don't have map –  Flambino Aug 12 at 16:50

Well code look good, any changes will be just a over thinking. one thing I want to make here is to specify variable for that calculation.this will make a bit readable ( not a very big point though)

// Switch font randomly
var font = ["lobster", "shadows", "oswald", "josefin", "gloria", "pacifico"];
var color = ["blue", "purple", "yellow", "red", "orange", "green"];
var background = ["bg-blue", "bg-purple", "bg-yellow", "bg-red", "bg-orange", "bg-green"];

$("#try-me").click(function() {
var selectedFontClass=font[Math.floor(Math.random()*font.length)];
var selectedColorClass=color[Math.floor(Math.random()*color.length)];
var selectedBackgroundClass=background[Math.floor(Math.random()*background.length)];
$("#content")
    .hide()
    .removeClass()
    .addClass(selectedFontClass) // Font
    .addClass(selectedColorClass) // Color
    .fadeIn(600);

$("html")
    .removeClass()
    .addClass(selectedBackgroundClass); // Background
});
share|improve this answer
    
Yep, sounds good. Thanks! –  morgi Aug 12 at 13:39
2  
On a personal note, I find the original version easier to read vs this version with variables (original looks cleaner). –  jsanc623 Aug 12 at 14:30

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.