Code Advice - How to make more concise (Javascript/Jquery)

Question

I'm trying to make my code more concise (i.e., less repeated code). I've gotten some advice from my supervisor as to how to make my recent code more concise, but I'm not exactly sure how to do it.

I have some coordinates that I am using to check if the user clicks within a certain area of a div. I was told that I should put all the coordinates in an array and "loop through" to get them when I need them. I -think- I understand what he was suggesting, but I can't quite wrap my head around it since I'm still inexperienced with programming. Here's what I have done so far so you can get a better idea of what's going on:

    $("#div1").click(function(e)
    {
        // Arrays containing the x and y values of the rectangular area around a farm
        // [minX, maxX, minY, maxY]
        var div1_Coord_Area1= [565, 747, 310, 423];
        var div1_Coord_Area2= [755, 947, 601, 715];

        if(areaX >= Reg2_Coord_Farm1[0] && areaX <= Reg2_Coord_Farm1[1] && areaY >= Reg2_Coord_Farm1[2] && areaY <= Reg2_Coord_Farm1[3]) 
        {
            alert("You clicked in the first area");
        }
        if(areaX >= Reg2_Coord_Farm2[0] && areaX <= Reg2_Coord_Farm2[1] && areaY >= Reg2_Coord_Farm2[2] && areaY <= Reg2_Coord_Farm2[3]) 
        {
            alert("You clicked in the second area");
        } 
});

Do not worry about how I do the calculations; I left that code out of the method since it is basically irrelevant, but it is there in case you were wondering.

I made an array for each set of coordinates and simply call those. This isn't "looping through" a giant array filled with all of the coordinates of each area, however. Can you conceive of a way of doing this, or is the best I could do at the moment?

mVChr · Accepted Answer · 2011-03-28 21:20:29Z

2

I think what he means is something more like this:

$("#div1").click(function(e){
    // Arrays containing the x and y values of the rectangular area around a farm
    // For each array: [area1, area2, area3, ... areaX]
    var minX = [565, 755];
    var maxX = [747, 947];
    var minY = [310, 601];
    var maxY = [423, 715];

    for (var i = 0; i < minX.length; i++) {
      if (areaX >= minX[i] && areaX <= maxX[i] && areaY >= minY[i] && areaY <= maxY[i]) {
        alert("You clicked in area " + (i+1));
      }
    }
});

This way, you can have many more areas to check but never have to change the loop as it will always iterate through all items in the array, be it 2 like above, or 10, or 100.

answered Mar 28, 2011 at 21:20

mVChr

50.3k11 gold badges111 silver badges105 bronze badges

Sign up to request clarification or add additional context in comments.

2 Comments

Briz Over a year ago

This looks good too. I'll study both of these suggestions and see which one works best for me.

Briz Over a year ago

This answer worked the best for me; it made my code much more concise, and I think it does what my supervisor wanted it to do. Thanks a ton!

David Basarab · Accepted Answer · 2011-03-28 21:24:35Z

From first and a very quick glance you can extract the click area into a separate function.

Something like this.

$("#div1").click(function(e)
{
    // Arrays containing the x and y values of the rectangular area around a farm
    // [minX, maxX, minY, maxY]
    var div1_Coord_Area1= [565, 747, 310, 423];
    var div1_Coord_Area2= [755, 947, 601, 715];

    if(inArea(div1_Coord_Area1, someStructForMouseLocation)) 
    {
        alert("You clicked in the first area");
    }
    if(inArea(div1_Coord_Area2, someStructForMouseLocation)) 
    {
        alert("You clicked in the second area");
    } 
});

function inArea(coordArea, mouseLocation)
{
    return mouseLocation.X >= coordArea[0] && mouseLocation.X <= coordArea[1] && mouseLocation.Y >= coordArea[2] && mouseLocation.Y <= coordArea[3]
}

You also seem to have some 'magic' numbers in the var div1_Coord_Area1= [565, 747, 310, 423]; and var div1_Coord_Area2= [755, 947, 601, 715]; I would consider making them global variables so they are out of the scope of the click function.

It would read like

// Arrays containing the x and y values of the rectangular area around a farm
// [minX, maxX, minY, maxY]
var div1_Coord_Area1= [565, 747, 310, 423];
var div1_Coord_Area2= [755, 947, 601, 715];

$("#div1").click(function(e)
{
    if(inArea(div1_Coord_Area1, someStructForMouseLocation)) 
    {
        alert("You clicked in the first area");
    }
    if(inArea(div1_Coord_Area2, someStructForMouseLocation)) 
    {
        alert("You clicked in the second area");
    } 
});

function inArea(coordArea, mouseLocation)
{
    return mouseLocation.X >= coordArea[0] && mouseLocation.X <= coordArea[1] && mouseLocation.Y >= coordArea[2] && mouseLocation.Y <= coordArea[3]
}

Hopefully you can see my process is one of further refinement. DO NOT expect to write clean code when you first create the method. You have to look at it afterward and see if it tells a story. Another change would be the name of div1_Coord_Area1, does the name really reval the intent of the variable? No. Would HotSpotArea1 mean more? Remember you are telling a story with code. The more you can do so the next person uses the least number of brain cycles to read the code the better.

You have to constantly refine to get really clean code.

This might work. I'll look into seeing how this implements into my code and let you know what I find.
I appreciate the additional advice and code. I will say that I have the name div1_Coord_Area1 because I have more than 1 div and a lot more areas within each of those.

typeof · Accepted Answer · 2011-03-28 21:44:24Z

1

If I were you, I'd make your areas objects:

// think of this as your ClickArea class
function ClickArea(minX, maxX, minY, maxY, description) {
    this.minX = minX;
    this.maxX = maxX;
    this.minY = minY;
    this.maxY = maxY;
    this.description = description;
};
ClickArea.prototype.isInside = function(areaX, areaY) {
    return (areaX >= this.minX && areaX <= this.maxX && areaY >= this.minY && areaY <= this.maxY);
};

Now, you can create a ClickArea object like this:

var area = new ClickArea(565, 747, 310, 423, "Area 1");

But you'll want them in an array, so I'd suggest creating them like this:

var areas = [
    new ClickArea(565, 747, 310, 423, "Area 1"),
    new ClickArea(365, 745, 330, 423, "Area 2")
];
// you can also add new ClickAreas using the array's push method:
areas.push(new ClickArea(333, 444, 555, 566, "Area 3"));

You can then loop over them using a for loop:

for(var i = 0; i < areas.length; i++) {
    if(areas[i].isInside(areaX, areaY)) {
        alert("You clicked in " + areas[i].description);
    }
}

answered Mar 28, 2011 at 21:44

typeof

5,9722 gold badges18 silver badges19 bronze badges

2 Comments

Briz Over a year ago

Thank you for your advice, but I just don't have time to completely redo the code right now. I'll keep this for future reference though.

typeof Over a year ago

I see. Down the line, this will offer greater flexibility because you can add more methods to your object -- reposition(), scale(), etc...

Collectives™ on Stack Overflow

Code Advice - How to make more concise (Javascript/Jquery)

3 Answers 3

2 Comments

2 Comments

2 Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

3 Answers 3

2 Comments

2 Comments

2 Comments

Your Answer

Sign up or log in

Post as a guest

Related