Avoiding nested conditionals and comparing strings

Question

I have a web page with a set of icons that are grey by default:

enter image description here

If a user clicks one, it toggles green (.toggleClass('green')):

enter image description here

If a user clicks a green one, it toggles back to grey (.removeClass('green')).

Only one icon can be green at any given time, so if you have a green icon and you click a grey icon, the icon that was originally green will change to grey while the one you clicked would change to green.

Each of these icons corresponds to a section on the page.

If all the icons are grey, all the sections on the page are visible.
If one icon is green, only that icon's section is visible, the sections represented by the grey icons are hidden.

Here is the code that controls that behavior:

$(document).on 'click', '.toggler', ->
  icons = [$('#toggle-videos'),$('#toggle-images'),$('#toggle-words')]
  sections = [$('#videos'),$('#images'),$('#words')]
  if $(this).hasClass('green')
    $(this).toggleClass('green')
    i = 0
    while i < sections.length
      sections[i].show()
      i++
  else
    $(this).addClass('green')
    i = 0
    while i < icons.length
      if icons[i][0] != $(this)[0]
        icons[i].removeClass('green')
        sections[i].hide()
      else      
        j = 0
        while j < sections.length
          if $(this).attr('id').indexOf(sections[j].attr('id')) != -1
            sections[j].show()
          j++
      i++

I feel it is very hard to understand what this code does simply by reading it. I feel I am missing a simpler way to accomplish the following:

Associate an icon with a specific section on the page
Keep the green state of an icon associated with the visibility of a specific section on the page

My primary concerns with the code:

I have several nested if statements with nested while statements
I am checking the relationship between a section and an icon by checking if the icon.id has part of the string (this is the icon element and j is a counter): if $(this).attr('id').indexOf(sections[j].attr('id')) != -1

Is this acceptable for javascript or is there a better way?

Stepan Stepanov · Answer 1 · 2014-04-04 13:10:19Z

I would propose to use data attributes. E.g. icons have data-section-id attribute.

if $(this).hasClass('green') // all should become greynow
{
    $('div.section').show(); // show all sections
    $(this).removeClass('green');
}
else // we turn one sectio on
{
    $('div.section').hide(); // show all sections
    $('#'+ $(this).data('sectionId')).show(); // show a corresponding section
    $('li.icon').removeClass('green'); // turn all icons off
    $(this).addClass('green'); // turn current one on
}

But of course, that requires some changes in HTML structure.

I have been using data attributes elsewhere in the same app to come up with a similar solution - glad to see it came up here. Not a bad idea. — Ecnalyr, yesterday

JoshWillik · Answer 2 · 2014-04-04 19:13:13Z

If you like fun jQuery chains, the following should work for you.

$(document).on 'click', '.toggler', ->
    targetSection = $(this)
        .parent()
        .find('green')
            .removeClass('green')
            .end()
        .end()
        .addClass('green')
        .data( 'target' )
    $('.section').not(targetSection).hide()
    $(targetSection).show()

One thing is that you would have to modify your icons to have data- attributes like so:

<a id="video-toggle" data-target='#videos'>...</a>

konijn · Answer 3 · 2014-04-04 12:56:55Z

I usually don't review CoffeeScript, because I find it too hard too read. Your code is not too hard to read, so I will give it a shot.

I think in essence, you are trying too hard to show all but the one section, I would simply had all sections, and then show the one section you need. I would also cache $(this) for speed reasons and use more [].forEach for readability. Finally, this.id is more readable and faster than $(this).attr('id')

This untested counter proposal should illustrate what I mean:

$(document).on 'click', '.toggler', ->
  //Look up by `id`
  icons = ['toggle-videos','toggle-images','toggle-words']
  //Show via '#` + id
  sections = ['#videos','#images','#words']
  $this = $(this);
  if $this.hasClass('green')
    $this.removeClass('green')
    sections.forEach( (section) => section.show() )
  else
    $this.addClass('green')
    sections.forEach( (section) => section.hide() )
    sections[ icons.indexOf( this.id ) ].show();

Personally, and that is a bit more hackish, I would abuse the fact that the section id is postfixed to the toggle id and do this:

