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\$
3
  • \$\begingroup\$ I think, I may go for changeVisibilityOfPassenger(i) \$\endgroup\$ Jan 16, 2015 at 16:01
  • \$\begingroup\$ SetPassengerVisibility? \$\endgroup\$
    – sodawillow
    Jan 16, 2015 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\$ Jan 16, 2015 at 17:04

1 Answer 1

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\$
3
  • \$\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\$ Jan 16, 2015 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, 2015 at 16:10
  • \$\begingroup\$ With updateVisibility I may use passengerIndex instead of i because the subject is not in the function name. \$\endgroup\$ Jan 16, 2015 at 16:12

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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