4
\$\begingroup\$

I have a function to display or hide some elements. What I want to do is to manage the display of those elements regarding a value.

How should I replace the term toggle? Toggling for me means 'show the element if it's hidden, or hide it if it shown.' (yes, I mean toggling visibility)

function togglePassengers() {
    var nbPassengers = $("#nbPassengersForTravel").val(),
        $passengers = $('#d_passenger').find('fieldset');

    $passengers.each( togglePassenger );

    function togglePassenger(i){
        $passenger = $(this);
        if( ++i <= nbPassengers )
            showPassenger($passenger);
        else
            hidePassenger($passenger);
    }

    function showPassenger($passenger){
        $passenger
            .removeClass("hidden")
            .find("select.age option[value='-1']").remove();
    }

    function hidePassenger($passenger){
        $passenger.addClass("hidden");
        var $passengerAge = $passenger.find("select.age");
        if ( ! $passengerAge.find("option[value='-1']:first-child").length )
            $passengerAge.prepend("<option value='-1'>---------</option>").val('-1');
    }
}
\$\endgroup\$
  • \$\begingroup\$ I think, I may go for changeVisibilityOfPassenger(i) \$\endgroup\$ – long-lazuli Jan 16 '15 at 16:01
  • \$\begingroup\$ SetPassengerVisibility? \$\endgroup\$ – sodawillow Jan 16 '15 at 16:52
  • 1
    \$\begingroup\$ I think a function named setSomething should take the value setted as a parameter. Which is not the case, here. (And I prefer the camelCase style :-) ). Thank you anyway. \$\endgroup\$ – long-lazuli Jan 16 '15 at 17:04
4
\$\begingroup\$

Naming

I think togglePassenger is not the only naming problem in your code.

A function parameter should not be a single letter except in a very limited number of cases. But I don't think that this is not one of them; a reader does not automatically understand what i actually does.

I would also rename nbPassengers, because it is unclear what nb means.

What I think your function does is show the first n passengers, and hide the rest. If that is the case, I would not use toggle, because as you said, there really is no toggling going on. You could go with something like updateVisibility to avoid this confusion, although it's a bit unconcrete. Whatever name you do choose, I would add a comment to the function explaining what it does.

Misc

  • I would use curly brackets even for one line statements to avoid future bugs
  • your spacing is sometimes inconsistent (eg spacing around variable in function calls)
\$\endgroup\$
  • \$\begingroup\$ updateVisibility sounds good ! However, I never use single letter variable BUT for index. I think it is readable. I could go for passengerIndex, true. for nbPassenger, I don't know... numberOfPassengers seems a bit long. But at least I should put nbPassengers with plural. Thanx. \$\endgroup\$ – long-lazuli Jan 16 '15 at 16:06
  • \$\begingroup\$ I don't think that numberOfPassengers is all that long. But you could also use passangerCount, which is quite clear and shorter. \$\endgroup\$ – tim Jan 16 '15 at 16:10
  • \$\begingroup\$ With updateVisibility I may use passengerIndex instead of i because the subject is not in the function name. \$\endgroup\$ – long-lazuli Jan 16 '15 at 16:12

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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