App for painting images

Question

I am a newcomer to jQuery and am working on an app that allows the user to paint images. To do that, I've created a color palette, and if a user clicks on a color, the paint brush successfully changes to that color. However, my code looks really ugly. I really want to clean it up.

$(document).ready(function() {
        $('#color-one').click(function() {
          brush.setColor('#990000');
        });
        $('#color-two').click(function() {
        brush.setColor('#CC0000');
    });
        $('#color-three').click(function() {
        brush.setColor('#CC3300');
    });
        $('#color-four').click(function() {
        brush.setColor('#FFCC00');
    });
        $('#color-five').click(function() {
        brush.setColor('#FFFF00');
    });
        $('#color-six').click(function() {
        brush.setColor('#CCFF00');
    });
        $('#color-seven').click(function() {
          brush.setColor('#66FF00');
        });
        $('#color-eight').click(function() {
        brush.setColor('#003300');
    });
        $('#color-nine').click(function() {
        brush.setColor('#33FFCC');
    });
        $('#color-ten').click(function() {
        brush.setColor('#3300CC');
    });
        $('#color-eleven').click(function() {
        brush.setColor('#660033');
    });
        $('#color-twelve').click(function() {
        brush.setColor('#660066');
    });
        $('#color-thirteen').click(function() {
        brush.setColor('#000000');
    });
        $('#color-fourteen').click(function() {
        brush.setColor('#ffffff');
    });
    });
</script>

HTML

<div id = "colors">
    <div id="color-one" style="background: #990000;"></div>
    <div id="color-two" style="background: #CC0000;"></div>
    <div id="color-three" style="background: #CC3300;"></div>
    <div id="color-four" style="background: #FFCC00;"></div>
    <div id="color-five" style="background: #FFFF00;"></div>
    <div id="color-six" style="background: #CCFF00;"></div>
    <div id="color-seven" style="background: #66FF00;"></div>
    <div id="color-eight" style="background: #003300;"></div>
    <div id="color-nine" style="background: #33FFCC;"></div>
    <div id="color-ten" style="background: #3300CC;"></div>
    <div id="color-eleven" style="background: #660033;"></div>
    <div id="color-twelve" style="background: #660066;"></div>
    <div id="color-thirteen" style="background: #000000;"></div>
    <div id="color-fourteen" style="background: #ffffff;"></div>
</div>

I was thinking that the refactored version might look something along the lines of this:

$('#color-one, #color-two, ... color-fourteen).click(function() {
  brush.setColor(this.val);
});

However, I would like to see what this community has to think.

Answer 1

You are right. You have way too much duplication there. And you are on the right track with your refactored version but I don't think it goes far enough.

Right now if you want to add a new color you have to do the following.

1. Add a new div to the color buttons with a unique id.
2. Add style rules (background) to that div with the color.
3. Add a button click rule that selects the button by id.
4. Change the brush color making sure it matches the background done in #2.

With your refactoring you would change this to:

1. Add a new div to the color buttons with a unique id.
2. Add style rules (background) to that div with the color.
3. Add a value to the div button.
4. Add the id to the button click function.

So you might have made the code shorter but didn't really make it much easier to maintain.

So, how can we get this shorter. Well, we can select elements by class name. So instead of using the id we just use class names.

<div id = "colors">
    <div class="brush-selector" style="background: #990000;" data-val="#990000;"></div>
    ...
    <div class="brush-selector" style="background: #ffffff;" data-val="#FFFFFF"></div>
</div>

And your script becomes:

$('#colors .brush-selector').click(function() {
    brush.setColor(this.data('val'));
});

Now to add a new color you have the following steps:

1. Add a new div to the color buttons with a unique id.
2. Add style rules (background) to that div with the color.
3. Add a value to the div button that matches the color.

Better, but we are still duplicating info. The background color and data-val fields are the same. And if we accidentally type one wrong then we have incorrect behavior. So why don't we just use the background-color of the div.

<div id = "colors">
    <div class="brush-selector" style="background: #990000;"></div>
    ...
    <div class="brush-selector" style="background: #ffffff;"></div>
</div>


$('#colors .brush-selector').click(function() {
    brush.setColor(this.css('background-color'));
});

That takes our steps down to:

1. Add a new div to the color buttons.
2. Add style rules (background) to that div with the color.

Which is pretty good. Basically one step to adjust colors. Just adjust the divs.

There are further steps you could take that I won't go into detail but leave to you.

If you knew the only divs in the #colors elements were brush selectors you could remove the class altogether and just select the divs within #colors $(#colors div).

You could also create a javascript array of allowed colors. Then create the buttons using the array. Then the only step you would need would be to adjust the colors in that array. This would also allow you to select colors dynamically since that array could come from anywhere.