$(document).on 'click', '.toggler', ->
  $this = $(this);
  if $this.hasClass('green')
    $this.removeClass('green')
    sections.forEach( (section) => section.show() )
  else
    $this.addClass('green')
    sections.forEach( (section) => section.hide() )
    sections[ this.id.split("-")[1] ].show();

I might be tempted to put a toggleClass in there as well, but somehow the if and then the removeClass or addClass make this code easier to parse for me.

I did not know I could cache $(this) (I only 'do javascript' when I HAVE to). You made some good points about performance, hadn't even considered it up to this point. — Ecnalyr, yesterday
It's interesting you find CoffeeScript harder to read; I have the exact opposite experience. These days I only write plain JS for CodeReview answers :) — Flambino, yesterday

Flambino · Answer 4 · 2014-04-04 13:27:44Z

up vote 1 down vote

jQuery generally operates on collections of elements, so you rarely need to old-fashioned loops and incrementing indexes. In addition, CoffeeScript has many syntactic niceties for dealing with collections, which means that you hardly ever have to do old-fashioned looping.

I'd suggest keeping the link-to-section relationship in the HTML, like so:

<a class="toggle" href="#videos">...</a>
<a class="toggle" href="#words">...</a>
<a class="toggle" href="#images">...</a>

(using href has the added benefit that it'll still work, even with no JavaScript support)

Then, use that in your code to hide/show what's needed, e.g.

$ ->
  toggles  = $ ".toggle"
  sections = $ ".section" # or whatever selector works

  toggles.on "click", (event) ->
    event.preventDefault()

    target  = $ this
    section = $ target.attr("href") # get the associated section

    if target.hasClass("selected")
      target.removeClass("selected")
      sections.show() # show all sections
    else
      toggles.removeClass("selected") # remove selection from other links
      target.addClass("selected")     # select the clicked link
      sections.not(section).hide()    # hide other sections
      section.show()                  # show only the relevant section

Here's a demo

I've changed the selection class to selected rather than green, because "green" isn't a very descriptive name - or, in a sense, it's much too descriptive. Either way, the name's tied to the look rather than the purpose/function of the class.

edited yesterday

answered yesterday

Flambino
6,273421

Good point about the 'green' to 'selected' - I should have made that change before I even posted this here. Someone else mentioned using data attributes, it seems you use the actual href attribute to fulfill a similar role. I like this because it would give feedback on mouse-hover about what is going to happen. – Ecnalyr yesterday

1

@Ecnalyr Using data attributes is a perfectly valid solution too. I only use the href attribute here, because what we need to store - an element selector - has exactly the same syntax as a regular anchor link. Otherwise, I'd use a data attribute too. But, as mentioned, the links will make the page scroll to the correct section even if you remove all the javascript/coffeescript. I.e. it degrades gracefully – Flambino yesterday

Also nice trick with sections.not(section). Probably doesn't make much sense in this case. But is more correct if show/hide will be replaced with fadeIn/Out e.g. – Stepan Stepanov yesterday

@StepanStepanov That depends how you the look & feel you want for the page; the original code just uses show/hide. But yes, you can just replace it with fadeIn/Out, if you want. – Flambino yesterday

add comment

200_success · Answer 5 · 2014-04-04 13:36:10Z

To start with, you shouldn't use "green" as a class name. The name should be something generic, like "selected" or "current". Your CSS will cause the current selection to be rendered as a green icon, but you shouldn't hard-code that assumption into the name of the CSS class itself.

What you have described is essentially a radio button (with the additional feature that clicking on the currently selected item will deselect everything). Therefore, if you start with radio buttons, styled with images using CSS, most of your work will be done already!

I would like the radio button idea to work - and it would ordinarily. But I've failed to style with iconfonts in the past (probably my fault, not the fault of css itself), so I always stray away from this idea. I may have to revisit it. — Ecnalyr, yesterday

asked	yesterday
viewed	300 times
active	yesterday

current community

your communities

more stack exchange communities

Avoiding nested conditionals and comparing strings

5 Answers

Your Answer

Not the answer you're looking for? Browse other questions tagged coffeescript or ask your own question.

Hot Network Questions

current community

your communities

more stack exchange communities

Avoiding nested conditionals and comparing strings

5 Answers

Your Answer

Sign up or log in

Post as a guest

Not the answer you're looking for? Browse other questions tagged coffeescript or ask your own question.

Related

Hot Network Questions