current community

your communities

more stack exchange communities

company blog
Stack Exchange Inbox Reputation and Badges
tour help
Sign up ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 2 down vote favorite

This is copied directly from Stackoverflow as I was suggested to post it here.

I have the below code, which does work fine but I find it quite big and had hoped there was a cleaner/smaller "toggle" function or alike which I could use but it seems to be related to visibility only - and I want to set a variable (for later usage).

Can this below be optimized, if I want a toggle function which should be used to sort a column (and get the value in to a variable)?

<table>
<thead>
  <tr>
    <td>Col 1</td>
    <td>Col 2</td>
    <td>Col 3</td>
  </tr>
</thead>
<tbody>
  <tr>
    <td>A</td>
    <td>B</td>
    <td>C</td>
  </tr>
  <tr>
    <td>B</td>
    <td>C</td>
    <td>A</td>
  </tr>
</tbody>
</table>

And the jQuery stuff:

var column;
var order;

$('thead th').click(function () {

  // Get the current column clicked
  var thisColumn = $(this).text();

  // Check if the column has changed
  if(thisColumn == column) {
    // column has not changed

    if(order == "ascending") {
      order = "descending";
    } else {
      order = "ascending";
    }
  } else {
    // column has changed

    column = thisColumn;
    order = "descending";
  }

  // Replace text in DIV
  $("div").text("column=["+column+"], order=["+order+"]");

  // future code will use the sort order to get database
  // stuff with Ajax

});

Check JSFiddle code here, http://jsfiddle.net/Psz5K/

share|improve this question
    
You don't have any th elements in your HTML code – rink.attendant.6 Aug 21 '13 at 17:20
    
@rink - good point but the th is in the Fiddle demo (I must have copied an old code snippet). – DHS Aug 22 '13 at 8:39

1 Answer 1

up vote 1 down vote accepted

Mostly I'ma java developer, but I think that there are a couple of things that you can do to tidy the code up.

If you do not want to store state in the DOM then you can remove the jQuery data statements and store in the global scope. If I were a jQuery developer I would probably wrap this functionality into a plugin (sortable?) and then to call it on a table with the sortabletable calss use $('.sortableTable').sortable(). It would be more reusable, more encapsulated, and less likely to cause you weird behavior if you have two tables in your page!

However here's my quick take on the above code:

var column;

$('thead th').click(function () {

    // Get the current column clicked
    var thisColumn = $(this);

    if (thisColumn.is(column)) {
        var sort = column.data("sort");
        var newSort = toggle(sort);
        column.data("sort", newSort);
    } else {
        column = thisColumn;
        column.data("sort", false);
    }
    // Replace text in DIV
    $("div").text("column=[" + column.text() + "], order=[" + column.data("sort") + "]");

});

function toggle(current) {
    return !current;
}

You could skip over setting an initial state and have toggle (and you Ajax code) work with undefined values properly (returning false).

share|improve this answer
    
I will set your answer as the correct answer as I like your approach on the toggle function. But why use the data("sort",val) approach instead of just setting a variable - what's the catch? – DHS Aug 22 '13 at 9:10
    
BTW: I have updated your Fiddle demo here, http://jsfiddle.net/Psz5K/4/ – DHS Aug 22 '13 at 9:12
    
Brief to fit in limits! Personally, no catch. Data call uses jQuery's managed storage helps prevent memory leaks. A great way for storing state and behavior against the related elements (maybe a progress bar?). Tracking a single variable(Object) ("column") you can track all important facets ("sort"). Less maintenance to be performed as code gets more complex, fewer errors likely to creep in. Complex plugins can store state within the objects themselves, a more proficient javascript developer can give approaches on that! Whether global, data, revealing module, look into namespacing. – JohnMark13 Aug 22 '13 at 10:28
    
How about then declaring var column as var global and a global.data("column",$(this)) - would this be any better? – DHS Aug 22 '13 at 10:54
    
I think that you shouldn't get hung up on the use of data and consider your question from the point of core javascript development. Take a look at namespacing (one popular syntax you could look for is var myNamespace = myNamespace || {};) Then look for tutorials on the (Revealing) Module Pattern which will help you with encapsulation and protecting your variables - whilst not polluting the global space. One of the joys of javascript is just how many ways you can achieve something, you will find a lot of opinions, many of them will disagree, many of them will be valid. – JohnMark13 Aug 22 '13 at 11:21

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.