Tell me more ×
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

The code allows you to select an area from the left column and another area from the right column followed by clicking on the Choose button which sends the chosen areas to the server:

<html>
  <head>
    <style>
    body {
      overflow: hidden;
    }

    article.left {
      overflow: hidden;
      float: left;
    }

    article.left section {
      float: left;
    }

    section {
      border: 1px solid black;
      height: 6em;
      margin-right: 1em;
      width: 4em;
    }

    article.right section {
      border: 1px dashed black;
    }

    section.ice {
      transform:rotate(-90deg);
      -moz-transform:rotate(-90deg); 
      -webkit-transform:rotate(-90deg); 
    }

    article.right {
      float: right;
    }

    section.section-selected, section.right-selected {
      border-color: #EEE;
    }

    input.choose {
      display: none;
    }
    </style>
    <script src="http://ajax.googleapis.com/ajax/libs/jquery/1.9.1/jquery.min.js"></script>
    <script>
    $(document).ready(function() {
      $('article.left section').click(function() {
        var was_selected = $(this).hasClass('section-selected');
        $('article.left section').removeClass('section-selected');
        if (!was_selected) {
          $(this).addClass('section-selected');
        }
      });

      $('article.right section').click(function() {
        $(this).toggleClass('right-selected');
        if ($('section.right-selected')) { 
          $(this).children('input.choose').toggle();
        }
      });

      $('input.choose').click(function() {
        var section = $('section.section-selected');
        if (section.length) {
          console.log(section.attr('section-id') + ' ' + $(this).attr('location-type'));
          console.log($(this).parents('article').attr('article-id'));
        }
        else {
          console.log('none selected');
        }
      });
    });
    </script>
  </head>
  <body>
    <article article-id="L" class="left">
      <section section-id="A">A</section>
      <section section-id="B">B</section>
    </article>

    <article article-id="R" class="right">
      <section section-id="C"><input type="button" class="choose" location-type="vertical" value="Choose" /></section>
      <section section-id="D" class="horizontal"><input type="button" class="choose" location-type="horizontal" value="Choose" /></section>
    </article>
  </body>
</html>

Here's a link to the jsfiddle http://jsfiddle.net/95WvB/. The code is working fine but I am wondering if the above is what is considered as spaghetti code that so many of those js frameworks like ember or angular are trying to solve. Is it best to use one of those framework to refactor the above code or use backbone?

This code is a sample of a much larger web application which repeats more or less of the same interactions between front and backend.

share|improve this question
1  
Why is your JavaScript, CSS and HTML all on the same file? Other than that, cache selector. I wouldn't use ember or angular for this sort of code since it is very small compared to a real web "application" – Benjamin Gruenbaum Mar 21 at 19:59
Just trying to make it simpler for people to test the code quickly, I separate the css and javascript in their own files in dev and production. – Thierry Lam Mar 21 at 20:02
"if ($('section.right-selected'))" - does this work properly? Empty arrays evaluate to true in Javascript so I thought "empty" jQuery objects would as well. – Krotton Mar 26 at 20:46
$('section.right-selected') evaluates to [] which is false, you can test it in the chrome console. Not sure if other browsers give different output for that. – Thierry Lam Mar 26 at 21:03
A) $('section.right-selected') doesn't evaluate to [], it evaluates to a jQuery object. B) Both [] as well as all objects are considered true in JavaScript. If you want to know if $('section.right-selected') has selected anything, you have to use if ($('section.right-selected').length > 0) {... . – RoToRa Apr 26 at 12:45

1 Answer

up vote 0 down vote

This bit needs refactoring. If you are writing a one time script or throw away code ,and you are absolutely certain it won't be more than throw away code, you needn't worry about refactoring.

But since this is a part of a large application everything needs a name indicating what it is or does, making explicit as much as possible implicit structure etc..

For example:

  $('article.left section').click(function() {
    var was_selected = $(this).hasClass('section-selected');
    $('article.left section').removeClass('section-selected');
    if (!was_selected) {
      $(this).addClass('section-selected');
    }
  });

Can be changed to

function selectAtMostOne(selection, className) {
  selection.click(function() {
    var was_selected = $(this).hasClass(className);
    selection.removeClass(className);
    if (!was_selected) {
      $(this).addClass(className);
    }
  });
}
selectAtMostOne($('article.left section'), 'section-selected');

And if you are doing this selectAtMostOnce [or whatever it was supposed to do] multiple times, you can add it to jquery or do something else to make using it easier, as in: 

$.fn.extend({
    selectAtMostOne: function(options) {
        var defaults = {
            selectedClass : 'selected'
        }
        var options = $.extend(defaults, options);

        var selection = this;
          return selection.click(function() {
            var was_selected = $(this).hasClass(options.selectedClass);
            selection.removeClass(options.selectedClass);
            if (!was_selected) {
              $(this).addClass(options.selectedClass);
            }
          });
}
});

$('article.left section').selectAtMostOne({selectedClass : 'section-selected'});

As an additional benefit, this will remind you to use variables instead of copy-pasted string constants. If you mistype any variable, hopefully your IDE or static-analysis (jslint etc) will tell you that you are using a variable before declaring it, or will have undefined error; which will be easier to debug than the case of mistyped string constants.

share|improve this answer

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.